fix: CCD/SMILES inconsistency by jandom · Pull Request #95 · aqlaboratory/openfold-3

jandom · 2026-01-21T15:30:23Z

Summary

Fixes #94

Changes

Added tests that show the problem

Related Issues

Testing

Other Notes

…sistency

openfold3/setup_openfold.py

ljarosch · 2026-01-22T11:50:15Z

openfold3/setup_openfold.py


+def setup_biotite_ccd() -> None:
+    logger.info("Starting Biotite CCD setup...")
+    if not biotite.setup_ccd.OUTPUT_CCD.exists():


Would we actually expect this condition to ever trigger? Biotite ships a CCD during its own setup, so I'd always expect one to exist there. Imo the problem in Pat's case isn't that there is no Biotite CCD downloaded, it is that the ligands he is working with are so recent that they won't be found in outdated CCDs, which is why one needs to trigger a manual update.

No, actually – empirically I found that I need to run this and then tests start to pass. The installed biotite somehow comes with a different CCD file?

Ah, no I think you're partially right actually – there is some default CCD file that installs with biotite but it's out-of-date and running the setup command updates it

Yeah this is caused by our version constraint of biotite 1.2.0, which likely ships an outdated CCD. I can confirm that a non-constrained install of biotite doesn't run into this problem.

It's still a general problem though, of course we can ameliorate it for some time by installing a fresh CCD on setup, but the PDB will keep adding new ligands weekly so you can quickly run into the same problem again. It might be reasonable enough to just include a helpful error message when a ligand could not be found in the CCD that suggests running the setup_ccd command.

(and we'd need to get rid of the guard because biotite.setup_ccd.OUTPUT_CCD.exists() should always return true after biotite was installed)

Overall this is great!

Is there a way to fetch the date / version of the CCD file? Then setup_ccd could prompt the user if they want to download a newer version when it is available.

Negative, we can get an ETag/last modified of the remote CCD file – but it's not the same file as biotite saves, because it does some modification prior. So we can't do a simple ETag1 == ETag2.

Instead I opted for a "stale ETag list" and if the file on disk is stale, it'll re-download.

I excluded all solutions with a sidecar file, this will only make it more messy.

This is overall not great because we hard-code a md5 sum in code but until we move beyond biotite==1.2.0, I don't see a better way.

jnwei

Nice addition, clean way to update the CCD automatically upon openfold3 setup.

docs/source/Installation.md

ljarosch

Generally I like that we're cleaner about this now and making our tests more robust. However how would this fix this issue for users like Pat at inference time? Is it because this update function will run at setup?

Generally we will keep running into this problem, this fix will update the CCD once, but if you wait a month and try to inference a recent compound you will run into the same issue again. I think it would make sense to add a more prominent pointer to python -m biotite.setup_ccd to our docs, and also reference it in the error message when a CCD entry can't be found.

ljarosch · 2026-01-26T21:25:51Z

docs/source/Installation.md

 - Setup a directory for OpenFold3 model parameters [default: `~/.openfold3`]
    - Writes the path to `$OPENFOLD_CACHE/ckpt_path` 
 - Download the model parameters, if the parameter file does not already exist 
+- Download and setup the [Chemical Component Dictionary (CCD) with Biotite](https://www.biotite-python.org/latest/apidoc/biotite.structure.info.get_ccd.html)


Not sure if the pointer of this link is super helpful, this is just a function to work with Biotite's CCD representation in code. Why not just add python -m biotite.setup_ccd if the intent is to show users how to download and set it up?

We could surface this as work-around but that's really our abstraction leaking. One day we might remove biotite, and the users should be none the wiser about it.

My intention was to provide users who might be unfamiliar with the CCD some context about what it is since it is now included in our top level Installation documentation and how biotite is used in relation to the CCD.

We can certainly replace this link with a more general pointer to the CCD (perhaps this one: https://www.wwpdb.org/data/ccd) if that would be more helpful. This might make more sense if we now plan to provide the CCD file ourselves

jandom · 2026-01-27T08:31:12Z

You do have a point about this working "once only" – only if the bcif has the stale hash, will this ever update, and after-wards it'll just be stuck. But I don't see a way around it other than by introducing state, which I really-really don't want to do.

jandom · 2026-01-27T10:15:52Z

Sorry, i know this is becoming a PR turned RFC nightmare. Here is what i think we should do, because there is a real limitation to the approach in this PR, in that the update will happen once-only.

Context

biotite periodically will update their CCD database, this will happen without our control, and might tail the CCD
missing CCD codes cause inference issues, where users need to swap-up "missing" CCD codes for smiles (Pat's problem)
we're baked into a particular biotite version for the time being - see [BUG] Broken test_tokenizer.py tests for biotite>=1.3 #96
there is no obvious way to detect a stale user CCD relative to the PDB, because biotite performs a transform internally, we can't use the ETag

Options

detect a stale CCD by hash (this PR), will correctly update a stale CCD inside biotite but works only once
manage the CCD file ourselves, this avoids hard-coding anything in code but requires us to manage a file on S3 (not too bad?). We then can control the update logic via ETags and pull in an update version when needed.

jnwei

Nice solution! Provides an easy way to provide an updated CCD bcif ourselves and inserting into biotite's default CCD. The only potential issue I can think of is if a structure gets added to the CCD that biotite cannot support, but that seems like a very narrow edge case.

Two requests:

Could we add a small script / workflow for how to update the CCD file to the bucket?
I added a suggestion to make the test for smiles / CCD comparision more robust.

jnwei · 2026-01-28T16:50:25Z

docs/source/Installation.md

 - Setup a directory for OpenFold3 model parameters [default: `~/.openfold3`]
    - Writes the path to `$OPENFOLD_CACHE/ckpt_path` 
 - Download the model parameters, if the parameter file does not already exist 
+- Download and setup the [Chemical Component Dictionary (CCD) with Biotite](https://www.biotite-python.org/latest/apidoc/biotite.structure.info.get_ccd.html)


My intention was to provide users who might be unfamiliar with the CCD some context about what it is since it is now included in our top level Installation documentation and how biotite is used in relation to the CCD.

We can certainly replace this link with a more general pointer to the CCD (perhaps this one: https://www.wwpdb.org/data/ccd) if that would be more helpful. This might make more sense if we now plan to provide the CCD file ourselves

jnwei · 2026-01-28T16:53:19Z

openfold3/tests/core/data/primitives/structure/test_query.py

+    struct_from_smiles = structure_with_ref_mol_from_smiles(smiles, chain_id="X")
+    struct_from_ccd = structure_with_ref_mol_from_ccd_code(ccd_code, chain_id="X")
+
+    assert len(struct_from_smiles.atom_array) == len(struct_from_ccd.atom_array)


Some quick suggestions to make this test more comprehensive:

Would it make sense to use the custom_assert_utils.assert_atom_array_equal utility here? Or would we expect some differences in atom ordering / bond ordering so that this assert would fail.

If assert_atom_array_equal is not a good fit here, perhaps we could add a test to check the number of carbons or some other aggregate property.

Ah, it'd be really nice indeed if we could use that – but it currently doesn't work, just tried empirically (atm atom names different between smiles and CCD, eg C01 vs C1) – maybe we can just match on element count?

Yeah figures. I think the atom counter is good enough here for now, thanks for adding it.

jnwei

LGTM

jnwei · 2026-01-29T16:43:21Z

openfold3/tests/core/data/primitives/structure/test_query.py

+    struct_from_smiles = structure_with_ref_mol_from_smiles(smiles, chain_id="X")
+    struct_from_ccd = structure_with_ref_mol_from_ccd_code(ccd_code, chain_id="X")
+
+    assert len(struct_from_smiles.atom_array) == len(struct_from_ccd.atom_array)


Yeah figures. I think the atom counter is good enough here for now, thanks for adding it.

fix: CCD/SMILES inconsistency

e645c37

jandom self-assigned this Jan 21, 2026

create a setup_biotite_ccd and update docs

cafc71b

jandom requested review from jnwei and ljarosch January 22, 2026 11:28

jandom marked this pull request as ready for review January 22, 2026 11:28

Merge branch 'main' into jandom/2026-01/fix/issue-94-ccd-smiles-incon…

bbbfe6a

…sistency

jandom added the safe-to-test Internal only label used to indicate PRs that are ready for automated CI testing. label Jan 22, 2026

jandom commented Jan 22, 2026

View reviewed changes

openfold3/setup_openfold.py Outdated Show resolved Hide resolved

ljarosch reviewed Jan 22, 2026

View reviewed changes

force the download (debug)

c9fb62e

jnwei added safe-to-test Internal only label used to indicate PRs that are ready for automated CI testing. and removed safe-to-test Internal only label used to indicate PRs that are ready for automated CI testing. labels Jan 22, 2026

jnwei approved these changes Jan 22, 2026

View reviewed changes

docs/source/Installation.md Outdated Show resolved Hide resolved

force the download (debug)

5a6393c

jandom requested review from jnwei and ljarosch January 22, 2026 14:00

jandom added safe-to-test Internal only label used to indicate PRs that are ready for automated CI testing. and removed safe-to-test Internal only label used to indicate PRs that are ready for automated CI testing. labels Jan 22, 2026

ljarosch requested changes Jan 26, 2026

View reviewed changes

jandom added 3 commits January 28, 2026 10:26

use a remote S3 reference file to trigger download

3bc3f92

better logging message

04b1fe5

remove fixme, better imports

85f6ec0

jandom requested a review from ljarosch January 28, 2026 09:30

jnwei requested changes Jan 28, 2026

View reviewed changes

jandom added 2 commits January 29, 2026 14:56

review: comments from Jennifer

3a21a38

review: script to update CCD on S3

2be65b8

jandom requested a review from jnwei January 29, 2026 14:11

jandom added safe-to-test Internal only label used to indicate PRs that are ready for automated CI testing. and removed safe-to-test Internal only label used to indicate PRs that are ready for automated CI testing. labels Jan 29, 2026

factor-out duplication

30bca1b

jnwei approved these changes Jan 29, 2026

View reviewed changes

jandom merged commit 359e6fd into main Jan 30, 2026
2 checks passed

jandom deleted the jandom/2026-01/fix/issue-94-ccd-smiles-inconsistency branch January 30, 2026 14:37

jnwei mentioned this pull request Feb 9, 2026

fix: setup CCD not actually downloading anything #118

Merged

Conversation

jandom commented Jan 21, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnwei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ljarosch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jandom commented Jan 27, 2026

Uh oh!

jandom commented Jan 27, 2026 • edited by jnwei Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnwei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnwei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jandom commented Jan 27, 2026 •

edited by jnwei

Loading