Skip to content

Tuo debug #1103

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

Open
wilfonba wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Tuo debug #1103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d5134fa
add tuo modules and template
wilfonba Dec 5, 2025
6db992d
Merge remote-tracking branch 'upstream/master' into tuoSetup
wilfonba Dec 5, 2025
e235612
Add tuo to ./mfc.sh load
wilfonba Dec 5, 2025
50a572d
Merge branch 'master' into master
wilfonba Dec 5, 2025
0c827ab
fixes for --debug build
wilfonba Dec 19, 2025
4dfff2e
merge
wilfonba Dec 19, 2025
aca57a9
Merge remote-tracking branch 'upstream/master' into tuoDebug
wilfonba Jan 9, 2026
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
3 changes: 1 addition & 2 deletions src/common/m_compute_levelset.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,9 +552,8 @@ contains
end if
else
levelset%sf(i, j, k, ib_patch_id) = dist_surface

xyz_local = xyz_local*dist_surface_vec
xyz_local = xyz_local/norm2(xyz_local)
xyz_local = xyz_local/max(norm2(xyz_local), sgm_eps)
levelset_norm%sf(i, j, k, ib_patch_id, :) = matmul(rotation, xyz_local)
end if
end do
Expand Down
2 changes: 1 addition & 1 deletion src/common/m_model.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ contains
do i = 1, boundary_edge_count
boundary_edge(1) = boundary_v(i, 2, 1) - boundary_v(i, 1, 1)
boundary_edge(2) = boundary_v(i, 2, 2) - boundary_v(i, 1, 2)
edgetan = boundary_edge(1)/boundary_edge(2)
edgetan = boundary_edge(1)/max(sgm_eps, boundary_edge(2))
Copy link
Contributor

@qodo-code-review qodo-code-review bot Jan 13, 2026

Choose a reason for hiding this comment

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

Suggestion: Preserve the sign of boundary_edge(2) when preventing division by zero to avoid an incorrect sign flip in the edgetan calculation. [possible issue, importance: 9]

Suggested change
edgetan = boundary_edge(1)/max(sgm_eps, boundary_edge(2))
edgetan = boundary_edge(1)/(sign(1.0_wp, boundary_edge(2))*max(abs(boundary_edge(2)), sgm_eps))

Copy link
Contributor

@codeant-ai codeant-ai bot Jan 13, 2026

Choose a reason for hiding this comment

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

Suggestion: Sign-preservation bug: using max(sgm_eps, abs(...)) fixes magnitude but loses the original sign of boundary_edge(2) when sgm_eps is chosen; compute the denominator with the magnitude but preserve the original sign so the computed tangent keeps the correct orientation. [logic error]

Severity Level: Minor ⚠️

Suggested change
edgetan = boundary_edge(1)/max(sgm_eps, boundary_edge(2))
edgetan = boundary_edge(1)/sign(max(sgm_eps, abs(boundary_edge(2))), boundary_edge(2))
Why it matters? ⭐

This is the correct approach: clamp the magnitude to avoid tiny denominators but restore the original sign so the tangent keeps the correct orientation. The sign(...) wrapper yields a denominator with the clamped magnitude and the original sign, so downstream checks that rely on edgetan's sign remain valid. This directly addresses the visible logic bug in the PR diff.

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

**Path:** src/common/m_model.fpp
**Line:** 681:681
**Comment:**
	*Logic Error: Sign-preservation bug: using `max(sgm_eps, abs(...))` fixes magnitude but loses the original sign of `boundary_edge(2)` when `sgm_eps` is chosen; compute the denominator with the magnitude but preserve the original sign so the computed tangent keeps the correct orientation.

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.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Jan 13, 2026

Choose a reason for hiding this comment

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

P1: Division protection breaks for negative values. When boundary_edge(2) is negative, max(sgm_eps, boundary_edge(2)) returns sgm_eps, causing incorrect sign and magnitude of edgetan. This will compute wrong normal directions. Consider using sign(max(sgm_eps, abs(boundary_edge(2))), boundary_edge(2)) to preserve the sign while preventing division by zero.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/common/m_model.fpp, line 681:

<comment>Division protection breaks for negative values. When `boundary_edge(2)` is negative, `max(sgm_eps, boundary_edge(2))` returns `sgm_eps`, causing incorrect sign and magnitude of `edgetan`. This will compute wrong normal directions. Consider using `sign(max(sgm_eps, abs(boundary_edge(2))), boundary_edge(2))` to preserve the sign while preventing division by zero.</comment>

<file context>
@@ -678,7 +678,7 @@ contains
             boundary_edge(1) = boundary_v(i, 2, 1) - boundary_v(i, 1, 1)
             boundary_edge(2) = boundary_v(i, 2, 2) - boundary_v(i, 1, 2)
-            edgetan = boundary_edge(1)/boundary_edge(2)
+            edgetan = boundary_edge(1)/max(sgm_eps, boundary_edge(2))
 
             if (abs(boundary_edge(2)) < threshold_vector_zero) then
</file context>
Suggested change
edgetan = boundary_edge(1)/max(sgm_eps, boundary_edge(2))
edgetan = boundary_edge(1)/sign(max(sgm_eps, abs(boundary_edge(2))), boundary_edge(2))


if (abs(boundary_edge(2)) < threshold_vector_zero) then
if (edgetan > 0._wp) then
Expand Down
3 changes: 1 addition & 2 deletions src/common/m_variables_conversion.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1377,8 +1377,7 @@ contains

#ifndef MFC_PRE_PROCESS
subroutine s_compute_speed_of_sound(pres, rho, gamma, pi_inf, H, adv, vel_sum, c_c, c, qv)
$:GPU_ROUTINE(function_name='s_compute_speed_of_sound', &
& parallelism='[seq]', cray_inline=True)
$:GPU_ROUTINE(parallelism='[seq]')
Copy link
Contributor

@codeant-ai codeant-ai bot Jan 13, 2026

Choose a reason for hiding this comment

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

Suggestion: The GPU annotation lacks an explicit function_name argument; other routines in this file provide function_name so the GPU toolchain may rely on it for symbol mapping—without it the offload/annotation processor may not correctly associate the pragma with this subroutine and the annotation may be ignored or misapplied. Add the function_name to match the pattern used elsewhere. [possible bug]

Severity Level: Critical 🚨

Suggested change
$:GPU_ROUTINE(parallelism='[seq]')
$:GPU_ROUTINE(function_name='s_compute_speed_of_sound', parallelism='[seq]', cray_inline=True)
Why it matters? ⭐

Other routines in this file (e.g. s_compute_pressure, s_compute_fast_magnetosonic_speed,
s_convert_species_to_mixture_variables_acc) include function_name and cray_inline=True in their
$:GPU_ROUTINE annotations. Keeping annotations consistent is actionable and low risk: adding
function_name='s_compute_speed_of_sound' (and optionally cray_inline=True) matches the established
pattern and helps the offload/annotation toolchain map symbols correctly. This does not fix a
correctness bug in the Fortran itself, but it is a sensible, minimal change to ensure the pragma
is applied as intended.

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

**Path:** src/common/m_variables_conversion.fpp
**Line:** 1380:1380
**Comment:**
	*Possible Bug: The GPU annotation lacks an explicit `function_name` argument; other routines in this file provide `function_name` so the GPU toolchain may rely on it for symbol mapping—without it the offload/annotation processor may not correctly associate the pragma with this subroutine and the annotation may be ignored or misapplied. Add the function_name to match the pattern used elsewhere.

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.


Comment on lines 1379 to 1381
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Search for GPU_ROUTINE macro definition
fd -e fpp -e f90 | xargs grep -l "define.*GPU_ROUTINE" || true
rg "GPU_ROUTINE" --type f90 --type fpp -B 2 -A 5 | head -100

Repository: MFlowCode/MFC

Length of output: 84

🏁 Script executed:

# Look for macro definitions in includes or preprocessor files
fd "macros" --type f
fd -e fpp | xargs grep -l "GPU_ROUTINE"

Repository: MFlowCode/MFC

Length of output: 951

🏁 Script executed:

# Check for fypp macro definitions
fd -e fpp -path "*/macros*" 
rg "\#:def.*GPU_ROUTINE" --type fpp || rg "GPU_ROUTINE" --type fpp | head -50

Repository: MFlowCode/MFC

Length of output: 281

🏁 Script executed:

# Read the main macros file to find GPU_ROUTINE definition
cat src/common/include/macros.fpp | head -200

Repository: MFlowCode/MFC

Length of output: 6174

🏁 Script executed:

# Search for GPU_ROUTINE in all .fpp files
grep -r "GPU_ROUTINE" src/common/include/ src/simulation/ src/common/ --include="*.fpp" | head -50

Repository: MFlowCode/MFC

Length of output: 4576

🏁 Script executed:

# Look for the specific macro definition
grep -n "GPU_ROUTINE" src/common/include/macros.fpp src/common/include/parallel_macros.fpp src/common/include/acc_macros.fpp 2>/dev/null | head -30

Repository: MFlowCode/MFC

Length of output: 225

🏁 Script executed:

# Read the GPU_ROUTINE macro definition
sed -n '51,150p' src/common/include/parallel_macros.fpp

Repository: MFlowCode/MFC

Length of output: 4689

Include function_name to match documented GPU helper decoration standards.

The $:GPU_ROUTINE macro does support omitting function_name (it defaults to None), so this change is technically valid. However, the project guidance explicitly recommends marking GPU-callable helpers with the function name for consistency and clarity. Restore the explicit naming:

Suggested change 
-        $:GPU_ROUTINE(parallelism='[seq]')
+        $:GPU_ROUTINE(function_name='s_compute_speed_of_sound', parallelism='[seq]')
📝 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.

Suggested change
subroutine s_compute_speed_of_sound(pres, rho, gamma, pi_inf, H, adv, vel_sum, c_c, c, qv)
$:GPU_ROUTINE(function_name='s_compute_speed_of_sound', &
& parallelism='[seq]', cray_inline=True)
$:GPU_ROUTINE(parallelism='[seq]')
subroutine s_compute_speed_of_sound(pres, rho, gamma, pi_inf, H, adv, vel_sum, c_c, c, qv)
$:GPU_ROUTINE(function_name='s_compute_speed_of_sound', parallelism='[seq]')
🤖 Prompt for AI Agents 
In @src/common/m_variables_conversion.fpp around lines 1379 - 1381, The GPU
helper subroutine s_compute_speed_of_sound is missing the explicit function_name
in its $:GPU_ROUTINE decoration; update the macro invocation for subroutine
s_compute_speed_of_sound to include function_name='s_compute_speed_of_sound'
(i.e., restore the explicit naming in the $:GPU_ROUTINE(...) parameters) so the
GPU-callable helper matches the documented decoration standard and improves
clarity.

real(wp), intent(in) :: pres
real(wp), intent(in) :: rho, gamma, pi_inf, qv
Expand Down
2 changes: 1 addition & 1 deletion src/simulation/m_data_output.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,7 +1233,7 @@ contains
q_cons_vf(mom_idx%beg)%sf(j - 2, k, l), G_local)
else
call s_compute_pressure( &
q_cons_vf(1)%sf(j - 2, k, l), &
q_cons_vf(E_idx)%sf(j - 2, k, l), &
q_cons_vf(alf_idx)%sf(j - 2, k, l), &
dyn_p, pi_inf, gamma, rho, qv, rhoYks(:), pres, T)
Copy link
Contributor

@codeant-ai codeant-ai bot Jan 13, 2026

Choose a reason for hiding this comment

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

Suggestion: Passing rhoYks(:) creates an explicit array section expression; in some compilers this can create a temporary and change argument association semantics. Use the array name rhoYks (no (:)) to pass the array directly and avoid inadvertent temporaries or subtle differences in argument passing. [possible bug]

Severity Level: Critical 🚨

Suggested change
dyn_p, pi_inf, gamma, rho, qv, rhoYks(:), pres, T)
dyn_p, pi_inf, gamma, rho, qv, rhoYks, pres, T)
Why it matters? ⭐

Passing an explicit section (rhoYks(:)) can cause the compiler to create temporaries or change argument association semantics depending on the procedure interface. The final file already contains inconsistent uses (some calls pass rhoYks, others rhoYks(:)), so standardizing to pass the array name avoids accidental temporaries and is a sensible, low-risk change.

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

**Path:** src/simulation/m_data_output.fpp
**Line:** 1238:1238
**Comment:**
	*Possible Bug: Passing `rhoYks(:)` creates an explicit array section expression; in some compilers this can create a temporary and change argument association semantics. Use the array name `rhoYks` (no `(:)`) to pass the array directly and avoid inadvertent temporaries or subtle differences in argument passing.

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.

end if
Expand Down
8 changes: 4 additions & 4 deletions src/simulation/m_derived_variables.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ module m_derived_variables

use m_data_output !< Data output module

use m_time_steppers !< Time-stepping algorithms

use m_compile_specific

use m_helper
Expand Down Expand Up @@ -120,9 +118,11 @@ contains

!> Writes coherent body information, communication files, and probes.
!! @param t_step Current time-step
subroutine s_compute_derived_variables(t_step)
subroutine s_compute_derived_variables(t_step, q_cons_vf, q_prim_ts1, q_prim_ts2)

integer, intent(in) :: t_step
type(scalar_field), dimension(:), intent(inout) :: q_cons_vf
type(vector_field), dimension(:), intent(inout) :: q_prim_ts1, q_prim_ts2
integer :: i, j, k !< Generic loop iterators

if (probe_wrt) then
Expand Down Expand Up @@ -169,7 +169,7 @@ contains

call s_derive_center_of_mass(q_prim_ts2(2)%vf, c_mass)

call s_write_probe_files(t_step, q_cons_ts(1)%vf, accel_mag)
call s_write_probe_files(t_step, q_cons_vf, accel_mag)

call s_write_com_files(t_step, c_mass)
end if
Expand Down
6 changes: 0 additions & 6 deletions src/simulation/m_start_up.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1123,11 +1123,6 @@ contains
end do
end if

call s_compute_derived_variables(t_step)

#ifdef DEBUG
print *, 'Computed derived vars'
#endif
mytime = mytime + dt

! Total-variation-diminishing (TVD) Runge-Kutta (RK) time-steppers
Expand All @@ -1138,7 +1133,6 @@ contains
if (relax) call s_infinite_relaxation_k(q_cons_ts(1)%vf)

! Time-stepping loop controls

t_step = t_step + 1

end subroutine s_perform_time_step
Expand Down
3 changes: 3 additions & 0 deletions src/simulation/m_time_steppers.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ module m_time_steppers

use m_body_forces

use m_derived_variables

implicit none

type(vector_field), allocatable, dimension(:) :: q_cons_ts !<
Expand Down Expand Up @@ -533,6 +535,7 @@ contains

if (probe_wrt) then
call s_time_step_cycling(t_step)
call s_compute_derived_variables(t_step, q_cons_ts(1)%vf, q_prim_ts1, q_prim_ts2)
Copy link
Contributor

@codeant-ai codeant-ai bot Jan 13, 2026

Choose a reason for hiding this comment

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

Suggestion: Missing GPU -> host synchronization before calling a host-side routine: s_compute_derived_variables reads arrays that may currently reside/been updated on the device (GPU) but were not updated back to the host, causing stale or invalid data to be used on the host and incorrect results or crashes. Insert a GPU update to ensure the host view of q_cons_ts(1)%vf, q_prim_ts1, and q_prim_ts2 is current before the call. [race condition]

Severity Level: Minor ⚠️

Suggested change
call s_compute_derived_variables(t_step, q_cons_ts(1)%vf, q_prim_ts1, q_prim_ts2)
$:GPU_UPDATE(host='[q_cons_ts(1)%vf,q_prim_ts1,q_prim_ts2]')
Why it matters? ⭐

The call to s_compute_derived_variables occurs after GPU kernels update q_cons_ts(1)%vf / related probe arrays.
Without a GPU->host synchronization the host routine can read stale/incomplete data. Adding a host GPU_UPDATE (as your improved_code shows) fixes a real potential data-race / stale-data bug.

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

**Path:** src/simulation/m_time_steppers.fpp
**Line:** 538:538
**Comment:**
	*Race Condition: Missing GPU -> host synchronization before calling a host-side routine: `s_compute_derived_variables` reads arrays that may currently reside/been updated on the device (GPU) but were not updated back to the host, causing stale or invalid data to be used on the host and incorrect results or crashes. Insert a GPU update to ensure the host view of `q_cons_ts(1)%vf`, `q_prim_ts1`, and `q_prim_ts2` is current before the call.

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.

end if

if (cfl_dt) then
Expand Down
5 changes: 3 additions & 2 deletions toolchain/bootstrap/modules.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,14 @@ done
if [ -v $u_c ]; then
log "Select a system:"
log "$G""ORNL$W: Ascent (a) | Frontier (f) | Summit (s) | Wombat (w)"
log "$B""LLNL $W: Tuolumne (tuo)"
log "$C""ACCESS$W: Bridges2 (b) | Expanse (e) | Delta (d) | DeltaAI (dai)"
log "$Y""Gatech$W: Phoenix (p)"
log "$R""Caltech$W: Richardson (r)"
log "$BR""Brown$W: Oscar (o)"
log "$BR""Brown$W: Oscar (o)"
log "$B""DoD$W: Carpenter Cray (cc) | Carpenter GNU (c) | Nautilus (n)"
log "$OR""Florida$W: HiPerGator (h)"
log_n "($G""a$W/$G""f$W/$G""s$W/$G""w$W/$C""b$W/$C""e$CR/$C""d/$C""dai$CR/$Y""p$CR/$R""r$CR/$B""cc$CR/$B""c$CR/$B""n$CR/$BR""o"$CR"/$OR""h"$CR"): "
log_n "($G""a$W/$G""f$W/$G""s$W/$G""w$W/$B""tuo$W/$C""b$W/$C""e$CR/$C""d/$C""dai$CR/$Y""p$CR/$R""r$CR/$B""cc$CR/$B""c$CR/$B""n$CR/$BR""o"$CR"/$OR""h"$CR"): "
read u_c
log
fi
Expand Down
2 changes: 1 addition & 1 deletion toolchain/dependencies/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ if (MFC_HIPFORT)
if (CMAKE_Fortran_COMPILER_ID STREQUAL "Cray")
ExternalProject_Add(hipfort
GIT_REPOSITORY "https://github.com/ROCmSoftwarePlatform/hipfort"
GIT_TAG rocm-6.0.2
GIT_TAG rocm-6.3.1
GIT_SHALLOW ON
GIT_PROGRESS ON
CMAKE_ARGS "-DHIPFORT_COMPILER=${CMAKE_Fortran_COMPILER}"
Expand Down
6 changes: 6 additions & 0 deletions toolchain/modules
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ f-all cpe/25.03 rocm/6.3.1
f-all cray-fftw cray-hdf5 python cmake
f-gpu python craype-accel-amd-gfx90a rocprofiler-compute/3.0.0

tuo OLCF Tuolumne
tuo-all cpe/25.03 rocm/6.3.1
tuo-all cray-fftw/3.3.10.9 cray-hdf5 python/3.12.2 cmake
tuo-gpu craype-accel-amd-gfx942
tuo-gpu HSA_XNACK=0

d NCSA Delta
d-all python/3.11.6
d-cpu gcc/11.4.0 openmpi
Expand Down
61 changes: 61 additions & 0 deletions toolchain/templates/tuo.mako
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#!/usr/bin/env bash

<%namespace name="helpers" file="helpers.mako"/>

% if engine == 'batch':
# flux: -N ${nodes}
# flux: -n ${tasks_per_node*nodes}
# flux: --job-name="${name}"
# flux: --output="${name}.out"
# flux: --error="${name}.err"
# flux: --time=${walltime}
# flux: --exclusive
# flux:--setattr=thp=always
Copy link
Contributor

@codeant-ai codeant-ai bot Jan 13, 2026

Choose a reason for hiding this comment

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

Suggestion: Logic/behaviour issue: the Flux directive comment is missing a space after the colon (# flux:--setattr=...), which will likely prevent Flux from recognizing the directive; add the missing space so the scheduler parses the directive. [logic error]

Severity Level: Minor ⚠️

Suggested change
# flux:--setattr=thp=always
# flux: --setattr=thp=always
Why it matters? ⭐

All other flux directives in the file use a space after "flux:". Many schedulers parse directives only when formatted consistently; adding the space makes the directive match the pattern used elsewhere and avoids it being ignored.

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

**Path:** toolchain/templates/tuo.mako
**Line:** 13:13
**Comment:**
	*Logic Error: Logic/behaviour issue: the Flux directive comment is missing a space after the colon (`# flux:--setattr=...`), which will likely prevent Flux from recognizing the directive; add the missing space so the scheduler parses the directive.

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.

Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

Missing space after colon. The flux comment directive should be "# flux: --setattr=thp=always" to match the formatting of other flux directives in this template (lines 6-12, 14, 16, 19).

Suggested change
# flux:--setattr=thp=always
# flux: --setattr=thp=always

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing space in Flux directive will cause parsing failure.

The directive # flux:--setattr is missing a space after the colon, inconsistent with all other flux directives in this file.

Proposed fix 
-# flux:--setattr=thp=always
+# flux: --setattr=thp=always
📝 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.

Suggested change
# flux:--setattr=thp=always
# flux: --setattr=thp=always
🤖 Prompt for AI Agents 
In @toolchain/templates/tuo.mako at line 13, The Flux directive line contains "#
flux:--setattr=thp=always" with a missing space after the colon which breaks
parsing; update that directive to include a space after the colon so it reads "#
flux: --setattr=thp=always" (preserving the flag and value) to match the other
flux directives in the template.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Jan 13, 2026

Choose a reason for hiding this comment

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

P2: Missing space after flux: - inconsistent with all other flux directives in this file. This typo may cause the flux scheduler to not recognize this directive.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At toolchain/templates/tuo.mako, line 13:

<comment>Missing space after `flux:` - inconsistent with all other flux directives in this file. This typo may cause the flux scheduler to not recognize this directive.</comment>

<file context>
@@ -0,0 +1,61 @@
+# flux: --error="${name}.err"
+# flux: --time=${walltime}
+# flux: --exclusive
+# flux:--setattr=thp=always
+# flux: --coral2-hugepages=512GB
+% if account:
</file context>
Suggested change
# flux:--setattr=thp=always
# flux: --setattr=thp=always

# flux: --coral2-hugepages=512GB
% if account:
# flux: --bank=${account}
% endif
% if partition:
# flux: --queue=${partition}
% endif
% endif

${helpers.template_prologue()}

ok ":) Loading modules:\n"
Copy link
Contributor

@codeant-ai codeant-ai bot Jan 13, 2026

Choose a reason for hiding this comment

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

Suggestion: Runtime error: ok is not a standard shell builtin/command and will cause "command not found" when the generated script runs; replace it with a portable print command (use printf or echo) so the message is actually emitted. [possible bug]

Severity Level: Critical 🚨

Suggested change
ok ":) Loading modules:\n"
printf "%s\n" ":) Loading modules:"
Why it matters? ⭐

The template emits a shell script. "ok" is not a standard shell builtin/command and will cause "command not found" when the script runs. Replacing it with a portable print (printf or echo) fixes a real runtime error.

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

**Path:** toolchain/templates/tuo.mako
**Line:** 25:25
**Comment:**
	*Possible Bug: Runtime error: `ok` is not a standard shell builtin/command and will cause "command not found" when the generated script runs; replace it with a portable print command (use `printf` or `echo`) so the message is actually emitted.

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.

cd "${MFC_ROOT_DIR}"
% if engine == 'batch':
. ./mfc.sh load -c t -m ${'g' if gpu else 'c'}
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Module load command uses wrong computer identifier.

The command uses -c t but the module slug defined in toolchain/modules is tuo, not t. This will cause the module loading to fail with "Computer not recognized" error.

Proposed fix 
-. ./mfc.sh load -c t -m ${'g' if gpu else 'c'}
+. ./mfc.sh load -c tuo -m ${'g' if gpu else 'c'}
📝 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.

Suggested change
. ./mfc.sh load -c t -m ${'g' if gpu else 'c'}
. ./mfc.sh load -c tuo -m ${'g' if gpu else 'c'}
🤖 Prompt for AI Agents 
In @toolchain/templates/tuo.mako at line 28, The module load command is using
the wrong computer identifier ("-c t") which doesn't match the module slug
"tuo"; update the command in the template (the line invoking "./mfc.sh load -c t
-m ${'g' if gpu else 'c'}") to use "-c tuo" so it passes the correct computer
slug when invoking mfc.sh.

% endif
cd - > /dev/null
echo

% if gpu:
export MPICH_GPU_SUPPORT_ENABLED=1
% else:
export MPICH_GPU_SUPPORT_ENABLED=0
% endif

export HSA_XNACK=0

% for target in targets:
${helpers.run_prologue(target)}

% if not mpi:
(set -x; ${profiler} "${target.get_install_binpath(case)}")
% else:
(set -x; flux run \
--nodes=${nodes} --ntasks=${tasks_per_node * nodes} \
-o spindle.level=off --exclusive \
% if gpu:
--gpus-per-task 1 \
% endif
${profiler} "${target.get_install_binpath(case)}")
% endif

${helpers.run_epilogue(target)}

echo
% endfor

${helpers.template_epilogue()}
Loading