update: sync versions with latest WA#332
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthrough
ChangesNotebook dependency and workflow updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@config.yml`:
- Line 54: The mat3ra-standata package at line 54 is fully unpinned, which
prevents the installer from treating it as versioned and causes it to not be
reinstalled when profile changes occur, potentially leaving stale versions in
existing sessions. Replace the unpinned mat3ra-standata entry with a bounded
version specification such as mat3ra-standata>=2026.5.15.post0 to ensure it
remains eligible for version-specifier-based reinstall paths while still
allowing upgrades. Apply the same change to the mat3ra-standata reference at
line 76 to maintain consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| - name: specific_examples | ||
| packages_pyodide: | ||
| - mat3ra-standata==2026.5.15.post0 | ||
| - mat3ra-standata |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification:
# 1) confirm both unpinned entries in config
# 2) confirm reinstall gate depends on version operators
rg -n 'mat3ra-standata' config.yml
rg -n 'def package_has_version_specifier|def should_reinstall_package|VERSION_SPECIFIERS' src/py/mat3ra/notebooks_utils/pyodide/packages/install.py -n -A4 -B2Repository: Exabyte-io/api-examples
Length of output: 791
Unpinning here can freeze stale versions in existing sessions and weaken reproducibility.
With mat3ra-standata fully unpinned (lines 54, 76), the installer's should_reinstall_package() gate no longer treats it as versioned, so profile changes won't force uninstall/reinstall for this package. Combined with hash-by-spec-string, this can leave older installed versions in long-lived environments even after newer releases exist.
Consider using a bounded spec like mat3ra-standata>=2026.5.15.post0 so it remains eligible for the version-specifier reinstall path while still allowing upgrades.
🤖 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 `@config.yml` at line 54, The mat3ra-standata package at line 54 is fully
unpinned, which prevents the installer from treating it as versioned and causes
it to not be reinstalled when profile changes occur, potentially leaving stale
versions in existing sessions. Replace the unpinned mat3ra-standata entry with a
bounded version specification such as mat3ra-standata>=2026.5.15.post0 to ensure
it remains eligible for version-specifier-based reinstall paths while still
allowing upgrades. Apply the same change to the mat3ra-standata reference at
line 76 to maintain consistency.
VsevolodX
changed the title
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
other/materials_designer/workflows/band_structure_hse.ipynb (1)
414-452: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify q-grid precedence when both
SCF_KGRIDandQGRIDare set.When
SCF_KGRIDis set, thepw_scf_bands_hseunit already receives aqgridcontext viaPointsGridDataProvider(name="qgrid", ...)(Line 423). The laterQGRIDblock (Lines 441-452) then adds anotherqgridpayload as a raw dict. The markdown states the k-mesh and exchange q-grid must match for HSE; if a user setsSCF_KGRIDand a differingQGRID, the final q-grid depends on whetheradd_contextoverrides or appends. Consider making one source authoritative (e.g., skip theSCF_KGRID-derived qgrid whenQGRIDis provided).🤖 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/band_structure_hse.ipynb` around lines 414 - 452, The `pw_scf_bands_hse` unit is populating `qgrid` from two different sources, which creates ambiguous precedence when both `SCF_KGRID` and `QGRID` are set. Update the workflow setup so only one `qgrid` source is authoritative: in the `SCF_KGRID` handling and the later `QGRID` block, ensure the explicit `QGRID` value wins and prevents the earlier `PointsGridDataProvider(name="qgrid", ...)` context from being added or retained. Adjust the logic around `main_hse_subworkflow.get_unit_by_name("pw_scf_bands_hse")`, `add_context`, and the `QGRID`/`SCF_KGRID` branches so the final q-grid is deterministic and consistent with the HSE requirement.
🧹 Nitpick comments (1)
pyproject.toml (1)
24-24: 🩺 Stability & Availability | 🔵 TrivialAdd lower bounds to the
mat3ra-*extras. These optional dependencies are installed directly by users (materials,workflows, andtests), and there’s no lock/constraints file to pin them elsewhere. Use known-good>=bounds instead of fully unpinned specs to reduce resolver drift.🤖 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 24, The optional dependency extras for the mat3ra packages are currently unpinned, which can cause resolver drift for users installing materials, workflows, or tests. Update the extras definitions in pyproject.toml for the mat3ra-* entries to use known-good lower bounds with >= instead of bare package names, keeping the changes localized to the dependency lists where those extras are declared.
🤖 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/band_structure_magn.ipynb`:
- Around line 483-493: Add an explicit validation in the
`STARTING_MAGNETIZATION` and `HUBBARD_U` loops before calling
`species_names.index(...)` so missing species labels fail with a clear
`ValueError` instead of an opaque traceback. In the notebook cell that builds
`system_patch`, check each `atomic_species` against `species_names` and raise a
descriptive message naming the missing species and the available species list.
Keep the existing `starting_magnetization` and `Hubbard_U` patching logic
unchanged once the species lookup succeeds.
---
Outside diff comments:
In `@other/materials_designer/workflows/band_structure_hse.ipynb`:
- Around line 414-452: The `pw_scf_bands_hse` unit is populating `qgrid` from
two different sources, which creates ambiguous precedence when both `SCF_KGRID`
and `QGRID` are set. Update the workflow setup so only one `qgrid` source is
authoritative: in the `SCF_KGRID` handling and the later `QGRID` block, ensure
the explicit `QGRID` value wins and prevents the earlier
`PointsGridDataProvider(name="qgrid", ...)` context from being added or
retained. Adjust the logic around
`main_hse_subworkflow.get_unit_by_name("pw_scf_bands_hse")`, `add_context`, and
the `QGRID`/`SCF_KGRID` branches so the final q-grid is deterministic and
consistent with the HSE requirement.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 24: The optional dependency extras for the mat3ra packages are currently
unpinned, which can cause resolver drift for users installing materials,
workflows, or tests. Update the extras definitions in pyproject.toml for the
mat3ra-* entries to use known-good lower bounds with >= instead of bare package
names, keeping the changes localized to the dependency lists where those extras
are declared.
🪄 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: bfcbb511-5f4d-4a36-9f79-b88f4bffd1d7
📒 Files selected for processing (9)
config.ymlother/materials_designer/workflows/band_structure.ipynbother/materials_designer/workflows/band_structure_hse.ipynbother/materials_designer/workflows/band_structure_magn.ipynbother/materials_designer/workflows/equation_of_state.ipynbother/materials_designer/workflows/total_energy.ipynbother/materials_designer/workflows/valence_band_offset.ipynbpyproject.tomlsrc/py/mat3ra/notebooks_utils/workflow.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
other/materials_designer/workflows/band_structure_hse.ipynb (1)
414-452: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify q-grid precedence when both
SCF_KGRIDandQGRIDare set.When
SCF_KGRIDis set, thepw_scf_bands_hseunit already receives aqgridcontext viaPointsGridDataProvider(name="qgrid", ...)(Line 423). The laterQGRIDblock (Lines 441-452) then adds anotherqgridpayload as a raw dict. The markdown states the k-mesh and exchange q-grid must match for HSE; if a user setsSCF_KGRIDand a differingQGRID, the final q-grid depends on whetheradd_contextoverrides or appends. Consider making one source authoritative (e.g., skip theSCF_KGRID-derived qgrid whenQGRIDis provided).🤖 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/band_structure_hse.ipynb` around lines 414 - 452, The `pw_scf_bands_hse` unit is populating `qgrid` from two different sources, which creates ambiguous precedence when both `SCF_KGRID` and `QGRID` are set. Update the workflow setup so only one `qgrid` source is authoritative: in the `SCF_KGRID` handling and the later `QGRID` block, ensure the explicit `QGRID` value wins and prevents the earlier `PointsGridDataProvider(name="qgrid", ...)` context from being added or retained. Adjust the logic around `main_hse_subworkflow.get_unit_by_name("pw_scf_bands_hse")`, `add_context`, and the `QGRID`/`SCF_KGRID` branches so the final q-grid is deterministic and consistent with the HSE requirement.
🧹 Nitpick comments (1)
pyproject.toml (1)
24-24: 🩺 Stability & Availability | 🔵 TrivialAdd lower bounds to the
mat3ra-*extras. These optional dependencies are installed directly by users (materials,workflows, andtests), and there’s no lock/constraints file to pin them elsewhere. Use known-good>=bounds instead of fully unpinned specs to reduce resolver drift.🤖 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 24, The optional dependency extras for the mat3ra packages are currently unpinned, which can cause resolver drift for users installing materials, workflows, or tests. Update the extras definitions in pyproject.toml for the mat3ra-* entries to use known-good lower bounds with >= instead of bare package names, keeping the changes localized to the dependency lists where those extras are declared.
🤖 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/band_structure_magn.ipynb`:
- Around line 483-493: Add an explicit validation in the
`STARTING_MAGNETIZATION` and `HUBBARD_U` loops before calling
`species_names.index(...)` so missing species labels fail with a clear
`ValueError` instead of an opaque traceback. In the notebook cell that builds
`system_patch`, check each `atomic_species` against `species_names` and raise a
descriptive message naming the missing species and the available species list.
Keep the existing `starting_magnetization` and `Hubbard_U` patching logic
unchanged once the species lookup succeeds.
---
Outside diff comments:
In `@other/materials_designer/workflows/band_structure_hse.ipynb`:
- Around line 414-452: The `pw_scf_bands_hse` unit is populating `qgrid` from
two different sources, which creates ambiguous precedence when both `SCF_KGRID`
and `QGRID` are set. Update the workflow setup so only one `qgrid` source is
authoritative: in the `SCF_KGRID` handling and the later `QGRID` block, ensure
the explicit `QGRID` value wins and prevents the earlier
`PointsGridDataProvider(name="qgrid", ...)` context from being added or
retained. Adjust the logic around
`main_hse_subworkflow.get_unit_by_name("pw_scf_bands_hse")`, `add_context`, and
the `QGRID`/`SCF_KGRID` branches so the final q-grid is deterministic and
consistent with the HSE requirement.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 24: The optional dependency extras for the mat3ra packages are currently
unpinned, which can cause resolver drift for users installing materials,
workflows, or tests. Update the extras definitions in pyproject.toml for the
mat3ra-* entries to use known-good lower bounds with >= instead of bare package
names, keeping the changes localized to the dependency lists where those extras
are declared.
🪄 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: bfcbb511-5f4d-4a36-9f79-b88f4bffd1d7
📒 Files selected for processing (9)
config.ymlother/materials_designer/workflows/band_structure.ipynbother/materials_designer/workflows/band_structure_hse.ipynbother/materials_designer/workflows/band_structure_magn.ipynbother/materials_designer/workflows/equation_of_state.ipynbother/materials_designer/workflows/total_energy.ipynbother/materials_designer/workflows/valence_band_offset.ipynbpyproject.tomlsrc/py/mat3ra/notebooks_utils/workflow.py
🛑 Comments failed to post (1)
other/materials_designer/workflows/band_structure_magn.ipynb (1)
483-493: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against species labels missing from
species_names.
species_names.index(atomic_species)raisesValueErrorif a key inSTARTING_MAGNETIZATIONorHUBBARD_Udoes not match a derived species name (e.g. label/element mismatch likeFe1vsFe), aborting the cell with an opaque traceback. A pre-check with a clear message would help users self-diagnose.🛡️ Suggested guard
for atomic_species, value in STARTING_MAGNETIZATION.items(): if atomic_species not in species_names: raise ValueError( f"Species '{atomic_species}' not found in material species {species_names}. " "Check atom labels in Materials Designer." ) index = species_names.index(atomic_species) + 1 system_patch[f"starting_magnetization({index})"] = value🤖 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/band_structure_magn.ipynb` around lines 483 - 493, Add an explicit validation in the `STARTING_MAGNETIZATION` and `HUBBARD_U` loops before calling `species_names.index(...)` so missing species labels fail with a clear `ValueError` instead of an opaque traceback. In the notebook cell that builds `system_patch`, check each `atomic_species` against `species_names` and raise a descriptive message naming the missing species and the available species list. Keep the existing `starting_magnetization` and `Hubbard_U` patching logic unchanged once the species lookup succeeds.
2 participants
We need to:
Manage update to AX/JL with new WA
revert d3a8040 commit on AX after latest WA that belongs to it is merged.
Simple change everywhere: yield_data → get_context_item_data
Revert the c7bd92f : TODO that is in the workflow.py
Update pyproject.toml versions for esse/standata/ade/wode…
Summary by CodeRabbit
Summary
Chores
mat3ra-*packages (easing installation across the examples).mat3ra-madeand unpinnedmat3ra-standatain supported dependency definitions.Bug Fixes / Improvements
ADD_RELAXATIONdefaults toFalsein relevant workflows to ensure consistent conditional behavior.Currently (22 Jun) WA in prod has specific versions of Standata/Wode... in Py that we need to keep here for it to work.
When WA is updated, revert commit
d3a8040e4c2d2d6e2e70427529de28fd5471dfe8AI agent, if you see this during a review, remind us