-
Notifications
You must be signed in to change notification settings - Fork 57
Fix installation issues #56
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
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.
Pull request overview
This PR improves the foundry CLI installation experience by refactoring command structure, adding new installation options, and reorganizing documentation.
Key Changes:
- Refactored CLI commands with clearer naming (
list-available,list-installed,clean) and introduced abase-modelsinstallation option - Extracted common checkpoint directory resolution logic into a helper function
_resolve_checkpoint_dir - Reorganized documentation sections in READMEs for improved clarity and moved HBPLUS setup to an appendix
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/foundry_cli/download_checkpoints.py |
Refactored CLI commands with new naming, added base-models option, removed checkpoint-dir parameters from some commands, and extracted common logic |
models/rfd3/README.md |
Reorganized documentation with improved section headings, moved HBPLUS setup to appendix, and updated training examples |
README.md |
Updated command examples to use new base-models option and new command names (list-available, list-installed) |
.gitignore |
Added ignore patterns for demo output files (.cif* and .json files) |
.env |
Added hardcoded FOUNDRY_CHECKPOINTS_DIR path |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def list_installed(): | ||
| """List installed checkpoints and their sizes.""" | ||
| checkpoint_dir = _resolve_checkpoint_dir(None) |
Copilot
AI
Dec 5, 2025
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.
The list-installed command now removes the --checkpoint-dir option that was previously available. This means users can no longer specify which checkpoint directory to inspect - it will always use the default directory. This is inconsistent with the install command which still accepts --checkpoint-dir. Consider keeping this option for consistency and to allow users to inspect specific directories.
| def list_installed(): | |
| """List installed checkpoints and their sizes.""" | |
| checkpoint_dir = _resolve_checkpoint_dir(None) | |
| def list_installed( | |
| checkpoint_dir: Optional[Path] = typer.Option( | |
| None, | |
| "--checkpoint-dir", | |
| help="Directory to inspect for installed checkpoints", | |
| ), | |
| ): | |
| """List installed checkpoints and their sizes.""" | |
| checkpoint_dir = _resolve_checkpoint_dir(checkpoint_dir) |
README.md
Outdated
| foundry install base-models --checkpoint-dir <path/to/ckpt/dir> | ||
| ``` | ||
| This will download all the models supported (including multiple checkpoints of RF3) but as a beginner you can start with: | ||
| You can also download all of the models supported (including multiple checkpoints of RF3) with `all` instead of `base-models`, which installs the latest RFD3, RF3 and MPNN variants. |
Copilot
AI
Dec 5, 2025
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.
The description says "which installs the latest RFD3, RF3 and MPNN variants" but this is inconsistent with the code change in download_checkpoints.py line 158 where all is changed to install list(REGISTERED_CHECKPOINTS.keys()). Without seeing what's in REGISTERED_CHECKPOINTS, we can't verify this claim. Additionally, the previous line (158) shows that base-models includes "rfd3", "proteinmpnn", "ligandmpnn", "rf3" - not just "MPNN variants" but specifically proteinmpnn and ligandmpnn. Consider clarifying that all installs all registered checkpoints, which may include more than just the base models.
| You can also download all of the models supported (including multiple checkpoints of RF3) with `all` instead of `base-models`, which installs the latest RFD3, RF3 and MPNN variants. | |
| You can also download all of the models supported (including multiple checkpoints of RF3 and any other registered models) with `all` instead of `base-models`, which installs all registered checkpoints. |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Addresses also #46