Brown Oscar Python module and mako file update#1352
Brown Oscar Python module and mako file update#1352mrodrig6 wants to merge 75 commits intoMFlowCode:masterfrom
Conversation
Review Summary by QodoAdd Purdue Anvil support and update Oscar configuration
WalkthroughsDescription• Added Purdue Anvil supercomputer support with mako template • Updated Oscar module configuration and batch script handling • Modified Oscar mako to use srun instead of mpirun • Added memory allocation and Python version updates for Oscar Diagramflowchart LR
A["Bootstrap System Selection"] -->|Add Anvil| B["modules.sh"]
C["Oscar Configuration"] -->|Update Modules| D["toolchain/modules"]
C -->|Update Batch Script| E["oscar.mako"]
F["New Anvil Support"] -->|Create Template| G["anvil.mako"]
D --> H["Enhanced HPC Setup"]
E --> H
G --> H
File Changes1. toolchain/bootstrap/modules.sh
|
Code Review by Qodo
1. MPI task count dropped
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| % else: | ||
| (set -x; ${profiler} \ | ||
| mpirun -np ${nodes*tasks_per_node} \ | ||
| srun \ | ||
| "${target.get_install_binpath(case)}") |
There was a problem hiding this comment.
1. Mpi task count dropped 🐞 Bug ≡ Correctness
In toolchain/templates/oscar.mako the MPI path now invokes plain "srun" without using nodes*tasks_per_node, so the configured task count no longer influences how many ranks are launched. This can under-launch MPI ranks and make MPI runs behave differently than intended for the same nodes/tasks_per_node settings.
Agent Prompt
## Issue description
Oscar’s batch template switched from `mpirun -np ${nodes*tasks_per_node}` to `srun` but no longer passes any task-count arguments derived from `nodes`/`tasks_per_node`. As a result, the configured MPI size is not enforced by the launcher.
## Issue Context
Other Slurm templates in this repo that use `srun` explicitly pass `--ntasks ${nodes*tasks_per_node}`.
## Fix Focus Areas
- toolchain/templates/oscar.mako[42-48]
## Suggested fix
Update the MPI branch to something like:
- `srun --ntasks ${nodes*tasks_per_node} "${target.get_install_binpath(case)}"`
(Optionally also align the SBATCH header to consistently use `--ntasks-per-node=${tasks_per_node}` if that’s the intended directive for this cluster.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| % if partition: | ||
| ##SBATCH --partition=${partition} | ||
| % endif |
There was a problem hiding this comment.
2. Partition sbatch is commented 🐞 Bug ≡ Correctness
In toolchain/templates/anvil.mako the partition directive is written as "##SBATCH", so it is treated as a comment even when partition is provided. This makes user-specified partitions ineffective and can lead to jobs scheduling on the wrong/default partition or not scheduling as expected.
Agent Prompt
## Issue description
The Anvil batch template uses `##SBATCH --partition=...`, which comments out the partition directive even when the user provides a partition.
## Issue Context
This causes the `partition` CLI/template variable to have no effect.
## Fix Focus Areas
- toolchain/templates/anvil.mako[11-13]
## Suggested fix
Change `##SBATCH --partition=${partition}` to `#SBATCH --partition=${partition}`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
📝 WalkthroughWalkthroughThe changes add support for a new Purdue Anvil cluster and update configurations for Brown Oscar. The interactive system selection prompt in the bootstrap module now includes Purdue Anvil as an option with slug code 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4281db83-0194-4fb6-bcc2-faa69a9153d1
📒 Files selected for processing (4)
toolchain/bootstrap/modules.shtoolchain/modulestoolchain/templates/anvil.makotoolchain/templates/oscar.mako
| % if partition: | ||
| ##SBATCH --partition=${partition} | ||
| % endif |
There was a problem hiding this comment.
Double hash comments out the partition directive.
Line 12 uses ##SBATCH instead of #SBATCH, which effectively comments out the partition directive. This means the partition will never be applied even when specified by the user.
Proposed fix
% if partition:
-##SBATCH --partition=${partition}
+#SBATCH --partition=${partition}
% endif📝 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.
| % if partition: | |
| ##SBATCH --partition=${partition} | |
| % endif | |
| % if partition: | |
| `#SBATCH` --partition=${partition} | |
| % endif |
| cd "${MFC_ROOT_DIR}" | ||
| . ./mfc.sh load -c pa -m ${'g' if gpu_enabled else 'c'} | ||
| cd - > /dev/null |
There was a problem hiding this comment.
GPU mode loads modules that don't exist.
When gpu_enabled is true, this loads with -m g (GPU mode), but toolchain/modules only defines pa-cpu entries—no pa-gpu modules exist. This will result in no GPU-specific modules being loaded.
If GPU support is intentionally deferred (CPU-only for now), consider adding a guard or comment. Otherwise, add pa-gpu definitions to toolchain/modules.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1352 +/- ##
=======================================
Coverage 64.67% 64.67%
=======================================
Files 70 70
Lines 18251 18251
Branches 1504 1504
=======================================
Hits 11804 11804
Misses 5492 5492
Partials 955 955 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| #SBATCH --job-name="${name}" | ||
| #SBATCH --time=${walltime} | ||
| % if partition: | ||
| ##SBATCH --partition=${partition} |
2 participants
Description
Summarize your changes and the motivation behind them.
Fixes #(issue)
Type of change
Testing
How did you test your changes?
Checked that MFC compiles on Oscar and ran both interactive and batch runs on the system up to 1 node (max limit)
Checklist
N/A
AI code reviews
Reviews are not triggered automatically. To request a review, comment on the PR: