Skip to content

Oscar Mako and module update #1082

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sbryngelson merged 3 commits into from
Dec 10, 2025
Merged

Oscar Mako and module update #1082

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e72f2cd
added updated modules for Oscar supercomputer
mrodrig6 Dec 9, 2025
8df72f8
fix to oscar mako file
mrodrig6 Dec 9, 2025
0056072
merger
mrodrig6 Dec 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion toolchain/modules
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ d-gpu MFC_CUDA_CC=80,86

o Brown Oscar
o-all python
o-cpu hpcx-mpi
o-cpu hpcx-mpi python/3.13.10s
Copy link

@codeant-ai codeant-ai bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Typo in the Python module name: the added token python/3.13.10s has an extra trailing "s" which will make the module name invalid/unavailable to the module loader and cause this entry to fail at load time; remove the stray "s" so the version matches the rest of the file. [possible bug]

Severity Level: Critical 🚨

Suggested change
o-cpu hpcx-mpi python/3.13.10s
o-cpu hpcx-mpi python/3.13.10
Why it matters? ⭐

The added token "python/3.13.10s" very likely contains a typo — all other entries use numeric version tokens (e.g., python/3.12.5). The stray "s" will most likely make the module name not match the expected modulefile and could break module loading. Changing it to python/3.13.10 is a straightforward, low-risk fix that improves correctness.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** toolchain/modules
**Line:** 57:57
**Comment:**
	*Possible Bug: Typo in the Python module name: the added token `python/3.13.10s` has an extra trailing "s" which will make the module name invalid/unavailable to the module loader and cause this entry to fail at load time; remove the stray "s" so the version matches the rest of the file.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

o-gpu nvhpc cuda/12.3.0 cmake/3.26.3
o-gpu CC=nvc CXX=nvc++ FC=nvfortran

Expand Down
12 changes: 5 additions & 7 deletions toolchain/templates/oscar.mako
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@
#SBATCH --account="${account}"
% endif
% if gpu_enabled:
#SBATCH --gpus-per-node=${tasks_per_node}
#SBATCH --mem=64G
#SBATCH --gpu-bind=closest
#SBATCH --gpu-bind=verbose,closest
#SBATCH --gres=gpu:v100-16:${tasks_per_node}
Copy link

@codeant-ai codeant-ai bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Hardcoded GPU resource/type may not exist on all clusters and can cause sbatch to reject the job; request the GPU count generically (or parameterize the GPU type) instead of forcing "v100-16". [possible bug]

Severity Level: Critical 🚨

Suggested change
#SBATCH --gres=gpu:v100-16:${tasks_per_node}
#SBATCH --gres=gpu:${tasks_per_node}
Why it matters? ⭐

This is a valid and practical suggestion: hardcoding a specific GPU model (v100-16) can indeed cause sbatch to reject jobs on clusters that don't expose that exact resource name. Replacing the line with a generic gres request (or better, parameterizing the GPU model with a template variable) is safer and fixes a real portability issue visible in the diff.
The PR already gates these lines with % if gpu_enabled so removing the explicit model is a low-risk change.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** toolchain/templates/oscar.mako
**Line:** 19:19
**Comment:**
	*Possible Bug: Hardcoded GPU resource/type may not exist on all clusters and can cause sbatch to reject the job; request the GPU count generically (or parameterize the GPU type) instead of forcing "v100-16".

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

% endif
#SBATCH --output="${name}.out"
#SBATCH --error="${name}.err"
Expand All @@ -31,7 +30,7 @@
${helpers.template_prologue()}

ok ":) Loading modules:\n"
cd "${MFC_ROOTDIR}"
cd "${MFC_ROOT_DIR}"
Copy link

@codeant-ai codeant-ai bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The added unconditional "cd ${MFC_ROOT_DIR}" will fail silently if MFC_ROOT_DIR is unset or not a directory, causing the subsequent source of mfc.sh to run in the wrong directory and break the script; check existence and directory-ness before changing directory and exit with a clear error if invalid. [resource leak]

Severity Level: Minor ⚠️

Suggested change
cd "${MFC_ROOT_DIR}"
if [ -z "${MFC_ROOT_DIR}" ] || [ ! -d "${MFC_ROOT_DIR}" ]; then
echo "MFC_ROOT_DIR is not set or is not a directory" >&2
exit 1
fi
Why it matters? ⭐

Adding a guard to ensure MFC_ROOT_DIR is set and is a directory is sensible: as written the script will attempt to cd and then source ./mfc.sh relative to the current dir, which will silently (or noisily) fail if the variable is unset or points nowhere. The proposed check makes the failure explicit and fails fast, which is desirable for a job script that must set up environment.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** toolchain/templates/oscar.mako
**Line:** 33:33
**Comment:**
	*Resource Leak: The added unconditional "cd ${MFC_ROOT_DIR}" will fail silently if `MFC_ROOT_DIR` is unset or not a directory, causing the subsequent source of `mfc.sh` to run in the wrong directory and break the script; check existence and directory-ness before changing directory and exit with a clear error if invalid.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

. ./mfc.sh load -c o -m ${'g' if gpu_enabled else 'c'}
cd - > /dev/null
echo
Expand All @@ -42,9 +41,8 @@ echo
% if not mpi:
(set -x; ${profiler} "${target.get_install_binpath(case)}")
% else:
(set -x; ${profiler} \
mpirun -np ${nodes*tasks_per_node} \
${' '.join([f"'{x}'" for x in ARG('--') ])} \
(set -x; ${profiler} \
mpirun -np ${nodes*tasks_per_node} \
"${target.get_install_binpath(case)}")
Comment on lines +44 to 46
Copy link
Contributor

@qodo-code-review qodo-code-review bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: To correctly profile the parallel application instead of the MPI launcher, move the ${profiler} command to be executed by mpirun on each rank. [possible issue, importance: 9]

Suggested change
(set -x; ${profiler} \
mpirun -np ${nodes*tasks_per_node} \
"${target.get_install_binpath(case)}")
(set -x; mpirun -np ${nodes*tasks_per_node} \
${profiler} "${target.get_install_binpath(case)}")

% endif

Expand Down