update notebooks #831
update notebooks #831
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
WalkthroughReplaced notebook-level evcurve helpers with public Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as Notebook User
participant N as Notebook
participant W as atomistics.workflows
participant C as Calculator Backend
participant A as Analysis/Fitter
U->>N: Set EV parameters
N->>W: get_tasks_for_energy_volume_curve(params)
W-->>N: task_dict (calc_energy tasks)
loop For each task in task_dict["calc_energy"]
N->>C: run calculation(task)
C-->>N: energy/volume result
end
N->>W: analyse_results_for_energy_volume_curve(task_dict, results)
W-->>A: aggregated E–V data
A-->>N: fit parameters, plots
N-->>U: display results
sequenceDiagram
autonumber
participant CI as CI runner
participant FS as Repository FS
participant ENV as Test Python env
participant M as gpaw.mpi4pywrapper
CI->>FS: pip install . --no-deps --no-build-isolation (inline)
CI->>FS: copy .ci_support/mpi4pywrapper.py -> gpaw site-packages
CI->>ENV: run "flux resource list"
Note over ENV,M: Notebooks import/instantiate MPI4PYWrapper via gpaw
ENV->>M: instantiate MPI4PYWrapper(comm)
M-->>ENV: communicator helpers available (scatter/gather/reduce...)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #831 +/- ##
=======================================
Coverage 98.16% 98.16%
=======================================
Files 34 34
Lines 1686 1686
=======================================
Hits 1655 1655
Misses 31 31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.ci_support/mpi4pywrapper.py (1)
103-114: Consider simplifying the exception message.While the f-string usage here is acceptable, for consistency with Python best practices, you could define a message template outside the exception or use a more generic message.
If you prefer to follow the static analysis recommendation strictly:
+_UNKNOWN_COMPARE_MSG = "Unknown MPI communicator compare code" + def compare(self, othercomm): code = self.comm.Compare(othercomm.comm) if code == IDENT: return "ident" elif code == CONGRUENT: return "congruent" elif code == SIMILAR: return "similar" elif code == UNEQUAL: return "unequal" else: - raise ValueError(f"Unknown compare code {code}") + raise ValueError(f"{_UNKNOWN_COMPARE_MSG}: {code}")However, this is a minor stylistic preference and the current implementation is clear enough.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.ci_support/mpi4pywrapper.py(1 hunks).github/workflows/pipeline.yml(1 hunks)
🧰 Additional context used
🪛 Ruff (0.13.3)
.ci_support/mpi4pywrapper.py
24-24: Unused method argument: root
(ARG002)
27-27: Unused method argument: root
(ARG002)
114-114: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
.github/workflows/pipeline.yml (1)
190-190: Verify the purpose of the resource listing step.The
flux resource listcommand appears to be a diagnostic step added before notebook execution. While this can be helpful for debugging resource availability, ensure it doesn't add unnecessary overhead or fail the job if resources are unavailable.If this is intended only for debugging purposes, consider adding a comment to clarify its role, or redirect output to a log file for post-mortem analysis.
.ci_support/mpi4pywrapper.py (1)
9-123: Well-structured MPI wrapper for test environment.The
MPI4PYWrapperclass provides a clean abstraction over mpi4py operations, supporting:
- Sub-communicator creation with parent tracking
- Common collective operations (reductions, scatter/gather, broadcast)
- Point-to-point communication with blocking/non-blocking modes
- Request management and synchronization primitives
The design appropriately exposes the underlying communicator via
get_c_object()for interoperability while maintaining a simplified interface for common operations.
| try: | ||
| from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
Silent import failure prevents clear error diagnosis.
The bare pass in the except clause silently suppresses import errors, which will lead to cryptic NameError exceptions later when MPI constants are referenced (lines 25, 28, 32, 35, etc.). This makes it difficult to diagnose missing dependencies.
Consider one of these approaches:
Option 1: Fail fast if mpi4py is unavailable:
try:
from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL
except ImportError:
- pass
+ raise ImportError("mpi4py is required but not installed")Option 2: Define stub constants if testing without mpi4py is intentional:
try:
from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL
except ImportError:
- pass
+ # Define stubs for environments without mpi4py
+ Request = SUM = MAX = MIN = IN_PLACE = IDENT = CONGRUENT = SIMILAR = UNEQUAL = None📝 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.
| try: | |
| from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL | |
| except ImportError: | |
| pass | |
| try: | |
| from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL | |
| except ImportError: | |
| raise ImportError("mpi4py is required but not installed") |
| try: | |
| from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL | |
| except ImportError: | |
| pass | |
| try: | |
| from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL | |
| except ImportError: | |
| # Define stubs for environments without mpi4py | |
| Request = SUM = MAX = MIN = IN_PLACE = IDENT = CONGRUENT = SIMILAR = UNEQUAL = None |
🤖 Prompt for AI Agents
In .ci_support/mpi4pywrapper.py lines 1-4, the except block currently silences
ImportError which causes later NameError when MPI names are referenced; update
this to either (a) fail fast by re-raising a clear ImportError with a helpful
message that mpi4py is required (e.g. raise ImportError("mpi4py is required for
tests; install mpi4py or run without MPI support") from err), or (b) if running
without mpi4py is intentional, provide explicit stub definitions for Request and
the MPI constants (e.g. set Request = None and define SUM, MAX, MIN, IN_PLACE,
IDENT, CONGRUENT, SIMILAR, UNEQUAL to safe sentinel values) plus a module-level
flag like MPI_AVAILABLE = False so callers can check; implement one of these
fixes and remove the bare pass.
| def max_scalar(self, a, root=-1): | ||
| return self.sum_scalar(a, root=-1, _op=MAX) | ||
|
|
||
| def min_scalar(self, a, root=-1): | ||
| return self.sum_scalar(a, root=-1, _op=MIN) |
There was a problem hiding this comment.
Unused root parameter causes incorrect reduction behavior.
Both max_scalar and min_scalar accept a root parameter but ignore it, always passing root=-1 to sum_scalar. This means these methods will always perform an all-reduce operation, even when the caller specifies a specific root rank.
Forward the root parameter correctly:
def max_scalar(self, a, root=-1):
- return self.sum_scalar(a, root=-1, _op=MAX)
+ return self.sum_scalar(a, root=root, _op=MAX)
def min_scalar(self, a, root=-1):
- return self.sum_scalar(a, root=-1, _op=MIN)
+ return self.sum_scalar(a, root=root, _op=MIN)📝 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.
| def max_scalar(self, a, root=-1): | |
| return self.sum_scalar(a, root=-1, _op=MAX) | |
| def min_scalar(self, a, root=-1): | |
| return self.sum_scalar(a, root=-1, _op=MIN) | |
| def max_scalar(self, a, root=-1): | |
| return self.sum_scalar(a, root=root, _op=MAX) | |
| def min_scalar(self, a, root=-1): | |
| return self.sum_scalar(a, root=root, _op=MIN) |
🧰 Tools
🪛 Ruff (0.13.3)
24-24: Unused method argument: root
(ARG002)
27-27: Unused method argument: root
(ARG002)
🤖 Prompt for AI Agents
In .ci_support/mpi4pywrapper.py around lines 24 to 28, both max_scalar and
min_scalar ignore the passed root parameter and hardcode root=-1 when calling
sum_scalar; update each method to forward the root argument (use the local
variable root) to sum_scalar while still defaulting to -1 so the
caller-specified root is honored.
| shell: bash -l {0} | ||
| timeout-minutes: 20 | ||
| run: | | ||
| cp .ci_support/mpi4pywrapper.py /home/runner/miniconda3/envs/test/lib/python3.12/site-packages/gpaw |
There was a problem hiding this comment.
Hardcoded Python version in path is brittle.
The path explicitly includes python3.12, which will break if the Python version specified in line 177 is updated without also updating this path.
Consider using a version-agnostic approach:
- cp .ci_support/mpi4pywrapper.py /home/runner/miniconda3/envs/test/lib/python3.12/site-packages/gpaw
+ cp .ci_support/mpi4pywrapper.py $(python -c "import sysconfig; print(sysconfig.get_path('purelib'))")/gpaw📝 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.
| cp .ci_support/mpi4pywrapper.py /home/runner/miniconda3/envs/test/lib/python3.12/site-packages/gpaw | |
| cp .ci_support/mpi4pywrapper.py $(python -c "import sysconfig; print(sysconfig.get_path('purelib'))")/gpaw |
🤖 Prompt for AI Agents
.github/workflows/pipeline.yml around line 189: the cp command hardcodes a
python3.12 path which will break when the runner Python/conda env version
changes; replace the hardcoded path with a runtime-resolved site-packages
destination—either derive site-packages from the active conda environment prefix
(CONDA_PREFIX) or invoke Python at runtime to print
sysconfig.get_paths()[“purelib”] and use that value as the target for cp—so the
workflow computes the correct site-packages path instead of embedding a specific
Python version.
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores