Probe the node with a solver binary, not just syscheck - #1871
Open
sbryngelson wants to merge 2 commits into
Open
Probe the node with a solver binary, not just syscheck#1871sbryngelson wants to merge 2 commits into
sbryngelson wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Updates the CI preflight to detect CPU microarchitecture mismatches by probing the node with a real solver binary (pre_process) after syscheck passes, so SIGILL failures can be auto-excluded instead of requiring manual node bans.
Changes:
- Add an
isa_probethat runs the newestbuild/install/**/pre_processand excludes the node only on SIGILL. - Invoke
isa_probeafter a successfulsyscheckprobe.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+155
to
+159
| if [ "${#launcher[@]}" -eq 0 ]; then | ||
| isa_out=$("$isa_bin" 2>&1) || isa_rc=$? | ||
| else | ||
| isa_out=$("${launcher[@]}" "$isa_bin" 2>&1) || isa_rc=$? | ||
| fi |
Comment on lines
+150
to
+151
| isa_bin=$(find build/install -name pre_process -type f -printf '%T@ %p\n' 2>/dev/null \ | ||
| | sort -rn | head -1 | cut -d' ' -f2-) |
| fi | ||
|
|
||
| case "$isa_rc:$isa_out" in | ||
| 132:*|*:*"Illegal instruction"*) |
|
Claude Code Review Head SHA: 63483e8 Files changed:
Findings:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#1865 excluded
atl1-1-01-002-28-0by hand. This closes the gap that made the hand-edit necessary, so the next such node is caught by the machinery that already exists.Why the existing probe missed it
node-exclude.shsays the preflight is meant to catch "SIGILL from a binary built for another microarchitecture", andrun_case_optimization.shnotes Phoenix's compute nodes are heterogeneous. The wiring is all there - the case-opt lane callspreflight.sh, which emitsMFC_FAULT_NODEforsubmit-slurm-job.shto read back and exclude.It never fired, because the probe is
syscheck. syscheck initialises MPI, makes a device context and launches a trivial kernel - a few hundred lines that do not contain the wide vector instructions the solver does. A binary built on one microarchitecture and run on an older one passes it and then dies in the real work.Measured over 2026-09-11..12:
atl1-1-01-002-28-0SIGILLed inpre_processon all five case-optimization benchmarks, across four attempts spanning 24 hours, ats_assign_default_values_to_user_inputs. syscheck passed on that node every time. No passing Case Opt job on any PR ran there; the same node also produced tolerance failures on #1843's test suite and the "non-physical phasic density" aborts quoted in #1865.Local work that pinned it down, for the record: the change #1845 was blamed for is provably inert (the toolchain's generated Fortran is byte-identical at
num_probes_max10 and 64, and the constant appears nowhere outsidesrc/simulation/), and CI's exact build - same sluggpu-mp-0e981924c0, case-optimized,--gpu mp, MPI - runs clean under both NVHPC 24.1 and 25.5 here. What #1845 actually did was changem_constants.fppand so force a full rebuild, which is what put a freshly-built binary on a node it had not been built for.What this does
After syscheck passes, probe again with the newest
pre_processunderbuild/install. It is compiled from the same vectorised Fortran the tests run, and it reachess_assign_default_values_to_user_inputs- exactly where the observed SIGILL landed - before it needs an input file, so it faults on a mismatched node without a case directory.Only SIGILL counts. Run without a case,
pre_processfails for a dozen ordinary reasons - no input file, no restart data, a missing module - and none of them say anything about the node. Anything that is not signal 4 is logged and ignored:That asymmetry is deliberate and matches the rest of the script, which already refuses to judge a node on a launch that never happened: excluding a healthy node costs three allocations and is far worse than missing one bad one.
If no
pre_processis built yet, the probe returns without an opinion, so nothing changes for callers that have not built one.Testing
Exercised the function directly against two stubs, since reproducing a real microarchitecture mismatch needs two unlike nodes:
SIGILL- emitsMFC_FAULT_NODE=<node>and exits 77, so the wrapper excludes and resubmits.bash -nclean,./mfc.sh precheckpasses 7/7.The real check is the next time a node mismatch happens: it should exclude itself and resubmit instead of failing five benchmarks four times over. Worth keeping #1865's static entry either way - this is the safety net, not a replacement.