Conversation
📝 WalkthroughWalkthroughThis pull request adds WPI Turing support to the toolchain infrastructure. A new interactive menu option is exposed in the bootstrap modules selection prompt. The toolchain modules configuration introduces a new cluster group with CPU-variant modules pinning GCC/12.1.0, OpenMPI/4.1.3, and Python/3.13.5, and GPU-variant modules pinning NVHPC/24.3, Python/3.13.5, with compiler environment variables and MPI configuration. A new Mako template generates HPC batch job scripts with conditional Slurm directives, module loading, and per-target execution supporting both MPI and non-MPI modes with optional GPU resource allocation. 🚥 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
🧹 Nitpick comments (1)
toolchain/templates/turing.mako (1)
17-21: Hardcoded single GPU and memory constraints may limit flexibility.The GPU configuration hardcodes:
--gres=gpu:1- Only 1 GPU regardless of job requirements--mem=208G- Fixed memory allocation-C "A30|A100"- Specific GPU constraintsThis may be appropriate for Turing's node configuration, but consider whether multi-GPU jobs are needed.
Please confirm with WPI cluster administrators that:
- Single GPU per job is the intended use pattern
- 208G is the appropriate memory allocation for GPU nodes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2454b169-22c8-4900-b9d8-ae3d855e67c8
📒 Files selected for processing (3)
toolchain/bootstrap/modules.shtoolchain/modulestoolchain/templates/turing.mako
| #SBATCH --export=ALL | ||
| % if email: | ||
| #SBATCH --mail-user=${email} | ||
| #SBATCH --mail-type="BEGIN, END, FAIL" |
There was a problem hiding this comment.
Minor: Mail-type value has spaces which may cause parsing issues.
The mail-type directive has spaces: "BEGIN, END, FAIL". Some Slurm versions expect comma-separated values without spaces.
🔧 Proposed fix
-#SBATCH --mail-type="BEGIN, END, FAIL"
+#SBATCH --mail-type=BEGIN,END,FAIL📝 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.
| #SBATCH --mail-type="BEGIN, END, FAIL" | |
| `#SBATCH` --mail-type=BEGIN,END,FAIL |
| (set -x; ${profiler} \ | ||
| srun --gres=gpu:1 -C "A30|A100" \ | ||
| $MPI_HOME/bin/mpirun --np ${nodes*tasks_per_node} \ | ||
| "${target.get_install_binpath(case)}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the turing.mako template file
find . -name "turing.mako" -type f | head -5Repository: MFlowCode/MFC
Length of output: 90
🏁 Script executed:
# List all .mako template files in the toolchain directory
ls -la toolchain/templates/*.mako 2>/dev/null || find toolchain -name "*.mako" -type fRepository: MFlowCode/MFC
Length of output: 1384
🏁 Script executed:
# Read the turing.mako file to verify the actual code
if [ -f "toolchain/templates/turing.mako" ]; then
cat -n toolchain/templates/turing.mako | sed -n '40,65p'
fiRepository: MFlowCode/MFC
Length of output: 1153
🏁 Script executed:
# Also check other template files to see the pattern used elsewhere
fd -e mako toolchain/ -x echo "File: {}" \; -x head -70 {}Repository: MFlowCode/MFC
Length of output: 402
🏁 Script executed:
# Check other template files for comparison - particularly GPU+MPI patterns
for file in toolchain/templates/*.mako; do
echo "=== $(basename $file) ==="
cat -n "$file" | grep -A 5 -B 2 "gpu_enabled\|mpirun\|srun" || echo "(no GPU/MPI pattern found)"
doneRepository: MFlowCode/MFC
Length of output: 14827
🏁 Script executed:
# Search for documentation or comments about the MPI launcher choice
rg -i "mpirun|srun.*mpi|gpu.*mpi" toolchain/ docs/ --max-count=5Repository: MFlowCode/MFC
Length of output: 5158
🏁 Script executed:
# Check if there's any cluster-specific documentation
find . -name "*turing*" -type f | head -10Repository: MFlowCode/MFC
Length of output: 90
🏁 Script executed:
# Check if there's any Turing cluster-specific documentation or comments
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.sh" \) | xargs grep -l -i "turing" | head -10Repository: MFlowCode/MFC
Length of output: 138
🏁 Script executed:
# Look at hipergator.mako more closely since it also uses NVHPC
cat -n toolchain/templates/hipergator.mako | sed -n '49,64p'Repository: MFlowCode/MFC
Length of output: 860
Fix inconsistency: GPU MPI path should use srun --mpi=pmi2 like the CPU path, not wrap mpirun.
The GPU execution (lines 50-53) unusually combines srun with mpirun, which can cause process management conflicts. The CPU path (lines 55-57) correctly uses srun --mpi=pmi2. For consistency and to avoid launcher conflicts, the GPU path should follow the same pattern:
Suggested fix
% if gpu_enabled:
(set -x; ${profiler} \
- srun --gres=gpu:1 -C "A30|A100" \
- $MPI_HOME/bin/mpirun --np ${nodes*tasks_per_node} \
+ srun --mpi=pmi2 --gres=gpu:1 -C "A30|A100" \
+ --ntasks=${nodes*tasks_per_node} \
"${target.get_install_binpath(case)}")
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1335 +/- ##
=======================================
Coverage 45.01% 45.01%
=======================================
Files 70 70
Lines 20562 20562
Branches 1962 1962
=======================================
Hits 9255 9255
Misses 10179 10179
Partials 1128 1128 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Description
This PR adds support for the Turing supercomputer, an HPC cluster hosted by Worcester Polytechnic Institute (WPI).
Type of change
Testing
How did you test your changes?
Checklist
See the developer guide for full coding standards.
GPU changes (expand if you modified
src/simulation/)AI code reviews
Reviews are not triggered automatically. To request a review, comment on the PR:
@coderabbitai review— incremental review (new changes only)@coderabbitai full review— full review from scratch/review— Qodo review/improve— Qodo code suggestions@claude full review— Claude full review (also triggers on PR open/reopen/ready)claude-full-review— Claude full review via label