Skip to content

Conversation

@Ubiquinone-dot
Copy link
Collaborator

@Ubiquinone-dot Ubiquinone-dot commented Dec 5, 2025

Addresses also #46

@Ubiquinone-dot Ubiquinone-dot marked this pull request as ready for review December 5, 2025 03:29
Copilot AI review requested due to automatic review settings December 5, 2025 03:29
Copilot finished reviewing on behalf of Ubiquinone-dot December 5, 2025 03:31
Copy link
Contributor

Copilot AI left a 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 a base-models installation 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.

Comment on lines +192 to +194
def list_installed():
"""List installed checkpoints and their sizes."""
checkpoint_dir = _resolve_checkpoint_dir(None)
Copy link

Copilot AI Dec 5, 2025

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.
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.
Copy link

Copilot AI Dec 5, 2025

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.

Suggested change
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.

Copilot uses AI. Check for mistakes.
@Ubiquinone-dot Ubiquinone-dot merged commit 798fd9c into production Dec 5, 2025
1 check passed
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.

2 participants