-
Notifications
You must be signed in to change notification settings - Fork 39
update metatomic checkpoint to fix tests #223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes update the URLs used to load the pretrained "pet-mad" metatomic model checkpoint in both the test fixture and the model constructor. The URLs now point to a specific version ("v1.1.0") instead of the previous "main" branch and "latest" checkpoint. Additionally, new dependencies were added to tutorial scripts, and a package dependency specification was changed from a GitHub URL to a version-pinned PyPI requirement. Changes
Estimated code review effort1 (~5 minutes) Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
torch_sim/models/metatomic.py (1)
105-108
: Pinning is great – consider centralising the checkpoint URL to avoid duplication.Hard-coding the full URL here and again in the test file means every upgrade (e.g.
v1.1.1
) requires a multi-file search-and-replace. Defining a single constant (or env var) will reduce drift and simplify future bumps.+# Module-level constants +PET_MAD_VERSION = "v1.1.0" +PET_MAD_CKPT_URL = ( + f"https://huggingface.co/lab-cosmo/pet-mad/resolve/{PET_MAD_VERSION}/models/" + f"pet-mad-{PET_MAD_VERSION}.ckpt" +) ... if model == "pet-mad": - path = "https://huggingface.co/lab-cosmo/pet-mad/resolve/v1.1.0/models/pet-mad-v1.1.0.ckpt" + path = PET_MAD_CKPT_URL self._model = load_model(path).export()This keeps the version string in one spot and lets tests import the same constant.
tests/models/test_metatomic.py (1)
30-33
: Reuse the production constant to keep tests in sync.The test now duplicates the checkpoint URL changed in
metatomic.py
. Importing the constant suggested above avoids divergence and reduces maintenance overhead.-from tests.models.conftest import ( +from torch_sim.models.metatomic import PET_MAD_CKPT_URL +from tests.models.conftest import ( ... - model=load_model( - "https://huggingface.co/lab-cosmo/pet-mad/resolve/v1.1.0/models/pet-mad-v1.1.0.ckpt" - ).export(), + model=load_model(PET_MAD_CKPT_URL).export(),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Summary
This solves #221
I just needed to update some links. This is what metatomic changed in their README when they discovered that the previous links broke. lab-cosmo/pet-mad#33
v1.2.0 is the latest on huggingface (https://huggingface.co/lab-cosmo/pet-mad), but when I went to https://huggingface.co/lab-cosmo/pet-mad/resolve/v1.2.0/models/pet-mad-v1.2.0.ckpt it's not found. So I'm using the v1.1.0 checkpoint (same as their README)
I also fixed a few other failing tests by updating their versions.
Checklist
Before a pull request can be merged, the following items must be checked:
Run ruff on your code.
We highly recommended installing the pre-commit hooks running in CI locally to speedup the development process. Simply run
pip install pre-commit && pre-commit install
to install the hooks which will check your code before each commit.Summary by CodeRabbit
Summary by CodeRabbit