[Documentation] Add notebook to explain the HPC setup#1008
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe HPC cluster executor notebook is substantially expanded and refreshed. It now includes complete SLURM background sections with MPI examples, ChangesHPC Cluster Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 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.15)notebooks/2-hpc-cluster.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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1008 +/- ##
=======================================
Coverage 94.19% 94.19%
=======================================
Files 39 39
Lines 2103 2103
=======================================
Hits 1981 1981
Misses 122 122 ☔ View full report in Codecov by Harness. 🚀 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: 2
🧹 Nitpick comments (1)
notebooks/0-hpc-schedulers.ipynb (1)
1-411: ⚖️ Poor tradeoffConsider adding this notebook to the CI test suite.
The CI pipeline currently executes notebooks 1, 2, 3, and 5 via
papermillbut does not include0-hpc-schedulers.ipynb. While this notebook is documentation-focused, adding it to the CI pipeline would help catch errors in the example code snippets (such as the missingpmi_modeparameter flagged above).If you choose to add CI coverage, insert this line in
.github/workflows/pipeline.ymlafter line 176:papermill notebooks/0-hpc-schedulers.ipynb notebooks/0-hpc-schedulers-out.ipynb -k python3Note: The current examples would need to be wrapped in proper code cells (rather than markdown code blocks) for papermill execution.
🤖 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 `@notebooks/0-hpc-schedulers.ipynb` around lines 1 - 411, The CI is not running notebooks/0-hpc-schedulers.ipynb so example code (e.g. MPI usage with pmi_mode) isn't validated; add the notebook to the pipeline and make its runnable examples real code cells. Update the workflow by inserting a papermill step that runs "notebooks/0-hpc-schedulers.ipynb" (producing notebooks/0-hpc-schedulers-out.ipynb) into .github/workflows/pipeline.yml (after the existing papermill calls), and edit notebooks/0-hpc-schedulers.ipynb to convert the markdown fenced code blocks (examples like the SlurmClusterExecutor, SlurmJobExecutor, FluxClusterExecutor usage and mpi_calc/mpi_calc snippets that reference pmi_mode and cores) into executable code cells so papermill can execute them during CI.
🤖 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 `@notebooks/0-hpc-schedulers.ipynb`:
- Around line 372-376: The MPI example fails because FluxClusterExecutor is
missing the pmi_mode parameter; update the example to construct the executor
with pmi_mode="pmix" (i.e., use FluxClusterExecutor(cache_directory="./cache",
pmi_mode="pmix")) so that MPI is initialized for mpi_calc, and keep the
resource_dict with "cores": N as shown; modify the example where
FluxClusterExecutor and the call exe.submit(mpi_calc, ...) appear to include
this parameter.
- Around line 108-110: The sacct --format field uses the wrong case; replace the
incorrect "AllocCPUS" with SLURM's documented "alloccpus" in the sacct
invocation string (the snippet calling sacct -j 12345
--format=JobID,State,AllocCPUS,Elapsed). Edit that sacct line so it reads
--format=JobID,State,alloccpus,Elapsed to match SLURM's expected field name.
---
Nitpick comments:
In `@notebooks/0-hpc-schedulers.ipynb`:
- Around line 1-411: The CI is not running notebooks/0-hpc-schedulers.ipynb so
example code (e.g. MPI usage with pmi_mode) isn't validated; add the notebook to
the pipeline and make its runnable examples real code cells. Update the workflow
by inserting a papermill step that runs "notebooks/0-hpc-schedulers.ipynb"
(producing notebooks/0-hpc-schedulers-out.ipynb) into
.github/workflows/pipeline.yml (after the existing papermill calls), and edit
notebooks/0-hpc-schedulers.ipynb to convert the markdown fenced code blocks
(examples like the SlurmClusterExecutor, SlurmJobExecutor, FluxClusterExecutor
usage and mpi_calc/mpi_calc snippets that reference pmi_mode and cores) into
executable code cells so papermill can execute them during CI.
🪄 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: 99aebe2c-a56d-42e6-b098-60bff540d071
📒 Files selected for processing (2)
docs/_toc.ymlnotebooks/0-hpc-schedulers.ipynb
| "```python\n", | ||
| "with FluxClusterExecutor(cache_directory=\"./cache\") as exe:\n", | ||
| " future = exe.submit(mpi_calc, 3, resource_dict={\"cores\": 2})\n", | ||
| " print(future.result())\n", | ||
| "```" |
There was a problem hiding this comment.
Missing pmi_mode parameter breaks the MPI example.
The FluxClusterExecutor MPI example is incomplete. According to the source code and your own summary table (line 391), MPI execution requires both pmi_mode="pmix" in the executor constructor and "cores": N in the resource_dict. This example only provides cores and will fail to properly initialize MPI for the mpi_calc function.
🐛 Proposed fix
-with FluxClusterExecutor(cache_directory="./cache") as exe:
+with FluxClusterExecutor(pmi_mode="pmix", cache_directory="./cache") as exe:
future = exe.submit(mpi_calc, 3, resource_dict={"cores": 2})
print(future.result())🤖 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 `@notebooks/0-hpc-schedulers.ipynb` around lines 372 - 376, The MPI example
fails because FluxClusterExecutor is missing the pmi_mode parameter; update the
example to construct the executor with pmi_mode="pmix" (i.e., use
FluxClusterExecutor(cache_directory="./cache", pmi_mode="pmix")) so that MPI is
initialized for mpi_calc, and keep the resource_dict with "cores": N as shown;
modify the example where FluxClusterExecutor and the call exe.submit(mpi_calc,
...) appear to include this parameter.
1 participant
Summary by CodeRabbit