Include combustion in submission flow. Add more NequIP-OAM-L benchmark results - #92
Include combustion in submission flow. Add more NequIP-OAM-L benchmark results#92chiang-yuan wants to merge 17 commits into
NequIP-OAM-L benchmark results#92Conversation
…stogram bin edge clipping bug
…bmit.py and report.py
…l instantiation refactoring
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughUpdates combustion benchmark execution to a calculator-based flow with Hub input, adds submission/reporting wiring and docs, refreshes benchmark notebooks and registry metadata, and changes Streamlit sizing plus stability histogram handling. ChangesCombustion benchmark flow and infrastructure
Streamlit rendering and stability display updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 (1)
.gitignore (1)
5-20: ⚡ Quick winReconsider narrowing the benchmark ignore policy.
This replaces the previously broad benchmark ignore behavior with selective patterns, which increases the chance of accidentally tracking benchmark-generated artifacts outside the listed globs.
Based on learnings, the repository explicitly treats the broad
benchmarks/ignore rule as intentional to avoid tracking temporary benchmark artifacts, with explicit opt-in (git add -f) when needed.🤖 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 @.gitignore around lines 5 - 20, The current gitignore patterns are too selective for the benchmarks directory, only ignoring specific files and increasing the risk of accidentally tracking temporary benchmark artifacts. Restore a broad ignore rule for the entire benchmarks/ directory as the default policy, and keep the existing negation patterns (lines starting with !) to explicitly opt-in for specific files that should be tracked. This approach aligns with the repository's intention to avoid temporary benchmark artifacts by default while allowing explicit inclusion of important files via git add -f when needed.Source: Learnings
🤖 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 `@benchmarks/combustion/README.md`:
- Around line 5-15: The example code in the combustion README references
undefined variables run_dir and model without defining them first, making the
snippet non-runnable. Add variable initialization before the hydrogen_combustion
function call to make the example self-contained. Either define run_dir as a
directory path string and model as an MLIPEnum name or model name string, or
replace these undefined references with explicit example values that demonstrate
proper usage.
In `@benchmarks/README.md`:
- Around line 13-26: The README.md file contains absolute file paths using
file:/// URIs pointing to machine-specific directories like
/pscratch/sd/c/cyrusyc/mlip-arena/. These paths will break for other users or
machines. Replace all occurrences of the full
file:///pscratch/sd/c/cyrusyc/mlip-arena/benchmarks/submit.py links with
repository-relative paths that reference benchmarks/submit.py instead, so the
documentation works across different environments and machines.
In `@benchmarks/report.py`:
- Around line 35-40: The broad exception handler in the except block (catching
all Exception types) silently swallows errors when attempting to resolve the
model family from REGISTRY, making it difficult to debug actual failures.
Instead of using a bare pass statement, either catch only the specific
exceptions that are expected during REGISTRY lookup (such as KeyError) or add
logging to capture what error occurred. This way, real failures can be properly
diagnosed rather than masked by the silent fallback to family="custom".
In `@benchmarks/stability/aggregate.py`:
- Around line 18-23: The bare except block that catches all exceptions when
iterating through REGISTRY.items() and populating family_map silently suppresses
errors, making the family_map incomplete and causing scans to redirect to wrong
directories. Replace the silent pass statement with proper error handling that
logs the exception details, so developers can identify and debug REGISTRY lookup
failures. Keep the exception handling in place to prevent the program from
crashing, but ensure errors are visible through logging rather than hidden.
In `@benchmarks/submit.py`:
- Around line 136-157: The combustion function accepts a calculator_kwargs
parameter but fails to forward it when calling
hydrogen_combustion.with_options(). Add calculator_kwargs to the invocation of
hydrogen_combustion on the line where it is called with run_dir and calculator
parameters. Pass calculator_kwargs as an additional keyword argument in the
hydrogen_combustion call so that custom calculator configuration is properly
propagated to the downstream flow.
In `@serve/tools/ptable.py`:
- Line 122: The st.bokeh_chart(p) call on line 122 needs to include
width="stretch" parameter to restore the container-filling behavior that was
previously provided by use_container_width. Add width="stretch" as a parameter
to the st.bokeh_chart() function call to ensure the chart fills the available
space and maintains consistency with the rest of the migration.
---
Nitpick comments:
In @.gitignore:
- Around line 5-20: The current gitignore patterns are too selective for the
benchmarks directory, only ignoring specific files and increasing the risk of
accidentally tracking temporary benchmark artifacts. Restore a broad ignore rule
for the entire benchmarks/ directory as the default policy, and keep the
existing negation patterns (lines starting with !) to explicitly opt-in for
specific files that should be tracked. This approach aligns with the
repository's intention to avoid temporary benchmark artifacts by default while
allowing explicit inclusion of important files via git add -f when needed.
🪄 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: 62b58e76-38a3-49ef-80d0-1408cb8af0ca
⛔ Files ignored due to path filters (3)
benchmarks/eos_bulk/NequIP-OAM-L_processed.parquetis excluded by!**/*.parquetbenchmarks/stability/nequip/NequIP-OAM-L-compression.parquetis excluded by!**/*.parquetbenchmarks/stability/nequip/NequIP-OAM-L-heating.parquetis excluded by!**/*.parquet
📒 Files selected for processing (27)
.gitignoreCONTRIBUTING.mdbenchmarks/README.mdbenchmarks/combustion/README.mdbenchmarks/combustion/run.ipynbbenchmarks/report.pybenchmarks/stability/aggregate.pybenchmarks/submit.pydocs/development.mdxdocs/quickstart.mdxmlip_arena/flows/combustion.pymlip_arena/flows/stability.pymlip_arena/models/registry.yamlmlip_arena/tasks/combustion/H256O128.extxyzserve/leaderboard.pyserve/ranks/combustion.pyserve/ranks/eos_bulk.pyserve/ranks/ev.pyserve/ranks/homonuclear-diatomics.pyserve/ranks/stability.pyserve/ranks/thermal-conductivity.pyserve/tasks/eos_bulk.pyserve/tasks/ev.pyserve/tasks/homonuclear-diatomics.pyserve/tasks/stability.pyserve/tasks/thermal-conductivity.pyserve/tools/ptable.py
💤 Files with no reviewable changes (1)
- mlip_arena/tasks/combustion/H256O128.extxyz
| ```python | ||
| from mlip_arena.tasks.combustion.flow import hydrogen_combustion | ||
| from mlip_arena.flows.combustion import hydrogen_combustion | ||
|
|
||
| # Run the flow | ||
| hydrogen_combustion( | ||
| run_dir=run_dir, | ||
| calculator=model, # MLIPEnum name, model name string, or ASE calculator object | ||
| ) | ||
| ``` | ||
| . See [run.ipynb](./run.ipynb) for detials. | ||
|
|
||
| See the [run.ipynb](./run.ipynb) notebook for details. |
There was a problem hiding this comment.
Make the example self-contained.
run_dir and model are both referenced but never introduced here, so the snippet won’t run as written. Add the missing setup or replace them with explicit example values.
🛠 Suggested fix
+from pathlib import Path
+
+run_dir = Path("./combustion")
+model = "NequIP-OAM-L"
+
from mlip_arena.flows.combustion import hydrogen_combustion🤖 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/combustion/README.md` around lines 5 - 15, The example code in the
combustion README references undefined variables run_dir and model without
defining them first, making the snippet non-runnable. Add variable
initialization before the hydrogen_combustion function call to make the example
self-contained. Either define run_dir as a directory path string and model as an
MLIPEnum name or model name string, or replace these undefined references with
explicit example values that demonstrate proper usage.
| from mlip_arena.models import REGISTRY | ||
|
|
||
| with open(benchmarks_dir.parent / "mlip_arena" / "models" / "registry.yaml", "r") as f: | ||
| registry = yaml.safe_load(f) | ||
| if model_name in registry: | ||
| family = registry[model_name].get("family", "custom").lower() | ||
| if model_name in REGISTRY: | ||
| family = REGISTRY[model_name].get("family", "custom").lower() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Avoid silent fallback on registry resolution errors.
Line [39]-[40] swallows all exceptions and silently uses family="custom". That can produce false missing-file warnings and mask real failures.
Suggested fix
- except Exception:
- pass
+ except Exception as e:
+ warnings.warn(f"Failed to resolve model family from REGISTRY: {e}. Falling back to 'custom'.")🤖 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/report.py` around lines 35 - 40, The broad exception handler in
the except block (catching all Exception types) silently swallows errors when
attempting to resolve the model family from REGISTRY, making it difficult to
debug actual failures. Instead of using a bare pass statement, either catch only
the specific exceptions that are expected during REGISTRY lookup (such as
KeyError) or add logging to capture what error occurred. This way, real failures
can be properly diagnosed rather than masked by the silent fallback to
family="custom".
| from mlip_arena.models import REGISTRY | ||
|
|
||
| with open(DATA_DIR.parent / "mlip_arena" / "models" / "registry.yaml", "r") as f: | ||
| registry = yaml.safe_load(f) | ||
| for k, v in registry.items(): | ||
| family_map[k] = v.get("family", "custom").lower() | ||
| for k, v in REGISTRY.items(): | ||
| family_map[k] = v.get("family", "custom").lower() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Do not suppress REGISTRY lookup failures silently.
Line [22]-[23] hides errors and leaves family_map incomplete, which can redirect scans to wrong directories.
Suggested fix
- except Exception:
- pass
+ except Exception as e:
+ warnings.warn(f"Failed to load REGISTRY for family mapping: {e}. Falling back to 'custom'.")🤖 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/aggregate.py` around lines 18 - 23, The bare except
block that catches all exceptions when iterating through REGISTRY.items() and
populating family_map silently suppresses errors, making the family_map
incomplete and causing scans to redirect to wrong directories. Replace the
silent pass statement with proper error handling that logs the exception
details, so developers can identify and debug REGISTRY lookup failures. Keep the
exception handling in place to prevent the program from crashing, but ensure
errors are visible through logging rather than hidden.
| @flow | ||
| def combustion(calculator: str | BaseCalculator, calculator_kwargs: dict | None = None): | ||
| ctx = FlowRunContext.get() | ||
| parent_task_runner = ctx.task_runner | ||
|
|
||
| model_name = get_model_name(calculator) | ||
|
|
||
| from mlip_arena.models import REGISTRY, MLIPEnum | ||
|
|
||
| if hasattr(MLIPEnum, model_name): | ||
| family = REGISTRY[model_name]["family"] | ||
| else: | ||
| family = "custom" | ||
| family = family.lower() | ||
|
|
||
| run_dir_combustion = Path(__file__).parent / "combustion" / family | ||
|
|
||
| hydrogen_combustion.with_options( | ||
| name=f"combustion-{model_name}", | ||
| task_runner=parent_task_runner, | ||
| )(run_dir=run_dir_combustion, calculator=calculator) | ||
|
|
There was a problem hiding this comment.
Forward calculator_kwargs when delegating to hydrogen_combustion.
Line [137] accepts calculator_kwargs, but Line [156] drops it. Custom calculator configuration never reaches the combustion flow.
Suggested fix
hydrogen_combustion.with_options(
name=f"combustion-{model_name}",
task_runner=parent_task_runner,
- )(run_dir=run_dir_combustion, calculator=calculator)
+ )(
+ run_dir=run_dir_combustion,
+ calculator=calculator,
+ calculator_kwargs=calculator_kwargs,
+ )🤖 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 136 - 157, The combustion function accepts
a calculator_kwargs parameter but fails to forward it when calling
hydrogen_combustion.with_options(). Add calculator_kwargs to the invocation of
hydrogen_combustion on the line where it is called with run_dir and calculator
parameters. Pass calculator_kwargs as an additional keyword argument in the
hydrogen_combustion call so that custom calculator configuration is properly
propagated to the downstream flow.
| p.js_on_event("tap", callback) | ||
|
|
||
| st.bokeh_chart(p, use_container_width=True) | ||
| st.bokeh_chart(p) |
There was a problem hiding this comment.
Restore stretch sizing for the Bokeh render call.
At Line 122, dropping use_container_width without adding width="stretch" makes this page inconsistent with the rest of the migration and can shrink the chart instead of filling available space.
Suggested fix
-st.bokeh_chart(p)
+st.bokeh_chart(p, width="stretch")📝 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.
| st.bokeh_chart(p) | |
| st.bokeh_chart(p, width="stretch") |
🤖 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/tools/ptable.py` at line 122, The st.bokeh_chart(p) call on line 122
needs to include width="stretch" parameter to restore the container-filling
behavior that was previously provided by use_container_width. Add
width="stretch" as a parameter to the st.bokeh_chart() function call to ensure
the chart fills the available space and maintains consistency with the rest of
the migration.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 @.gitignore:
- Around line 6-12: The broad benchmarks/ ignore is fine, but the negated
artifact patterns cannot take effect unless Git is first allowed back into the
parent directories. Update the .gitignore rules so the benchmarks hierarchy is
reopened before the specific !benchmarks/... exceptions, ensuring the
combustion/parquet JSON and parquet artifacts remain trackable while preserving
the intended ignore default.
In `@benchmarks/c2db/analysis.ipynb`:
- Line 98: The mask logic in the analysis notebook is incorrect because `not
stabilities` is not valid for NumPy arrays and `stabilities is None` is only a
scalar check, so the row selection can fail or collapse to a single value.
Update the affected cells that build the stability/confusion-matrix/F1 filters
to use proper elementwise NumPy/pandas masking on `stabilities` instead of
Python boolean operators, and make sure the expressions referenced by the
stability-related summaries select rows by per-element truth values rather than
object identity.
- Around line 188-191: The mask logic in the benchmarking notebook is using
identity checks (`stabilities_sorted is None` / `stabilities is None`) that
always evaluate to a scalar, so the subsequent `~` produces the wrong index
behavior. Update the metric preparation in the blocks around `y_true`/`y_pred`
to use a real elementwise boolean filter or remove the mask entirely if it is
not needed, and make the same correction in the second matching block that uses
`stabilities`.
In `@benchmarks/energy_conservation/run.ipynb`:
- Around line 145-146: The benchmark notebook is passing an undefined variable
to the flow invocation, so update the cells that call the flow to use the
already defined `structures` variable instead of `traj`. Locate the
`structures=` argument in the notebook cells around the `MACE-MPA` flow usage
and make both invocations consistent with the `structures` assignment created
earlier in the notebook.
- Around line 109-110: The notebook has executed cell outputs embedded in
run.ipynb, including environment-specific paths, workspace URLs, and stack
traces, so clear all outputs before committing. Remove the runtime output from
the affected cells and any other executed cells with the same issue, especially
the benchmark/logging cells in run.ipynb, so the notebook only contains the
reproducible benchmark code and not transient execution artifacts.
- Around line 38-41: The benchmark notebook is using a hardcoded
machine-specific input path, so it won’t run outside the author’s environment.
Update the data-loading setup around data_dir, sample_size, and
random_sample_from_directory to resolve the dataset path relative to the
repository or read it from a parameter/environment variable instead of a local
filesystem path. Apply the same fix to the later notebook cell that repeats this
pattern so all inputs are portable.
- Around line 239-240: The UMA probe cell is eagerly initializing Warp/CUDA via
MLIPEnum["UMA-S-1P1"]() and can crash the notebook on CPU-only or driver-less
machines. Update the notebook cell to either guard that call behind a CUDA
availability check before invoking the enum entry, or remove the probe entirely
if it is only meant as a smoke test. Keep the rest of the benchmark workflow
runnable even when UMA is unavailable.
- Around line 142-143: The TaskRunner setup for
`differential_entropy_along_nve_trajectory.with_options` can pass
`max_workers=0` when `torch.cuda.device_count()` is zero, which breaks Prefect
startup on CPU-only machines. Update the `ThreadPoolTaskRunner` configuration to
always supply at least 1 worker by clamping the CUDA device count or branching
on CPU vs GPU so the flow remains runnable in both environments.
In `@benchmarks/stability/plot.ipynb`:
- Around line 122-124: Update the throughput metrics in the benchmark summary
logic so undefined `steps_per_second` is represented as `NaN` instead of `0`
when `total_time_seconds` is zero. Keep the existing `seconds_per_step` and
`seconds_per_step_per_atom` handling in the same block, and adjust the code that
builds the metrics dict in `plot.ipynb` so the plot pipeline can naturally drop
invalid values before regression.
In `@benchmarks/stability/pressure.ipynb`:
- Around line 131-135: The notebook cell around the
run_one.with_options(...).submit() call is swallowing submission failures and
continuing, which can leave the benchmark with missing model/material pairs.
Update the try/except handling so enqueue errors are surfaced or recorded
explicitly with the affected atoms/model pair, rather than being ignored; use
the existing futures collection and submit loop to track failures. Make sure the
final output/report clearly distinguishes skipped submissions from real
benchmark results.
🪄 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: 095fb345-8a93-4b82-a0a4-abb794a86586
📒 Files selected for processing (17)
.gitignorebenchmarks/README.mdbenchmarks/bzo/dft.ipynbbenchmarks/bzo/run.ipynbbenchmarks/c2db/analysis.ipynbbenchmarks/combustion/nequip/NequIP-OAM-L_H256O128.jsonbenchmarks/diatomics/run.ipynbbenchmarks/energy_conservation/run.ipynbbenchmarks/eos_alloy/run_Fe-Ni-Cr.ipynbbenchmarks/eos_bulk/run.pybenchmarks/mof/classification/analysis.ipynbbenchmarks/mof/golddac.ipynbbenchmarks/stability/plot.ipynbbenchmarks/stability/pressure.ipynbbenchmarks/stability/temperature.ipynbserve/tasks/combustion.pytests/test_combustion.py
✅ Files skipped from review due to trivial changes (7)
- benchmarks/eos_bulk/run.py
- benchmarks/README.md
- benchmarks/diatomics/run.ipynb
- benchmarks/stability/temperature.ipynb
- benchmarks/bzo/run.ipynb
- benchmarks/mof/golddac.ipynb
- benchmarks/mof/classification/analysis.ipynb
👮 Files not reviewed due to content moderation or server errors (1)
- benchmarks/eos_alloy/run_Fe-Ni-Cr.ipynb
| benchmarks/ | ||
| !benchmarks/c2db/*.parquet | ||
| !benchmarks/**/*_processed.parquet | ||
| !benchmarks/stability/*-heating.parquet | ||
| !benchmarks/stability/*-compression.parquet | ||
| !benchmarks/combustion/**/*.json | ||
| !benchmarks/diatomics/**/*.json |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
These negated patterns will not re-include files under benchmarks/.
Once benchmarks/ is ignored, Git stops descending into it, so the later !benchmarks/... rules never make the combustion/parquet artifacts trackable. That breaks the new artifact contract used by report generation.
Suggested fix
benchmarks/
+!benchmarks/
+!benchmarks/**/
!benchmarks/c2db/*.parquet
!benchmarks/**/*_processed.parquet
!benchmarks/stability/*-heating.parquet
!benchmarks/stability/*-compression.parquet
!benchmarks/combustion/**/*.json
!benchmarks/diatomics/**/*.jsonBased on learnings, the broad benchmarks/ ignore is intentional; the issue here is that Git will not unignore descendants of an ignored parent unless the directories are reopened first.
📝 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.
| benchmarks/ | |
| !benchmarks/c2db/*.parquet | |
| !benchmarks/**/*_processed.parquet | |
| !benchmarks/stability/*-heating.parquet | |
| !benchmarks/stability/*-compression.parquet | |
| !benchmarks/combustion/**/*.json | |
| !benchmarks/diatomics/**/*.json | |
| benchmarks/ | |
| !benchmarks/ | |
| !benchmarks/**/ | |
| !benchmarks/c2db/*.parquet | |
| !benchmarks/**/*_processed.parquet | |
| !benchmarks/stability/*-heating.parquet | |
| !benchmarks/stability/*-compression.parquet | |
| !benchmarks/combustion/**/*.json | |
| !benchmarks/diatomics/**/*.json |
🤖 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 @.gitignore around lines 6 - 12, The broad benchmarks/ ignore is fine, but
the negated artifact patterns cannot take effect unless Git is first allowed
back into the parent directories. Update the .gitignore rules so the benchmarks
hierarchy is reopened before the specific !benchmarks/... exceptions, ensuring
the combustion/parquet JSON and parquet artifacts remain trackable while
preserving the intended ignore default.
Source: Learnings
| "stabilities = np.array(stabilities)\n", | ||
| "\n", | ||
| "(stabilities == True).sum(), (stabilities == False).sum(), (stabilities == None).sum()" | ||
| "(stabilities).sum(), (not stabilities).sum(), (stabilities is None).sum()" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
path = Path('benchmarks/c2db/analysis.ipynb')
print('exists', path.exists(), 'size', path.stat().st_size)
nb = json.loads(path.read_text())
for i, cell in enumerate(nb.get('cells', [])):
src = ''.join(cell.get('source', []))
if 'stabilities' in src or 'None' in src or 'confusion' in src or 'f1' in src:
print(f'\n--- cell {i} ({cell.get("cell_type")}) ---')
print(src)
PYRepository: atomind-ai/mlip-arena
Length of output: 7572
Fix the NumPy mask logic. not stabilities is invalid for an ndarray, and stabilities is None is a scalar check, so this can either raise immediately or collapse the filter to a single index. The same pattern in the confusion-matrix/F1 cells will also select the wrong rows.
🤖 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/c2db/analysis.ipynb` at line 98, The mask logic in the analysis
notebook is incorrect because `not stabilities` is not valid for NumPy arrays
and `stabilities is None` is only a scalar check, so the row selection can fail
or collapse to a single value. Update the affected cells that build the
stability/confusion-matrix/F1 filters to use proper elementwise NumPy/pandas
masking on `stabilities` instead of Python boolean operators, and make sure the
expressions referenced by the stability-related summaries select rows by
per-element truth values rather than object identity.
| " mask = ~(stabilities_sorted is None)\n", | ||
| "\n", | ||
| " y_true = stabilities_sorted[mask].astype(\"int\")\n", | ||
| " y_pred = sorted_df[\"dyn_stab\"][mask].fillna(-1).astype(\"int\")\n", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
import json, pathlib, re
path = pathlib.Path("benchmarks/c2db/analysis.ipynb")
nb = json.loads(path.read_text())
for i, cell in enumerate(nb.get("cells", [])):
if cell.get("cell_type") != "code":
continue
src = "".join(cell.get("source", []))
if "stabilities_sorted" in src or "stabilities" in src:
print(f"\n=== cell {i} ===")
for j, line in enumerate(src.splitlines(), 1):
if "stabilities_sorted" in line or "stabilities" in line or "mask =" in line:
print(f"{j:03d}: {line}")
PYRepository: atomind-ai/mlip-arena
Length of output: 767
🏁 Script executed:
python3 - <<'PY'
import json, pathlib
path = pathlib.Path("benchmarks/c2db/analysis.ipynb")
nb = json.loads(path.read_text())
for idx in [4, 5]:
cell = nb["cells"][idx]
print(f"\n=== cell {idx} ===")
for j, line in enumerate("".join(cell.get("source", [])).splitlines(), 1):
print(f"{j:03d}: {line}")
PYRepository: atomind-ai/mlip-arena
Length of output: 6197
Use a real boolean mask here. stabilities_sorted is None / stabilities is None is always False for these NumPy arrays, so ~False becomes -1 and the metric code ends up indexing a single row instead of filtering. Remove the mask or replace it with an elementwise missing-value check; same issue in the second block.
🤖 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/c2db/analysis.ipynb` around lines 188 - 191, The mask logic in the
benchmarking notebook is using identity checks (`stabilities_sorted is None` /
`stabilities is None`) that always evaluate to a scalar, so the subsequent `~`
produces the wrong index behavior. Update the metric preparation in the blocks
around `y_true`/`y_pred` to use a real elementwise boolean filter or remove the
mask entirely if it is not needed, and make the same correction in the second
matching block that uses `stabilities`.
| "data_dir = \"./christine/mlip-arena/mptrj\"\n", | ||
| "sample_size = 5000\n", | ||
| "\n", | ||
| "random_files = random_sample_from_directory(data_dir, sample_size)\n", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
These paths are machine-specific, so the notebook will not run for anyone else.
./christine/mlip-arena/... is tied to one local filesystem. The committed benchmark notebook should resolve inputs relative to the repo or accept them as parameters/env vars; otherwise the first data-loading cells fail immediately outside the author's environment.
Also applies to: 62-62
🤖 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/energy_conservation/run.ipynb` around lines 38 - 41, The benchmark
notebook is using a hardcoded machine-specific input path, so it won’t run
outside the author’s environment. Update the data-loading setup around data_dir,
sample_size, and random_sample_from_directory to resolve the dataset path
relative to the repository or read it from a parameter/environment variable
instead of a local filesystem path. Apply the same fix to the later notebook
cell that repeats this pattern so all inputs are portable.
| "\u001b[32m2026-03-26 23:10:14.079\u001b[0m | \u001b[1mINFO \u001b[0m | \u001b[36mmlip_arena.models\u001b[0m:\u001b[36m_make_mlip_enum\u001b[0m:\u001b[36m69\u001b[0m - \u001b[1mSkipping deprecated models: ['eqV2(OMat)', 'eSEN', 'EquiformerV2(OC22)', 'EquiformerV2(OC20)', 'eSCN(OC20)', 'ORB']\u001b[0m\n", | ||
| "\u001b[32m2026-03-26 23:10:14.082\u001b[0m | \u001b[1mINFO \u001b[0m | \u001b[36mmlip_arena.models\u001b[0m:\u001b[36m_make_mlip_enum\u001b[0m:\u001b[36m71\u001b[0m - \u001b[1mRegistered models: ['MACE-MP(M)', 'CHGNet', 'M3GNet', 'MatterSim', 'ORBv2', 'SevenNet', 'MACE-MPA', 'MACE-OFF(M)', 'ANI2x', 'ALIGNN', 'DeepMD', 'NequIP-OAM-L', 'UMA-S-1P1']\u001b[0m\n" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Clear the executed outputs before committing this notebook.
These outputs embed absolute /pscratch/... paths, a Prefect Cloud workspace URL, and full environment-specific stack traces. That leaks internal environment details and makes the notebook diff noisy without adding reproducible benchmark logic.
Also applies to: 315-319, 329-333, 588-691
🤖 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/energy_conservation/run.ipynb` around lines 109 - 110, The
notebook has executed cell outputs embedded in run.ipynb, including
environment-specific paths, workspace URLs, and stack traces, so clear all
outputs before committing. Remove the runtime output from the affected cells and
any other executed cells with the same issue, especially the benchmark/logging
cells in run.ipynb, so the notebook only contains the reproducible benchmark
code and not transient execution artifacts.
| "differential_entropy_along_nve_trajectory.with_options(\n", | ||
| " task_runner=ThreadPoolTaskRunner(max_workers=torch.cuda.device_count()), persist_result=False\n", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
import json
from pathlib import Path
path = Path("benchmarks/energy_conservation/run.ipynb")
nb = json.loads(path.read_text())
targets = {"differential_entropy_along_nve_trajectory", "ThreadPoolTaskRunner", "device_count"}
for i, cell in enumerate(nb.get("cells", [])):
src = "".join(cell.get("source", []))
if any(t in src for t in targets):
print(f"\n--- CELL {i} ({cell.get('cell_type')}) ---")
for n, line in enumerate(cell.get("source", []), 1):
if any(t in line for t in targets):
print(f"{n:03d}: {line.rstrip()}")
PYRepository: atomind-ai/mlip-arena
Length of output: 852
🏁 Script executed:
python3 - <<'PY'
import json
from pathlib import Path
path = Path("benchmarks/energy_conservation/run.ipynb")
nb = json.loads(path.read_text())
for idx in [5, 8]:
cell = nb["cells"][idx]
print(f"\n=== CELL {idx} ({cell.get('cell_type')}) ===")
for n, line in enumerate(cell.get("source", []), 1):
print(f"{n:03d}: {line.rstrip()}")
PYRepository: atomind-ai/mlip-arena
Length of output: 1314
🏁 Script executed:
python3 - <<'PY'
import inspect
try:
from prefect.task_runners import ThreadPoolTaskRunner
print("ThreadPoolTaskRunner signature:", inspect.signature(ThreadPoolTaskRunner))
try:
print("\nSource snippet:\n")
print(inspect.getsource(ThreadPoolTaskRunner)[:4000])
except Exception as e:
print("Could not read source:", type(e).__name__, e)
except Exception as e:
print("Could not import ThreadPoolTaskRunner:", type(e).__name__, e)
try:
import torch
print("\ntorch.cuda.device_count():", torch.cuda.device_count())
except Exception as e:
print("\nCould not import torch:", type(e).__name__, e)
PYRepository: atomind-ai/mlip-arena
Length of output: 313
🌐 Web query:
Prefect ThreadPoolTaskRunner max_workers 0 behavior source code
💡 Result:
In Prefect, the max_workers parameter for the ThreadPoolTaskRunner is designed to be a positive integer [1]. Specifically, the underlying settings model in Prefect enforces a constraint where thread_pool_max_workers must be greater than zero (gt=0) [1]. Passing a value of 0 to max_workers is not a supported or valid configuration for the ThreadPoolTaskRunner. If you attempt to set this value via configuration settings, the Pydantic-based validation logic will reject it because it fails the gt=0 validation rule [1]. If you pass 0 directly to the constructor in Python code, the behavior is undefined by the official API, as the expected type is a positive integer or None (which defaults to sys.maxsize) [2][1]. For controlling task execution concurrency, you should always use a positive integer value representing the number of threads you wish to utilize [3][4].
Citations:
- 1: https://github.com/PrefectHQ/prefect/blob/d6d16486/src/prefect/settings/models/tasks.py
- 2: https://github.com/PrefectHQ/prefect/blob/d6d16486/src/prefect/task_runners.py
- 3: https://docs.prefect.io/v3/api-ref/python/prefect-task_runners
- 4: https://docs.prefect.io/v3/concepts/task-runners
Clamp ThreadPoolTaskRunner workers to a positive value. torch.cuda.device_count() can return 0 on CPU-only hosts, but Prefect requires max_workers > 0, so this flow will fail to start there. Use max(1, torch.cuda.device_count()) or branch on CPU vs GPU.
🤖 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/energy_conservation/run.ipynb` around lines 142 - 143, The
TaskRunner setup for `differential_entropy_along_nve_trajectory.with_options`
can pass `max_workers=0` when `torch.cuda.device_count()` is zero, which breaks
Prefect startup on CPU-only machines. Update the `ThreadPoolTaskRunner`
configuration to always supply at least 1 worker by clamping the CUDA device
count or branching on CPU vs GPU so the flow remains runnable in both
environments.
| " model=\"MACE-MPA\",\n", | ||
| " structures=traj,\n", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Use the defined structures variable here.
The notebook creates structures in Line 12, but both flow invocations pass traj, which is never assigned in this notebook. These cells will fail with NameError before the benchmark runs.
Suggested fix
- structures=traj,
+ structures=structures,
...
- structures=traj[:5],
+ structures=structures[:5],Also applies to: 764-765
🤖 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/energy_conservation/run.ipynb` around lines 145 - 146, The
benchmark notebook is passing an undefined variable to the flow invocation, so
update the cells that call the flow to use the already defined `structures`
variable instead of `traj`. Locate the `structures=` argument in the notebook
cells around the `MACE-MPA` flow usage and make both invocations consistent with
the `structures` assignment created earlier in the notebook.
| "source": [ | ||
| "MLIPEnum[\"UMA-S-1P1\"]()" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
import json
from pathlib import Path
p = Path('benchmarks/energy_conservation/run.ipynb')
nb = json.loads(p.read_text())
for i, cell in enumerate(nb.get('cells', [])):
src = ''.join(cell.get('source', []))
if 'MLIPEnum["UMA-S-1P1"]()' in src or 'UMA-S-1P1' in src:
print(f'CELL {i} type={cell.get("cell_type")}')
print('SOURCE:')
print(src)
print('OUTPUTS:')
for out in cell.get('outputs', []):
print(out.get('output_type'), out.get('ename'), out.get('evalue'))
txt = ''.join(out.get('text', [])) if isinstance(out.get('text'), list) else out.get('text')
if txt:
print(txt)
if 'traceback' in out:
print('\n'.join(out['traceback']))
print('-' * 80)
PYRepository: atomind-ai/mlip-arena
Length of output: 22285
Guard or remove the UMA probe cell. MLIPEnum["UMA-S-1P1"]() initializes Warp/CUDA during import and raises RuntimeError on CPU-only or driver-less machines, which stops the notebook before the rest of the workflow runs. Gate it behind a CUDA check or drop it if it’s only a smoke test.
🤖 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/energy_conservation/run.ipynb` around lines 239 - 240, The UMA
probe cell is eagerly initializing Warp/CUDA via MLIPEnum["UMA-S-1P1"]() and can
crash the notebook on CPU-only or driver-less machines. Update the notebook cell
to either guard that call behind a CUDA availability check before invoking the
enum entry, or remove the probe entirely if it is only meant as a smoke test.
Keep the rest of the benchmark workflow runnable even when UMA is unavailable.
| " \"steps_per_second\": total_steps / total_time_seconds if total_time_seconds != 0 else 0,\n", | ||
| " \"seconds_per_step\": total_time_seconds / total_steps if total_steps != 0 else float(\"inf\"),\n", | ||
| " \"seconds_per_step_per_atom\": total_time_seconds / total_steps / natoms if total_steps != 0 else float(\"inf\"),\n", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use NaN for undefined throughput instead of 0.
Lines 122-124 turn a zero-duration run into steps_per_second = 0, but both benchmark plots use a log y-axis and only drop NaN before fitting. That makes missing/invalid timing metadata look like a real measurement and can skew the regression or disappear silently from the chart.
Proposed fix
- "steps_per_second": total_steps / total_time_seconds if total_time_seconds != 0 else 0,
- "seconds_per_step": total_time_seconds / total_steps if total_steps != 0 else float("inf"),
- "seconds_per_step_per_atom": total_time_seconds / total_steps / natoms if total_steps != 0 else float("inf"),
+ "steps_per_second": total_steps / total_time_seconds if total_time_seconds != 0 else np.nan,
+ "seconds_per_step": total_time_seconds / total_steps if total_steps != 0 else np.nan,
+ "seconds_per_step_per_atom": total_time_seconds / total_steps / natoms if total_steps != 0 else np.nan,📝 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.
| " \"steps_per_second\": total_steps / total_time_seconds if total_time_seconds != 0 else 0,\n", | |
| " \"seconds_per_step\": total_time_seconds / total_steps if total_steps != 0 else float(\"inf\"),\n", | |
| " \"seconds_per_step_per_atom\": total_time_seconds / total_steps / natoms if total_steps != 0 else float(\"inf\"),\n", | |
| " \"steps_per_second\": total_steps / total_time_seconds if total_time_seconds != 0 else np.nan,\n", | |
| " \"seconds_per_step\": total_time_seconds / total_steps if total_steps != 0 else np.nan,\n", | |
| " \"seconds_per_step_per_atom\": total_time_seconds / total_steps / natoms if total_steps != 0 else np.nan,\n", |
🤖 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/plot.ipynb` around lines 122 - 124, Update the
throughput metrics in the benchmark summary logic so undefined
`steps_per_second` is represented as `NaN` instead of `0` when
`total_time_seconds` is zero. Keep the existing `seconds_per_step` and
`seconds_per_step_per_atom` handling in the same block, and adjust the code that
builds the metrics dict in `plot.ipynb` so the plot pipeline can naturally drop
invalid values before regression.
| " future = run_one.with_options(timeout_seconds=600, retries=2, refresh_cache=False).submit(\n", | ||
| " atoms.copy(), model\n", | ||
| " )\n", | ||
| " futures.append(future)\n", | ||
| " except:\n", | ||
| " except Exception:\n", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't silently drop failed submissions.
Lines 131-135 suppress enqueue failures and keep going, so this notebook can finish with an incomplete result set and no indication which model/material pairs were skipped. That makes downstream stability plots/reporting indistinguishable from genuine benchmark failures.
Proposed fix
- except Exception:
- continue
+ except Exception as exc:
+ print(
+ f"Failed to submit {model.name} for "
+ f"{atoms.info.get('material_id', 'unknown')}: {exc}"
+ )
+ continue📝 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.
| " future = run_one.with_options(timeout_seconds=600, retries=2, refresh_cache=False).submit(\n", | |
| " atoms.copy(), model\n", | |
| " )\n", | |
| " futures.append(future)\n", | |
| " except:\n", | |
| " except Exception:\n", | |
| " future = run_one.with_options(timeout_seconds=600, retries=2, refresh_cache=False).submit(\n", | |
| " atoms.copy(), model\n", | |
| " )\n", | |
| " futures.append(future)\n", | |
| " except Exception as exc:\n", | |
| " print(\n", | |
| " f\"Failed to submit {model.name} for \"\n", | |
| " f\"{atoms.info.get('material_id', 'unknown')}: {exc}\"\n", | |
| " )\n", | |
| " continue\n", |
🤖 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/pressure.ipynb` around lines 131 - 135, The notebook
cell around the run_one.with_options(...).submit() call is swallowing submission
failures and continuing, which can leave the benchmark with missing
model/material pairs. Update the try/except handling so enqueue errors are
surfaced or recorded explicitly with the affected atoms/model pair, rather than
being ignored; use the existing futures collection and submit loop to track
failures. Make sure the final output/report clearly distinguishes skipped
submissions from real benchmark results.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarks/report.py (1)
27-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFamily resolution diverges from
submit.pyfor deprecated models, risking wrong artifact path lookups.
submit.py'scombustionflow computesfamilyviahasattr(MLIPEnum, model_name), andMLIPEnumis built from REGISTRY.items() by iterating and skipping entries where meta.get("deprecated", False) is true. So for a deprecated model,submit.pyfalls back tofamily = "custom"and writes outputs underbenchmarks/combustion/custom/....Here,
familyis resolved viaif model_name in REGISTRY:— this check isTrueeven for deprecated models, sinceREGISTRYitself (loaded directly fromregistry.yaml) still contains deprecated entries. This means for a deprecated model,report.pywould resolve the actual family (not"custom") and look for the combustion/stability outputs under the wrong directory, producing a spurious "missing" warning even thoughsubmit.pycorrectly wrote the file undercustom.Align the resolution logic with
submit.py(e.g., check againstMLIPEnummembership, or otherwise account for deprecated entries) to keep writer/reader path contracts consistent.🤖 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/report.py` around lines 27 - 44, The family lookup in the report generation flow is inconsistent with the submission flow for deprecated models, which can make artifact searches point at the wrong directory. Update the family resolution in report.py’s model_name handling to match submit.py’s logic by using MLIPEnum membership (or otherwise excluding deprecated REGISTRY entries) so deprecated models fall back to custom. Keep the existing path construction for p_stability and p_combustion unchanged once the resolved family is corrected.
🤖 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.
Outside diff comments:
In `@benchmarks/report.py`:
- Around line 27-44: The family lookup in the report generation flow is
inconsistent with the submission flow for deprecated models, which can make
artifact searches point at the wrong directory. Update the family resolution in
report.py’s model_name handling to match submit.py’s logic by using MLIPEnum
membership (or otherwise excluding deprecated REGISTRY entries) so deprecated
models fall back to custom. Keep the existing path construction for p_stability
and p_combustion unchanged once the resolved family is corrected.
1 participant
Summary by CodeRabbit