Refactor CI test environments - #87
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors the calculator parameter API across Prefect flows, adds PET model support with device-aware wrappers, consolidates stability benchmark orchestration into a unified flow, updates CI to split tests by model group with conditional dependency installation, and adds file-existence validation for served benchmark artifacts. ChangesModel parameter refactoring, PET model addition, and stability benchmark consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
mlip_arena/models/externals/pet.py (2)
19-20: ⚡ Quick winRemove redundant
calculatemethod override.The
calculatemethod is a pure delegation to the parent class with no additional behavior. Unless there's a specific reason to override (e.g., future customization or documentation), this method can be removed.♻️ Proposed simplification
- def calculate(self, atoms=None, properties=["energy", "forces", "stress"], system_changes=all_changes): - super().calculate(atoms, properties, system_changes)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlip_arena/models/externals/pet.py` around lines 19 - 20, The calculate method in pet.py is a redundant override that only calls super().calculate with the same signature (def calculate(self, atoms=None, properties=["energy", "forces", "stress"], system_changes=all_changes): super().calculate(atoms, properties, system_changes)); remove this method entirely so the class inherits the parent implementation directly (no other changes needed unless you intend to add custom behavior later).
32-33: ⚡ Quick winRemove redundant
calculatemethod override.Same as the
PET_OAMclass above, this method override adds no value and can be removed.♻️ Proposed simplification
- def calculate(self, atoms=None, properties=["energy", "forces", "stress"], system_changes=all_changes): - super().calculate(atoms, properties, system_changes)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlip_arena/models/externals/pet.py` around lines 32 - 33, The calculate override in class PET (the def calculate(self, atoms=None, properties=["energy", "forces", "stress"], system_changes=all_changes) method) is redundant and should be removed; delete this method so the class inherits the base implementation (same simplification as PET_OAM) and ensure no other code relies on this explicit override signature or default mutable default arguments—if callers depend on different defaults, adjust call sites instead of keeping the redundant method.benchmarks/submit.py (1)
80-87: 💤 Low valueRemove or document the commented-out code.
The
distribution_shiftsexecution block is commented out. If this workflow is permanently disabled, remove the dead code. Otherwise, add a comment explaining why it's temporarily disabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/submit.py` around lines 80 - 87, The commented-out distribution_shifts invocation (the block using distribution_shifts.with_options, DaskTaskRunner, and the calculator/calculator_kwargs parameters) is dead code; either remove it if the workflow is permanently disabled or add a brief inline comment above the block explaining why it is temporarily disabled and when/how it will be re-enabled (e.g., note dependency or environment reason), and ensure you keep references to DaskTaskRunner, persist_result, calculator, and calculator_kwargs in the comment so future readers know what the block does.tests/test_external_calculators.py (1)
12-28: 💤 Low valueConsider using a mapping for cleaner mark assignment.
The current if/elif chain for assigning pytest marks could be simplified with a mapping dictionary, improving maintainability as more models/marks are added.
♻️ Optional refactor using a mapping
+mark_map = { + "SevenNet": [pytest.mark.sevennet], + "NequIP-OAM-L": [pytest.mark.nequip], +} +family_mark_map = { + "fairchem": [pytest.mark.fairchem], +} + model_params = [] for model in MLIPEnum: - marks = [] - if model.name == "SevenNet": - marks.append(pytest.mark.sevennet) - elif model.name == "NequIP-OAM-L": - marks.append(pytest.mark.nequip) - elif "MACE" in model.name: - marks.append(pytest.mark.mace) - elif model.value.get("family") == "fairchem": - marks.append(pytest.mark.fairchem) + marks = mark_map.get(model.name, []).copy() + if "MACE" in model.name: + marks.append(pytest.mark.mace) + family = model.value.get("family") + if family in family_mark_map: + marks.extend(family_mark_map[family]) if marks: model_params.append(pytest.param(model, marks=marks, id=model.name))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_external_calculators.py` around lines 12 - 28, Replace the if/elif chain that assigns pytest marks for each MLIPEnum entry with a lookup mapping: define a dict mapping either model.name strings or predicates to pytest marks (e.g., {"SevenNet": pytest.mark.sevennet, "NequIP-OAM-L": pytest.mark.nequip, "MACE": pytest.mark.mace, "fairchem": pytest.mark.fairchem}) and then for each model in MLIPEnum build marks by checking the mapping keys (use substring check for "MACE" and family lookup for "fairchem") and append matching marks to marks list before creating pytest.param; update the logic around model_params, marks, and pytest.param to use the mapping so adding new model-to-mark rules only requires updating the dict.benchmarks/stability.py (1)
60-60: 💤 Low valueClarify or document the hardcoded structure limits.
The slice limits
[:120]for heating and[:80]for compression are marked "tentatively," suggesting these are experimental values. If these are intended as permanent limits, document the rationale; otherwise, consider making them configurable.Also applies to: 68-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/stability.py` at line 60, The hardcoded slice limits on compositions (e.g., df = df[df["formula"].isin(compositions[:120])].copy() and the similar compositions[:80] for the other case) are experimental; either document the rationale for using 120 and 80 in a comment or make these values configurable (e.g., add constants or function parameters like HEATING_LIMIT and COMPRESSION_LIMIT or pass max_structures into the enclosing function) and use those named variables instead of literal slices so the limits are explicit and easily changed. Ensure you update any docstring or top-level comments to mention the defaults and why those numbers were chosen if you keep them hardcoded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 16-17: Replace mutable action tags with specific commit SHAs and
disable credential persistence on checkout steps: update the "Checkout code"
step which currently uses "uses: actions/checkout@v4" (and the other checkout
step that uses the same) to reference the exact commit SHA for actions/checkout
and add "persist-credentials: false" to the step; likewise replace other
floating tags (e.g., actions referenced as `@v6/`@v3 etc.) with their audited
commit SHAs so every "uses:" entry is pinned to a specific SHA to prevent
supply-chain tampering.
In `@scripts/install-linux.sh`:
- Around line 14-15: The DGL find-links URL (the command "uv pip install dgl -f
https://data.dgl.ai/wheels/torch-${TORCH}/${CUDA}/repo.html" and its duplicates)
is returning 403 in CI; update the install step to use a reachable source and
add a fallback: try the existing data.dgl.ai find-links first, and if it fails
then fall back to "pip install dgl" (PyPI) or another known-working wheel URL
for the current TORCH/CUDA combo; apply the same change to the other occurrences
(the duplicate lines at the other blocks), and keep the TORCH and CUDA variable
interpolation intact so the script still targets the correct wheel when
available.
In `@serve/ranks/combustion.py`:
- Around line 11-16: The list comprehension building valid_models calls
metadata.get("family").lower() which will raise if family is missing; update the
comprehension to guard that family exists before calling lower (e.g. require
metadata.get("family") truthy first and then use metadata["family"].lower() or
metadata.get("family").lower()) so models with no family are skipped and the
DATA_DIR / ... .exists() check only runs when family is present; refer to
valid_models, MODELS, metadata, and DATA_DIR when making the change.
In `@serve/ranks/homonuclear-diatomics.py`:
- Around line 11-16: The list comprehension building valid_models uses
metadata.get("family") directly which can be None and cause Path / None
TypeError; update the comprehension (valid_models, MODELS, metadata) to first
ensure a valid family string exists (e.g., check metadata.get("family") is
truthy or is instance of str) before constructing the path and checking
.exists(), so short-circuit when family is missing and skip that model.
In `@tests/test_data_integrity.py`:
- Around line 47-56: MODELS[model].get("family") can be None causing Path(...)
to raise; after assigning family in the block where fpath is built (the branch
on rank_module_name), add a guard that if family is falsy/None you skip this
model (e.g., continue the loop) or set fpath to None and handle it downstream;
update the code around the family assignment and the fpath construction so that
functions/variables referenced (MODELS, family, rank_module_name, fpath) do not
attempt Path(...) when family is missing.
- Around line 44-58: The fallback logic currently adds models when fpath is None
(unknown rank_module_name), which wrongly includes all models; update the loop
that builds filtered_expected_models so it only appends a model when a concrete
path was computed and that path exists—i.e. check fpath is not None AND
fpath.exists()—so unknown rank_module_name cases are skipped; modify the
condition around the append to use that check and leave the rest of the fpath
construction in the block using MODELS, expected_models, and rank_module_name.
---
Nitpick comments:
In `@benchmarks/stability.py`:
- Line 60: The hardcoded slice limits on compositions (e.g., df =
df[df["formula"].isin(compositions[:120])].copy() and the similar
compositions[:80] for the other case) are experimental; either document the
rationale for using 120 and 80 in a comment or make these values configurable
(e.g., add constants or function parameters like HEATING_LIMIT and
COMPRESSION_LIMIT or pass max_structures into the enclosing function) and use
those named variables instead of literal slices so the limits are explicit and
easily changed. Ensure you update any docstring or top-level comments to mention
the defaults and why those numbers were chosen if you keep them hardcoded.
In `@benchmarks/submit.py`:
- Around line 80-87: The commented-out distribution_shifts invocation (the block
using distribution_shifts.with_options, DaskTaskRunner, and the
calculator/calculator_kwargs parameters) is dead code; either remove it if the
workflow is permanently disabled or add a brief inline comment above the block
explaining why it is temporarily disabled and when/how it will be re-enabled
(e.g., note dependency or environment reason), and ensure you keep references to
DaskTaskRunner, persist_result, calculator, and calculator_kwargs in the comment
so future readers know what the block does.
In `@mlip_arena/models/externals/pet.py`:
- Around line 19-20: The calculate method in pet.py is a redundant override that
only calls super().calculate with the same signature (def calculate(self,
atoms=None, properties=["energy", "forces", "stress"],
system_changes=all_changes): super().calculate(atoms, properties,
system_changes)); remove this method entirely so the class inherits the parent
implementation directly (no other changes needed unless you intend to add custom
behavior later).
- Around line 32-33: The calculate override in class PET (the def
calculate(self, atoms=None, properties=["energy", "forces", "stress"],
system_changes=all_changes) method) is redundant and should be removed; delete
this method so the class inherits the base implementation (same simplification
as PET_OAM) and ensure no other code relies on this explicit override signature
or default mutable default arguments—if callers depend on different defaults,
adjust call sites instead of keeping the redundant method.
In `@tests/test_external_calculators.py`:
- Around line 12-28: Replace the if/elif chain that assigns pytest marks for
each MLIPEnum entry with a lookup mapping: define a dict mapping either
model.name strings or predicates to pytest marks (e.g., {"SevenNet":
pytest.mark.sevennet, "NequIP-OAM-L": pytest.mark.nequip, "MACE":
pytest.mark.mace, "fairchem": pytest.mark.fairchem}) and then for each model in
MLIPEnum build marks by checking the mapping keys (use substring check for
"MACE" and family lookup for "fairchem") and append matching marks to marks list
before creating pytest.param; update the logic around model_params, marks, and
pytest.param to use the mapping so adding new model-to-mark rules only requires
updating the dict.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1632ccf-573d-4b7f-b6fc-2de71d3e9b2b
📒 Files selected for processing (19)
.github/workflows/ci.yamlbenchmarks/asymptotes.pybenchmarks/stability.pybenchmarks/stability/analysis.pybenchmarks/stability/run.pybenchmarks/submit.pymlip_arena/flows/diatomics.pymlip_arena/flows/eos_bulk.pymlip_arena/flows/stability.pymlip_arena/models/externals/ani.pymlip_arena/models/externals/pet.pymlip_arena/models/registry.yamlpyproject.tomlscripts/install-linux.shscripts/install-perlmutter.shserve/ranks/combustion.pyserve/ranks/homonuclear-diatomics.pytests/test_data_integrity.pytests/test_external_calculators.py
💤 Files with no reviewable changes (3)
- benchmarks/stability/analysis.py
- scripts/install-perlmutter.sh
- benchmarks/stability/run.py
| - name: Checkout code | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/ci.yamlRepository: atomind-ai/mlip-arena
Length of output: 4853
🏁 Script executed:
# Check for security scanning configuration files
find . -maxdepth 2 -type f \( -name ".pre-commit-config.yaml" -o -name ".github/codeql-analysis.yml" -o -name ".github/workflows/*security*" -o -name "*.trivy*" -o -name ".semgrep*" \) 2>/dev/null | head -20Repository: atomind-ai/mlip-arena
Length of output: 1964
🏁 Script executed:
# Look for any linting configuration that might check workflow files
find . -maxdepth 2 -type f \( -name "actionlint*" -o -name ".actionlintrc*" -o -name ".yamllint*" \) 2>/dev/nullRepository: atomind-ai/mlip-arena
Length of output: 1964
🌐 Web query:
GitHub Actions best practices pinning commit SHA vs version tags security
💡 Result:
In GitHub Actions, pinning to a full-length commit SHA is the recommended security best practice for ensuring the immutability of your CI/CD pipelines [1][2]. The primary differences and security considerations are: Commit SHA Pinning - Security: Provides maximum security by referencing a specific, immutable version of the action [1][3]. Even if a repository is compromised and a tag is moved or deleted, the pinned SHA will continue to resolve to the exact same code that you have verified [4][5]. - Enforcement: Administrators can enforce SHA pinning at the organization or repository level via GitHub Actions policies to ensure all workflows use immutable references [1][2]. - Workflow Maintenance: Because SHAs do not auto-update, you must manually update them or use automated dependency management tools like Dependabot or Renovate [2][4]. A common practice is to use a comment in the workflow file to keep track of the original version, such as uses: owner/repo@sha123... # v1.0.0; tools like Dependabot use this convention to suggest updates while maintaining the security benefit of the pin [4][5]. Version Tag Pinning - Security: Referencing a tag (e.g., @v1) is mutable and carries significant risk [5][3]. If an attacker gains access to the action's repository, they can overwrite the existing tag to point to malicious code, which would immediately affect your workflows [1][5]. - Convenience: It is more convenient for developers as it allows for automatic updates when tags are moved, but it lacks the security guarantees of an immutable reference [1][6]. - Recommendation: Only pin to tags if you fully trust the creator and the source, such as first-party GitHub-owned actions [5][6]. Even then, SHA pinning remains the superior approach for high-security environments [3]. Summary For production and high-security workflows, you should always pin actions to a full 40-character commit SHA [1][5]. This protects against supply chain attacks where a malicious actor alters a previously trusted tag [2][3]. Automated tools can help manage these updates, allowing you to benefit from both security and maintainability [4][5].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
- 3: https://www.stepsecurity.io/blog/pinning-github-actions-for-enhanced-security-a-complete-guide
- 4: https://safeguard.sh/resources/blog/github-actions-pinning-sha-vs-tag
- 5: https://safeguard.sh/resources/blog/how-to-pin-github-actions-to-shas-correctly
- 6: https://github.com/github/docs/blob/962a1c8dccb8c0f66548b324e5b921b5e4fbc3d6/content/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions.md
Pin third-party actions to commit SHAs and disable persisted checkout credentials.
Lines 17 and 20 use floating action version tags (@v4, @v6), which are mutable and can be exploited to compromise your CI pipeline. The checkout actions (lines 17 and 44) also persist credentials by default, increasing token exposure risk. Pin all actions to specific commit SHAs and disable credential persistence on checkout steps.
Suggested hardening diff
- name: Checkout code
- uses: actions/checkout@v4
+ uses: actions/checkout@<pinned-commit-sha>
+ with:
+ persist-credentials: false
- name: Install uv
- uses: astral-sh/setup-uv@v6
+ uses: astral-sh/setup-uv@<pinned-commit-sha>Also applies to checkout at line 44 and other floating-tag actions throughout the workflow.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 16-17: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 17-17: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yaml around lines 16 - 17, Replace mutable action tags
with specific commit SHAs and disable credential persistence on checkout steps:
update the "Checkout code" step which currently uses "uses: actions/checkout@v4"
(and the other checkout step that uses the same) to reference the exact commit
SHA for actions/checkout and add "persist-credentials: false" to the step;
likewise replace other floating tags (e.g., actions referenced as `@v6/`@v3 etc.)
with their audited commit SHAs so every "uses:" entry is pinned to a specific
SHA to prevent supply-chain tampering.
Source: Linters/SAST tools
| uv pip install torch-scatter torch-sparse -f https://data.pyg.org/whl/torch-${TORCH}.0+${CUDA}.html | ||
| uv pip install dgl -f https://data.dgl.ai/wheels/torch-${TORCH}/${CUDA}/repo.html |
There was a problem hiding this comment.
Use a reachable DGL install source for the Torch/CUDA combos in CI.
These lines are currently blocking CI: sevennet/mace jobs fail with HTTP 403 on the data.dgl.ai find-links URL (torch-2.8/cu128). This is a release blocker for grouped test execution.
Also applies to: 21-22, 28-29, 35-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-linux.sh` around lines 14 - 15, The DGL find-links URL (the
command "uv pip install dgl -f
https://data.dgl.ai/wheels/torch-${TORCH}/${CUDA}/repo.html" and its duplicates)
is returning 403 in CI; update the install step to use a reachable source and
add a fallback: try the existing data.dgl.ai find-links first, and if it fails
then fall back to "pip install dgl" (PyPI) or another known-working wheel URL
for the current TORCH/CUDA combo; apply the same change to the other occurrences
(the duplicate lines at the other blocks), and keep the TORCH and CUDA variable
interpolation intact so the script still targets the correct wheel when
available.
Source: Pipeline failures
| valid_models = [ | ||
| model | ||
| for model, metadata in MODELS.items() | ||
| if Path(__file__).stem in metadata.get("gpu-tasks", []) | ||
| and (DATA_DIR / metadata.get("family").lower() / f"{model}_H256O128.json").exists() | ||
| ] |
There was a problem hiding this comment.
Guard against missing family metadata.
Line 15 uses metadata.get("family").lower() in path construction. If family is missing from the model's metadata, this will raise an AttributeError (NoneType has no attribute 'lower').
🛡️ Proposed fix to add defensive check
valid_models = [
model
for model, metadata in MODELS.items()
if Path(__file__).stem in metadata.get("gpu-tasks", [])
+ and metadata.get("family") is not None
and (DATA_DIR / metadata.get("family").lower() / f"{model}_H256O128.json").exists()
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| valid_models = [ | |
| model | |
| for model, metadata in MODELS.items() | |
| if Path(__file__).stem in metadata.get("gpu-tasks", []) | |
| and (DATA_DIR / metadata.get("family").lower() / f"{model}_H256O128.json").exists() | |
| ] | |
| valid_models = [ | |
| model | |
| for model, metadata in MODELS.items() | |
| if Path(__file__).stem in metadata.get("gpu-tasks", []) | |
| and metadata.get("family") is not None | |
| and (DATA_DIR / metadata.get("family").lower() / f"{model}_H256O128.json").exists() | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@serve/ranks/combustion.py` around lines 11 - 16, The list comprehension
building valid_models calls metadata.get("family").lower() which will raise if
family is missing; update the comprehension to guard that family exists before
calling lower (e.g. require metadata.get("family") truthy first and then use
metadata["family"].lower() or metadata.get("family").lower()) so models with no
family are skipped and the DATA_DIR / ... .exists() check only runs when family
is present; refer to valid_models, MODELS, metadata, and DATA_DIR when making
the change.
| valid_models = [ | ||
| model | ||
| for model, metadata in MODELS.items() | ||
| if Path(__file__).stem in metadata.get("gpu-tasks", []) | ||
| and (DATA_DIR / metadata.get("family") / f"{model}.json").exists() | ||
| ] |
There was a problem hiding this comment.
Guard against missing family metadata.
Line 15 uses metadata.get("family") in path construction. If family is missing from the model's metadata, this will raise a TypeError when constructing the path (Path / None).
🛡️ Proposed fix to add defensive check
valid_models = [
model
for model, metadata in MODELS.items()
if Path(__file__).stem in metadata.get("gpu-tasks", [])
+ and metadata.get("family") is not None
and (DATA_DIR / metadata.get("family") / f"{model}.json").exists()
]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@serve/ranks/homonuclear-diatomics.py` around lines 11 - 16, The list
comprehension building valid_models uses metadata.get("family") directly which
can be None and cause Path / None TypeError; update the comprehension
(valid_models, MODELS, metadata) to first ensure a valid family string exists
(e.g., check metadata.get("family") is truthy or is instance of str) before
constructing the path and checking .exists(), so short-circuit when family is
missing and skip that model.
| # Filter expected models to those that have benchmark data files in the repository | ||
| filtered_expected_models = [] | ||
| for model in expected_models: | ||
| family = MODELS[model].get("family") | ||
| if rank_module_name == "homonuclear-diatomics": | ||
| fpath = Path("benchmarks/diatomics") / family / f"{model}.json" | ||
| elif rank_module_name == "combustion": | ||
| fpath = Path("benchmarks/combustion") / family.lower() / f"{model}_H256O128.json" | ||
| elif rank_module_name == "stability": | ||
| fpath = Path("benchmarks/stability") / family.lower() / f"{model}-heating.parquet" | ||
| else: | ||
| fpath = None | ||
|
|
||
| if fpath is None or fpath.exists(): | ||
| filtered_expected_models.append(model) |
There was a problem hiding this comment.
Fix fallback logic for unknown rank modules.
Line 57's condition if fpath is None or fpath.exists() includes models when fpath is None (i.e., when rank_module_name doesn't match any known pattern). This means for any new rank module not in the if/elif chain, all expected models are included regardless of whether their benchmark files exist, potentially masking missing data.
🔧 Proposed fix to handle unknown rank modules explicitly
if fpath is None or fpath.exists():
filtered_expected_models.append(model)
+ elif fpath is None:
+ pytest.fail(f"Unknown rank_module_name '{rank_module_name}' - add file path pattern")Alternative: Only include models when fpath exists:
- if fpath is None or fpath.exists():
+ if fpath is not None and fpath.exists():
filtered_expected_models.append(model)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_data_integrity.py` around lines 44 - 58, The fallback logic
currently adds models when fpath is None (unknown rank_module_name), which
wrongly includes all models; update the loop that builds
filtered_expected_models so it only appends a model when a concrete path was
computed and that path exists—i.e. check fpath is not None AND fpath.exists()—so
unknown rank_module_name cases are skipped; modify the condition around the
append to use that check and leave the rest of the fpath construction in the
block using MODELS, expected_models, and rank_module_name.
| family = MODELS[model].get("family") | ||
| if rank_module_name == "homonuclear-diatomics": | ||
| fpath = Path("benchmarks/diatomics") / family / f"{model}.json" | ||
| elif rank_module_name == "combustion": | ||
| fpath = Path("benchmarks/combustion") / family.lower() / f"{model}_H256O128.json" | ||
| elif rank_module_name == "stability": | ||
| fpath = Path("benchmarks/stability") / family.lower() / f"{model}-heating.parquet" | ||
| else: | ||
| fpath = None | ||
|
|
There was a problem hiding this comment.
Guard against missing family metadata.
Line 47 uses MODELS[model].get("family"), which could return None. Subsequent path construction on lines 49, 51, and 53 would raise a TypeError if family is None.
🛡️ Proposed fix to skip models with missing family
for model in expected_models:
family = MODELS[model].get("family")
+ if family is None:
+ continue
if rank_module_name == "homonuclear-diatomics":📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| family = MODELS[model].get("family") | |
| if rank_module_name == "homonuclear-diatomics": | |
| fpath = Path("benchmarks/diatomics") / family / f"{model}.json" | |
| elif rank_module_name == "combustion": | |
| fpath = Path("benchmarks/combustion") / family.lower() / f"{model}_H256O128.json" | |
| elif rank_module_name == "stability": | |
| fpath = Path("benchmarks/stability") / family.lower() / f"{model}-heating.parquet" | |
| else: | |
| fpath = None | |
| family = MODELS[model].get("family") | |
| if family is None: | |
| continue | |
| if rank_module_name == "homonuclear-diatomics": | |
| fpath = Path("benchmarks/diatomics") / family / f"{model}.json" | |
| elif rank_module_name == "combustion": | |
| fpath = Path("benchmarks/combustion") / family.lower() / f"{model}_H256O128.json" | |
| elif rank_module_name == "stability": | |
| fpath = Path("benchmarks/stability") / family.lower() / f"{model}-heating.parquet" | |
| else: | |
| fpath = None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_data_integrity.py` around lines 47 - 56,
MODELS[model].get("family") can be None causing Path(...) to raise; after
assigning family in the block where fpath is built (the branch on
rank_module_name), add a guard that if family is falsy/None you skip this model
(e.g., continue the loop) or set fpath to None and handle it downstream; update
the code around the family assignment and the fpath construction so that
functions/variables referenced (MODELS, family, rank_module_name, fpath) do not
attempt Path(...) when family is missing.
…' and revise CI install scripts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/install-linux.sh (1)
8-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a fallback for DGL wheel installs to prevent CI hard-failures.
Line 8/15/22/29/36 still use the
data.dgl.aiindex directly; earlier CI runs already showed 403 for some Torch/CUDA combos, so grouped test jobs can fail before tests start.Proposed minimal fix
+install_dgl() { + uv pip install dgl -f "https://data.dgl.ai/wheels/torch-${TORCH}/${CUDA}/repo.html" \ + || uv pip install dgl +} + if [ "$GROUP" == "nequip" ]; then @@ - uv pip install dgl -f https://data.dgl.ai/wheels/torch-${TORCH}/${CUDA}/repo.html + install_dgl @@ - uv pip install dgl -f https://data.dgl.ai/wheels/torch-${TORCH}/${CUDA}/repo.html + install_dgl @@ - uv pip install dgl -f https://data.dgl.ai/wheels/torch-${TORCH}/${CUDA}/repo.html + install_dgl @@ - uv pip install dgl -f https://data.dgl.ai/wheels/torch-${TORCH}/${CUDA}/repo.html + install_dgl @@ - uv pip install dgl -f https://data.dgl.ai/wheels/torch-${TORCH}/${CUDA}/repo.html + install_dglAlso applies to: 15-15, 22-22, 29-29, 36-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-linux.sh` at line 8, Replace the direct DGL wheel installs that use the data.dgl.ai index (the lines calling "pip install dgl -f https://data.dgl.ai/wheels/torch-${TORCH}/${CUDA}/repo.html") with a resilient two-step command so CI won't hard-fail on 403s: attempt the indexed install first and, if it fails, fall back to a normal PyPI install (e.g. "pip install dgl -f ... || pip install dgl"); update every occurrence of that exact command (the ones at lines with the same pattern) in scripts/install-linux.sh accordingly.Source: Pipeline failures
🧹 Nitpick comments (1)
pyproject.toml (1)
32-32: ⚡ Quick winConsolidate duplicated
pymatgenrequirement.Line 32 (
pymatgen) and Line 41 (pymatgen>=2025.1.9) are redundant; keep only the constrained entry to avoid ambiguity in dependency intent.Also applies to: 41-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` at line 32, Remove the duplicate unconstrained dependency entry "pymatgen" and retain only the constrained entry "pymatgen>=2025.1.9" in pyproject.toml so the package intent is unambiguous; locate the two entries (the plain "pymatgen" and the "pymatgen>=2025.1.9" lines) and delete the unconstrained one.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyproject.toml`:
- Around line 54-58: The test extra currently forces "torch>=2.8.0" in
pyproject.toml which can override the CI's nequip-installed torch==2.5.0 and
break prebuilt wheels; remove or relax that constraint—either delete the
"torch>=2.8.0" entry from the test extras so tests use the environment-provided
torch, or replace it with a non-conflicting range (e.g., "torch>=1.13,<2.6") if
you must declare a version; update the test extras list in pyproject.toml where
the string "torch>=2.8.0" appears.
---
Duplicate comments:
In `@scripts/install-linux.sh`:
- Line 8: Replace the direct DGL wheel installs that use the data.dgl.ai index
(the lines calling "pip install dgl -f
https://data.dgl.ai/wheels/torch-${TORCH}/${CUDA}/repo.html") with a resilient
two-step command so CI won't hard-fail on 403s: attempt the indexed install
first and, if it fails, fall back to a normal PyPI install (e.g. "pip install
dgl -f ... || pip install dgl"); update every occurrence of that exact command
(the ones at lines with the same pattern) in scripts/install-linux.sh
accordingly.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 32: Remove the duplicate unconstrained dependency entry "pymatgen" and
retain only the constrained entry "pymatgen>=2025.1.9" in pyproject.toml so the
package intent is unambiguous; locate the two entries (the plain "pymatgen" and
the "pymatgen>=2025.1.9" lines) and delete the unconstrained one.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be4e78f3-c8ea-4b8c-bb11-9636b4897467
📒 Files selected for processing (3)
mlip_arena/models/classicals/zbl.pypyproject.tomlscripts/install-linux.sh
| "torch>=2.8.0", | ||
| "pytest", | ||
| "pytest-cov", | ||
| "streamlit>=1.55.0", | ||
| ] |
There was a problem hiding this comment.
test extra’s Torch constraint conflicts with nequip CI environment.
Line 54 (torch>=2.8.0) can override the nequip branch’s torch==2.5.0 install (from scripts/install-linux.sh), which can invalidate the preinstalled torch-scatter/torch-sparse wheels built for 2.5.
Proposed fix
test = [
- "torch>=2.8.0",
"pytest",
"pytest-cov",
"streamlit>=1.55.0",
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "torch>=2.8.0", | |
| "pytest", | |
| "pytest-cov", | |
| "streamlit>=1.55.0", | |
| ] | |
| "pytest", | |
| "pytest-cov", | |
| "streamlit>=1.55.0", | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` around lines 54 - 58, The test extra currently forces
"torch>=2.8.0" in pyproject.toml which can override the CI's nequip-installed
torch==2.5.0 and break prebuilt wheels; remove or relax that constraint—either
delete the "torch>=2.8.0" entry from the test extras so tests use the
environment-provided torch, or replace it with a non-conflicting range (e.g.,
"torch>=1.13,<2.6") if you must declare a version; update the test extras list
in pyproject.toml where the string "torch>=2.8.0" appears.
…orch-scatter/torch-sparse compilation
…ement local scatter helpers, and mark MACE test workflows
cf8fcc2 to
ca06673
Compare
17d6a44 to
ba8ee45
Compare
…ase lock race conditions
….0 in pyproject.toml
…nsure isolation before prefect is imported
…or in Prefect < 3.7.0
… prefect/fastapi constraints
Summary by CodeRabbit
New Features
Chores