Skip to content

Implement fetching by doi and custom hashes#61

Merged
j-wags merged 14 commits intomainfrom
fetch-by-doi2
Jul 11, 2025
Merged

Implement fetching by doi and custom hashes#61
j-wags merged 14 commits intomainfrom
fetch-by-doi2

Conversation

@j-wags
Copy link
Copy Markdown
Member

@j-wags j-wags commented Jun 27, 2025

Companion to openforcefield/openff-toolkit#2048 and openforcefield/openff-interchange#1206 for preliminary <NAGLCharges> spec support

Comment thread openff/nagl_models/_dynamic_fetch.py Outdated
return cached_path.as_posix()

if doi:
zenodo_id = re.findall("10.5072/zenodo.([0-9]+)", doi)[0]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we support non-Zenodo DOIs? Figshare etc?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this version of the NAGLCharges spec, but other sources could be added in a higher NAGLCharges section version.

@j-wags j-wags assigned mattwthompson and unassigned j-wags Jul 8, 2025
Copy link
Copy Markdown
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a handful of suggestions for minor improvements but I don't identify any as blocking

Comment thread openff/nagl_models/_dynamic_fetch.py Outdated
Comment thread openff/nagl_models/_dynamic_fetch.py Outdated
Comment thread openff/nagl_models/_dynamic_fetch.py Outdated
Comment thread openff/nagl_models/_dynamic_fetch.py Outdated
def test_get_model_by_doi_and_hash(hide_cache):
get_model(
"my_favorite_model.pt",
doi="10.5072/zenodo.278300",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This record must be sand-box only? This is my first Google result, which seems unlikely to be what you actually want to point to: https://zenodo.org/records/14335473

A comment or note about where this lives and how the hash was generated would be useful for future developers, I don't think anything else would be necessary here

@j-wags
Copy link
Copy Markdown
Member Author

j-wags commented Jul 10, 2025

Why in the world did I put this in the hidden icebox? Re-assigning to myself 🤦

@j-wags
Copy link
Copy Markdown
Member Author

j-wags commented Jul 11, 2025

Hmm, the tests may be hitting rate limits occasionally, but I'm gonna merge this and fix that separately if needed

@j-wags j-wags merged commit 89935c8 into main Jul 11, 2025
8 of 9 checks passed
@j-wags j-wags deleted the fetch-by-doi2 branch July 11, 2025 19:52
@mattwthompson
Copy link
Copy Markdown
Member

I have no complaints if we end up rolling this back for the 2.3.0 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants