feature/SOF 7901#318
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a Pyodide-compatible MACE model loader, parameterizes MACE model selection in JupyterLite notebooks, expands pre-commit notebook coverage, adds Plotly progress and device-flow UI tweaks, and delivers a full local MACE-MP relaxation workflow notebook integrating auth, relaxation, analysis, and optional persistence. ChangesMACE Pyodide Integration and JupyterLite Updates
Supporting Library Enhancements
New Relax Structure (MACE-MP) Notebook
Sequence Diagram(s) sequenceDiagram
participant Notebook as relax_structure_with_mace.ipynb
participant Helper as get_mace_model_pyodide
participant ModelMap as MODEL_PATHS_MAP
participant mace_mp as mace_mp (import)
participant Calculator as MACECalculator
Notebook->>mace_mp: import using MACE_MODEL (non-Pyodide)
Note right of mace_mp: returns mace_mp factory
Notebook->>Helper: request(model=MACE_MODEL, dispersion=True) (Pyodide path)
Helper->>ModelMap: validate model name
ModelMap-->>Helper: model_path
Helper->>Calculator: construct MACECalculator(model_path, dispersion, dtype, device)
Calculator-->>Helper: configured_calculator
Helper-->>Notebook: calculator instance
Estimated code review effort: Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.15.13)other/experiments/jarvis/import_material_from_jarvis_db_entry.ipynbUnexpected end of JSON input other/experiments/mtrsm/calculate_energy_for_vacancy_mattersim.ipynbUnexpected end of JSON input other/experiments/jarvis/run_job_using_material_from_jarvis_db.ipynbUnexpected end of JSON input
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: 1
🤖 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 `@src/py/mat3ra/notebooks_utils/pyodide/packages/mace.py`:
- Around line 10-14: The function get_mace_model_pyodide currently defines its
first parameter as model_name which mismatches callers expecting model and will
raise a TypeError; rename the parameter model_name to model (and update any
internal references like MODEL_PATHS_MAP lookup and the variable used to build
model_path) so the signature is def get_mace_model_pyodide(model: str,
dispersion=False, default_dtype="float32", device="cpu") and it still returns
MACECalculator(model_path=model_path, ...); ensure any local variable names and
docstring/comments reflect the new parameter name.
🪄 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: ecdfea09-ffe8-4c9d-a7ef-09307f6cffe6
📒 Files selected for processing (2)
other/experiments/jupyterlite/relax_structure_with_mace.ipynbsrc/py/mat3ra/notebooks_utils/pyodide/packages/mace.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
other/experiments/jupyterlite/relax_structure_with_mace.ipynb (1)
150-151: ⚡ Quick winAPI parameters are compatible; consider clarifying variable naming to avoid shadowing.
The conditional swap works correctly—
get_mace_model_pyodideaccepts all the parameters being passed (model,dispersion,default_dtype,device). The parameter names align exactly with the function signature.For improved readability, consider using a distinct variable name instead of shadowing the
mace_mpimport:♻️ Suggested refactor
-mace_mp = mace_mp if not is_pyodide_environment() else get_mace_model_pyodide -calculator = mace_mp(model=MACE_MODEL, dispersion=True, default_dtype="float32", device="cpu") +model_factory = get_mace_model_pyodide if is_pyodide_environment() else mace_mp +calculator = model_factory(model=MACE_MODEL, dispersion=True, default_dtype="float32", device="cpu")🤖 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 `@other/experiments/jupyterlite/relax_structure_with_mace.ipynb` around lines 150 - 151, The current code conditionally reassigns the imported symbol mace_mp to get_mace_model_pyodide which shadows the original import; change the conditional to produce a new factory name (e.g., mace_factory or mace_constructor) instead of overwriting mace_mp, then instantiate the calculator via that factory (pass MACE_MODEL, dispersion=True, default_dtype="float32", device="cpu") to preserve clarity and avoid shadowing the mace_mp import and keep usage consistent where calculator is created.other/materials_designer/workflows/relax_structure_mlp.ipynb (1)
284-284: ⚡ Quick winReplace deprecated
set_calculator()with direct.calcproperty assignment.ASE deprecated
set_calculator()in version 3.20.0 in favor of direct property assignment. Update all three occurrences in this notebook:♻️ Proposed fixes
-atoms_relaxed.set_calculator(calculator) +atoms_relaxed.calc = calculator-ase_original_structure.set_calculator(calculator) +ase_original_structure.calc = calculator-atoms.set_calculator(calc) +atoms.calc = calc🤖 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 `@other/materials_designer/workflows/relax_structure_mlp.ipynb` at line 284, Replace the deprecated set_calculator call with direct calculator property assignment: locate each occurrence of atoms_relaxed.set_calculator(calculator) (three places in this notebook) and change them to assign the calculator to the .calc property (atoms_relaxed.calc = calculator) so ASE 3.20+ uses the supported API.
🤖 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 `@other/materials_designer/workflows/relax_structure_mlp.ipynb`:
- Around line 413-419: The computation of effective_delta_relaxed can raise
ZeroDivisionError if any of get_global_number_of_atoms() returns 0 (n_interface,
n_substrate, or n_layer); add a guard before performing the divisions in the
block that computes effective_delta_relaxed: check n_interface, n_substrate and
n_layer for zero and handle explicitly (for example, raise a clear ValueError
mentioning which tag/part has zero atoms or return/assign NaN) so the code using
substrate_relaxed, layer_relaxed and the effective_delta_relaxed calculation
fails with an informative message instead of a ZeroDivisionError.
- Line 271: Three calls to the deprecated ASE method set_calculator() must be
replaced with direct assignment to the .calc attribute: find each occurrence of
set_calculator(calc) (e.g., atoms.set_calculator(calc) or
structure.set_calculator(mace_calc)) in this notebook (likely in
relax_structure_mlp or related cell functions) and change them to assign the
calculator object to the .calc attribute (atoms.calc = calc or structure.calc =
mace_calc); leave the calculator object creation (e.g., get_mace_model_pyodide
or any MACE/ASE calculator constructors) untouched and only change the setter
calls to the .calc assignment so the code is compatible with ASE 3.20+.
---
Nitpick comments:
In `@other/experiments/jupyterlite/relax_structure_with_mace.ipynb`:
- Around line 150-151: The current code conditionally reassigns the imported
symbol mace_mp to get_mace_model_pyodide which shadows the original import;
change the conditional to produce a new factory name (e.g., mace_factory or
mace_constructor) instead of overwriting mace_mp, then instantiate the
calculator via that factory (pass MACE_MODEL, dispersion=True,
default_dtype="float32", device="cpu") to preserve clarity and avoid shadowing
the mace_mp import and keep usage consistent where calculator is created.
In `@other/materials_designer/workflows/relax_structure_mlp.ipynb`:
- Line 284: Replace the deprecated set_calculator call with direct calculator
property assignment: locate each occurrence of
atoms_relaxed.set_calculator(calculator) (three places in this notebook) and
change them to assign the calculator to the .calc property (atoms_relaxed.calc =
calculator) so ASE 3.20+ uses the supported API.
🪄 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: d5e630e1-60d5-42e6-999f-0d31e40b0b1a
📒 Files selected for processing (4)
.pre-commit-config.yamlother/experiments/jupyterlite/relax_structure_with_mace.ipynbother/materials_designer/workflows/relax_structure_mlp.ipynbsrc/py/mat3ra/notebooks_utils/pyodide/packages/mace.py
✅ Files skipped from review due to trivial changes (1)
- .pre-commit-config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/py/mat3ra/notebooks_utils/pyodide/packages/mace.py
| "source": [ | ||
| "from mat3ra.notebooks_utils.ipython.plot._plotly import create_realtime_plot, create_update_callback\n", | ||
| "from mat3ra.notebooks_utils.primitive.environment import is_pyodide_environment\n", | ||
| "from mat3ra.notebooks_utils.pyodide.packages.mace import get_mace_model_pyodide\n", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if get_mace_model_pyodide has Pyodide-specific dependencies
fd -t f 'mace.py' --exec cat {} \; | head -50Repository: Exabyte-io/api-examples
Length of output: 795
🏁 Script executed:
#!/bin/bash
# Extract the notebook JSON and look at the code around lines 271
# First, check the file structure
file other/materials_designer/workflows/relax_structure_mlp.ipynb
# Get the file size to determine how to read it
wc -l other/materials_designer/workflows/relax_structure_mlp.ipynbRepository: Exabyte-io/api-examples
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Read the notebook file (it's JSON) and extract relevant cells
# Look for the import statement and its context
cat other/materials_designer/workflows/relax_structure_mlp.ipynb | python <<'EOF'
import json
import sys
data = json.load(sys.stdin)
cells = data.get('cells', [])
# Look for cells containing the get_mace_model_pyodide import
for i, cell in enumerate(cells):
if cell.get('cell_type') == 'code':
source = ''.join(cell.get('source', []))
if 'get_mace_model_pyodide' in source or 'mace_mp' in source:
print(f"=== CELL {i} ===")
print(source)
print()
EOFRepository: Exabyte-io/api-examples
Length of output: 788
🌐 Web query:
ASE calculator assignment deprecated set_calculator method
💡 Result:
In the Atomic Simulation Environment (ASE), the set_calculator method has been deprecated since version 3.20.0 [1]. Users are advised to use the.calc attribute directly to attach or update a calculator on an Atoms object [1][2]. To attach a calculator, you should assign it to the calc property: atoms.calc = calculator Similarly, to retrieve the currently attached calculator, you should access the.calc property: calculator = atoms.calc This change was implemented to simplify the interface by treating the calculator as a property of the Atoms object rather than requiring explicit method calls [3][1]. Using the.calc attribute is the standard, recommended approach in modern versions of ASE [1][2].
Citations:
- 1: https://ase-lib.org/ase/atoms.html
- 2: https://ase-lib.org/examples_generated/gettingstarted/01-atoms-and-calculators.html
- 3: https://listserv.fysik.dtu.dk/pipermail/ase-users/2020-May/005523.html
🏁 Script executed:
#!/bin/bash
# Try a different approach - use jq to parse the notebook
# Extract cells that contain 'get_mace_model_pyodide' or 'mace_mp'
jq -r '.cells[] | select(.cell_type == "code") | select((.source | join("")) | contains("get_mace_model_pyodide") or contains("mace_mp")) | .source | join("")' other/materials_designer/workflows/relax_structure_mlp.ipynbRepository: Exabyte-io/api-examples
Length of output: 1516
🏁 Script executed:
#!/bin/bash
# Check if the pyodide packages module exists and what's in it
fd -t d 'pyodide' other/materials_designer/ --exec find {} -type f -name '*.py' \;
# Also search for how this module is imported elsewhere
rg 'from mat3ra.notebooks_utils.pyodide' --type py
rg 'from mat3ra.notebooks_utils.pyodide' --type ipynbRepository: Exabyte-io/api-examples
Length of output: 213
🏁 Script executed:
#!/bin/bash
# Check the structure of mat3ra packages
fd -type f -path '*/mat3ra/notebooks_utils/*' | head -20Repository: Exabyte-io/api-examples
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Search for other instances of set_calculator in the repository
rg 'set_calculator' --type ipynb -A 2 -B 2Repository: Exabyte-io/api-examples
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search for set_calculator in all files (not just by type)
rg 'set_calculator' -A 2 -B 2Repository: Exabyte-io/api-examples
Length of output: 9284
🏁 Script executed:
#!/bin/bash
# Look for pyodide imports in Python files and notebooks
rg 'from mat3ra.notebooks_utils.pyodide' -A 2 -B 2Repository: Exabyte-io/api-examples
Length of output: 3700
🏁 Script executed:
#!/bin/bash
# Check how the pyodide module is structured in mat3ra
fd 'pyodide' -type dRepository: Exabyte-io/api-examples
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Search for how is_pyodide_environment is used with imports
rg 'is_pyodide_environment' -A 5 -B 2Repository: Exabyte-io/api-examples
Length of output: 8276
🏁 Script executed:
#!/bin/bash
# Search for the definition of pyodide packages modules
fd 'pyodide' --type d | head -10
# Look for mace.py in pyodide structure
find . -path '*/pyodide/packages/mace.py' -o -path '*/pyodide/packages/mace/*'Repository: Exabyte-io/api-examples
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Check if mat3ra.notebooks_utils.pyodide is a real importable module
rg 'def get_mace_model_pyodide' -B 5 -A 10Repository: Exabyte-io/api-examples
Length of output: 1305
🏁 Script executed:
#!/bin/bash
# Look at the structure - where are pyodide packages defined?
rg 'packages.*mace' | grep -E '(def|import|__init__)' | head -20Repository: Exabyte-io/api-examples
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Verify that set_calculator appears exactly 3 times in the notebook
rg 'set_calculator' other/materials_designer/workflows/relax_structure_mlp.ipynb | wc -lRepository: Exabyte-io/api-examples
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Get context around each set_calculator call in the notebook
rg 'set_calculator' other/materials_designer/workflows/relax_structure_mlp.ipynb -B 1 -A 1Repository: Exabyte-io/api-examples
Length of output: 486
Replace set_calculator() with direct .calc assignment (ASE 3.20+ deprecation).
ASE has deprecated set_calculator() in favor of direct attribute assignment. Update the three occurrences:
Fix: Use `.calc` instead of `set_calculator()`
- atoms_relaxed.set_calculator(calculator)
+ atoms_relaxed.calc = calculator- ase_original_structure.set_calculator(calculator)
+ ase_original_structure.calc = calculator def calculate_energy(atoms, calc):
- atoms.set_calculator(calc)
+ atoms.calc = calc
return atoms.get_total_energy()The import of get_mace_model_pyodide is safe—the module exists in the codebase and is importable in all environments.
🤖 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 `@other/materials_designer/workflows/relax_structure_mlp.ipynb` at line 271,
Three calls to the deprecated ASE method set_calculator() must be replaced with
direct assignment to the .calc attribute: find each occurrence of
set_calculator(calc) (e.g., atoms.set_calculator(calc) or
structure.set_calculator(mace_calc)) in this notebook (likely in
relax_structure_mlp or related cell functions) and change them to assign the
calculator object to the .calc attribute (atoms.calc = calc or structure.calc =
mace_calc); leave the calculator object creation (e.g., get_mace_model_pyodide
or any MACE/ASE calculator constructors) untouched and only change the setter
calls to the .calc assignment so the code is compatible with ASE 3.20+.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@other/materials_designer/workflows/local/relaxation_mlff_mace.ipynb`:
- Around line 198-200: The code assumes platform_matches has at least one
element when using platform_matches[0] during Material creation; if
load_material_from_folder(FOLDER, MATERIAL_NAME) returns falsy and
client.materials.list(...) yields an empty list this raises IndexError. Fix by
checking platform_matches for truthiness before indexing: if platform_matches is
non-empty use Material.create(platform_matches[0]), otherwise handle the empty
case (e.g., raise a clear ValueError/RuntimeError mentioning MATERIAL_NAME and
ACCOUNT_ID or create a new standalone Material) so the code never directly
indexes platform_matches when it may be empty.
In `@src/py/mat3ra/notebooks_utils/ipython/ui.py`:
- Around line 178-179: The anchor tag built using verification_uri_complete in
src/py/mat3ra/notebooks_utils/ipython/ui.py opens a new tab with target='_blank'
but lacks rel='noopener noreferrer'; update the HTML string that constructs the
link (the f-string containing verification_uri_complete and "Click here if the
browser tab did not open automatically") to add rel="noopener noreferrer" to the
<a> tag to mitigate window.opener/security risks.
🪄 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: 89d1fed3-8966-4b48-a810-70cfbaee992e
📒 Files selected for processing (4)
other/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/local/relaxation_mlff_mace.ipynbsrc/py/mat3ra/notebooks_utils/ipython/plot/_plotly.pysrc/py/mat3ra/notebooks_utils/ipython/ui.py
✅ Files skipped from review due to trivial changes (1)
- other/materials_designer/workflows/Introduction.ipynb
| "platform_matches = client.materials.list({\"name\": MATERIAL_NAME, \"owner._id\": ACCOUNT_ID})\n", | ||
| "material = load_material_from_folder(FOLDER, MATERIAL_NAME) or Material.create(platform_matches[0])\n", | ||
| "\n", |
There was a problem hiding this comment.
Handle empty platform search results before indexing.
Line 199 can crash with IndexError when local load fails and platform_matches is empty.
Suggested fix
platform_matches = client.materials.list({"name": MATERIAL_NAME, "owner._id": ACCOUNT_ID})
-material = load_material_from_folder(FOLDER, MATERIAL_NAME) or Material.create(platform_matches[0])
+material = load_material_from_folder(FOLDER, MATERIAL_NAME)
+if material is None:
+ if not platform_matches:
+ raise ValueError(
+ f'No material named "{MATERIAL_NAME}" found in "{FOLDER}" or in account "{ACCOUNT_ID}".'
+ )
+ material = Material.create(platform_matches[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 `@other/materials_designer/workflows/local/relaxation_mlff_mace.ipynb` around
lines 198 - 200, The code assumes platform_matches has at least one element when
using platform_matches[0] during Material creation; if
load_material_from_folder(FOLDER, MATERIAL_NAME) returns falsy and
client.materials.list(...) yields an empty list this raises IndexError. Fix by
checking platform_matches for truthiness before indexing: if platform_matches is
non-empty use Material.create(platform_matches[0]), otherwise handle the empty
case (e.g., raise a clear ValueError/RuntimeError mentioning MATERIAL_NAME and
ACCOUNT_ID or create a new standalone Material) so the code never directly
indexes platform_matches when it may be empty.
| f"<a href='{verification_uri_complete}' target='_blank' style='color: #1976d2;'>" | ||
| f"Click here if the browser tab did not open automatically</a>" |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to the anchor tag.
When using target='_blank', it's a security best practice to include rel="noopener noreferrer" to prevent the opened page from accessing the window.opener property. While the risk is minimal in this OIDC context (the verification URI comes from a trusted identity provider), this attribute provides defense-in-depth protection.
🔒 Proposed fix
-f"<a href='{verification_uri_complete}' target='_blank' style='color: `#1976d2`;'>"
+f"<a href='{verification_uri_complete}' target='_blank' rel='noopener noreferrer' style='color: `#1976d2`;'>"🤖 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 `@src/py/mat3ra/notebooks_utils/ipython/ui.py` around lines 178 - 179, The
anchor tag built using verification_uri_complete in
src/py/mat3ra/notebooks_utils/ipython/ui.py opens a new tab with target='_blank'
but lacks rel='noopener noreferrer'; update the HTML string that constructs the
link (the f-string containing verification_uri_complete and "Click here if the
browser tab did not open automatically") to add rel="noopener noreferrer" to the
<a> tag to mitigate window.opener/security risks.
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores
UX