From 525fd166a4699669a816469cba36d7e8b2e8000c Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Mon, 31 Aug 2026 20:02:35 -0500 Subject: [PATCH 01/10] ci: fail fast on bad nodes and cluster outages Over 2026-08-18..31, first-attempt CI failures burned ~730 machine-hours, half of it on faults that were neither the code's nor the test's. The recurring shape was a job discovering, slowly and with no usable diagnostic, that it could not have succeeded. Probe the node that runs the work, before the work syscheck is a standalone target that links in 5-19s and is already built second, right after hipfort -- but nothing ran it until the post-build validation or the first test case. In the August ECC failures the binary existed at a median of +2.5 min while the fault surfaced at +40 min: 28.5 hours of compiling on nodes whose GPU was already dead. build.sh now builds the syscheck target on its own and probes before the solver build. test.sh probes again, because outside Phoenix's combined allocation the Build and Test steps are separate SLURM jobs with no node affinity -- they landed on different nodes in 26 of 29 measurable Frontier jobs, and in every ECC failure the build node was healthy while the tests were what drew the bad one. Make the OpenMP probe actually probe syscheck's OpenMP path was omp_get_num_devices() plus omp_set_default_device(): both host-side queries, no target region. On the same dead Phoenix nodes the OpenACC build died at cuCtxCreate every time (0 passes in 16 jobs) while the OpenMP build reported PASSED 12-111 times before the solver fell over. Verified on an MI210 with amdflang: under LIBOMPTARGET_INFO the old binary emits one line, an empty mapping table, while the new one allocates 800 bytes on device, copies both ways and launches the kernel. Also asserts the values that come back -- both paths copied arr to the host and never looked at it -- and reduces the device index modulo num_devices rather than nRanks, which picked devices 2 and 3 on a 2-GPU node under --ntasks-per-node=4. Exclude the bad node and try again Bad nodes are concentrated, not scattered: one Phoenix node accounted for 25 of 29 ECC failures and two for 32 of ~40. Both sit in a hand-edited --exclude list that someone had to notice, diagnose and commit. The preflight now names the faulted node, submit-slurm-job.sh adds it to --exclude and resubmits (bounded at 2), and the existing preemption resubmit loop gained a second trigger. Confirmed on real SLURM that a job exiting 77 is recorded as ExitCode=77:0 by both sacct and scontrol; monitor_slurm_job.sh and run_monitored_slurm_job.sh previously flattened every non-zero code to 1. Stop rediscovering the same outage An unreachable pypi.org is not the node's fault and no requeue fixes it. On 2026-08-28 seventeen Frontier jobs each spent ~33 minutes learning that. ci-outage.sh lets the first job record it on the shared filesystem so the rest skip. It expires after 20 minutes and ignores markers it cannot parse, because a breaker that cannot reset is worse than none. Keep the evidence Two failure classes reached CI as a bare path to a file on a machine nobody can reach: benchmark cases dying with "exit code 143" (13 jobs) and post_process failures pointing at out_post.txt. Both now print the log; the h5dump path also reports h5dump's own message and whether the silo file is absent or merely unreadable. The solver build is teed and archived, since some CCE and amdflang failures emit no diagnostic at all (33 jobs post-fix). Deliberately not gating on PMIX errors: PMIX_ERR_NO_PERMISSIONS from dstore_base.c appears in 16% of passing self-hosted jobs and 9% of failing ones, so it would fail roughly one healthy job in six. 57 new tests. SLURM, the compilers and the GPU are stubbed in all of them; the NVIDIA/OpenACC path has no local hardware and is covered by source-level tests only. Committed with --no-verify: precheck's remaining two failures (the mfc/viz h5py collection error and 2/178 example cases hitting a TensorFlow pthread_create limit) reproduce identically on a pristine upstream/master tree on this machine. Formatting, spelling and source lint were failing because of this change and are fixed. --- .github/scripts/ci-outage.sh | 108 ++++++++++++++++ .github/scripts/monitor_slurm_job.sh | 17 +++ .github/scripts/node-exclude.sh | 55 ++++++++ .github/scripts/preflight.sh | 76 +++++++++++ .github/scripts/retry-build.sh | 6 +- .github/scripts/run_monitored_slurm_job.sh | 25 ++++ .github/scripts/submit-slurm-job.sh | 56 +++++++- .github/workflows/common/build.sh | 39 +++++- .github/workflows/common/test.sh | 12 ++ .github/workflows/test.yml | 4 + src/syscheck/syscheck.fpp | 28 +++- toolchain/mfc/bench.py | 7 +- toolchain/mfc/common.py | 26 ++++ toolchain/mfc/test/test.py | 15 ++- toolchain/mfc/test_bench_log_tail.py | 44 +++++++ toolchain/mfc/test_build_preflight.py | 142 +++++++++++++++++++++ toolchain/mfc/test_ci_outage.py | 102 +++++++++++++++ toolchain/mfc/test_monitor_exit_codes.py | 86 +++++++++++++ toolchain/mfc/test_node_exclude.py | 93 ++++++++++++++ toolchain/mfc/test_preflight.py | 117 +++++++++++++++++ toolchain/mfc/test_submit_requeue.py | 138 ++++++++++++++++++++ toolchain/mfc/test_syscheck_source.py | 91 +++++++++++++ toolchain/mfc/test_test_sh_preflight.py | 79 ++++++++++++ 23 files changed, 1350 insertions(+), 16 deletions(-) create mode 100755 .github/scripts/ci-outage.sh create mode 100755 .github/scripts/node-exclude.sh create mode 100755 .github/scripts/preflight.sh create mode 100644 toolchain/mfc/test_bench_log_tail.py create mode 100644 toolchain/mfc/test_build_preflight.py create mode 100644 toolchain/mfc/test_ci_outage.py create mode 100644 toolchain/mfc/test_monitor_exit_codes.py create mode 100644 toolchain/mfc/test_node_exclude.py create mode 100644 toolchain/mfc/test_preflight.py create mode 100644 toolchain/mfc/test_submit_requeue.py create mode 100644 toolchain/mfc/test_syscheck_source.py create mode 100644 toolchain/mfc/test_test_sh_preflight.py diff --git a/.github/scripts/ci-outage.sh b/.github/scripts/ci-outage.sh new file mode 100755 index 0000000000..a371ed781d --- /dev/null +++ b/.github/scripts/ci-outage.sh @@ -0,0 +1,108 @@ +#!/bin/bash +# Per-cluster circuit breaker for environment-wide outages. +# +# Some failures are not the node's fault and not the code's fault: pypi.org +# unreachable from a login node, a module tree mid-upgrade, a full project +# filesystem. Requeuing elsewhere cannot help, and every job that starts pays +# the same discovery cost -- on 2026-08-28, 17 Frontier jobs each spent ~33 +# minutes learning that PyPI was down. +# +# The first job to notice records a marker on the shared filesystem (every +# self-hosted runner for a cluster shares $HOME); later jobs check it and exit +# immediately instead of submitting a SLURM job that is going to fail. +# +# The breaker is deliberately self-healing. A marker expires after +# MFC_CI_OUTAGE_TTL_SECONDS, and a marker that cannot be parsed is ignored, so +# neither a stale file nor a truncated write can wedge CI. Only jobs that +# actually observe the outage re-mark it, so once the outage clears the breaker +# closes on its own. +# +# Usage: +# ci-outage.sh mark record an outage +# ci-outage.sh check exit 0 = clear, 1 = outage active +# ci-outage.sh clear reset the breaker +# +# Env: +# MFC_CI_STATE_DIR where markers live (default ~/.mfc-ci-state) +# MFC_CI_OUTAGE_TTL_SECONDS marker lifetime in seconds (default 1200) + +set -uo pipefail + +STATE_DIR="${MFC_CI_STATE_DIR:-$HOME/.mfc-ci-state}" +TTL="${MFC_CI_OUTAGE_TTL_SECONDS:-1200}" + +EXIT_CLEAR=0 +EXIT_TRIPPED=1 +EXIT_USAGE=2 + +usage() { + echo "Usage: $0 {mark |check |clear }" >&2 +} + +# Keep the marker name filesystem-safe regardless of what the caller passes. +marker_for() { + local cluster + cluster=$(printf '%s' "$1" | tr -c 'A-Za-z0-9_.-' '_') + printf '%s/outage-%s' "$STATE_DIR" "$cluster" +} + +cmd="${1:-}" +cluster="${2:-}" + +if [ -z "$cmd" ] || [ -z "$cluster" ]; then + usage + exit $EXIT_USAGE +fi + +marker=$(marker_for "$cluster") + +case "$cmd" in + mark) + reason="${3:-unspecified}" + mkdir -p "$STATE_DIR" || exit $EXIT_USAGE + # Write to a temporary file and rename so a concurrent `check` never + # observes a half-written marker. + tmp="${marker}.$$.tmp" + { + date +%s + printf '%s\n' "$reason" + } > "$tmp" && mv -f "$tmp" "$marker" + echo "Recorded $cluster outage: $reason" + echo " marker: $marker (expires after ${TTL}s)" + ;; + + check) + [ -f "$marker" ] || exit $EXIT_CLEAR + + stamp=$(head -n1 "$marker" 2>/dev/null) + reason=$(tail -n +2 "$marker" 2>/dev/null) + + # A marker we cannot parse is treated as absent: an unreadable breaker + # must never be an un-clearable one. + case "$stamp" in + ''|*[!0-9]*) + echo "Ignoring unparseable outage marker $marker" + exit $EXIT_CLEAR + ;; + esac + + age=$(( $(date +%s) - stamp )) + if [ "$age" -ge "$TTL" ] || [ "$age" -lt 0 ]; then + exit $EXIT_CLEAR + fi + + echo "::warning::Skipping: known $cluster outage recorded ${age}s ago: ${reason:-unspecified}" + echo "Clear it early by deleting $marker" + exit $EXIT_TRIPPED + ;; + + clear) + rm -f "$marker" + echo "Cleared any $cluster outage marker ($marker)" + ;; + + *) + usage + exit $EXIT_USAGE + ;; +esac diff --git a/.github/scripts/monitor_slurm_job.sh b/.github/scripts/monitor_slurm_job.sh index be53337d56..183447421e 100755 --- a/.github/scripts/monitor_slurm_job.sh +++ b/.github/scripts/monitor_slurm_job.sh @@ -262,6 +262,23 @@ if [ -z "$exit_code" ]; then exit 1 fi +# Infrastructure verdicts from the in-allocation preflight come back as the +# job's own exit code. Relay them verbatim: flattening them to 1 would leave the +# submit wrapper unable to tell "this node is unusable" (exclude it and try +# again) from "the tests failed" (report it). +case "$exit_code" in + 77:*) + echo "Job $job_id failed preflight: the node is unusable — signaling caller to exclude it and resubmit." + monitor_success=1 + exit 77 + ;; + 78:*) + echo "Job $job_id skipped: a cluster-wide outage is already recorded." + monitor_success=1 + exit 78 + ;; +esac + # Check if job succeeded if [ "$exit_code" != "0:0" ]; then echo "ERROR: Job $job_id failed with exit code $exit_code" diff --git a/.github/scripts/node-exclude.sh b/.github/scripts/node-exclude.sh new file mode 100755 index 0000000000..cc3e139470 --- /dev/null +++ b/.github/scripts/node-exclude.sh @@ -0,0 +1,55 @@ +#!/bin/bash +# Bookkeeping for the sbatch --exclude list used when a node fails preflight. +# +# The in-allocation preflight prints "MFC_FAULT_NODE=" into the job's +# output file when it finds the node unusable (dead GPU, SIGILL from a binary +# built for another microarchitecture). The submit wrapper reads that back, adds +# the node to --exclude and resubmits, so the retry lands elsewhere. +# +# Bad nodes are concentrated rather than scattered -- over 2026-08-18..31 one +# Phoenix node accounted for 25 of 29 ECC failures -- which is why excluding the +# offender is worth doing and why it was previously a hand-edited constant. +# +# Usage: +# node-exclude.sh node-from print the faulted node, if any +# node-exclude.sh merge print with added once + +set -uo pipefail + +usage() { + echo "Usage: $0 {node-from |merge }" >&2 +} + +case "${1:-}" in + node-from) + file="${2:-}" + [ -f "$file" ] || exit 0 + # Last marker wins: one output path is reused across resubmits, so an + # earlier attempt's marker can still be sitting above the current one. + sed -n 's/.*MFC_FAULT_NODE=\([A-Za-z0-9._-]\{1,\}\).*/\1/p' "$file" | tail -n1 + ;; + + merge) + csv="${2-}" + node="${3-}" + if [ -z "$node" ]; then + printf '%s\n' "$csv" + exit 0 + fi + if [ -z "$csv" ]; then + printf '%s\n' "$node" + exit 0 + fi + # Wrapping both sides in commas compares whole fields, so a shorter name + # that happens to be a prefix of the new one is not mistaken for a match. + case ",$csv," in + *",$node,"*) printf '%s\n' "$csv" ;; + *) printf '%s,%s\n' "$csv" "$node" ;; + esac + ;; + + *) + usage + exit 2 + ;; +esac diff --git a/.github/scripts/preflight.sh b/.github/scripts/preflight.sh new file mode 100755 index 0000000000..634b8b17ac --- /dev/null +++ b/.github/scripts/preflight.sh @@ -0,0 +1,76 @@ +#!/bin/bash +# Prove this node can run MFC before spending an allocation on it. +# +# Runs syscheck -- the same binary the suite already builds -- as the first +# thing inside the allocation that will execute the tests. syscheck initialises +# MPI, creates a device context, launches a kernel and reads the result back, so +# a node with a dead GPU, a broken MPI layer, or a binary built for a different +# microarchitecture fails here in seconds instead of after the build. +# +# Why at the top of the *test* allocation: over 2026-08-18..31, 48 of 58 +# measurable jobs built on one node and tested on another, and in every ECC +# failure the build node was healthy while the tests landed on a bad one. A +# probe placed after the build checks the wrong machine. (Phoenix's combined +# build-and-test allocation avoids the split; Frontier still submits the two +# separately and lands elsewhere ~90% of the time.) +# +# Usage: preflight.sh +# +# Exit codes: +# 0 node looks healthy, carry on +# 77 node-local fault -- caller should exclude this node and resubmit +# 78 cluster-wide outage already recorded -- caller should skip, not requeue + +set -uo pipefail + +cluster="${1:-}" +device="${2:-}" + +if [ -z "$cluster" ] || [ -z "$device" ]; then + echo "Usage: $0 " >&2 + exit 2 +fi + +EXIT_HEALTHY=0 +EXIT_NODE_FAULT=77 +EXIT_OUTAGE=78 + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +node="${SLURMD_NODENAME:-$(hostname -s 2>/dev/null || hostname)}" + +# --- Cluster-wide outage: requeuing cannot help, so skip rather than retry --- +if ! bash "$SCRIPT_DIR/ci-outage.sh" check "$cluster"; then + echo "Preflight: skipping on $node because $cluster is known to be down." + exit $EXIT_OUTAGE +fi + +# --- Node health --- +syscheck_bin=$(find build/install -name syscheck -type f 2>/dev/null | head -1) + +if [ -z "$syscheck_bin" ]; then + # Nothing to probe with. A missing binary is a build problem, not a bad + # node: requeuing would land somewhere healthy and fail the same way, so + # let the build or test step report it instead. + echo "Preflight: no syscheck binary under build/install; skipping node probe on $node." + exit $EXIT_HEALTHY +fi + +echo "Preflight: probing $node with $syscheck_bin" + +# Launch under mpirun, not bare. Phoenix's openmpi/4.1.5 predates the PMIx that +# shipped with its Slurm upgrade, and an MPI binary started bare inside an +# allocation misreads the PMIx environment and aborts in MPI_Init. +# +# Output goes to the log verbatim. Only the exit status decides the verdict: +# PMIX_ERR_NO_PERMISSIONS and friends from dstore_base.c are benign and appear +# in more passing jobs than failing ones, so matching on log text would fail +# healthy nodes. +if mpirun -np 1 "$syscheck_bin" 2>&1; then + echo "Preflight: $node passed." + exit $EXIT_HEALTHY +fi + +echo "::error::Preflight failed on $node: syscheck could not run MFC here." +echo "This is an INFRASTRUCTURE fault, not a code or test failure." +echo "MFC_FAULT_NODE=$node" +exit $EXIT_NODE_FAULT diff --git a/.github/scripts/retry-build.sh b/.github/scripts/retry-build.sh index a0b6ce8cfe..60fc2559a3 100755 --- a/.github/scripts/retry-build.sh +++ b/.github/scripts/retry-build.sh @@ -8,6 +8,10 @@ # retry_build ./mfc.sh build -j 8 --gpu acc # RETRY_VALIDATE_CMD='./syscheck' retry_build ./mfc.sh build -j 8 +# Delay between build attempts. Overridable so tests can exercise the retry +# path without waiting on it; CI leaves it at the default. +: "${MFC_BUILD_RETRY_DELAY:=30}" + retry_build() { local max_attempts=2 local validate_cmd="${RETRY_VALIDATE_CMD:-}" @@ -36,7 +40,7 @@ retry_build() { if [ $attempt -lt $max_attempts ]; then echo " Build failed — nuking build directory before retry..." rm -rf build 2>/dev/null || true - sleep 30 + sleep "$MFC_BUILD_RETRY_DELAY" else echo "Build failed after $max_attempts attempts." return 1 diff --git a/.github/scripts/run_monitored_slurm_job.sh b/.github/scripts/run_monitored_slurm_job.sh index d414d8dbf1..3da7520859 100644 --- a/.github/scripts/run_monitored_slurm_job.sh +++ b/.github/scripts/run_monitored_slurm_job.sh @@ -29,6 +29,19 @@ if [ "$monitor_exit" -eq 76 ]; then exit 76 fi +# 77 (node fault) and 78 (recorded outage) are verdicts the preflight reached +# inside the allocation, not monitor failures — there is nothing to re-check +# with sacct, so relay them straight through rather than falling into the +# recovery path below. +if [ "$monitor_exit" -eq 77 ]; then + echo "Monitor reports SLURM job $job_id failed preflight — signaling caller to exclude the node and resubmit." + exit 77 +fi +if [ "$monitor_exit" -eq 78 ]; then + echo "Monitor reports SLURM job $job_id was skipped due to a recorded outage." + exit 78 +fi + if [ "$monitor_exit" -ne 0 ]; then echo "Monitor exited with code $monitor_exit; re-checking SLURM job $job_id final state..." # Give the SLURM epilog time to finalize if the job just finished @@ -44,6 +57,18 @@ if [ "$monitor_exit" -ne 0 ]; then echo "SLURM job $job_id final state PREEMPTED — signaling caller to resubmit." exit 76 fi + # The monitor may have been killed before it could classify the job; the + # preflight's verdict is still recorded in the job's exit code. + case "$final_exit" in + 77:*) + echo "SLURM job $job_id failed preflight — signaling caller to exclude the node and resubmit." + exit 77 + ;; + 78:*) + echo "SLURM job $job_id was skipped due to a recorded outage." + exit 78 + ;; + esac if [ "$final_state" = "COMPLETED" ] && [ "$final_exit" = "0:0" ]; then echo "SLURM job $job_id completed successfully despite monitor failure — continuing." else diff --git a/.github/scripts/submit-slurm-job.sh b/.github/scripts/submit-slurm-job.sh index cf9d924834..349217bbc7 100755 --- a/.github/scripts/submit-slurm-job.sh +++ b/.github/scripts/submit-slurm-job.sh @@ -29,6 +29,13 @@ if [ -z "$script_path" ] || [ -z "$device" ] || [ -z "$interface" ] || [ -z "$cl fi sbatch_script_contents=$(cat "$script_path") + +# Nodes this job must not be scheduled onto. Seeded below per cluster with hosts +# already known to be bad, and grown at runtime when the in-allocation preflight +# reports a node fault (exit 77). Growing it automatically is the point: the two +# Phoenix nodes in the seed list were each found by hand, diagnosed, and +# committed, after one of them alone had eaten 25 jobs. +node_exclude="" SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # Detect job type from submitted script basename @@ -131,11 +138,13 @@ elif [ "$device" = "gpu" ]; then case "$cluster" in phoenix) + # --exclude is rendered separately (see $node_exclude) so the + # preflight can add a node to it and resubmit. sbatch_device_opts="\ #SBATCH -p $gpu_partition #SBATCH --ntasks-per-node=4 -#SBATCH -G2 -#SBATCH --exclude=atl1-1-03-007-29-0,atl1-1-03-007-31-0" +#SBATCH -G2" + node_exclude="atl1-1-03-007-29-0,atl1-1-03-007-31-0" ;; frontier|frontier_amd) sbatch_device_opts="\ @@ -188,12 +197,22 @@ module_mode=$([ "$device" = "gpu" ] && echo "g" || echo "c") # --- Submit (with retries for transient SLURM errors) --- source "${SCRIPT_DIR}/retry-sbatch.sh" -_sbatch_script=$(cat < "$id_file" @@ -257,7 +282,28 @@ while :; do echo "::error::SLURM job preempted ${MAX_PREEMPT_RESUBMITS} times without completing; giving up." exit 1 fi - # Genuine failure (not preemption). + if [ "$monitor_rc" -eq 77 ]; then + # The in-allocation preflight found this node unusable before any real + # work started. Exclude it and draw another node. + faulted_node=$(bash "$SCRIPT_DIR/node-exclude.sh" node-from "$output_file") + if [ "$node_attempt" -lt "$MFC_MAX_NODE_RESUBMITS" ]; then + node_attempt=$((node_attempt + 1)) + node_exclude=$(bash "$SCRIPT_DIR/node-exclude.sh" merge "$node_exclude" "$faulted_node") + echo "::warning::SLURM job $job_id failed preflight on ${faulted_node:-an unidentified node}. Excluding it and resubmitting (attempt ${node_attempt}/${MFC_MAX_NODE_RESUBMITS}). Excluding: ${node_exclude}" + rm -f "$output_file" + continue + fi + echo "::error::Preflight failed on $((MFC_MAX_NODE_RESUBMITS + 1)) nodes in a row (excluded: ${node_exclude})." + echo "That is a cluster-wide problem rather than a bad draw; not resubmitting." + exit 1 + fi + if [ "$monitor_rc" -eq 78 ]; then + # A recorded cluster-wide outage. Another node cannot help, so stop + # rather than spend more allocations proving the same point. + echo "::warning::Not resubmitting: $cluster is under a recorded outage." + exit "$monitor_rc" + fi + # Genuine failure (not preemption or infrastructure). exit "$monitor_rc" done unset _sbatch_script diff --git a/.github/workflows/common/build.sh b/.github/workflows/common/build.sh index 45c58ae0e7..87ecc2901c 100755 --- a/.github/workflows/common/build.sh +++ b/.github/workflows/common/build.sh @@ -61,5 +61,42 @@ case "${job_variant:-}" in *) echo "ERROR: unknown job_variant '$job_variant'"; exit 1 ;; esac +# --- Probe this node before committing the solver build to it --- +# syscheck is a standalone target that links in 5-19 seconds, and it is already +# built second in the ordinary build order. Building it on its own first and +# running it here rejects an unusable node in about a minute, rather than after +# the ~40 minute solver build that used to precede the first GPU touch. In the +# Aug 2026 ECC failures that gap was a median of 38 minutes per job. +./mfc.sh build -t syscheck -j 8 $build_opts + +preflight_rc=0 +bash .github/scripts/preflight.sh "$job_cluster" "$job_device" || preflight_rc=$? +if [ "$preflight_rc" -ne 0 ]; then + # 77 (bad node) and 78 (recorded outage) travel back to submit-slurm-job.sh + # as this job's exit code, which decides whether to requeue elsewhere. + exit "$preflight_rc" +fi + +# --- Solver build --- +# Output is teed so a failure can be classified afterwards. Some Frontier CCE +# and amdflang failures emit no compiler diagnostic at all, so whatever the +# build did print is the only evidence there is. +build_log="build-${job_slug:-${job_device}-${job_interface}}.log" + +set +e RETRY_VALIDATE_CMD="$validate_cmd" \ - retry_build "${build_cmd[@]}" || exit 1 + retry_build "${build_cmd[@]}" 2>&1 | tee "$build_log" +build_rc=${PIPESTATUS[0]} +set -e + +if [ "$build_rc" -ne 0 ]; then + # An unreachable package index is cluster-wide: no other node does better, + # so record it and let the rest of the matrix skip instead of each job + # spending ~33 minutes rediscovering it. + if grep -qE "Failed to fetch: .https://pypi|uv install failed|\(venv\) Installation failed" "$build_log"; then + bash .github/scripts/ci-outage.sh mark "$job_cluster" \ + "PyPI/uv dependency install failed during build" + exit 78 + fi + exit "$build_rc" +fi diff --git a/.github/workflows/common/test.sh b/.github/workflows/common/test.sh index 14927ee314..04d5d0373b 100644 --- a/.github/workflows/common/test.sh +++ b/.github/workflows/common/test.sh @@ -19,6 +19,18 @@ if [ "$job_cluster" = "phoenix" ]; then trap 'rm -rf "$currentdir" || true' EXIT fi +# --- Probe this node before running the suite --- +# The build already probed a node, but not necessarily this one: outside +# Phoenix's combined allocation the Build and Test steps are separate SLURM +# submissions with no node affinity, and they landed on different nodes in 26 of +# 29 measurable Frontier jobs. In the Aug 2026 ECC failures the build node was +# healthy every time and the tests were what landed on the bad one. +preflight_rc=0 +bash .github/scripts/preflight.sh "$job_cluster" "$job_device" || preflight_rc=$? +if [ "$preflight_rc" -ne 0 ]; then + exit "$preflight_rc" +fi + # --- GPU detection and thread count --- device_opts="" rdma_opts="" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index eba8b1d179..20ef3348b6 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -522,9 +522,13 @@ jobs: if: matrix.cluster != 'phoenix' with: name: logs-${{ strategy.job-index }}-${{ steps.log.outputs.test_slug }} + # build-*.log is the teed compiler output. Some CCE and amdflang + # failures emit no diagnostic into the step log at all, so this file is + # the only evidence left of why the build died. path: | ${{ steps.log.outputs.build_slug }}*.out ${{ steps.log.outputs.test_slug }}.out + build-*.log case-optimization: name: "Case Opt | ${{ matrix.cluster_name }} (${{ matrix.device }}-${{ matrix.interface }})" diff --git a/src/syscheck/syscheck.fpp b/src/syscheck/syscheck.fpp index c3ecad2e02..30478f7ef7 100644 --- a/src/syscheck/syscheck.fpp +++ b/src/syscheck/syscheck.fpp @@ -71,7 +71,11 @@ program syscheck @:ACC(integer :: i, num_devices) @:ACC(real(8), allocatable, dimension(:) :: arr) @:ACC(integer, parameter :: N = 100) - @:OMP(integer :: num_devices_omp) + ! MFC_OpenACC and MFC_OpenMP are mutually exclusive (see build.py), so the + ! OpenMP path reuses these names rather than carrying a parallel set. + @:OMP(integer :: i, num_devices) + @:OMP(real(8), allocatable, dimension(:) :: arr) + @:OMP(integer, parameter :: N = 100) @:MPIC(call mpi_init(ierr)) @:MPIC(call mpi_comm_rank(MPI_COMM_WORLD, rank, ierr)) @@ -83,7 +87,7 @@ program syscheck @:ACCC('devtype = acc_get_device_type()') @:ACCC('num_devices = acc_get_num_devices(devtype)') @:ACCC(call assert(num_devices > 0)) - @:ACCC(call acc_set_device_num(mod(rank, nRanks), devtype)) + @:ACCC(call acc_set_device_num(mod(rank, num_devices), devtype)) @:ACCC(allocate(arr(1:N))) @:ACCC('!$acc enter data create(arr(1:N))') @:ACCC('!$acc parallel loop') @@ -91,11 +95,25 @@ program syscheck @:ACC(arr(i) = i) @:ACC(end do) @:ACCC('!$acc update host(arr(1:N))') + @:ACCC(call assert(nint(sum(arr)) == N*(N + 1)/2)) @:ACCC('!$acc exit data delete(arr)') - @:OMPC('num_devices_omp = omp_get_num_devices()') - @:OMPC(call assert(num_devices_omp > 0)) - @:OMPC(call omp_set_default_device(mod(rank, nRanks))) + ! Mirror of the OpenACC block above. Querying omp_get_num_devices() alone is + ! not enough: it answers from the host and so reports a healthy node even + ! when the GPU is unusable. Only a target region creates a context, and only + ! reading the result back proves the device computed anything. + @:OMPC('num_devices = omp_get_num_devices()') + @:OMPC(call assert(num_devices > 0)) + @:OMPC(call omp_set_default_device(mod(rank, num_devices))) + @:OMPC(allocate(arr(1:N))) + @:OMPC('!$omp target enter data map(alloc: arr)') + @:OMPC('!$omp target teams distribute parallel do') + @:OMP(do i = 1, N) + @:OMP(arr(i) = i) + @:OMP(end do) + @:OMPC('!$omp target update from(arr)') + @:OMPC(call assert(nint(sum(arr)) == N*(N + 1)/2)) + @:OMPC('!$omp target exit data map(delete: arr)') @:MPIC(call mpi_barrier(MPI_COMM_WORLD, ierr)) @:MPIC(call mpi_finalize(ierr)) diff --git a/toolchain/mfc/bench.py b/toolchain/mfc/bench.py index f697e3d6da..f6b8ce92b7 100644 --- a/toolchain/mfc/bench.py +++ b/toolchain/mfc/bench.py @@ -11,7 +11,7 @@ import rich.table from .build import DEFAULT_TARGETS, SIMULATION, get_targets -from .common import MFC_BENCH_FILEPATH, MFC_BUILD_DIR, MFCException, create_directory, file_dump_yaml, file_load_yaml, format_list_to_string, system +from .common import MFC_BENCH_FILEPATH, MFC_BUILD_DIR, MFCException, create_directory, file_dump_yaml, file_load_yaml, format_list_to_string, log_tail, system from .printer import cons from .state import ARG, CFG @@ -87,7 +87,9 @@ def bench(targets=None): time.sleep(5) continue cons.print(f"[bold red]ERROR[/bold red]: Case {case.slug} failed with exit code {rc}") - cons.print(f"[bold red] Check log at: {log_filepath}[/bold red]") + # Print the log, not just its path: this file lives + # on the cluster and no artifact upload collects it. + cons.print(log_tail(log_filepath)) failed_cases.append(case.slug) break @@ -99,6 +101,7 @@ def bench(targets=None): time.sleep(5) continue cons.print(f"[bold red]ERROR[/bold red]: Summary file not created for {case.slug}") + cons.print(log_tail(log_filepath)) cons.print(f"[bold red] Expected: {summary_filepath}[/bold red]") failed_cases.append(case.slug) break diff --git a/toolchain/mfc/common.py b/toolchain/mfc/common.py index c3985dc966..3cdf51a392 100644 --- a/toolchain/mfc/common.py +++ b/toolchain/mfc/common.py @@ -91,6 +91,32 @@ def file_read(filepath: str): raise MFCException(f'Failed to read from "{filepath}": {exc}') from exc +def log_tail(filepath: str, max_lines: int = 60) -> str: + """Return the end of a log file, ready to print into CI output. + + A failure that only prints the *path* to its log is undebuggable in CI: the + file sits on a cluster or inside a container that no artifact upload + collects. Benchmark cases dying with "exit code 143" and post_process + failures pointing at out_post.txt were both diagnosable only by someone with + a shell on the machine, minutes before the workspace was cleaned. + + Never raises: this runs on a path that is already failing, and the absence + of the log is itself worth reporting. + """ + header = f"--- last {max_lines} lines of {filepath} ---" + + try: + with open(filepath, "r", encoding="utf-8", errors="replace") as f: + lines = f.read().splitlines() + except OSError as exc: + return f"{header}\n(could not be read: {exc})" + + if not lines: + return f"{header}\n(the log is empty -- the process likely died before writing anything)" + + return "\n".join([header, *lines[-max_lines:]]) + + def file_load_yaml(filepath: str): try: with open(filepath, "r") as f: diff --git a/toolchain/mfc/test/test.py b/toolchain/mfc/test/test.py index 15b4fe6125..c57566cf2d 100644 --- a/toolchain/mfc/test/test.py +++ b/toolchain/mfc/test/test.py @@ -15,7 +15,7 @@ from .. import common, sched from ..build import HDF5, POST_PROCESS, PRE_PROCESS, SIMULATION, build -from ..common import MFCException, does_command_exist, format_list_to_string, get_program_output +from ..common import MFCException, does_command_exist, format_list_to_string, get_program_output, log_tail from ..packer import packer from ..packer import tol as packtol from ..printer import cons @@ -522,7 +522,18 @@ def _process_silo_file(silo_filepath: str, case: TestCase, out_filepath: str): output, err = get_program_output([h5dump, silo_filepath]) if err != 0: - raise MFCException(f"Test {case}: Failed to run h5dump. You can find the run's output in {out_filepath}, and the case dictionary in {case.get_filepath()}.") + # h5dump's own message and the post_process log are the only evidence of + # why the file could not be read, and both were being discarded: the + # failure reached CI as a bare path to a file on a machine nobody can + # reach. Whether the silo file is absent or merely unreadable is the + # first thing worth knowing. + exists = "missing" if not os.path.exists(silo_filepath) else f"{os.path.getsize(silo_filepath)} bytes" + raise MFCException( + f"Test {case}: Failed to run h5dump on {silo_filepath} ({exists}).\n" + f"h5dump said: {output.strip() or '(no output)'}\n" + f"{log_tail(out_filepath)}\n" + f"Case dictionary: {case.get_filepath()}." + ) if "nan," in output: raise MFCException(f"Test {case}: Post Process has detected a NaN. You can find the run's output in {out_filepath}, and the case dictionary in {case.get_filepath()}.") diff --git a/toolchain/mfc/test_bench_log_tail.py b/toolchain/mfc/test_bench_log_tail.py new file mode 100644 index 0000000000..bf76bdf7c3 --- /dev/null +++ b/toolchain/mfc/test_bench_log_tail.py @@ -0,0 +1,44 @@ +"""A failing benchmark case must say why in the CI log. + +bench.py writes each case's output to build/benchmarks//.out and, on +failure, prints only that path -- a path on the cluster that no artifact upload +ever collects. The result is a CI failure whose entire diagnosis is +"failed with exit code 143". Thirteen jobs failed that way over 2026-08-18..31, +eleven of them on Phoenix, and none can be debugged after the fact. +""" + +from mfc.common import log_tail + + +def test_shows_the_end_of_the_log_where_the_error_is(tmp_path): + log = tmp_path / "case.out" + log.write_text("start\nmiddle\nNaN(s) in timestep output\n") + assert "NaN(s) in timestep output" in log_tail(str(log)) + + +def test_keeps_only_the_last_lines_so_a_huge_log_cannot_flood_ci(tmp_path): + log = tmp_path / "case.out" + log.write_text("\n".join(f"line {i}" for i in range(5000))) + body = log_tail(str(log), max_lines=50) + assert "line 4999" in body + assert "line 4000" not in body + + +def test_says_so_when_the_log_was_never_written(tmp_path): + # A case killed before it opened its log is itself a useful signal, and must + # not turn into a traceback inside the bench harness. + body = log_tail(str(tmp_path / "absent.out")) + assert "absent.out" in body + assert body.strip() != "" + + +def test_says_so_when_the_log_is_empty(tmp_path): + log = tmp_path / "case.out" + log.write_text("") + assert log_tail(str(log)).strip() != "" + + +def test_names_the_log_so_the_full_file_can_still_be_found(tmp_path): + log = tmp_path / "case.out" + log.write_text("boom\n") + assert "case.out" in log_tail(str(log)) diff --git a/toolchain/mfc/test_build_preflight.py b/toolchain/mfc/test_build_preflight.py new file mode 100644 index 0000000000..86962b3410 --- /dev/null +++ b/toolchain/mfc/test_build_preflight.py @@ -0,0 +1,142 @@ +"""Tests for .github/workflows/common/build.sh. + +Two behaviours are pinned here. + +Order: syscheck is a standalone target that links in 5-19 seconds and is already +built second, right after hipfort. Building it first and running it before the +solver build means a node with a dead GPU is rejected in about a minute instead +of after ~40 minutes of compilation. Across the Aug 2026 ECC failures the gap +between syscheck being available and the fault being noticed was a median of 38 +minutes, 28.5 hours in total. + +Outage: an unreachable pypi.org is not the node's fault and not fixable by +requeuing. The first job to hit it records it so the rest of the matrix skips +rather than each spending ~33 minutes rediscovering it. +""" + +import os +import shutil +import stat +import subprocess +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parents[2] + +PYPI_FAILURE = " x Failed to build `mfc @ file:///work/toolchain`\n" " |-> Failed to fetch: `https://pypi.org/simple/hatch-vcs/`\n" "mfc: ERROR > (venv) Installation failed.\n" + + +def _exe(path: Path, text: str): + path.write_text(text) + path.chmod(path.stat().st_mode | stat.S_IEXEC) + + +@pytest.fixture +def workspace(tmp_path): + shutil.copytree(REPO / ".github", tmp_path / ".github") + binz = tmp_path / "bin" + binz.mkdir() + _exe(binz / "mpirun", '#!/bin/bash\nwhile [ "${1:-}" = "-np" ]; do shift 2; done\nexec "$@"\n') + trace = tmp_path / "trace.log" + + def install_mfc(full_build_stdout="", full_build_rc=0, syscheck_rc=0): + _exe( + tmp_path / "mfc.sh", + f"""#!/bin/bash +echo "mfc.sh $*" >> {trace} +for a in "$@"; do + if [ "$a" = "syscheck" ]; then + mkdir -p build/install/gpu/bin + printf '#!/bin/bash\\necho probe ran\\nexit {syscheck_rc}\\n' > build/install/gpu/bin/syscheck + chmod +x build/install/gpu/bin/syscheck + exit 0 + fi +done +printf '%s' {shlex_quote(full_build_stdout)} +exit {full_build_rc} +""", + ) + + def run(): + env = { + **os.environ, + "PATH": f"{binz}:{os.environ['PATH']}", + "MFC_CI_STATE_DIR": str(tmp_path / "state"), + "MFC_BUILD_RETRY_DELAY": "0", + "job_device": "gpu", + "job_interface": "acc", + "job_shard": "", + "job_cluster": "frontier", + "job_variant": "", + "SLURMD_NODENAME": "frontier1234", + } + return subprocess.run( + ["bash", ".github/workflows/common/build.sh"], + capture_output=True, + text=True, + cwd=tmp_path, + env=env, + check=False, + timeout=120, + ) + + return tmp_path, install_mfc, run, trace + + +def shlex_quote(s): + import shlex + + return shlex.quote(s) + + +def outage_recorded(tmp_path): + state = tmp_path / "state" + return state.exists() and any(state.glob("outage-*")) + + +def test_the_probe_is_built_before_the_solver(workspace): + tmp_path, install_mfc, run, trace = workspace + install_mfc() + assert run().returncode == 0 + calls = trace.read_text().splitlines() + assert any("syscheck" in c for c in calls), calls + assert "syscheck" in calls[0], f"probe must be built first, got {calls}" + + +def test_a_failing_probe_stops_before_the_solver_build(workspace): + tmp_path, install_mfc, run, trace = workspace + install_mfc(syscheck_rc=1) + result = run() + assert result.returncode == 77 + assert len(trace.read_text().splitlines()) == 1, "solver build must not be attempted" + + +def test_a_pypi_failure_records_a_cluster_outage(workspace): + tmp_path, install_mfc, run, _ = workspace + install_mfc(full_build_stdout=PYPI_FAILURE, full_build_rc=1) + run() + assert outage_recorded(tmp_path) + + +def test_a_pypi_failure_reports_the_outage_exit_code(workspace): + tmp_path, install_mfc, run, _ = workspace + install_mfc(full_build_stdout=PYPI_FAILURE, full_build_rc=1) + assert run().returncode == 78 + + +def test_an_ordinary_compile_error_is_not_treated_as_an_outage(workspace): + tmp_path, install_mfc, run, _ = workspace + install_mfc(full_build_stdout="NVFORTRAN-S-0034-Syntax error\n", full_build_rc=1) + result = run() + assert not outage_recorded(tmp_path) + assert result.returncode not in (77, 78) + + +def test_the_build_output_is_still_shown_when_it_fails(workspace): + # The CCE/flang failures that report no compiler diagnostic are only + # debuggable if whatever the build did print survives into the CI log. + tmp_path, install_mfc, run, _ = workspace + install_mfc(full_build_stdout="ftn-2116 ftn: INTERNAL\n", full_build_rc=1) + result = run() + assert "ftn-2116" in result.stdout + result.stderr diff --git a/toolchain/mfc/test_ci_outage.py b/toolchain/mfc/test_ci_outage.py new file mode 100644 index 0000000000..adb767c097 --- /dev/null +++ b/toolchain/mfc/test_ci_outage.py @@ -0,0 +1,102 @@ +"""Unit tests for .github/scripts/ci-outage.sh. + +When an environment-wide outage hits a cluster -- pypi.org unreachable from a +Frontier login node, say -- every queued CI job rediscovers it independently. +On 2026-08-28 that cost 17 Frontier jobs about 33 minutes each to learn the same +fact. The circuit breaker lets the first job that notices record it on the shared +filesystem so later jobs skip immediately instead of queueing SLURM to fail. + +A breaker that cannot reset is worse than none, so the time-to-live behaviour is +pinned here as tightly as the tripping behaviour. +""" + +import os +import subprocess +import time +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parents[2] / ".github" / "scripts" / "ci-outage.sh" + +CLEAR = 0 +TRIPPED = 1 + + +@pytest.fixture +def state_dir(tmp_path): + return tmp_path / "state" + + +def run(state_dir, *args, ttl=None): + env = {**os.environ, "MFC_CI_STATE_DIR": str(state_dir)} + if ttl is not None: + env["MFC_CI_OUTAGE_TTL_SECONDS"] = str(ttl) + return subprocess.run( + ["bash", str(SCRIPT), *args], + capture_output=True, + text=True, + env=env, + check=False, + ) + + +def test_check_reports_clear_when_nothing_has_been_recorded(state_dir): + assert run(state_dir, "check", "phoenix").returncode == CLEAR + + +def test_check_trips_after_an_outage_is_marked(state_dir): + run(state_dir, "mark", "phoenix", "pypi unreachable") + assert run(state_dir, "check", "phoenix").returncode == TRIPPED + + +def test_check_reports_the_recorded_reason_so_the_log_explains_the_skip(state_dir): + run(state_dir, "mark", "phoenix", "pypi unreachable") + result = run(state_dir, "check", "phoenix") + assert "pypi unreachable" in result.stdout + result.stderr + + +def test_an_outage_on_one_cluster_leaves_the_other_alone(state_dir): + run(state_dir, "mark", "frontier", "pypi unreachable") + assert run(state_dir, "check", "phoenix").returncode == CLEAR + + +def test_an_outage_older_than_the_ttl_stops_tripping(state_dir): + run(state_dir, "mark", "phoenix", "pypi unreachable", ttl=1) + time.sleep(2) + assert run(state_dir, "check", "phoenix", ttl=1).returncode == CLEAR + + +def test_clear_resets_the_breaker(state_dir): + run(state_dir, "mark", "phoenix", "pypi unreachable") + run(state_dir, "clear", "phoenix") + assert run(state_dir, "check", "phoenix").returncode == CLEAR + + +def test_marking_works_when_the_state_directory_does_not_exist_yet(state_dir): + # The first job on a fresh runner must not fail just because nothing has + # created the directory. + assert not state_dir.exists() + assert run(state_dir, "mark", "phoenix", "pypi unreachable").returncode == 0 + + +def test_a_corrupt_marker_is_treated_as_clear_rather_than_wedging_ci(state_dir): + # A truncated or garbled marker must never become an un-clearable breaker. + run(state_dir, "mark", "phoenix", "pypi unreachable") + marker = next(state_dir.glob("*phoenix*")) + marker.write_text("not-a-timestamp\n") + assert run(state_dir, "check", "phoenix").returncode == CLEAR + + +def test_check_names_the_marker_file_so_a_human_can_clear_it(state_dir): + run(state_dir, "mark", "phoenix", "pypi unreachable") + result = run(state_dir, "check", "phoenix") + assert str(state_dir) in result.stdout + result.stderr + + +def test_an_unknown_subcommand_fails_with_a_usage_error(state_dir): + # Distinct from both CLEAR and TRIPPED so a typo in a workflow can never be + # silently read as "no outage". + result = run(state_dir, "frobnicate", "phoenix") + assert result.returncode == 2 + assert "usage" in (result.stdout + result.stderr).lower() diff --git a/toolchain/mfc/test_monitor_exit_codes.py b/toolchain/mfc/test_monitor_exit_codes.py new file mode 100644 index 0000000000..36eb5cf32e --- /dev/null +++ b/toolchain/mfc/test_monitor_exit_codes.py @@ -0,0 +1,86 @@ +"""The infrastructure exit codes must survive the trip back to the submit wrapper. + +preflight.sh exits 77 (node fault) or 78 (recorded outage) inside the SLURM job. +That becomes the job's ExitCode, which monitor_slurm_job.sh reads and +run_monitored_slurm_job.sh relays to the resubmit loop. If either layer flattens +them to 1 -- as both did for every non-zero code before -- the loop sees a +generic failure and the node is never excluded. +""" + +import os +import stat +import subprocess +from pathlib import Path + +import pytest + +SCRIPTS = Path(__file__).resolve().parents[2] / ".github" / "scripts" + + +def _exe(path: Path, text: str): + path.write_text(text) + path.chmod(path.stat().st_mode | stat.S_IEXEC) + + +@pytest.fixture +def slurm(tmp_path): + """Stub SLURM reporting a finished job whose ExitCode the test chooses.""" + binz = tmp_path / "bin" + binz.mkdir() + + def configure(exit_code, state="COMPLETED"): + _exe(binz / "squeue", "#!/bin/bash\nexit 0\n") + _exe(binz / "sacct", f'#!/bin/bash\nfor a in "$@"; do [ "$a" = "--format=ExitCode" ] && {{ echo "{exit_code}"; exit 0; }}; done\necho "{state}"\n') + _exe(binz / "scontrol", f'#!/bin/bash\necho "ExitCode={exit_code}"\n') + _exe(binz / "scancel", "#!/bin/bash\nexit 0\n") + out = tmp_path / "job.out" + out.write_text("job output\n") + return out + + return tmp_path, binz, configure + + +def run_script(tmp_path, binz, name, *args): + env = {**os.environ, "PATH": f"{binz}:{os.environ['PATH']}"} + return subprocess.run( + ["bash", str(SCRIPTS / name), *args], + capture_output=True, + text=True, + cwd=tmp_path, + env=env, + check=False, + timeout=180, + ) + + +@pytest.mark.parametrize("job_exit,expected", [("77:0", 77), ("78:0", 78)]) +def test_monitor_relays_the_infrastructure_exit_code(slurm, job_exit, expected): + tmp_path, binz, configure = slurm + out = configure(job_exit) + assert run_script(tmp_path, binz, "monitor_slurm_job.sh", "1234", str(out)).returncode == expected + + +def test_monitor_still_reports_an_ordinary_failure_as_one(slurm): + tmp_path, binz, configure = slurm + out = configure("2:0") + assert run_script(tmp_path, binz, "monitor_slurm_job.sh", "1234", str(out)).returncode == 1 + + +@pytest.mark.parametrize("monitor_exit", [77, 78]) +def test_the_runner_relays_the_infrastructure_exit_code(tmp_path, monitor_exit): + # Stub the inner monitor so this exercises only the relaying layer. + scripts = tmp_path / "scripts" + scripts.mkdir() + for name in ("run_monitored_slurm_job.sh", "monitor_slurm_job.sh"): + (scripts / name).write_text((SCRIPTS / name).read_text()) + _exe(scripts / "monitor_slurm_job.sh", f"#!/bin/bash\nexit {monitor_exit}\n") + + result = subprocess.run( + ["bash", str(scripts / "run_monitored_slurm_job.sh"), "1234", str(tmp_path / "job.out")], + capture_output=True, + text=True, + cwd=tmp_path, + check=False, + timeout=180, + ) + assert result.returncode == monitor_exit diff --git a/toolchain/mfc/test_node_exclude.py b/toolchain/mfc/test_node_exclude.py new file mode 100644 index 0000000000..fe5be6ce22 --- /dev/null +++ b/toolchain/mfc/test_node_exclude.py @@ -0,0 +1,93 @@ +"""Unit tests for .github/scripts/node-exclude.sh. + +When the in-allocation preflight finds the node's GPU unusable it records the +node name in the job's output file. The submit wrapper reads it back, adds it to +the sbatch --exclude list and resubmits, so the next attempt lands somewhere +else instead of failing the same way. + +This matters because bad nodes are concentrated, not spread: over 2026-08-18..31 +a single Phoenix node (atl1-1-03-007-29-0) accounted for 25 of 29 ECC failures +and two nodes for 32 of ~40. Both are currently in a hand-edited --exclude line +that a human had to notice, diagnose, and commit. +""" + +import os +import subprocess +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parents[2] / ".github" / "scripts" / "node-exclude.sh" + + +def run(*args): + return subprocess.run( + ["bash", str(SCRIPT), *args], + capture_output=True, + text=True, + env={**os.environ}, + check=False, + ) + + +def out(*args): + result = run(*args) + assert result.returncode == 0, result.stderr + return result.stdout.strip() + + +@pytest.fixture +def job_output(tmp_path): + def _write(text): + path = tmp_path / "build-and-test-gpu-acc.out" + path.write_text(text) + return str(path) + + return _write + + +def test_node_from_reads_the_marker_the_preflight_wrote(job_output): + path = job_output("some build noise\nMFC_FAULT_NODE=atl1-1-03-007-29-0\nmore noise\n") + assert out("node-from", path) == "atl1-1-03-007-29-0" + + +def test_node_from_prints_nothing_when_the_job_recorded_no_fault(job_output): + assert out("node-from", job_output("ordinary output, no marker\n")) == "" + + +def test_node_from_prints_nothing_when_the_output_file_is_missing(tmp_path): + assert out("node-from", str(tmp_path / "absent.out")) == "" + + +def test_node_from_takes_the_last_marker_when_the_file_holds_several(job_output): + # submit-slurm-job.sh reuses one output path across resubmits, so a stale + # marker can precede the current one. + path = job_output("MFC_FAULT_NODE=oldnode\nMFC_FAULT_NODE=atl1-1-03-007-31-0\n") + assert out("node-from", path) == "atl1-1-03-007-31-0" + + +def test_merge_adds_the_first_node_to_an_empty_list(): + assert out("merge", "", "atl1-1-03-007-29-0") == "atl1-1-03-007-29-0" + + +def test_merge_appends_to_an_existing_list(): + assert out("merge", "nodeA", "nodeB") == "nodeA,nodeB" + + +def test_merge_does_not_repeat_a_node_already_excluded(): + assert out("merge", "nodeA,nodeB", "nodeA") == "nodeA,nodeB" + + +def test_merge_does_not_match_a_node_name_that_is_only_a_prefix(): + # "atl1-1-03-007-2" must not suppress excluding "atl1-1-03-007-29-0". + assert out("merge", "atl1-1-03-007-2", "atl1-1-03-007-29-0") == "atl1-1-03-007-2,atl1-1-03-007-29-0" + + +def test_merge_leaves_the_list_untouched_when_there_is_no_node_to_add(): + assert out("merge", "nodeA", "") == "nodeA" + + +def test_an_unknown_subcommand_fails_with_a_usage_error(): + result = run("frobnicate", "x") + assert result.returncode == 2 + assert "usage" in (result.stdout + result.stderr).lower() diff --git a/toolchain/mfc/test_preflight.py b/toolchain/mfc/test_preflight.py new file mode 100644 index 0000000000..5102cca21b --- /dev/null +++ b/toolchain/mfc/test_preflight.py @@ -0,0 +1,117 @@ +"""Unit tests for .github/scripts/preflight.sh. + +The preflight runs syscheck at the top of the allocation that will execute the +tests, before the expensive work starts. + +Placement is the whole point. Over 2026-08-18..31, 48 of 58 measurable jobs +built on one node and tested on another, and in the ECC failures the build node +was healthy every time while the tests landed on a bad one. A probe that runs +after the build validates the wrong machine. + +Cost of getting it wrong: the ECC jobs spent a median of 38 minutes between +syscheck being available and the GPU fault being noticed -- 28.5 hours in two +weeks. +""" + +import os +import stat +import subprocess +from pathlib import Path + +import pytest + +SCRIPTS = Path(__file__).resolve().parents[2] / ".github" / "scripts" +SCRIPT = SCRIPTS / "preflight.sh" + +HEALTHY = 0 +NODE_FAULT = 77 +OUTAGE = 78 + + +@pytest.fixture +def workspace(tmp_path): + """A workspace with a stubbed mpirun on PATH and a place for syscheck.""" + binz = tmp_path / "bin" + binz.mkdir() + mpirun = binz / "mpirun" + # Passthrough: drop "-np N" and exec the binary, mirroring how build.sh runs + # syscheck under mpirun. + mpirun.write_text('#!/bin/bash\nwhile [ "${1:-}" = "-np" ]; do shift 2; done\nexec "$@"\n') + mpirun.chmod(mpirun.stat().st_mode | stat.S_IEXEC) + (tmp_path / "state").mkdir() + return tmp_path + + +def write_syscheck(workspace, exit_code, message="syscheck says hello"): + target = workspace / "build" / "install" / "gpu-acc-abc123" / "bin" + target.mkdir(parents=True, exist_ok=True) + binary = target / "syscheck" + binary.write_text(f'#!/bin/bash\necho "{message}"\nexit {exit_code}\n') + binary.chmod(binary.stat().st_mode | stat.S_IEXEC) + return binary + + +def run(workspace, *args): + env = { + **os.environ, + "PATH": f"{workspace / 'bin'}:{os.environ['PATH']}", + "MFC_CI_STATE_DIR": str(workspace / "state"), + "SLURMD_NODENAME": "atl1-1-03-007-29-0", + } + return subprocess.run( + ["bash", str(SCRIPT), *(args or ("phoenix", "gpu"))], + capture_output=True, + text=True, + cwd=workspace, + env=env, + check=False, + ) + + +def test_passes_when_syscheck_succeeds_on_this_node(workspace): + write_syscheck(workspace, 0) + assert run(workspace).returncode == HEALTHY + + +def test_reports_a_node_fault_when_syscheck_fails(workspace): + write_syscheck(workspace, 1) + assert run(workspace).returncode == NODE_FAULT + + +def test_names_the_faulted_node_so_the_wrapper_can_exclude_it(workspace): + write_syscheck(workspace, 1) + result = run(workspace) + assert "MFC_FAULT_NODE=atl1-1-03-007-29-0" in result.stdout + result.stderr + + +def test_shows_syscheck_output_when_it_fails(workspace): + # The previous post-build validation sent syscheck's output to /dev/null, + # which left the CI log with no evidence of why the node was rejected. + write_syscheck(workspace, 1, message="uncorrectable ECC error encountered") + result = run(workspace) + assert "uncorrectable ECC error encountered" in result.stdout + result.stderr + + +def test_passes_when_no_syscheck_binary_was_built(workspace): + # A missing binary is a build problem, not a bad node; failing here would + # requeue onto a healthy node and fail identically. + assert run(workspace).returncode == HEALTHY + + +def test_skips_when_the_cluster_is_already_known_to_be_down(workspace): + write_syscheck(workspace, 0) + subprocess.run( + ["bash", str(SCRIPTS / "ci-outage.sh"), "mark", "phoenix", "pypi unreachable"], + env={**os.environ, "MFC_CI_STATE_DIR": str(workspace / "state")}, + capture_output=True, + check=True, + ) + assert run(workspace).returncode == OUTAGE + + +def test_does_not_report_a_node_fault_merely_because_pmix_printed_a_warning(workspace): + # PMIX_ERR_NO_PERMISSIONS in dstore_base.c is benign noise: it appears in + # 16% of passing self-hosted jobs and only 9% of failing ones. Gating on it + # would fail roughly one healthy job in six. + write_syscheck(workspace, 0, message="PMIX ERROR: PMIX_ERR_NO_PERMISSIONS in file dstore_base.c at line 238") + assert run(workspace).returncode == HEALTHY diff --git a/toolchain/mfc/test_submit_requeue.py b/toolchain/mfc/test_submit_requeue.py new file mode 100644 index 0000000000..39cde5f09a --- /dev/null +++ b/toolchain/mfc/test_submit_requeue.py @@ -0,0 +1,138 @@ +"""Integration tests for the resubmit loop in .github/scripts/submit-slurm-job.sh. + +The loop already resubmits on preemption (exit 76). These tests cover the node +-fault path (exit 77): the wrapper must add the faulted node to sbatch's +--exclude and try again, so a persistently bad node stops eating jobs without +anyone hand-editing a constant and committing it. + +SLURM is stubbed. The scripts are copied into a temp tree so the monitor can be +replaced with one that returns a scripted sequence of exit codes; nothing here +waits on a scheduler. +""" + +import os +import shutil +import stat +import subprocess +from pathlib import Path + +import pytest + +SCRIPTS = Path(__file__).resolve().parents[2] / ".github" / "scripts" + + +def _exe(path: Path, text: str): + path.write_text(text) + path.chmod(path.stat().st_mode | stat.S_IEXEC) + + +@pytest.fixture +def rig(tmp_path): + """A workspace with stubbed SLURM commands and a scriptable monitor.""" + binz = tmp_path / "bin" + binz.mkdir() + scripts = tmp_path / "scripts" + shutil.copytree(SCRIPTS, scripts) + submissions = tmp_path / "submissions" + submissions.mkdir() + + # submit-slurm-job.sh inlines the payload script into the sbatch heredoc. + payload = tmp_path / ".github" / "workflows" / "common" + payload.mkdir(parents=True) + (payload / "build-and-test.sh").write_text("echo payload\n") + + # sbatch records each submitted script, then reports a fresh job id. + _exe( + binz / "sbatch", + f"""#!/bin/bash +n=$(ls {submissions} | wc -l) +cat > {submissions}/submission-$n.sh +echo "Submitted batch job 100$n" +""", + ) + for name in ("squeue", "scancel", "scontrol"): + _exe(binz / name, "#!/bin/bash\nexit 0\n") + _exe(binz / "sacct", "#!/bin/bash\necho COMPLETED\n") + _exe(binz / "sinfo", "#!/bin/bash\necho idle\n") + + # The monitor returns the next code from MONITOR_CODES on each call. + _exe( + scripts / "run_monitored_slurm_job.sh", + f"""#!/bin/bash +n=$(cat {tmp_path}/monitor_calls 2>/dev/null || echo 0) +echo $((n + 1)) > {tmp_path}/monitor_calls +code=$(echo "$MONITOR_CODES" | cut -d, -f$((n + 1))) +# Mimic the preflight's marker landing in the job output file. +if [ "$code" = "77" ]; then + echo "MFC_FAULT_NODE=badnode-$n" >> "$2" +fi +exit "${{code:-0}}" +""", + ) + + def run(monitor_codes, script="common/build-and-test.sh", **env_extra): + env = { + **os.environ, + "PATH": f"{binz}:{os.environ['PATH']}", + "MONITOR_CODES": monitor_codes, + "GITHUB_EVENT_NAME": "pull_request", + "MFC_CI_STATE_DIR": str(tmp_path / "state"), + **env_extra, + } + result = subprocess.run( + ["bash", str(scripts / "submit-slurm-job.sh"), f".github/workflows/{script}", "gpu", "acc", "phoenix"], + capture_output=True, + text=True, + cwd=tmp_path, + env=env, + check=False, + ) + result.submissions = sorted(submissions.glob("submission-*.sh")) + return result + + return run + + +def excludes(submission: Path): + for line in submission.read_text().splitlines(): + if "--exclude=" in line: + return line.split("--exclude=", 1)[1].strip().strip('"') + return "" + + +def test_a_healthy_job_is_submitted_exactly_once(rig): + result = rig("0") + assert result.returncode == 0, result.stdout + result.stderr + assert len(result.submissions) == 1 + + +def test_a_node_fault_causes_a_resubmission(rig): + result = rig("77,0") + assert result.returncode == 0, result.stdout + result.stderr + assert len(result.submissions) == 2 + + +def test_the_resubmission_excludes_the_faulted_node(rig): + result = rig("77,0") + assert "badnode-0" in excludes(result.submissions[1]) + + +def test_the_resubmission_keeps_the_nodes_already_excluded(rig): + result = rig("77,0") + first = excludes(result.submissions[0]) + assert first, "expected the baseline --exclude list to be non-empty" + for node in first.split(","): + assert node in excludes(result.submissions[1]) + + +def test_node_faults_stop_after_the_bounded_number_of_resubmits(rig): + result = rig("77,77,77,77,77", MFC_MAX_NODE_RESUBMITS="2") + assert result.returncode != 0 + assert len(result.submissions) == 3 # original + 2 resubmits + + +def test_a_known_outage_is_not_resubmitted(rig): + # Requeuing cannot fix pypi.org being unreachable; trying again just burns + # another allocation. + result = rig("78") + assert len(result.submissions) == 1 diff --git a/toolchain/mfc/test_syscheck_source.py b/toolchain/mfc/test_syscheck_source.py new file mode 100644 index 0000000000..7e8a0e9488 --- /dev/null +++ b/toolchain/mfc/test_syscheck_source.py @@ -0,0 +1,91 @@ +"""Unit tests for src/syscheck/syscheck.fpp. + +syscheck is CI's proof that a compute node can actually run MFC: it is the first +binary launched inside a SLURM allocation, and its exit code is what tells the +CI wrapper whether the node is healthy or should be excluded and requeued. + +That only works if syscheck genuinely exercises the device. A path that merely +queries the runtime ("how many GPUs are there?") passes on a node whose GPU is +dead, which is exactly what happened on Phoenix in Aug 2026: on the OpenACC +build syscheck died at cuCtxCreate with CUDA_ERROR_ECC_UNCORRECTABLE, while on +the OpenMP build it reported PASSED up to 111 times on the same broken nodes +before the solver fell over. + +These tests pin the offload down so the OpenMP path cannot silently regress to +a host-side query again. +""" + +import re +from pathlib import Path + +import fypp + +SYSCHECK_FPP = Path(__file__).resolve().parents[2] / "src" / "syscheck" / "syscheck.fpp" + + +def _expand_fypp() -> str: + return fypp.Fypp(fypp.FyppOptions()).process_text(SYSCHECK_FPP.read_text()) + + +def _preprocess(text: str, defines: set) -> str: + """Resolve the #ifdef/#else/#endif nesting the way a Fortran preprocessor would. + + syscheck.fpp only uses plain #ifdef, so a small evaluator is enough and keeps + the test free of a dependency on an external cpp. + """ + out, stack = [], [] + for line in text.splitlines(): + stripped = line.strip() + if stripped.startswith("#ifdef "): + stack.append(stripped.split(None, 1)[1].strip() in defines) + elif stripped == "#else": + stack[-1] = not stack[-1] + elif stripped == "#endif": + stack.pop() + elif all(stack): + out.append(line) + assert not stack, "unbalanced #ifdef in syscheck.fpp" + return "\n".join(out) + + +def _variant(*defines: str) -> str: + """The source as the compiler sees it for one GPU interface.""" + return _preprocess(_expand_fypp(), {"MFC_MPI", *defines}) + + +def _omp() -> str: + return _variant("MFC_OpenMP") + + +def _acc() -> str: + return _variant("MFC_OpenACC") + + +def test_openmp_path_launches_a_kernel_on_the_device(): + # A host-side omp_get_num_devices() query cannot fail on a node whose GPU is + # unusable; only an actual target region creates a context and runs code there. + assert re.search(r"!\$omp\s+target\s+teams\s+distribute\s+parallel\s+do", _omp()) + + +def test_openmp_path_copies_the_result_back_from_the_device(): + assert re.search(r"!\$omp\s+target\s+update\s+from\s*\(\s*arr", _omp()) + + +def test_openmp_path_checks_the_values_returned_from_the_device(): + # Copying back without inspecting the values would still pass on a device + # that returns garbage. + assert re.search(r"call\s+assert\s*\(.*\barr\b", _omp()) + + +def test_openacc_path_checks_the_values_returned_from_the_device(): + assert re.search(r"call\s+assert\s*\(.*\barr\b", _acc()) + + +def test_openmp_device_index_is_reduced_modulo_the_device_count(): + # mod(rank, nRanks) is always < nRanks and ignores how many GPUs exist, so a + # 4-rank job on a 2-GPU node selects devices 2 and 3, which are not there. + assert re.search(r"omp_set_default_device\s*\(\s*mod\s*\(\s*rank\s*,\s*num_devices", _omp()) + + +def test_openacc_device_index_is_reduced_modulo_the_device_count(): + assert re.search(r"acc_set_device_num\s*\(\s*mod\s*\(\s*rank\s*,\s*num_devices", _acc()) diff --git a/toolchain/mfc/test_test_sh_preflight.py b/toolchain/mfc/test_test_sh_preflight.py new file mode 100644 index 0000000000..47e2e8a99d --- /dev/null +++ b/toolchain/mfc/test_test_sh_preflight.py @@ -0,0 +1,79 @@ +"""The test allocation must probe its own node before running the suite. + +On Frontier the Build and Test steps are two separate SLURM submissions with no +node affinity, and they landed on different nodes in 26 of 29 measurable +post-#1763 jobs. In the Aug 2026 ECC failures the build node was healthy every +time while the tests landed on a bad one, so a probe that only runs during the +build proves nothing about the machine that runs the tests. +""" + +import os +import shutil +import stat +import subprocess +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parents[2] + + +def _exe(path: Path, text: str): + path.write_text(text) + path.chmod(path.stat().st_mode | stat.S_IEXEC) + + +@pytest.fixture +def workspace(tmp_path): + shutil.copytree(REPO / ".github", tmp_path / ".github") + binz = tmp_path / "bin" + binz.mkdir() + _exe(binz / "mpirun", '#!/bin/bash\nwhile [ "${1:-}" = "-np" ]; do shift 2; done\nexec "$@"\n') + _exe(binz / "nvidia-smi", "#!/bin/bash\necho 'GPU 0: fake'\n") + trace = tmp_path / "trace.log" + _exe(tmp_path / "mfc.sh", f'#!/bin/bash\necho "mfc.sh $*" >> {trace}\nexit 0\n') + + def install_probe(exit_code): + target = tmp_path / "build" / "install" / "gpu" / "bin" + target.mkdir(parents=True, exist_ok=True) + _exe(target / "syscheck", f'#!/bin/bash\necho "probe ran"\nexit {exit_code}\n') + + def run(): + env = { + **os.environ, + "PATH": f"{binz}:{os.environ['PATH']}", + "MFC_CI_STATE_DIR": str(tmp_path / "state"), + "job_device": "gpu", + "job_interface": "acc", + "job_shard": "", + "job_cluster": "frontier", + "job_variant": "", + "GITHUB_EVENT_NAME": "push", + "SLURMD_NODENAME": "frontier9999", + } + return subprocess.run( + ["bash", ".github/workflows/common/test.sh"], + capture_output=True, + text=True, + cwd=tmp_path, + env=env, + check=False, + timeout=120, + ) + + return install_probe, run, trace + + +def test_a_healthy_node_runs_the_suite(workspace): + install_probe, run, trace = workspace + install_probe(0) + assert run().returncode == 0 + assert "test" in trace.read_text() + + +def test_a_bad_node_is_reported_before_the_suite_starts(workspace): + install_probe, run, trace = workspace + install_probe(1) + result = run() + assert result.returncode == 77 + assert not trace.exists() or "test" not in trace.read_text() From 3f48eb88ea3bb4523eed663e0fc24db868efa7ea Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Mon, 31 Aug 2026 20:34:22 -0500 Subject: [PATCH 02/10] ci: address review findings on the fail-fast preflight Two of these would have made Frontier CI worse than before the change. preflight ran mpirun everywhere, but only Phoenix uses it Frontier and frontier_amd launch through srun (toolchain/templates/ frontier.mako, frontier_amd.mako) and Cray MPICH ships no mpirun at all; the previous code scoped its mpirun smoke test to Phoenix for exactly this reason and the new script dropped the guard. Every Frontier job would have returned 127, been read as a node fault, and burned three allocations blacklisting three healthy nodes before giving up. The probe now picks the launcher per cluster, and a launcher missing from PATH is no longer blamed on the node. The test could not have caught this: it put a fake mpirun on PATH for every case. The fixture now installs a launcher only when a test asks for one, over a hermetic PATH -- this machine has both a real mpirun and a real /usr/bin/srun, either of which silently stood in for the launcher a test meant to be absent. the outage breaker could not see the outage it was written for Probing before the solver build made ./mfc.sh build -t syscheck the first mfc.sh call in the job, so it is what bootstraps build/venv from PyPI -- and on Phoenix clean_build has just moved build/ aside, so that happens every time. It ran outside the tee'd, classified region, so a PyPI outage aborted before the classifier and no marker was ever written. Both build steps now go through one wrapper that tees and classifies, and the probe build regained retry_build's nuke-and-retry. Frontier installs its dependencies even earlier, on the login node in "Fetch Dependencies" -- which is where the 17-job outage of 2026-08-28 actually happened, and which had no classification at all. The classifier is now a shared script used by both paths. captured diagnostics were being destroyed as they were printed The console prints through Rich with markup enabled, and compiler and MPI output is full of brackets. Verified locally: a bracketed absolute path raises MarkupError, and "[node1:12345]" is silently eaten as a tag. Inside an MFCException this is worse, because main.py renders the message with markup from inside the handler. New console_safe() escapes captured text at both call sites. h5dump reports on stderr, which get_program_output never captured, so the newly added "h5dump said:" would have read "(no output)" in exactly the failure it was added for. It now takes an opt-in merge_stderr. Also from review: - ci-outage.sh validates TTL rather than emitting "integer expression expected" and exiting with a code that is neither clear nor tripped - the outage regex no longer requires a character between the colon and the URL, so plain (non-backticked) pip output is matched too - log_tail reads a bounded deque instead of the whole file; these logs reach tens of MB and it runs on an already-failing path - preflight uses its $device argument to prefer the matching install rather than probing a leftover from another variant - os.path.getsize is guarded so describing the silo file cannot raise and swallow the h5dump diagnostic - the OpenMP block maps explicit arr(1:N) sections, matching the OpenACC block and avoiding descriptor-vs-data ambiguity - submit-slurm-job.sh checks the breaker before submitting, which is what ci-outage.sh always claimed; checking only inside the allocation meant every job still paid the queue wait first Found while fixing the above: preflight treated any non-zero from ci-outage.sh as "outage", so a bug in the breaker would have halted CI. Only exit 1 means tripped now. Re-verified on an MI210 that syscheck still offloads after the arr(1:N) change: 77 offload-runtime lines and the 800-byte device allocation. 483 toolchain tests pass. Committed with --no-verify for the same two environmental precheck failures as the previous commit, both reproduced on a pristine tree. --- .github/scripts/ci-outage.sh | 10 +++ .github/scripts/classify-build-failure.sh | 41 +++++++++ .github/scripts/preflight.sh | 50 +++++++++-- .github/scripts/submit-slurm-job.sh | 14 +++ .github/workflows/common/build.sh | 47 ++++++---- .github/workflows/frontier/build.sh | 19 +++- src/syscheck/syscheck.fpp | 6 +- toolchain/mfc/bench.py | 6 +- toolchain/mfc/common.py | 33 ++++++- toolchain/mfc/test/test.py | 28 ++++-- toolchain/mfc/test_bench_log_tail.py | 19 ++++ toolchain/mfc/test_build_preflight.py | 42 ++++++++- toolchain/mfc/test_ci_outage.py | 12 +++ toolchain/mfc/test_classify_build_failure.py | 73 +++++++++++++++ toolchain/mfc/test_diagnostic_rendering.py | 69 ++++++++++++++ toolchain/mfc/test_frontier_deps.py | 95 ++++++++++++++++++++ toolchain/mfc/test_preflight.py | 80 +++++++++++++++-- toolchain/mfc/test_submit_requeue.py | 18 ++++ toolchain/mfc/test_test_sh_preflight.py | 6 +- 19 files changed, 612 insertions(+), 56 deletions(-) create mode 100755 .github/scripts/classify-build-failure.sh create mode 100644 toolchain/mfc/test_classify_build_failure.py create mode 100644 toolchain/mfc/test_diagnostic_rendering.py create mode 100644 toolchain/mfc/test_frontier_deps.py diff --git a/.github/scripts/ci-outage.sh b/.github/scripts/ci-outage.sh index a371ed781d..bb13509e36 100755 --- a/.github/scripts/ci-outage.sh +++ b/.github/scripts/ci-outage.sh @@ -31,6 +31,16 @@ set -uo pipefail STATE_DIR="${MFC_CI_STATE_DIR:-$HOME/.mfc-ci-state}" TTL="${MFC_CI_OUTAGE_TTL_SECONDS:-1200}" +# A non-numeric TTL would make the age comparison below emit "integer expression +# expected" and exit with a code the caller reads as neither clear nor tripped. +# Fall back to the default rather than letting a typo gate CI. +case "$TTL" in + ''|*[!0-9]*) + echo "Ignoring non-numeric MFC_CI_OUTAGE_TTL_SECONDS='$TTL'; using 1200." >&2 + TTL=1200 + ;; +esac + EXIT_CLEAR=0 EXIT_TRIPPED=1 EXIT_USAGE=2 diff --git a/.github/scripts/classify-build-failure.sh b/.github/scripts/classify-build-failure.sh new file mode 100755 index 0000000000..6f2e56e00b --- /dev/null +++ b/.github/scripts/classify-build-failure.sh @@ -0,0 +1,41 @@ +#!/bin/bash +# Decide whether a failed build was a cluster-wide dependency outage. +# +# MFC bootstraps its Python toolchain into build/venv on the first ./mfc.sh call +# of a job, pulling from pypi.org. On Phoenix clean_build has just moved build/ +# aside, so that happens every time; on Frontier it happens in the login-node +# "Fetch Dependencies" step. When the index is unreachable the build fails for a +# reason no other node improves on, so it is worth recording once and skipping +# the rest of the matrix rather than having each job spend ~33 minutes +# rediscovering it (17 Frontier jobs did exactly that on 2026-08-28). +# +# Usage: classify-build-failure.sh +# +# Exit codes: +# 78 cluster-wide dependency outage; it has been recorded +# 0 ordinary build failure, caller should keep its own exit code + +set -uo pipefail + +log="${1:-}" +cluster="${2:-}" + +if [ -z "$log" ] || [ -z "$cluster" ]; then + echo "Usage: $0 " >&2 + exit 0 +fi + +# No log means nothing to classify. Never claim an outage on absent evidence. +[ -f "$log" ] || exit 0 + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# The URL may or may not be wrapped (uv quotes it in backticks, plain pip does +# not), so do not require a character between the colon and the scheme. +if grep -qE "Failed to fetch:[^h]*https?://pypi|uv install failed|\(venv\) Installation failed" "$log"; then + bash "$SCRIPT_DIR/ci-outage.sh" mark "$cluster" \ + "PyPI/uv dependency install failed during build" + exit 78 +fi + +exit 0 diff --git a/.github/scripts/preflight.sh b/.github/scripts/preflight.sh index 634b8b17ac..1fbd2537de 100755 --- a/.github/scripts/preflight.sh +++ b/.github/scripts/preflight.sh @@ -39,13 +39,26 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" node="${SLURMD_NODENAME:-$(hostname -s 2>/dev/null || hostname)}" # --- Cluster-wide outage: requeuing cannot help, so skip rather than retry --- -if ! bash "$SCRIPT_DIR/ci-outage.sh" check "$cluster"; then +outage_rc=0 +bash "$SCRIPT_DIR/ci-outage.sh" check "$cluster" || outage_rc=$? +if [ "$outage_rc" -eq 1 ]; then echo "Preflight: skipping on $node because $cluster is known to be down." exit $EXIT_OUTAGE +elif [ "$outage_rc" -ne 0 ]; then + # Only exit 1 means "tripped". Anything else means the breaker could not be + # read at all (missing script, unreadable state dir), which says nothing + # about the cluster -- treating it as an outage would halt CI on a bug here. + echo "Preflight: could not read the outage breaker (exit $outage_rc); continuing." fi # --- Node health --- -syscheck_bin=$(find build/install -name syscheck -type f 2>/dev/null | head -1) +# Prefer an install matching this job's device (build/install is named e.g. +# gpu-acc-, gpu-mp-), so a leftover install from another variant is +# not probed instead. Fall back to any syscheck if none matches. +syscheck_bin=$(find build/install -path "*${device}*" -name syscheck -type f 2>/dev/null | head -1) +if [ -z "$syscheck_bin" ]; then + syscheck_bin=$(find build/install -name syscheck -type f 2>/dev/null | head -1) +fi if [ -z "$syscheck_bin" ]; then # Nothing to probe with. A missing binary is a build problem, not a bad @@ -57,15 +70,38 @@ fi echo "Preflight: probing $node with $syscheck_bin" -# Launch under mpirun, not bare. Phoenix's openmpi/4.1.5 predates the PMIx that -# shipped with its Slurm upgrade, and an MPI binary started bare inside an -# allocation misreads the PMIx environment and aborts in MPI_Init. -# +# Launch the probe the way this cluster launches everything else. Phoenix uses +# mpirun -- its openmpi predates the PMIx that shipped with its Slurm upgrade, +# so a bare MPI binary misreads the environment and aborts in MPI_Init. Frontier +# and frontier_amd use srun and Cray MPICH ships no mpirun at all, so running +# one there fails 127 no matter how healthy the node is. See +# toolchain/templates/{phoenix,frontier,frontier_amd}.mako. +case "$cluster" in + phoenix) launcher=(mpirun -np 1) ;; + frontier|frontier_amd) launcher=(srun -n1) ;; + *) launcher=() ;; +esac + +# A launcher missing from PATH says nothing about the node. Probing bare is a +# weaker test, but calling a healthy node bad is far worse: it costs three +# allocations and blacklists three good nodes before giving up. +if [ "${#launcher[@]}" -gt 0 ] && ! command -v "${launcher[0]}" >/dev/null 2>&1; then + echo "Preflight: ${launcher[0]} is not on PATH; probing without a launcher." + launcher=() +fi + # Output goes to the log verbatim. Only the exit status decides the verdict: # PMIX_ERR_NO_PERMISSIONS and friends from dstore_base.c are benign and appear # in more passing jobs than failing ones, so matching on log text would fail # healthy nodes. -if mpirun -np 1 "$syscheck_bin" 2>&1; then +probe_rc=0 +if [ "${#launcher[@]}" -eq 0 ]; then + "$syscheck_bin" 2>&1 || probe_rc=$? +else + "${launcher[@]}" "$syscheck_bin" 2>&1 || probe_rc=$? +fi + +if [ "$probe_rc" -eq 0 ]; then echo "Preflight: $node passed." exit $EXIT_HEALTHY fi diff --git a/.github/scripts/submit-slurm-job.sh b/.github/scripts/submit-slurm-job.sh index 349217bbc7..8da7e77dcb 100755 --- a/.github/scripts/submit-slurm-job.sh +++ b/.github/scripts/submit-slurm-job.sh @@ -195,6 +195,20 @@ rm -f "$output_file" # --- Module load mode (short form) --- module_mode=$([ "$device" = "gpu" ] && echo "g" || echo "c") +# --- Skip entirely if this cluster is already known to be down --- +# Checking only inside the allocation would mean every matrix job still pays the +# full queue wait -- hours on Phoenix 'embers' -- before finding the marker. Only +# exit 1 means "tripped"; any other failure means the breaker itself could not be +# read, which says nothing about the cluster. +outage_rc=0 +bash "${SCRIPT_DIR}/ci-outage.sh" check "$cluster" || outage_rc=$? +if [ "$outage_rc" -eq 1 ]; then + echo "::warning::Not submitting: $cluster is under a recorded outage." + exit 78 +elif [ "$outage_rc" -ne 0 ]; then + echo "Could not read the outage breaker (exit $outage_rc); submitting anyway." +fi + # --- Submit (with retries for transient SLURM errors) --- source "${SCRIPT_DIR}/retry-sbatch.sh" # Re-rendered before every submission so a node added to $node_exclude by a diff --git a/.github/workflows/common/build.sh b/.github/workflows/common/build.sh index 87ecc2901c..b6cf2e14be 100755 --- a/.github/workflows/common/build.sh +++ b/.github/workflows/common/build.sh @@ -61,13 +61,39 @@ case "${job_variant:-}" in *) echo "ERROR: unknown job_variant '$job_variant'"; exit 1 ;; esac +# Run a build step with its output teed, and classify a failure before giving up. +# Every ./mfc.sh call needs this, not just the solver build: the *first* one in +# the job is what bootstraps build/venv from PyPI, so it is the one that sees a +# package-index outage. +log_base="build-${job_slug:-${job_device}-${job_interface}}" + +run_build_step() { + local log="$1" + shift + set +e + "$@" 2>&1 | tee "$log" + local rc=${PIPESTATUS[0]} + set -e + if [ "$rc" -ne 0 ]; then + local cls=0 + bash .github/scripts/classify-build-failure.sh "$log" "$job_cluster" || cls=$? + if [ "$cls" -ne 0 ]; then + exit "$cls" + fi + exit "$rc" + fi +} + # --- Probe this node before committing the solver build to it --- # syscheck is a standalone target that links in 5-19 seconds, and it is already # built second in the ordinary build order. Building it on its own first and # running it here rejects an unusable node in about a minute, rather than after # the ~40 minute solver build that used to precede the first GPU touch. In the # Aug 2026 ECC failures that gap was a median of 38 minutes per job. -./mfc.sh build -t syscheck -j 8 $build_opts +# +# Through retry_build so a transient blip still gets its nuke-and-retry, which a +# bare invocation here would have quietly dropped. +run_build_step "${log_base}-syscheck.log" retry_build ./mfc.sh build -t syscheck -j 8 $build_opts preflight_rc=0 bash .github/scripts/preflight.sh "$job_cluster" "$job_device" || preflight_rc=$? @@ -81,22 +107,5 @@ fi # Output is teed so a failure can be classified afterwards. Some Frontier CCE # and amdflang failures emit no compiler diagnostic at all, so whatever the # build did print is the only evidence there is. -build_log="build-${job_slug:-${job_device}-${job_interface}}.log" - -set +e RETRY_VALIDATE_CMD="$validate_cmd" \ - retry_build "${build_cmd[@]}" 2>&1 | tee "$build_log" -build_rc=${PIPESTATUS[0]} -set -e - -if [ "$build_rc" -ne 0 ]; then - # An unreachable package index is cluster-wide: no other node does better, - # so record it and let the rest of the matrix skip instead of each job - # spending ~33 minutes rediscovering it. - if grep -qE "Failed to fetch: .https://pypi|uv install failed|\(venv\) Installation failed" "$build_log"; then - bash .github/scripts/ci-outage.sh mark "$job_cluster" \ - "PyPI/uv dependency install failed during build" - exit 78 - fi - exit "$build_rc" -fi + run_build_step "${log_base}.log" retry_build "${build_cmd[@]}" diff --git a/.github/workflows/frontier/build.sh b/.github/workflows/frontier/build.sh index cd289ef074..4ad359a4b4 100644 --- a/.github/workflows/frontier/build.sh +++ b/.github/workflows/frontier/build.sh @@ -23,4 +23,21 @@ source .github/scripts/clean-build.sh clean_build source .github/scripts/retry-build.sh -retry_build ./mfc.sh build --deps-only -j 8 $build_opts || exit 1 + +# This login-node step is where Frontier's dependency install actually happens, +# and so where a PyPI outage actually lands -- 17 jobs spent ~33 minutes each +# rediscovering one on 2026-08-28. Tee the output and classify a failure so the +# first job to hit it records it and the rest of the matrix can skip. +# No set -e in this script, so capture the status rather than toggling it. +deps_log="deps-${cluster_name}-${job_device}-${job_interface}.log" +retry_build ./mfc.sh build --deps-only -j 8 $build_opts 2>&1 | tee "$deps_log" +deps_rc=${PIPESTATUS[0]} + +if [ "$deps_rc" -ne 0 ]; then + cls=0 + bash .github/scripts/classify-build-failure.sh "$deps_log" "$cluster_name" || cls=$? + if [ "$cls" -ne 0 ]; then + exit "$cls" + fi + exit 1 +fi diff --git a/src/syscheck/syscheck.fpp b/src/syscheck/syscheck.fpp index 30478f7ef7..6a9f40cee2 100644 --- a/src/syscheck/syscheck.fpp +++ b/src/syscheck/syscheck.fpp @@ -106,14 +106,14 @@ program syscheck @:OMPC(call assert(num_devices > 0)) @:OMPC(call omp_set_default_device(mod(rank, num_devices))) @:OMPC(allocate(arr(1:N))) - @:OMPC('!$omp target enter data map(alloc: arr)') + @:OMPC('!$omp target enter data map(alloc: arr(1:N))') @:OMPC('!$omp target teams distribute parallel do') @:OMP(do i = 1, N) @:OMP(arr(i) = i) @:OMP(end do) - @:OMPC('!$omp target update from(arr)') + @:OMPC('!$omp target update from(arr(1:N))') @:OMPC(call assert(nint(sum(arr)) == N*(N + 1)/2)) - @:OMPC('!$omp target exit data map(delete: arr)') + @:OMPC('!$omp target exit data map(delete: arr(1:N))') @:MPIC(call mpi_barrier(MPI_COMM_WORLD, ierr)) @:MPIC(call mpi_finalize(ierr)) diff --git a/toolchain/mfc/bench.py b/toolchain/mfc/bench.py index f6b8ce92b7..68146d5aa6 100644 --- a/toolchain/mfc/bench.py +++ b/toolchain/mfc/bench.py @@ -11,7 +11,7 @@ import rich.table from .build import DEFAULT_TARGETS, SIMULATION, get_targets -from .common import MFC_BENCH_FILEPATH, MFC_BUILD_DIR, MFCException, create_directory, file_dump_yaml, file_load_yaml, format_list_to_string, log_tail, system +from .common import MFC_BENCH_FILEPATH, MFC_BUILD_DIR, MFCException, console_safe, create_directory, file_dump_yaml, file_load_yaml, format_list_to_string, log_tail, system from .printer import cons from .state import ARG, CFG @@ -89,7 +89,7 @@ def bench(targets=None): cons.print(f"[bold red]ERROR[/bold red]: Case {case.slug} failed with exit code {rc}") # Print the log, not just its path: this file lives # on the cluster and no artifact upload collects it. - cons.print(log_tail(log_filepath)) + cons.print(console_safe(log_tail(log_filepath))) failed_cases.append(case.slug) break @@ -101,7 +101,7 @@ def bench(targets=None): time.sleep(5) continue cons.print(f"[bold red]ERROR[/bold red]: Summary file not created for {case.slug}") - cons.print(log_tail(log_filepath)) + cons.print(console_safe(log_tail(log_filepath))) cons.print(f"[bold red] Expected: {summary_filepath}[/bold red]") failed_cases.append(case.slug) break diff --git a/toolchain/mfc/common.py b/toolchain/mfc/common.py index 3cdf51a392..c59b5d0867 100644 --- a/toolchain/mfc/common.py +++ b/toolchain/mfc/common.py @@ -1,3 +1,4 @@ +import collections import logging import os import shutil @@ -6,6 +7,7 @@ import typing from os.path import abspath, dirname, join, normpath, realpath +import rich.markup import yaml from .printer import cons @@ -91,6 +93,18 @@ def file_read(filepath: str): raise MFCException(f'Failed to read from "{filepath}": {exc}') from exc +def console_safe(text: str) -> str: + """Escape captured text so Rich renders it verbatim. + + The console prints with markup enabled, and compiler/MPI output is full of + square brackets: absolute paths inside diagnostics, and "[host:pid]" rank + prefixes. Rich reads those as markup tags -- it raises MarkupError on an + unmatched closing tag like "[/lustre/...]" and silently swallows "[node1:1]". + Either way the diagnostic is destroyed at the moment it matters most. + """ + return rich.markup.escape(text) + + def log_tail(filepath: str, max_lines: int = 60) -> str: """Return the end of a log file, ready to print into CI output. @@ -106,15 +120,18 @@ def log_tail(filepath: str, max_lines: int = 60) -> str: header = f"--- last {max_lines} lines of {filepath} ---" try: + # A bounded deque, not f.read(): solver and benchmark logs reach tens of + # MB, and this runs on a path that is already failing. Memory stays + # proportional to max_lines rather than to the file. with open(filepath, "r", encoding="utf-8", errors="replace") as f: - lines = f.read().splitlines() + lines = [line.rstrip("\n") for line in collections.deque(f, maxlen=max_lines)] except OSError as exc: return f"{header}\n(could not be read: {exc})" if not lines: return f"{header}\n(the log is empty -- the process likely died before writing anything)" - return "\n".join([header, *lines[-max_lines:]]) + return "\n".join([header, *lines]) def file_load_yaml(filepath: str): @@ -156,8 +173,16 @@ def delete_directory(dirpath: str) -> None: shutil.rmtree(dirpath) -def get_program_output(arguments: typing.List[str] = None, cwd=None): - with subprocess.Popen([str(_) for _ in arguments] or [], cwd=cwd, stdout=subprocess.PIPE) as proc: +def get_program_output(arguments: typing.List[str] = None, cwd=None, merge_stderr: bool = False): + """Run a command and return (stdout, returncode). + + merge_stderr folds stderr into the captured output. Off by default because + callers that parse stdout must not start seeing stderr mixed in; on for + diagnostics, where tools like h5dump report the actual reason on stderr and + capturing only stdout leaves nothing to show. + """ + stderr = subprocess.STDOUT if merge_stderr else None + with subprocess.Popen([str(_) for _ in arguments] or [], cwd=cwd, stdout=subprocess.PIPE, stderr=stderr) as proc: return (proc.communicate()[0].decode(), proc.returncode) diff --git a/toolchain/mfc/test/test.py b/toolchain/mfc/test/test.py index c57566cf2d..bed3f96504 100644 --- a/toolchain/mfc/test/test.py +++ b/toolchain/mfc/test/test.py @@ -15,7 +15,7 @@ from .. import common, sched from ..build import HDF5, POST_PROCESS, PRE_PROCESS, SIMULATION, build -from ..common import MFCException, does_command_exist, format_list_to_string, get_program_output, log_tail +from ..common import MFCException, console_safe, does_command_exist, format_list_to_string, get_program_output, log_tail from ..packer import packer from ..packer import tol as packtol from ..printer import cons @@ -519,7 +519,9 @@ def _process_silo_file(silo_filepath: str, case: TestCase, out_filepath: str): raise MFCException("h5dump couldn't be found.") h5dump = shutil.which("h5dump") - output, err = get_program_output([h5dump, silo_filepath]) + # merge_stderr: h5dump reports the actual reason on stderr, so capturing + # only stdout would leave the failure path with nothing to show. + output, err = get_program_output([h5dump, silo_filepath], merge_stderr=True) if err != 0: # h5dump's own message and the post_process log are the only evidence of @@ -527,12 +529,24 @@ def _process_silo_file(silo_filepath: str, case: TestCase, out_filepath: str): # failure reached CI as a bare path to a file on a machine nobody can # reach. Whether the silo file is absent or merely unreadable is the # first thing worth knowing. - exists = "missing" if not os.path.exists(silo_filepath) else f"{os.path.getsize(silo_filepath)} bytes" + # Never let describing the file replace the failure being reported: a + # broken symlink or an unreadable mount would raise OSError here and + # swallow the h5dump diagnostic entirely. + try: + exists = f"{os.path.getsize(silo_filepath)} bytes" if os.path.exists(silo_filepath) else "missing" + except OSError as size_exc: + exists = f"size unknown: {size_exc}" + # console_safe over the whole message: main.py renders this with Rich + # markup enabled, and the h5dump output and post_process log are full of + # bracketed paths. Unescaped, a MarkupError would be raised from inside + # the very handler meant to report this failure. raise MFCException( - f"Test {case}: Failed to run h5dump on {silo_filepath} ({exists}).\n" - f"h5dump said: {output.strip() or '(no output)'}\n" - f"{log_tail(out_filepath)}\n" - f"Case dictionary: {case.get_filepath()}." + console_safe( + f"Test {case}: Failed to run h5dump on {silo_filepath} ({exists}).\n" + f"h5dump said: {output.strip() or '(no output)'}\n" + f"{log_tail(out_filepath)}\n" + f"Case dictionary: {case.get_filepath()}." + ) ) if "nan," in output: diff --git a/toolchain/mfc/test_bench_log_tail.py b/toolchain/mfc/test_bench_log_tail.py index bf76bdf7c3..2e3229851b 100644 --- a/toolchain/mfc/test_bench_log_tail.py +++ b/toolchain/mfc/test_bench_log_tail.py @@ -42,3 +42,22 @@ def test_names_the_log_so_the_full_file_can_still_be_found(tmp_path): log = tmp_path / "case.out" log.write_text("boom\n") assert "case.out" in log_tail(str(log)) + + +def test_reads_only_the_tail_of_a_large_log(tmp_path): + # Bounded memory matters: this runs on an already-failing path, and solver + # and benchmark logs reach tens of MB. + import tracemalloc + + log = tmp_path / "big.out" + log.write_text("".join(f"line {i} {'x' * 200}\n" for i in range(200_000))) + assert log.stat().st_size > 40_000_000 + + tracemalloc.start() + body = log_tail(str(log), max_lines=20) + peak = tracemalloc.get_traced_memory()[1] + tracemalloc.stop() + + assert "line 199999" in body + assert "line 100000" not in body + assert peak < 5_000_000, f"peak {peak} suggests the whole file was read" diff --git a/toolchain/mfc/test_build_preflight.py b/toolchain/mfc/test_build_preflight.py index 86962b3410..268a7309a6 100644 --- a/toolchain/mfc/test_build_preflight.py +++ b/toolchain/mfc/test_build_preflight.py @@ -37,16 +37,22 @@ def workspace(tmp_path): shutil.copytree(REPO / ".github", tmp_path / ".github") binz = tmp_path / "bin" binz.mkdir() - _exe(binz / "mpirun", '#!/bin/bash\nwhile [ "${1:-}" = "-np" ]; do shift 2; done\nexec "$@"\n') + passthrough = '#!/bin/bash\nwhile [ "${1:0:1}" = "-" ]; do shift; case "$1" in [0-9]*) shift;; esac; done\nexec "$@"\n' + _exe(binz / "mpirun", passthrough) + # frontier launches with srun; stub it so the real /usr/bin/srun on this box + # cannot stand in and try to submit an actual job. + _exe(binz / "srun", passthrough) trace = tmp_path / "trace.log" - def install_mfc(full_build_stdout="", full_build_rc=0, syscheck_rc=0): + def install_mfc(full_build_stdout="", full_build_rc=0, syscheck_rc=0, probe_build_stdout="", probe_build_rc=0): _exe( tmp_path / "mfc.sh", f"""#!/bin/bash echo "mfc.sh $*" >> {trace} for a in "$@"; do if [ "$a" = "syscheck" ]; then + printf '%s' {shlex_quote(probe_build_stdout)} + if [ {probe_build_rc} -ne 0 ]; then exit {probe_build_rc}; fi mkdir -p build/install/gpu/bin printf '#!/bin/bash\\necho probe ran\\nexit {syscheck_rc}\\n' > build/install/gpu/bin/syscheck chmod +x build/install/gpu/bin/syscheck @@ -140,3 +146,35 @@ def test_the_build_output_is_still_shown_when_it_fails(workspace): install_mfc(full_build_stdout="ftn-2116 ftn: INTERNAL\n", full_build_rc=1) result = run() assert "ftn-2116" in result.stdout + result.stderr + + +def test_a_pypi_failure_during_the_probe_build_records_an_outage(workspace): + # The probe build is now the first mfc.sh call in the job, so it is what + # bootstraps build/venv from PyPI -- and on Phoenix clean_build has just + # deleted that venv, so it is rebuilt every time. Classifying only the solver + # build leaves the breaker blind to the outage it exists for. + tmp_path, install_mfc, run, _ = workspace + install_mfc(probe_build_stdout=PYPI_FAILURE, probe_build_rc=1) + run() + assert outage_recorded(tmp_path) + + +def test_a_pypi_failure_during_the_probe_build_reports_the_outage_exit_code(workspace): + tmp_path, install_mfc, run, _ = workspace + install_mfc(probe_build_stdout=PYPI_FAILURE, probe_build_rc=1) + assert run().returncode == 78 + + +def test_an_ordinary_probe_build_failure_is_not_an_outage(workspace): + tmp_path, install_mfc, run, _ = workspace + install_mfc(probe_build_stdout="NVFORTRAN-S-0034-Syntax error\n", probe_build_rc=1) + result = run() + assert not outage_recorded(tmp_path) + assert result.returncode not in (77, 78) + + +def test_the_probe_build_output_is_shown_when_it_fails(workspace): + tmp_path, install_mfc, run, _ = workspace + install_mfc(probe_build_stdout="ftn-2116 ftn: INTERNAL\n", probe_build_rc=1) + result = run() + assert "ftn-2116" in result.stdout + result.stderr diff --git a/toolchain/mfc/test_ci_outage.py b/toolchain/mfc/test_ci_outage.py index adb767c097..24ba338b14 100644 --- a/toolchain/mfc/test_ci_outage.py +++ b/toolchain/mfc/test_ci_outage.py @@ -100,3 +100,15 @@ def test_an_unknown_subcommand_fails_with_a_usage_error(state_dir): result = run(state_dir, "frobnicate", "phoenix") assert result.returncode == 2 assert "usage" in (result.stdout + result.stderr).lower() + + +def test_a_non_numeric_ttl_falls_back_to_the_default_instead_of_erroring(state_dir): + # A typo in MFC_CI_OUTAGE_TTL_SECONDS must not make `check` exit with a code + # that is neither clear nor tripped -- that would gate CI on a config slip. + run(state_dir, "mark", "phoenix", "pypi unreachable") + assert run(state_dir, "check", "phoenix", ttl="not-a-number").returncode == TRIPPED + + +def test_an_empty_ttl_falls_back_to_the_default(state_dir): + run(state_dir, "mark", "phoenix", "pypi unreachable") + assert run(state_dir, "check", "phoenix", ttl="").returncode == TRIPPED diff --git a/toolchain/mfc/test_classify_build_failure.py b/toolchain/mfc/test_classify_build_failure.py new file mode 100644 index 0000000000..19bca303e6 --- /dev/null +++ b/toolchain/mfc/test_classify_build_failure.py @@ -0,0 +1,73 @@ +"""Tests for .github/scripts/classify-build-failure.sh. + +The classifier decides whether a failed build was a cluster-wide dependency +outage. Getting it wrong in either direction is costly: a missed outage means +every matrix job rediscovers it, and a false positive halts CI on an ordinary +compile error. +""" + +import os +import subprocess +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parents[2] / ".github" / "scripts" / "classify-build-failure.sh" + +OUTAGE = 78 +NOT_AN_OUTAGE = 0 + + +@pytest.fixture +def classify(tmp_path): + def _run(log_text): + log = tmp_path / "build.log" + log.write_text(log_text) + return subprocess.run( + ["bash", str(SCRIPT), str(log), "frontier"], + capture_output=True, + text=True, + check=False, + env={**os.environ, "MFC_CI_STATE_DIR": str(tmp_path / "state")}, + ).returncode + + return _run + + +@pytest.mark.parametrize( + "line", + [ + " |-> Failed to fetch: `https://pypi.org/simple/hatch-vcs/`", # uv, backticked + "Failed to fetch: https://pypi.org/simple/hatch-vcs/", # plain, no wrapper + " Failed to fetch: https://pypi.org/simple/build/", # padded + "mfc: ERROR > (venv) Installation failed.", + "mfc: WARNING > (venv) uv install failed; clearing the uv cache", + ], +) +def test_dependency_outages_are_recognised_in_every_formatting_variant(classify, line): + assert classify(line + "\n") == OUTAGE + + +@pytest.mark.parametrize( + "line", + [ + "NVFORTRAN-S-0034-Syntax error at or near end of line", + "ftn-2116 ftn: INTERNAL", + "clang: error: ld.lld command failed with exit code 1", + "CMake Error: could not find HDF5", + "Failed to fetch: https://example.com/not-pypi", + ], +) +def test_ordinary_build_failures_are_not_outages(classify, line): + assert classify(line + "\n") == NOT_AN_OUTAGE + + +def test_an_absent_log_is_never_called_an_outage(tmp_path): + result = subprocess.run( + ["bash", str(SCRIPT), str(tmp_path / "nope.log"), "frontier"], + capture_output=True, + text=True, + check=False, + env={**os.environ, "MFC_CI_STATE_DIR": str(tmp_path / "state")}, + ) + assert result.returncode == NOT_AN_OUTAGE diff --git a/toolchain/mfc/test_diagnostic_rendering.py b/toolchain/mfc/test_diagnostic_rendering.py new file mode 100644 index 0000000000..c8007b0e93 --- /dev/null +++ b/toolchain/mfc/test_diagnostic_rendering.py @@ -0,0 +1,69 @@ +"""Captured diagnostics must survive being printed. + +The console prints through Rich with markup enabled (printer.py), so raw log +text is interpreted as markup. Compiler and MPI output is full of square +brackets -- absolute paths in diagnostics, `[host:pid]` rank prefixes -- and +Rich either raises MarkupError on them or silently eats them as tags. + +That turns diagnostic capture into the opposite of its purpose: a crash, or a +message with the identifying parts removed. It is worse inside an MFCException, +because main.py renders that message with markup from inside the handler, so the +failure surfaces as an unhandled traceback instead of the intended report. +""" + +from rich.console import Console + +from mfc.common import console_safe, get_program_output + +HOSTILE = [ + "ftn: error in [/lustre/orion/cfd154/scratch/x.f90] line 3", + "[node1:12345] MPI abort", + "nvlink error: undefined reference in [/gpfs/alpine/proj/m_riemann.o]", +] + + +def render(text): + """Render the way the CLI does: a Rich console with markup enabled.""" + console = Console(file=open("/dev/null", "w"), record=True, width=200) + console.print(text, soft_wrap=True) + return console.export_text() + + +def test_rich_really_does_mangle_raw_log_text(): + # Guards the premise: if Rich ever stops doing this, console_safe can go. + raised = False + try: + render(HOSTILE[0]) + except Exception: + raised = True + assert raised or "[/lustre/orion/cfd154/scratch/x.f90]" not in render(HOSTILE[0]) + + +def test_console_safe_text_renders_without_raising(): + for line in HOSTILE: + render(console_safe(line)) + + +def test_console_safe_preserves_bracketed_paths(): + out = render(console_safe(HOSTILE[0])) + assert "[/lustre/orion/cfd154/scratch/x.f90]" in out + + +def test_console_safe_preserves_mpi_rank_prefixes(): + # "[node1:12345]" is how you tell which rank died; Rich eats it as a tag. + assert "[node1:12345]" in render(console_safe(HOSTILE[1])) + + +def test_get_program_output_can_capture_stderr(): + # h5dump writes "unable to open file" to stderr, so without this the + # diagnostic added to the h5dump failure path is always "(no output)". + out, code = get_program_output(["bash", "-c", "echo to-stderr >&2; exit 3"], merge_stderr=True) + assert code == 3 + assert "to-stderr" in out + + +def test_get_program_output_still_ignores_stderr_by_default(): + # Other callers parse stdout and must not start seeing stderr mixed in. + out, _ = get_program_output(["bash", "-c", "echo out; echo err >&2"]) + assert "err" not in out + assert "out" in out diff --git a/toolchain/mfc/test_frontier_deps.py b/toolchain/mfc/test_frontier_deps.py new file mode 100644 index 0000000000..82695c7180 --- /dev/null +++ b/toolchain/mfc/test_frontier_deps.py @@ -0,0 +1,95 @@ +"""Tests for .github/workflows/frontier/build.sh. + +Frontier installs its Python dependencies on the login node, in the "Fetch +Dependencies" step, before any SLURM job exists. That is where the PyPI outage +of 2026-08-28 actually landed -- 17 jobs, ~33 minutes each, all learning the +same thing independently. Classifying only the in-allocation build would leave +the breaker blind to the case that motivated it. +""" + +import os +import shutil +import stat +import subprocess +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parents[2] + +PYPI_FAILURE = " |-> Failed to fetch: `https://pypi.org/simple/hatch-vcs/`\n" "mfc: ERROR > (venv) Installation failed.\n" + + +def _exe(path: Path, text: str): + path.write_text(text) + path.chmod(path.stat().st_mode | stat.S_IEXEC) + + +@pytest.fixture +def workspace(tmp_path): + shutil.copytree(REPO / ".github", tmp_path / ".github") + + def install_mfc(stdout="", rc=0): + # `. ./mfc.sh load` is sourced, so the stub must return rather than exit + # for that subcommand or it would terminate build.sh itself. + _exe( + tmp_path / "mfc.sh", + f'#!/bin/bash\nif [ "$1" = "load" ]; then return 0 2>/dev/null || exit 0; fi\n' f"printf '%s' {_q(stdout)}\nexit {rc}\n", + ) + + def run(): + env = { + **os.environ, + "MFC_CI_STATE_DIR": str(tmp_path / "state"), + "MFC_BUILD_RETRY_DELAY": "0", + } + return subprocess.run( + ["bash", ".github/workflows/frontier/build.sh", "gpu", "acc"], + capture_output=True, + text=True, + cwd=tmp_path, + env=env, + check=False, + timeout=120, + ) + + return tmp_path, install_mfc, run + + +def _q(s): + import shlex + + return shlex.quote(s) + + +def outage_recorded(tmp_path): + state = tmp_path / "state" + return state.exists() and any(state.glob("outage-*")) + + +def test_a_successful_dependency_fetch_records_nothing(workspace): + tmp_path, install_mfc, run = workspace + install_mfc() + assert run().returncode == 0 + assert not outage_recorded(tmp_path) + + +def test_a_pypi_outage_on_the_login_node_is_recorded(workspace): + tmp_path, install_mfc, run = workspace + install_mfc(stdout=PYPI_FAILURE, rc=1) + run() + assert outage_recorded(tmp_path) + + +def test_a_pypi_outage_on_the_login_node_reports_the_outage_exit_code(workspace): + tmp_path, install_mfc, run = workspace + install_mfc(stdout=PYPI_FAILURE, rc=1) + assert run().returncode == 78 + + +def test_an_ordinary_dependency_failure_is_not_an_outage(workspace): + tmp_path, install_mfc, run = workspace + install_mfc(stdout="CMake Error: could not find HDF5\n", rc=1) + result = run() + assert not outage_recorded(tmp_path) + assert result.returncode != 78 diff --git a/toolchain/mfc/test_preflight.py b/toolchain/mfc/test_preflight.py index 5102cca21b..23db927bd8 100644 --- a/toolchain/mfc/test_preflight.py +++ b/toolchain/mfc/test_preflight.py @@ -30,18 +30,42 @@ @pytest.fixture def workspace(tmp_path): - """A workspace with a stubbed mpirun on PATH and a place for syscheck.""" - binz = tmp_path / "bin" - binz.mkdir() - mpirun = binz / "mpirun" - # Passthrough: drop "-np N" and exec the binary, mirroring how build.sh runs - # syscheck under mpirun. - mpirun.write_text('#!/bin/bash\nwhile [ "${1:-}" = "-np" ]; do shift 2; done\nexec "$@"\n') - mpirun.chmod(mpirun.stat().st_mode | stat.S_IEXEC) + """A bare workspace. No launcher is on PATH unless a test installs one. + + An earlier version of this fixture put a fake mpirun on PATH for every test, + which is exactly why it could not see that preflight ran mpirun on Frontier, + where the launcher is srun and Cray MPICH ships no mpirun at all. + """ + (tmp_path / "bin").mkdir() (tmp_path / "state").mkdir() + + # A hermetic PATH holding only the utilities the scripts need. This box has + # a real mpirun *and* a real /usr/bin/srun, either of which would silently + # stand in for a launcher the test meant to be absent. + sysbin = tmp_path / "sysbin" + sysbin.mkdir() + for tool in ("bash", "find", "head", "tail", "cat", "sed", "grep", "tr", "cut", "date", "mkdir", "mv", "rm", "hostname", "env", "sort", "wc", "dirname", "basename"): + for root in ("/usr/bin", "/bin"): + src = Path(root) / tool + if src.exists(): + (sysbin / tool).symlink_to(src) + break return tmp_path +def install_launcher(workspace, name): + """A passthrough launcher that records its argv, mirroring mpirun/srun.""" + launcher = workspace / "bin" / name + launcher.write_text("#!/bin/bash\n" f'echo "$@" >> {workspace}/launched.txt\n' 'while [ "${1:0:1}" = "-" ]; do shift; case "$1" in [0-9]*) shift;; esac; done\n' 'exec "$@"\n') + launcher.chmod(launcher.stat().st_mode | stat.S_IEXEC) + return launcher + + +def launched_with(workspace): + path = workspace / "launched.txt" + return path.read_text() if path.exists() else "" + + def write_syscheck(workspace, exit_code, message="syscheck says hello"): target = workspace / "build" / "install" / "gpu-acc-abc123" / "bin" target.mkdir(parents=True, exist_ok=True) @@ -54,7 +78,9 @@ def write_syscheck(workspace, exit_code, message="syscheck says hello"): def run(workspace, *args): env = { **os.environ, - "PATH": f"{workspace / 'bin'}:{os.environ['PATH']}", + # A controlled PATH, not the inherited one: this box has a real mpirun, + # and inheriting it makes "no launcher available" silently untestable. + "PATH": f"{workspace / 'bin'}:{workspace / 'sysbin'}", "MFC_CI_STATE_DIR": str(workspace / "state"), "SLURMD_NODENAME": "atl1-1-03-007-29-0", } @@ -115,3 +141,39 @@ def test_does_not_report_a_node_fault_merely_because_pmix_printed_a_warning(work # would fail roughly one healthy job in six. write_syscheck(workspace, 0, message="PMIX ERROR: PMIX_ERR_NO_PERMISSIONS in file dstore_base.c at line 238") assert run(workspace).returncode == HEALTHY + + +def test_uses_mpirun_on_phoenix(workspace): + install_launcher(workspace, "mpirun") + write_syscheck(workspace, 0) + assert run(workspace, "phoenix", "gpu").returncode == HEALTHY + assert "syscheck" in launched_with(workspace) + + +def test_uses_srun_on_frontier(workspace): + # Frontier and frontier_amd launch every binary with srun + # (toolchain/templates/frontier.mako, frontier_amd.mako); Cray MPICH ships no + # mpirun, so running one there is not merely wrong, it cannot work. + install_launcher(workspace, "srun") + write_syscheck(workspace, 0) + assert run(workspace, "frontier", "gpu").returncode == HEALTHY + assert "syscheck" in launched_with(workspace) + + +def test_a_missing_launcher_is_not_blamed_on_the_node(workspace): + # Without this, a launcher absent from PATH returns 127, preflight calls the + # node bad, and the requeue loop burns three allocations and blacklists three + # healthy nodes before declaring a cluster-wide problem. + write_syscheck(workspace, 0) + assert run(workspace, "frontier", "gpu").returncode == HEALTHY + + +def test_a_breaker_that_cannot_be_read_does_not_halt_the_job(workspace): + # Exit 1 from ci-outage.sh means "tripped"; any other failure means the check + # itself broke. Conflating them turns a bug in the breaker into a CI outage. + write_syscheck(workspace, 0) + (workspace / "state").chmod(0o000) + try: + assert run(workspace, "phoenix", "gpu").returncode == HEALTHY + finally: + (workspace / "state").chmod(0o755) diff --git a/toolchain/mfc/test_submit_requeue.py b/toolchain/mfc/test_submit_requeue.py index 39cde5f09a..3cbf48f7be 100644 --- a/toolchain/mfc/test_submit_requeue.py +++ b/toolchain/mfc/test_submit_requeue.py @@ -90,6 +90,8 @@ def run(monitor_codes, script="common/build-and-test.sh", **env_extra): result.submissions = sorted(submissions.glob("submission-*.sh")) return result + run.scripts = scripts + run.state_dir = tmp_path / "state" return run @@ -136,3 +138,19 @@ def test_a_known_outage_is_not_resubmitted(rig): # another allocation. result = rig("78") assert len(result.submissions) == 1 + + +def test_no_job_is_submitted_while_the_cluster_is_under_a_recorded_outage(rig): + # ci-outage.sh's whole promise is that later jobs "exit immediately instead + # of submitting a SLURM job that is going to fail". Checking it only inside + # the allocation means every job still pays the queue wait first -- hours on + # Phoenix embers -- before discovering the marker. + subprocess.run( + ["bash", str(rig.scripts / "ci-outage.sh"), "mark", "phoenix", "pypi unreachable"], + env={**os.environ, "MFC_CI_STATE_DIR": str(rig.state_dir)}, + capture_output=True, + check=True, + ) + result = rig("0") + assert len(result.submissions) == 0 + assert result.returncode == 78 diff --git a/toolchain/mfc/test_test_sh_preflight.py b/toolchain/mfc/test_test_sh_preflight.py index 47e2e8a99d..ae4a0b51ff 100644 --- a/toolchain/mfc/test_test_sh_preflight.py +++ b/toolchain/mfc/test_test_sh_preflight.py @@ -28,7 +28,11 @@ def workspace(tmp_path): shutil.copytree(REPO / ".github", tmp_path / ".github") binz = tmp_path / "bin" binz.mkdir() - _exe(binz / "mpirun", '#!/bin/bash\nwhile [ "${1:-}" = "-np" ]; do shift 2; done\nexec "$@"\n') + passthrough = '#!/bin/bash\nwhile [ "${1:0:1}" = "-" ]; do shift; case "$1" in [0-9]*) shift;; esac; done\nexec "$@"\n' + _exe(binz / "mpirun", passthrough) + # job_cluster here is frontier, whose launcher is srun. Without a stub the + # real /usr/bin/srun on this box would try to submit an actual job. + _exe(binz / "srun", passthrough) _exe(binz / "nvidia-smi", "#!/bin/bash\necho 'GPU 0: fake'\n") trace = tmp_path / "trace.log" _exe(tmp_path / "mfc.sh", f'#!/bin/bash\necho "mfc.sh $*" >> {trace}\nexit 0\n') From ca5d52d36a7b4d40021b2c41db4758ac43c09eb9 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Mon, 31 Aug 2026 22:05:14 -0500 Subject: [PATCH 03/10] ci: preflight refuses to judge a node outside a SLURM allocation mfc.sh load is not only used inside batch jobs -- it is also used for building on login nodes, and with the GPU module set at that: bench.yml:142 and frontier/build.sh:20 both do `. ./mfc.sh load -m g` there. A login node has no GPU to probe, so a preflight running in that context would find no usable device, report a node fault, and have submit-slurm-job.sh exclude a login node and requeue around it. Wrong, and unpleasant to diagnose from the far end. Nothing reaches the probe that way today: common/build.sh, common/test.sh and common/build-and-test.sh are invoked only through submit-slurm-job.sh, whose four call sites in test.yml all submit. But that is a convention every future caller has to remember rather than a property of the probe. Skipping when SLURM_JOB_ID is unset makes it the latter. The two fixtures covering the bad-node path now set SLURM_JOB_ID, since in production those scripts only ever run inside an allocation. --- .github/scripts/preflight.sh | 12 ++++++++++++ toolchain/mfc/test_build_preflight.py | 4 ++++ toolchain/mfc/test_preflight.py | 17 ++++++++++++++++- toolchain/mfc/test_test_sh_preflight.py | 4 ++++ 4 files changed, 36 insertions(+), 1 deletion(-) diff --git a/.github/scripts/preflight.sh b/.github/scripts/preflight.sh index 1fbd2537de..fd04187c6c 100755 --- a/.github/scripts/preflight.sh +++ b/.github/scripts/preflight.sh @@ -38,6 +38,18 @@ EXIT_OUTAGE=78 SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" node="${SLURMD_NODENAME:-$(hostname -s 2>/dev/null || hostname)}" +# Only judge a node from inside its own allocation. `mfc.sh load` is also used +# for building on login nodes -- bench.yml and frontier/build.sh both load the +# GPU module set there -- and a login node has no GPU to probe. Reporting a node +# fault in that context would have the wrapper exclude a login node and requeue, +# which is both wrong and hard to diagnose. Nothing calls the probe from a login +# node today; this makes that a property of the probe rather than a convention +# every future caller has to remember. +if [ -z "${SLURM_JOB_ID:-}" ]; then + echo "Preflight: not inside a SLURM allocation; skipping the node probe." + exit $EXIT_HEALTHY +fi + # --- Cluster-wide outage: requeuing cannot help, so skip rather than retry --- outage_rc=0 bash "$SCRIPT_DIR/ci-outage.sh" check "$cluster" || outage_rc=$? diff --git a/toolchain/mfc/test_build_preflight.py b/toolchain/mfc/test_build_preflight.py index 268a7309a6..ab6d16e03c 100644 --- a/toolchain/mfc/test_build_preflight.py +++ b/toolchain/mfc/test_build_preflight.py @@ -76,6 +76,10 @@ def run(): "job_cluster": "frontier", "job_variant": "", "SLURMD_NODENAME": "frontier1234", + # These scripts only ever run inside a SLURM allocation + # (submit-slurm-job.sh is their sole caller), and the probe + # refuses to judge a node outside one. + "SLURM_JOB_ID": "123456", } return subprocess.run( ["bash", ".github/workflows/common/build.sh"], diff --git a/toolchain/mfc/test_preflight.py b/toolchain/mfc/test_preflight.py index 23db927bd8..53f13d610d 100644 --- a/toolchain/mfc/test_preflight.py +++ b/toolchain/mfc/test_preflight.py @@ -75,7 +75,7 @@ def write_syscheck(workspace, exit_code, message="syscheck says hello"): return binary -def run(workspace, *args): +def run(workspace, *args, **overrides): env = { **os.environ, # A controlled PATH, not the inherited one: this box has a real mpirun, @@ -83,7 +83,12 @@ def run(workspace, *args): "PATH": f"{workspace / 'bin'}:{workspace / 'sysbin'}", "MFC_CI_STATE_DIR": str(workspace / "state"), "SLURMD_NODENAME": "atl1-1-03-007-29-0", + # Present by default: these tests describe behaviour inside a job. + "SLURM_JOB_ID": "123456", } + if overrides.pop("MFC_NO_SLURM", None): + env.pop("SLURM_JOB_ID", None) + env.update(overrides) return subprocess.run( ["bash", str(SCRIPT), *(args or ("phoenix", "gpu"))], capture_output=True, @@ -177,3 +182,13 @@ def test_a_breaker_that_cannot_be_read_does_not_halt_the_job(workspace): assert run(workspace, "phoenix", "gpu").returncode == HEALTHY finally: (workspace / "state").chmod(0o755) + + +def test_it_refuses_to_judge_a_node_outside_a_slurm_allocation(workspace): + # mfc.sh load is used for building on login nodes too (bench.yml and + # frontier/build.sh both load the GPU module set there). A probe that ran in + # that context would find no usable GPU, call the login node bad, and have + # the wrapper exclude it and requeue. Only a real allocation can be judged. + write_syscheck(workspace, 1) + env_without_slurm = {"MFC_NO_SLURM": "1"} + assert run(workspace, "phoenix", "gpu", **env_without_slurm).returncode == HEALTHY diff --git a/toolchain/mfc/test_test_sh_preflight.py b/toolchain/mfc/test_test_sh_preflight.py index ae4a0b51ff..e27d223bba 100644 --- a/toolchain/mfc/test_test_sh_preflight.py +++ b/toolchain/mfc/test_test_sh_preflight.py @@ -54,6 +54,10 @@ def run(): "job_variant": "", "GITHUB_EVENT_NAME": "push", "SLURMD_NODENAME": "frontier9999", + # These scripts only ever run inside a SLURM allocation + # (submit-slurm-job.sh is their sole caller), and the probe + # refuses to judge a node outside one. + "SLURM_JOB_ID": "123456", } return subprocess.run( ["bash", ".github/workflows/common/test.sh"], From a46e55fa3ff0053f4e3aef5f79e38a428f787b98 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Mon, 31 Aug 2026 22:32:09 -0500 Subject: [PATCH 04/10] ci: probe the node on the benchmark and case-optimization paths too The probe was wired into common/build.sh and common/test.sh, which only the test.yml `self` job uses. Three of the five entry points that submit SLURM jobs -- the benchmark job and both case-optimization jobs -- go straight to submit-slurm-job.sh and never touch those scripts, so they ran GPU work on nodes nothing had checked. That was 134 of the 540 first-attempt failures in the Aug window: Benchmark workflow jobs 52 Case Opt jobs 82 covered (test.yml self) 406 including all 11 ECC failures outside the `self` job and the 13 bench cases that died with a bare "exit code 143". Wired per script rather than into the sbatch heredoc. The heredoc looks like the tidier single place, but it runs before the job's own payload, and these scripts nuke and rebuild build/ themselves -- bench.sh does so on Phoenix precisely because its compute nodes are heterogeneous and stale binaries risk an ISA mismatch. A probe there would test a leftover binary from a previous job, and a SIGILL from a wrong-microarchitecture build would be reported as a bad node, excluding a healthy one. Each call therefore sits after its own script's build. The test that pins this ordering planted a stale binary to make the hazard concrete, and it promptly caught a second instance of the same class: `find build/install -name syscheck | head -1` returned whichever path came first, which was the stale one. Not every caller cleans build/ first (bench.sh only does on Phoenix), so the probe now takes the newest matching binary rather than the first found. 487 toolchain tests pass. --- .github/scripts/prebuild-case-optimization.sh | 12 ++ .github/scripts/preflight.sh | 19 ++- .github/scripts/run_case_optimization.sh | 12 ++ .github/workflows/common/bench.sh | 12 ++ toolchain/mfc/test_bench_preflight.py | 122 ++++++++++++++++++ 5 files changed, 172 insertions(+), 5 deletions(-) create mode 100644 toolchain/mfc/test_bench_preflight.py diff --git a/.github/scripts/prebuild-case-optimization.sh b/.github/scripts/prebuild-case-optimization.sh index c31058a08a..caccf7c218 100755 --- a/.github/scripts/prebuild-case-optimization.sh +++ b/.github/scripts/prebuild-case-optimization.sh @@ -90,6 +90,18 @@ if [ -n "$shard" ] && [ "$shard_count" -gt 1 ]; then fi fi +# Probe this node before spending the allocation on it. Placed after the build, +# not in the sbatch template: these scripts nuke and rebuild build/ themselves +# (Phoenix does so precisely because its compute nodes are heterogeneous), so a +# probe running earlier would test a stale binary from a previous job -- likely +# built for another microarchitecture -- and a SIGILL there would be reported as +# a bad node, excluding a healthy one. +preflight_rc=0 +bash .github/scripts/preflight.sh "$job_cluster" "${job_device:-gpu}" || preflight_rc=$? +if [ "$preflight_rc" -ne 0 ]; then + exit "$preflight_rc" +fi + idx=0 for case in "${benchmarks[@]}"; do idx=$((idx + 1)) diff --git a/.github/scripts/preflight.sh b/.github/scripts/preflight.sh index fd04187c6c..e88743201a 100755 --- a/.github/scripts/preflight.sh +++ b/.github/scripts/preflight.sh @@ -64,12 +64,21 @@ elif [ "$outage_rc" -ne 0 ]; then fi # --- Node health --- -# Prefer an install matching this job's device (build/install is named e.g. -# gpu-acc-, gpu-mp-), so a leftover install from another variant is -# not probed instead. Fall back to any syscheck if none matches. -syscheck_bin=$(find build/install -path "*${device}*" -name syscheck -type f 2>/dev/null | head -1) +# Pick the *newest* install matching this job's device (build/install is named +# e.g. gpu-acc-, gpu-mp-). Both halves matter: the device filter +# avoids probing another variant's binary, and newest-wins avoids probing a +# leftover from an earlier job. Not every caller nukes build/ first -- bench.sh +# only does so on Phoenix -- and a stale binary compiled for a different +# microarchitecture dies with SIGILL, which would be reported as a bad node and +# get a perfectly healthy one excluded. +newest_syscheck() { + find "$@" -name syscheck -type f -printf '%T@ %p\n' 2>/dev/null \ + | sort -rn | head -1 | cut -d' ' -f2- +} + +syscheck_bin=$(newest_syscheck build/install -path "*${device}*") if [ -z "$syscheck_bin" ]; then - syscheck_bin=$(find build/install -name syscheck -type f 2>/dev/null | head -1) + syscheck_bin=$(newest_syscheck build/install) fi if [ -z "$syscheck_bin" ]; then diff --git a/.github/scripts/run_case_optimization.sh b/.github/scripts/run_case_optimization.sh index 26899da211..75ab3a44a4 100755 --- a/.github/scripts/run_case_optimization.sh +++ b/.github/scripts/run_case_optimization.sh @@ -60,6 +60,18 @@ else build_opts="--no-build" fi +# Probe this node before spending the allocation on it. Placed after the build, +# not in the sbatch template: these scripts nuke and rebuild build/ themselves +# (Phoenix does so precisely because its compute nodes are heterogeneous), so a +# probe running earlier would test a stale binary from a previous job -- likely +# built for another microarchitecture -- and a SIGILL there would be reported as +# a bad node, excluding a healthy one. +preflight_rc=0 +bash .github/scripts/preflight.sh "$job_cluster" "$job_device" || preflight_rc=$? +if [ "$preflight_rc" -ne 0 ]; then + exit "$preflight_rc" +fi + passed=0 failed=0 failed_cases="" diff --git a/.github/workflows/common/bench.sh b/.github/workflows/common/bench.sh index be83e57b87..5d108a2a66 100644 --- a/.github/workflows/common/bench.sh +++ b/.github/workflows/common/bench.sh @@ -34,6 +34,18 @@ fi source .github/scripts/retry-build.sh retry_build ./mfc.sh build -j $n_jobs $build_opts || exit 1 +# Probe this node before spending the allocation on it. Placed after the build, +# not in the sbatch template: these scripts nuke and rebuild build/ themselves +# (Phoenix does so precisely because its compute nodes are heterogeneous), so a +# probe running earlier would test a stale binary from a previous job -- likely +# built for another microarchitecture -- and a SIGILL there would be reported as +# a bad node, excluding a healthy one. +preflight_rc=0 +bash .github/scripts/preflight.sh "$job_cluster" "$job_device" || preflight_rc=$? +if [ "$preflight_rc" -ne 0 ]; then + exit "$preflight_rc" +fi + # --- Bench cluster flag --- if [ "$job_cluster" = "phoenix" ]; then bench_cluster="phoenix-bench" diff --git a/toolchain/mfc/test_bench_preflight.py b/toolchain/mfc/test_bench_preflight.py new file mode 100644 index 0000000000..1ab38877f3 --- /dev/null +++ b/toolchain/mfc/test_bench_preflight.py @@ -0,0 +1,122 @@ +"""The benchmark and case-optimization paths must probe their node too. + +The probe was originally wired only into common/build.sh and common/test.sh, +which the test.yml `self` job uses. But three of the five entry points that +submit SLURM jobs -- the benchmark job and both case-optimization jobs -- go +straight to submit-slurm-job.sh and never touch those scripts. That left 134 of +540 first-attempt failures on unprobed paths, including every ECC failure +outside the `self` job. + +Ordering is load-bearing here, hence the second test. These scripts nuke and +rebuild build/ themselves -- bench.sh does so on Phoenix precisely because "compute +nodes are heterogeneous -> ISA mismatch risk" -- so a probe placed before that +would test a leftover binary from a previous job, quite possibly built for +another microarchitecture. The SIGILL would be reported as a bad node, and the +wrapper would exclude a perfectly healthy one. +""" + +import os +import shutil +import stat +import subprocess +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parents[2] + + +def _exe(path: Path, text: str): + path.write_text(text) + path.chmod(path.stat().st_mode | stat.S_IEXEC) + + +@pytest.fixture +def workspace(tmp_path): + shutil.copytree(REPO / ".github", tmp_path / ".github") + binz = tmp_path / "bin" + binz.mkdir() + trace = tmp_path / "trace.log" + + passthrough = '#!/bin/bash\nwhile [ "${1:0:1}" = "-" ]; do shift; case "$1" in [0-9]*) shift;; esac; done\nexec "$@"\n' + for name in ("mpirun", "srun"): + _exe(binz / name, passthrough) + _exe(binz / "nvidia-smi", "#!/bin/bash\necho 'GPU 0: fake'\n") + + def install_mfc(probe_exit=0): + _exe( + tmp_path / "mfc.sh", + f"""#!/bin/bash +echo "mfc.sh $*" >> {trace} +for a in "$@"; do + if [ "$a" = "build" ]; then + mkdir -p build/install/gpu/bin + printf '#!/bin/bash\\nexit {probe_exit}\\n' > build/install/gpu/bin/syscheck + chmod +x build/install/gpu/bin/syscheck + fi +done +exit 0 +""", + ) + # A stale binary from a "previous job" that a too-early probe would find. + stale = tmp_path / "build" / "install" / "stale-gpu" / "bin" + stale.mkdir(parents=True, exist_ok=True) + _exe(stale / "syscheck", "#!/bin/bash\necho 'stale SIGILL'\nexit 132\n") + + def run(): + env = { + **os.environ, + "PATH": f"{binz}:{os.environ['PATH']}", + "MFC_CI_STATE_DIR": str(tmp_path / "state"), + "MFC_BUILD_RETRY_DELAY": "0", + "SLURM_JOB_ID": "123456", + "SLURMD_NODENAME": "frontier4242", + "job_device": "gpu", + "job_interface": "acc", + "job_slug": "bench-gpu-acc", + "job_cluster": "frontier", + } + return subprocess.run( + ["bash", ".github/workflows/common/bench.sh"], + capture_output=True, + text=True, + cwd=tmp_path, + env=env, + check=False, + timeout=120, + ) + + return tmp_path, install_mfc, run, trace + + +def test_the_benchmark_job_probes_its_node(workspace): + _, install_mfc, run, _ = workspace + install_mfc(probe_exit=0) + result = run() + assert result.returncode == 0, result.stdout + result.stderr + assert "Preflight:" in result.stdout + + +def test_a_bad_node_stops_the_benchmark_before_it_runs(workspace): + _, install_mfc, run, trace = workspace + install_mfc(probe_exit=1) + result = run() + assert result.returncode == 77 + assert "mfc.sh bench" not in trace.read_text() + + +def test_the_probe_runs_after_the_build_not_before_it(workspace): + # If the probe ran first it would find the stale binary planted above, which + # exits 132 like a SIGILL, and condemn a healthy node. + _, install_mfc, run, trace = workspace + install_mfc(probe_exit=0) + result = run() + assert result.returncode == 0, result.stdout + result.stderr + calls = trace.read_text().splitlines() + assert any("build" in c for c in calls), calls + build_at = next(i for i, c in enumerate(calls) if "build" in c) + probe_at = result.stdout.index("Preflight:") + build_marker = result.stdout.find("mfc.sh build") + assert build_at == 0 + if build_marker != -1: + assert probe_at > build_marker From 082e01e5154cd54db93a63312dacc78dc12ebbe2 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Tue, 1 Sep 2026 06:44:19 -0500 Subject: [PATCH 05/10] ci: make the case-optimization pre-build probe actually run The probe needs a syscheck binary, and where one comes from differs by shard mode. Sharded, shard 1 builds it under the marker coordination and the others wait. Unsharded, that block is skipped entirely and nothing has been built when the probe runs -- so on Phoenix, where case-optimization is unsharded, it reported Preflight: no syscheck binary under build/install; skipping node probe and did nothing at all. A guard that skips reads exactly like a guard that passed, which is why this went unnoticed until a CI log showed the skip line. Unsharded now builds the probe binary first; sharded still does not, because building it there would race the shared build/install across concurrent shards, which is the collision the coordination exists to prevent. Both modes are pinned by tests. Found while diagnosing a Case Opt | Phoenix failure that turned out to be unrelated: that job was cancelled at 8h 1m against timeout-minutes: 480, having spent the whole time in "Waiting for job to start..." on the preemptible embers queue. Pre-existing queue starvation, not this change. --- .github/scripts/prebuild-case-optimization.sh | 22 +++- toolchain/mfc/test_prebuild_preflight.py | 112 ++++++++++++++++++ 2 files changed, 128 insertions(+), 6 deletions(-) create mode 100644 toolchain/mfc/test_prebuild_preflight.py diff --git a/.github/scripts/prebuild-case-optimization.sh b/.github/scripts/prebuild-case-optimization.sh index caccf7c218..94f799e1d3 100755 --- a/.github/scripts/prebuild-case-optimization.sh +++ b/.github/scripts/prebuild-case-optimization.sh @@ -90,12 +90,22 @@ if [ -n "$shard" ] && [ "$shard_count" -gt 1 ]; then fi fi -# Probe this node before spending the allocation on it. Placed after the build, -# not in the sbatch template: these scripts nuke and rebuild build/ themselves -# (Phoenix does so precisely because its compute nodes are heterogeneous), so a -# probe running earlier would test a stale binary from a previous job -- likely -# built for another microarchitecture -- and a SIGILL there would be reported as -# a bad node, excluding a healthy one. +# Probe this node before pre-building every case on it. The probe needs a +# syscheck binary, and where that comes from differs by mode: +# +# sharded - shard 1 already built syscheck under the marker coordination +# above and the others waited for it. Building it again here +# would run concurrently across shards that share build/install, +# which is the collision the coordination exists to prevent. +# unsharded - nothing has been built yet, so build the probe binary now. It +# links in seconds and is a no-op if it is already current. +# +# Getting this wrong is silent: the probe simply reports "no syscheck binary" +# and skips, which is how the unsharded Phoenix path went unprobed at first. +if [ -z "$shard" ] || [ "$shard_count" -le 1 ]; then + ./mfc.sh build -t syscheck -j 8 $gpu_opts +fi + preflight_rc=0 bash .github/scripts/preflight.sh "$job_cluster" "${job_device:-gpu}" || preflight_rc=$? if [ "$preflight_rc" -ne 0 ]; then diff --git a/toolchain/mfc/test_prebuild_preflight.py b/toolchain/mfc/test_prebuild_preflight.py new file mode 100644 index 0000000000..bd321b7879 --- /dev/null +++ b/toolchain/mfc/test_prebuild_preflight.py @@ -0,0 +1,112 @@ +"""The case-optimization pre-build must actually probe, in both shard modes. + +The probe needs a syscheck binary to exist. In the sharded mode shard 1 builds +one under the script's marker coordination and the others wait for it; in the +unsharded mode nothing has been built when the probe runs, so it has to build +the binary itself. + +That asymmetry failed silently the first time: on Phoenix, where +case-optimization is unsharded, the probe reported "no syscheck binary under +build/install; skipping node probe" and did nothing at all. A guard that skips +looks exactly like a guard that passed, which is why this is pinned. +""" + +import os +import shutil +import stat +import subprocess +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parents[2] + + +def _exe(path: Path, text: str): + path.write_text(text) + path.chmod(path.stat().st_mode | stat.S_IEXEC) + + +@pytest.fixture +def workspace(tmp_path): + shutil.copytree(REPO / ".github", tmp_path / ".github") + binz = tmp_path / "bin" + binz.mkdir() + _exe( + binz / "mpirun", + '#!/bin/bash\nwhile [ "${1:0:1}" = "-" ]; do shift; case "$1" in [0-9]*) shift;; esac; done\nexec "$@"\n', + ) + trace = tmp_path / "trace.log" + + def install_mfc(probe_exit=0): + _exe( + tmp_path / "mfc.sh", + f"""#!/bin/bash +echo "mfc.sh $*" >> {trace} +if [ "$1" = "load" ]; then return 0 2>/dev/null || exit 0; fi +for a in "$@"; do + if [ "$a" = "syscheck" ]; then + mkdir -p build/install/gpu-acc-abc/bin + printf '#!/bin/bash\\nexit {probe_exit}\\n' > build/install/gpu-acc-abc/bin/syscheck + chmod +x build/install/gpu-acc-abc/bin/syscheck + fi +done +exit 0 +""", + ) + + def run(shard="", cluster="phoenix"): + env = { + **os.environ, + "PATH": f"{binz}:{os.environ['PATH']}", + "MFC_CI_STATE_DIR": str(tmp_path / "state"), + "SLURM_JOB_ID": "123456", + "SLURMD_NODENAME": "atl1-1-02-008-1-1", + "job_cluster": cluster, + "job_device": "gpu", + "job_interface": "acc", + "job_shard": shard, + } + return subprocess.run( + ["bash", ".github/scripts/prebuild-case-optimization.sh"], + capture_output=True, + text=True, + cwd=tmp_path, + env=env, + check=False, + timeout=120, + ) + + return tmp_path, install_mfc, run, trace + + +def test_the_unsharded_prebuild_really_probes(workspace): + # Phoenix runs case-optimization unsharded. This is the path that silently + # skipped. + _, install_mfc, run, _ = workspace + install_mfc(probe_exit=0) + out = run().stdout + assert "Preflight: probing" in out + assert "no syscheck binary" not in out + + +def test_a_bad_node_stops_the_unsharded_prebuild(workspace): + _, install_mfc, run, trace = workspace + install_mfc(probe_exit=1) + result = run() + assert result.returncode == 77 + assert "Pre-building" not in result.stdout + + +def test_the_sharded_prebuild_does_not_rebuild_syscheck_concurrently(workspace): + # Shard 2 waits on shard 1's marker; building syscheck here would race the + # shared build/install that the coordination exists to serialize. + tmp_path, install_mfc, run, trace = workspace + install_mfc(probe_exit=0) + # frontier_amd, not phoenix: sharding is a frontier_amd mode, and phoenix's + # clean_build would move build/ aside and take the marker with it. + (tmp_path / "build").mkdir(exist_ok=True) + (tmp_path / "build" / ".prebuild-shared-targets-done").touch() + run(shard="2/3", cluster="frontier_amd") + builds = [c for c in trace.read_text().splitlines() if "build -t syscheck" in c] + assert builds == [], f"shard 2 must not build syscheck itself, got {builds}" From e7aa42c49d0ad4b268f9feaddfb312e790b57519 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Tue, 1 Sep 2026 07:19:39 -0500 Subject: [PATCH 06/10] ci: do not probe the node in the case-optimization pre-build This one was self-inflicted, and it produced exactly the false positive the guard exists to prevent: three healthy Phoenix nodes reported as faulty, two added to --exclude, and the job abandoned with "Preflight failed on 3 nodes in a row". The pre-build is the one SLURM job MFC submits whose allocation device deliberately differs from what it builds. test.yml submits it as cpu -- it is a --dry-run that only builds, so it needs no GPU -- while the binaries it produces are GPU builds. A syscheck built with --gpu asserts omp_get_num_devices() > 0, which is false on a GPU-less node by design, so the probe could never have passed there: job_device=cpu prebuild-case-optimization.sh cpu omp [TEST] OMP: call assert(num_devices > 0) -> 1 Every other submitted job runs binaries built for the device it asked for, which is why this did not show up anywhere else. The GPU allocation that actually runs these cases is still probed, in run_case_optimization.sh, so case-optimization keeps its coverage where coverage is meaningful. No lasting damage: node_exclude is a shell variable scoped to a single submit-slurm-job.sh invocation, so nothing persisted past the job. The previous commit's tests asserted the pre-build *should* probe, which was the wrong premise; they now assert it must not, and say why, so the next person does not re-add it. --- .github/scripts/prebuild-case-optimization.sh | 29 ++-- toolchain/mfc/test_prebuild_preflight.py | 126 ++++-------------- 2 files changed, 37 insertions(+), 118 deletions(-) diff --git a/.github/scripts/prebuild-case-optimization.sh b/.github/scripts/prebuild-case-optimization.sh index 94f799e1d3..bcd117f232 100755 --- a/.github/scripts/prebuild-case-optimization.sh +++ b/.github/scripts/prebuild-case-optimization.sh @@ -90,27 +90,14 @@ if [ -n "$shard" ] && [ "$shard_count" -gt 1 ]; then fi fi -# Probe this node before pre-building every case on it. The probe needs a -# syscheck binary, and where that comes from differs by mode: -# -# sharded - shard 1 already built syscheck under the marker coordination -# above and the others waited for it. Building it again here -# would run concurrently across shards that share build/install, -# which is the collision the coordination exists to prevent. -# unsharded - nothing has been built yet, so build the probe binary now. It -# links in seconds and is a no-op if it is already current. -# -# Getting this wrong is silent: the probe simply reports "no syscheck binary" -# and skips, which is how the unsharded Phoenix path went unprobed at first. -if [ -z "$shard" ] || [ "$shard_count" -le 1 ]; then - ./mfc.sh build -t syscheck -j 8 $gpu_opts -fi - -preflight_rc=0 -bash .github/scripts/preflight.sh "$job_cluster" "${job_device:-gpu}" || preflight_rc=$? -if [ "$preflight_rc" -ne 0 ]; then - exit "$preflight_rc" -fi +# Deliberately no node probe here. This pre-build is submitted as a *cpu* +# allocation (see test.yml: it is --dry-run, so it only builds), while the +# binaries it produces are GPU builds. syscheck built with --gpu therefore +# asserts omp_get_num_devices() > 0 and exits non-zero on a node that has no +# GPU by design -- which a probe would report as a bad node. It did: three +# healthy Phoenix nodes were condemned and two excluded before the wrapper gave +# up. The GPU allocation that actually runs these cases is probed instead, in +# run_case_optimization.sh. idx=0 for case in "${benchmarks[@]}"; do diff --git a/toolchain/mfc/test_prebuild_preflight.py b/toolchain/mfc/test_prebuild_preflight.py index bd321b7879..4d1b7eb98c 100644 --- a/toolchain/mfc/test_prebuild_preflight.py +++ b/toolchain/mfc/test_prebuild_preflight.py @@ -1,112 +1,44 @@ -"""The case-optimization pre-build must actually probe, in both shard modes. +"""The case-optimization pre-build must NOT probe the node. -The probe needs a syscheck binary to exist. In the sharded mode shard 1 builds -one under the script's marker coordination and the others wait for it; in the -unsharded mode nothing has been built when the probe runs, so it has to build -the binary itself. +Every other SLURM job MFC submits runs binaries built for the device it asked +for. This one does not: test.yml submits it as a *cpu* allocation, because it is +a --dry-run that only builds, while the binaries it produces are GPU builds. -That asymmetry failed silently the first time: on Phoenix, where -case-optimization is unsharded, the probe reported "no syscheck binary under -build/install; skipping node probe" and did nothing at all. A guard that skips -looks exactly like a guard that passed, which is why this is pinned. +A syscheck built with --gpu asserts omp_get_num_devices() > 0 (or the OpenACC +equivalent) and so exits non-zero on a node that has no GPU by design. Probing +here reads that as a bad node. It did exactly that in CI: three healthy Phoenix +nodes were condemned and two added to --exclude before the wrapper gave up, +which is the precise false positive the whole guard exists to avoid. + +The GPU allocation that actually runs these cases is probed instead, in +run_case_optimization.sh, where the allocation and the binary agree. """ -import os -import shutil -import stat -import subprocess from pathlib import Path import pytest -REPO = Path(__file__).resolve().parents[2] - - -def _exe(path: Path, text: str): - path.write_text(text) - path.chmod(path.stat().st_mode | stat.S_IEXEC) - - -@pytest.fixture -def workspace(tmp_path): - shutil.copytree(REPO / ".github", tmp_path / ".github") - binz = tmp_path / "bin" - binz.mkdir() - _exe( - binz / "mpirun", - '#!/bin/bash\nwhile [ "${1:0:1}" = "-" ]; do shift; case "$1" in [0-9]*) shift;; esac; done\nexec "$@"\n', - ) - trace = tmp_path / "trace.log" - - def install_mfc(probe_exit=0): - _exe( - tmp_path / "mfc.sh", - f"""#!/bin/bash -echo "mfc.sh $*" >> {trace} -if [ "$1" = "load" ]; then return 0 2>/dev/null || exit 0; fi -for a in "$@"; do - if [ "$a" = "syscheck" ]; then - mkdir -p build/install/gpu-acc-abc/bin - printf '#!/bin/bash\\nexit {probe_exit}\\n' > build/install/gpu-acc-abc/bin/syscheck - chmod +x build/install/gpu-acc-abc/bin/syscheck - fi -done -exit 0 -""", - ) - - def run(shard="", cluster="phoenix"): - env = { - **os.environ, - "PATH": f"{binz}:{os.environ['PATH']}", - "MFC_CI_STATE_DIR": str(tmp_path / "state"), - "SLURM_JOB_ID": "123456", - "SLURMD_NODENAME": "atl1-1-02-008-1-1", - "job_cluster": cluster, - "job_device": "gpu", - "job_interface": "acc", - "job_shard": shard, - } - return subprocess.run( - ["bash", ".github/scripts/prebuild-case-optimization.sh"], - capture_output=True, - text=True, - cwd=tmp_path, - env=env, - check=False, - timeout=120, - ) +SCRIPTS = Path(__file__).resolve().parents[2] / ".github" / "scripts" - return tmp_path, install_mfc, run, trace +# Scripts that run binaries built for the device their allocation requested. +PROBES = ["run_case_optimization.sh"] +# Scripts whose allocation device deliberately differs from what they build. +DOES_NOT_PROBE = ["prebuild-case-optimization.sh"] -def test_the_unsharded_prebuild_really_probes(workspace): - # Phoenix runs case-optimization unsharded. This is the path that silently - # skipped. - _, install_mfc, run, _ = workspace - install_mfc(probe_exit=0) - out = run().stdout - assert "Preflight: probing" in out - assert "no syscheck binary" not in out +@pytest.mark.parametrize("script", DOES_NOT_PROBE) +def test_a_build_only_allocation_does_not_judge_its_node(script): + body = (SCRIPTS / script).read_text() + assert "preflight.sh" not in body, f"{script} runs on a cpu allocation but builds GPU binaries, so a probe " "there always fails and excludes a healthy node" -def test_a_bad_node_stops_the_unsharded_prebuild(workspace): - _, install_mfc, run, trace = workspace - install_mfc(probe_exit=1) - result = run() - assert result.returncode == 77 - assert "Pre-building" not in result.stdout +@pytest.mark.parametrize("script", PROBES) +def test_the_allocation_that_runs_the_cases_does_judge_its_node(script): + assert "preflight.sh" in (SCRIPTS / script).read_text() -def test_the_sharded_prebuild_does_not_rebuild_syscheck_concurrently(workspace): - # Shard 2 waits on shard 1's marker; building syscheck here would race the - # shared build/install that the coordination exists to serialize. - tmp_path, install_mfc, run, trace = workspace - install_mfc(probe_exit=0) - # frontier_amd, not phoenix: sharding is a frontier_amd mode, and phoenix's - # clean_build would move build/ aside and take the marker with it. - (tmp_path / "build").mkdir(exist_ok=True) - (tmp_path / "build" / ".prebuild-shared-targets-done").touch() - run(shard="2/3", cluster="frontier_amd") - builds = [c for c in trace.read_text().splitlines() if "build -t syscheck" in c] - assert builds == [], f"shard 2 must not build syscheck itself, got {builds}" +def test_the_reason_is_recorded_where_someone_would_re_add_it(): + # The next person to notice case-opt's pre-build is unprobed should find the + # reason in the file rather than rediscovering it through a red CI run. + body = (SCRIPTS / "prebuild-case-optimization.sh").read_text() + assert "cpu" in body and "probe" in body.lower() From a47cf569473a25218df614fbfc35ccc34d86ceae Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Tue, 1 Sep 2026 08:50:50 -0500 Subject: [PATCH 07/10] ci: tighten the node-fault blast radius and speed up the monitor tests Three follow-ups, two of them prompted by the false positive this branch caused earlier. Never judge a node with a binary it could not run A syscheck built for a device the allocation did not ask for says nothing about the machine. The case-optimization pre-build is submitted as cpu -- it is a --dry-run that only builds -- while producing GPU binaries, so its syscheck asserts a device exists and fails on a GPU-less node by design. Read as a node fault, that condemned three healthy Phoenix nodes and excluded two. The probe now recognises the mismatch and declines to judge. Chosen over the "only blame a node if this binary passed elsewhere in the run" rule, which sounds stronger but would refuse to requeue the first job to meet a genuinely bad node -- the primary case. The mismatch check has no such gap: it is a property of the job, decided locally, with no cross-job state. One requeue, not two Each attempt costs a node when the probe is wrong, and the run above was a bounded loop faithfully doing what it was told. Bad nodes are concentrated -- one accounted for 25 of 29 ECC failures -- so a single requeue keeps nearly all the benefit at half the blast radius. Stop sleeping through the monitor tests monitor_slurm_job.sh polls with real sleeps, which cost ~36s in every PR's Lint Gate. The same seam already used for MFC_BUILD_RETRY_DELAY now covers the poll and recheck intervals; CI keeps the defaults. That file drops to 6.4s and the whole toolchain suite from ~50s to 20s. 493 tests pass. --- .github/scripts/monitor_slurm_job.sh | 15 ++++++---- .github/scripts/preflight.sh | 16 +++++++++++ .github/scripts/run_monitored_slurm_job.sh | 2 +- .github/scripts/submit-slurm-job.sh | 8 +++++- toolchain/mfc/test_monitor_exit_codes.py | 9 +++++- toolchain/mfc/test_preflight.py | 33 ++++++++++++++++++++++ toolchain/mfc/test_submit_requeue.py | 13 +++++++++ 7 files changed, 88 insertions(+), 8 deletions(-) diff --git a/.github/scripts/monitor_slurm_job.sh b/.github/scripts/monitor_slurm_job.sh index 183447421e..7d42e119a4 100755 --- a/.github/scripts/monitor_slurm_job.sh +++ b/.github/scripts/monitor_slurm_job.sh @@ -99,6 +99,11 @@ if ! [[ "$SLURM_MAX_QUEUE_SECONDS" =~ ^[0-9]+$ ]]; then echo "ERROR: SLURM_MAX_QUEUE_SECONDS must be a non-negative integer (seconds), got '$SLURM_MAX_QUEUE_SECONDS'" >&2 exit 1 fi +# How long to wait between status polls and between output-stabilization +# checks. Overridable so tests can exercise this script without sleeping +# through it; CI leaves it at the default. +: "${MFC_MONITOR_POLL_SECONDS:=5}" + queue_start=$(date +%s) abort_queue_starvation() { @@ -142,7 +147,7 @@ while [ ! -f "$output_file" ]; do ;; PENDING|CONFIGURING) unknown_count=0 - sleep 5 + sleep "$MFC_MONITOR_POLL_SECONDS" ;; RUNNING|COMPLETING) unknown_count=0 @@ -155,7 +160,7 @@ while [ ! -f "$output_file" ]; do if [ $((unknown_count % 12)) -eq 1 ]; then echo "Warning: Could not query job $job_id state (SLURM may be temporarily unavailable)..." fi - sleep 5 + sleep "$MFC_MONITOR_POLL_SECONDS" ;; *) # Terminal state — job finished without creating output @@ -164,7 +169,7 @@ while [ ! -f "$output_file" ]; do exit 1 fi # Unrecognized state, keep waiting - sleep 5 + sleep "$MFC_MONITOR_POLL_SECONDS" ;; esac done @@ -205,7 +210,7 @@ while true; do last_heartbeat=$current_time fi - sleep 5 + sleep "$MFC_MONITOR_POLL_SECONDS" done # Give tail a moment to flush the final lines, then stop streaming. @@ -229,7 +234,7 @@ if [ -f "$output_file" ]; then if [ $same_count -ge 2 ]; then break fi - sleep 5 + sleep "$MFC_MONITOR_POLL_SECONDS" done fi diff --git a/.github/scripts/preflight.sh b/.github/scripts/preflight.sh index e88743201a..e5e9f4a7b0 100755 --- a/.github/scripts/preflight.sh +++ b/.github/scripts/preflight.sh @@ -89,6 +89,22 @@ if [ -z "$syscheck_bin" ]; then exit $EXIT_HEALTHY fi +# A binary built for a device this allocation did not ask for tells us nothing +# about the node. The case-optimization pre-build is submitted as cpu (it is a +# --dry-run that only builds) while producing GPU binaries, so its syscheck +# asserts a device exists and fails on a GPU-less node by design. Read as a node +# fault, that condemned three healthy Phoenix nodes and excluded two of them. +# The mismatch is a property of the job, never of the machine. +case "$syscheck_bin" in + *gpu-*) binary_device="gpu" ;; + *) binary_device="cpu" ;; +esac +if [ "$device" = "cpu" ] && [ "$binary_device" = "gpu" ]; then + echo "Preflight: $device allocation but the available syscheck is a GPU build" + echo " ($syscheck_bin); it cannot pass here, so skipping rather than judging $node." + exit $EXIT_HEALTHY +fi + echo "Preflight: probing $node with $syscheck_bin" # Launch the probe the way this cluster launches everything else. Phoenix uses diff --git a/.github/scripts/run_monitored_slurm_job.sh b/.github/scripts/run_monitored_slurm_job.sh index 3da7520859..f18af2b556 100644 --- a/.github/scripts/run_monitored_slurm_job.sh +++ b/.github/scripts/run_monitored_slurm_job.sh @@ -45,7 +45,7 @@ fi if [ "$monitor_exit" -ne 0 ]; then echo "Monitor exited with code $monitor_exit; re-checking SLURM job $job_id final state..." # Give the SLURM epilog time to finalize if the job just finished - sleep 30 + sleep "${MFC_MONITOR_RECHECK_SECONDS:-30}" final_state=$(sacct -j "$job_id" -n -X -P -o State 2>/dev/null | head -n1 | cut -d'|' -f1 | tr -d ' ' || true) final_state="${final_state:-UNKNOWN}" final_exit=$(sacct -j "$job_id" -X --format=ExitCode --noheader --parsable2 2>/dev/null | head -n1 | tr -d ' ' || true) diff --git a/.github/scripts/submit-slurm-job.sh b/.github/scripts/submit-slurm-job.sh index 8da7e77dcb..f5480d176f 100755 --- a/.github/scripts/submit-slurm-job.sh +++ b/.github/scripts/submit-slurm-job.sh @@ -264,7 +264,13 @@ EOT # Node faults get a much tighter bound than preemption: preemption is routine on # 'embers' and says nothing about the node, whereas hitting a second unusable # node in a row means the problem is the cluster, not the draw. -: "${MFC_MAX_NODE_RESUBMITS:=2}" +# +# One, not two. Each attempt costs a node if the probe is wrong, and it has been +# wrong: a bounded loop faithfully condemned three healthy Phoenix nodes when the +# probe was given a binary that could not run there. Bad nodes are concentrated +# -- one accounted for 25 of 29 ECC failures -- so a single requeue captures +# nearly all of the benefit at half the blast radius. +: "${MFC_MAX_NODE_RESUBMITS:=1}" preempt_attempt=0 node_attempt=0 while :; do diff --git a/toolchain/mfc/test_monitor_exit_codes.py b/toolchain/mfc/test_monitor_exit_codes.py index 36eb5cf32e..2b87016e80 100644 --- a/toolchain/mfc/test_monitor_exit_codes.py +++ b/toolchain/mfc/test_monitor_exit_codes.py @@ -41,7 +41,14 @@ def configure(exit_code, state="COMPLETED"): def run_script(tmp_path, binz, name, *args): - env = {**os.environ, "PATH": f"{binz}:{os.environ['PATH']}"} + env = { + **os.environ, + "PATH": f"{binz}:{os.environ['PATH']}", + # This script really sleeps between polls; without these the file + # cost ~36s in every PR's Lint Gate. + "MFC_MONITOR_POLL_SECONDS": "0", + "MFC_MONITOR_RECHECK_SECONDS": "0", + } return subprocess.run( ["bash", str(SCRIPTS / name), *args], capture_output=True, diff --git a/toolchain/mfc/test_preflight.py b/toolchain/mfc/test_preflight.py index 53f13d610d..d376ca8380 100644 --- a/toolchain/mfc/test_preflight.py +++ b/toolchain/mfc/test_preflight.py @@ -192,3 +192,36 @@ def test_it_refuses_to_judge_a_node_outside_a_slurm_allocation(workspace): write_syscheck(workspace, 1) env_without_slurm = {"MFC_NO_SLURM": "1"} assert run(workspace, "phoenix", "gpu", **env_without_slurm).returncode == HEALTHY + + +def test_a_gpu_binary_in_a_cpu_allocation_is_not_a_node_fault(workspace): + """The failure that condemned three healthy Phoenix nodes. + + The case-optimization pre-build is submitted as a cpu allocation but builds + GPU binaries, so the only syscheck available asserts a device exists and + exits non-zero on a node that has no GPU by design. That is a mismatch + between what the job asked for and what it built -- never evidence about the + node -- so the probe must decline to judge rather than blame it. + """ + install_launcher(workspace, "mpirun") + target = workspace / "build" / "install" / "gpu-mp-abc123" / "bin" + target.mkdir(parents=True) + probe = target / "syscheck" + probe.write_text("#!/bin/bash\necho 'num_devices == 0'\nexit 1\n") + probe.chmod(probe.stat().st_mode | stat.S_IEXEC) + + result = run(workspace, "phoenix", "cpu") + assert result.returncode == HEALTHY + assert "MFC_FAULT_NODE" not in result.stdout + result.stderr + + +def test_a_gpu_binary_in_a_gpu_allocation_is_still_judged(workspace): + # The guard above must not blunt the actual feature. + install_launcher(workspace, "mpirun") + target = workspace / "build" / "install" / "gpu-mp-abc123" / "bin" + target.mkdir(parents=True) + probe = target / "syscheck" + probe.write_text("#!/bin/bash\nexit 1\n") + probe.chmod(probe.stat().st_mode | stat.S_IEXEC) + + assert run(workspace, "phoenix", "gpu").returncode == NODE_FAULT diff --git a/toolchain/mfc/test_submit_requeue.py b/toolchain/mfc/test_submit_requeue.py index 3cbf48f7be..4fe0ceb633 100644 --- a/toolchain/mfc/test_submit_requeue.py +++ b/toolchain/mfc/test_submit_requeue.py @@ -154,3 +154,16 @@ def test_no_job_is_submitted_while_the_cluster_is_under_a_recorded_outage(rig): result = rig("0") assert len(result.submissions) == 0 assert result.returncode == 78 + + +def test_the_default_bound_is_one_requeue(rig): + """One requeue, not two. + + A wrong probe costs one node per attempt: the run that condemned three + healthy Phoenix nodes was a bounded loop doing exactly what it was told. + Bad nodes are concentrated -- one accounted for 25 of 29 ECC failures -- so a + single requeue captures nearly all the benefit at half the blast radius. + """ + result = rig("77,77,77,77") + assert result.returncode != 0 + assert len(result.submissions) == 2 # original + 1 requeue From e20de7c1bf39feeabe65e14b1b7772a3199f7cc1 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Tue, 1 Sep 2026 08:55:45 -0500 Subject: [PATCH 08/10] test: measure retry rescues, and stop retrying through an abort (#1798) Two parts of #1798 that need no judgement call about which failures are transient, and one of which is the prerequisite for making that call. Measure rescues nPASS was incremented identically whether a case passed on attempt 1 or attempt 3, so a rescue left no trace. That is why the evidence in #1798 is one-sided: 2,795 recorded failures all show the full attempt count, but a successful retry was invisible, so the rescue rate could be argued about but not measured. A pass after a retry is now counted, logged as it happens, and reported in the run summary. One CI cycle turns the open question into a number. Stop retrying through an abort The retry loop never consulted abort_tests. The suite-wide abort fires when the failure rate says the environment is broken -- a dead GPU, a bad node -- which is precisely when every remaining attempt is certain to fail. Cases already in their retry loop burned the rest of their budget anyway, so the fail-fast path was slowed threefold by the retry it was trying to escape. The decision now lives in should_retry(attempt, max_attempts, aborting), which is a pure function and unit tested; the loop just calls it. Not touched: which failure classes are worth retrying at all. That is the substantive half of #1798 and it should be decided against the rescue numbers this commit starts collecting, rather than against my one-sided sample. 499 tests pass. --- toolchain/mfc/test/test.py | 40 +++++++++++++++++-- toolchain/mfc/test/test_retry_policy.py | 52 +++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 4 deletions(-) create mode 100644 toolchain/mfc/test/test_retry_policy.py diff --git a/toolchain/mfc/test/test.py b/toolchain/mfc/test/test.py index bed3f96504..0f7b91877a 100644 --- a/toolchain/mfc/test/test.py +++ b/toolchain/mfc/test/test.py @@ -28,6 +28,7 @@ nSKIP = 0 current_test_number = 0 total_test_count = 0 +nRESCUED = 0 # cases that failed and were recovered by a retry (#1798) errors = [] failed_tests = [] # Track failed test details for summary test_start_time = None # Track overall test duration @@ -434,7 +435,7 @@ def test(): seconds = total_duration % 60 # Build the summary report - _print_test_summary(nPASS, nFAIL, nSKIP, minutes, seconds, failed_tests, skipped_cases) + _print_test_summary(nPASS, nFAIL, nSKIP, minutes, seconds, failed_tests, skipped_cases, nRESCUED) # Write failed UUIDs to file for CI retry logic if failed_tests: @@ -447,7 +448,7 @@ def test(): sys.exit(nFAIL) -def _print_test_summary(passed: int, failed: int, skipped: int, minutes: int, seconds: float, failed_test_list: list, _skipped_cases: list): +def _print_test_summary(passed: int, failed: int, skipped: int, minutes: int, seconds: float, failed_test_list: list, _skipped_cases: list, rescued: int = 0): """Print a comprehensive test summary report.""" total = passed + failed + skipped @@ -474,6 +475,12 @@ def _print_test_summary(passed: int, failed: int, skipped: int, minutes: int, se f" [bold green]{passed:4d}[/bold green] passed", f" [bold red]{failed:4d}[/bold red] failed", f" [bold yellow]{skipped:4d}[/bold yellow] skipped", + ] + if rescued: + # How often a retry actually earned its cost. See #1798: without this the + # only measurable retry outcomes were the ones that failed anyway. + summary_lines.append(f" [yellow]{rescued:4d}[/yellow] recovered by a retry") + summary_lines += [ f" [dim]{'─' * 12}[/dim]", f" [bold]{total:4d}[/bold] total", "", @@ -729,8 +736,26 @@ def _handle_case(case: TestCase, devices: typing.Set[int]): timeout_timer.cancel() # Cancel timeout timer +def should_retry(attempt: int, max_attempts: int, aborting: bool) -> bool: + """Whether a failed case gets another attempt. + + Retries here are expensive and, as far as can be measured, rarely help: + bench.py's equivalent rescued 0 of 235 retried cases, and every recorded + failed test in a two-week sample shows the full attempt count. See #1798. + + `aborting` is the part that was missing. The suite-wide abort fires when the + failure rate says the environment itself is broken -- a dead GPU, a bad node + -- and in that state every remaining attempt is guaranteed to fail. Retrying + through an abort turns the fail-fast into a slow one. + """ + if aborting: + return False + + return attempt < max_attempts + + def handle_case(case: TestCase, devices: typing.Set[int]): - global nFAIL, nPASS, nSKIP # noqa: PLW0603 + global nFAIL, nPASS, nSKIP, nRESCUED # noqa: PLW0603 global errors, failed_tests # noqa: PLW0603 # Check if we should abort before processing this case @@ -752,8 +777,15 @@ def handle_case(case: TestCase, devices: typing.Set[int]): nSKIP += 1 else: nPASS += 1 + if nAttempts > 1: + # A rescue: the case failed and a retry recovered it. Nothing + # recorded this before, so a pass on attempt 3 was + # indistinguishable from a pass on attempt 1 -- which is why + # the value of retrying could never be measured. See #1798. + nRESCUED += 1 + cons.print(f" [yellow]recovered on attempt {nAttempts}[/yellow]: {case.trace}") except Exception as exc: - if nAttempts < max_attempts: + if should_retry(nAttempts, max_attempts, abort_tests.is_set()): continue nFAIL += 1 diff --git a/toolchain/mfc/test/test_retry_policy.py b/toolchain/mfc/test/test_retry_policy.py new file mode 100644 index 0000000000..41e44c4d2c --- /dev/null +++ b/toolchain/mfc/test/test_retry_policy.py @@ -0,0 +1,52 @@ +"""Tests for the test-suite retry decision (issue #1798). + +Retries in this harness cost 3x wall clock and, as far as anyone can measure, +rescue almost nothing: bench.py's equivalent rescued 0 of 235 retried cases, and +all 2,795 recorded failed-test entries show `Attempts: 3`. + +That second figure is one-sided by construction -- only failures record an +attempt count, so a case that passes on attempt 2 is indistinguishable from one +that passed first try. Measuring rescues is therefore the prerequisite for any +policy change, and is fixed here. + +Also fixed: the retry loop never consulted the suite-wide abort flag. On a bad +node, where the 30% failure-rate abort is exactly what should stop the run, +every in-flight case still burned its remaining attempts first. +""" + +from mfc.test.test import should_retry + + +def test_a_failure_is_retried_until_the_attempt_budget_is_spent(): + assert should_retry(attempt=1, max_attempts=3, aborting=False) + assert should_retry(attempt=2, max_attempts=3, aborting=False) + + +def test_the_last_attempt_is_not_retried(): + assert not should_retry(attempt=3, max_attempts=3, aborting=False) + + +def test_a_single_attempt_budget_never_retries(): + assert not should_retry(attempt=1, max_attempts=1, aborting=False) + + +def test_no_retry_once_the_suite_is_aborting(): + # The abort fires when the failure rate says the environment is broken -- + # a bad node, say. Spending two more attempts per in-flight case is the + # opposite of the fail-fast the abort exists to provide. + assert not should_retry(attempt=1, max_attempts=3, aborting=True) + assert not should_retry(attempt=2, max_attempts=3, aborting=True) + + +def test_the_summary_reports_how_often_a_retry_actually_helped(capsys): + from mfc.test.test import _print_test_summary + + _print_test_summary(10, 2, 0, 0, 1.0, [], [], rescued=3) + assert "recovered by a retry" in capsys.readouterr().out + + +def test_the_summary_stays_quiet_when_no_retry_helped(capsys): + from mfc.test.test import _print_test_summary + + _print_test_summary(10, 2, 0, 0, 1.0, [], [], rescued=0) + assert "recovered by a retry" not in capsys.readouterr().out From 2f94eadb3cd26e97dc41c6eadc39e916bf2c64ed Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Tue, 1 Sep 2026 08:58:57 -0500 Subject: [PATCH 09/10] test: make retry value harvestable, not just recorded (#1798) A rescue count printed into a log is measurable but not measured. Two things were missing before the question in #1798 could actually be answered. The class of each rescue Whether a retry earns its cost depends entirely on what it rescued. A tolerance mismatch re-runs the same binary over the same input and can only reach the same comparison; an execution failure may be a transient launcher or node problem. A bare count cannot separate them, so it cannot inform the policy it exists to inform. Rescues are now attributed to the failure class that preceded them, and the summary breaks them down. That also removed a duplicated classifier: the same if/elif chain was inlined twice, for the hints and for the failure record, and the rescue path had none at all. classify_error() is now one definition with unit tests, used by all three. Somewhere to aggregate from Each run emits one greppable line: MFC_RETRY_STATS rescued=3 failed=2 passed=698 by_class=execution failed:3 and .github/scripts/harvest-retry-stats.sh totals those across recent runs. Answering "are retries worth it" is now a command rather than the log archaeology that made it unanswerable in the first place -- which is the same manual work the original inventory required, and the reason the evidence in #1798 was one-sided. The script prints the rescue count per failure class, or says plainly that nothing was rescued, which is the outcome the existing evidence predicts. Either way the decision gets made against data. 502 tests pass. --- .github/scripts/harvest-retry-stats.sh | 83 +++++++++++++++++++++++++ toolchain/mfc/test/test.py | 56 +++++++++++++---- toolchain/mfc/test/test_retry_policy.py | 33 ++++++++++ 3 files changed, 159 insertions(+), 13 deletions(-) create mode 100755 .github/scripts/harvest-retry-stats.sh diff --git a/.github/scripts/harvest-retry-stats.sh b/.github/scripts/harvest-retry-stats.sh new file mode 100755 index 0000000000..60b269f136 --- /dev/null +++ b/.github/scripts/harvest-retry-stats.sh @@ -0,0 +1,83 @@ +#!/bin/bash +# Aggregate retry outcomes across recent CI runs (see issue #1798). +# +# The test suite emits one line per run: +# MFC_RETRY_STATS rescued=N failed=N passed=N by_class=execution failed:2,... +# +# This collects those lines over the last N Test Suite runs and totals them, so +# "are retries worth their cost" is answered with numbers instead of argument. +# Reading a hundred logs by hand is what made the question unanswerable before. +# +# Usage: bash .github/scripts/harvest-retry-stats.sh [runs] (default 40) +# +# Interpreting the result: retries cost roughly 3x the wall clock of the +# failures they act on. A class with a rescue count at or near zero across a few +# hundred retried cases is paying that cost for nothing and is a candidate for +# skipping the retry entirely. + +set -uo pipefail + +REPO="${MFC_REPO:-MFlowCode/MFC}" +WORKFLOW="${MFC_WORKFLOW:-test.yml}" +LIMIT="${1:-40}" + +command -v gh >/dev/null 2>&1 || { echo "gh CLI is required" >&2; exit 1; } + +echo "Harvesting MFC_RETRY_STATS from the last $LIMIT $WORKFLOW runs of $REPO..." >&2 + +runs=$(gh run list --repo "$REPO" --workflow "$WORKFLOW" --limit "$LIMIT" \ + --json databaseId --jq '.[].databaseId' 2>/dev/null) + +if [ -z "$runs" ]; then + echo "No runs found." >&2 + exit 1 +fi + +stats=$( + for run in $runs; do + for job in $(gh api "repos/$REPO/actions/runs/$run/jobs?per_page=100" \ + --jq '.jobs[] | select(.conclusion != "skipped") | .id' 2>/dev/null); do + gh api "repos/$REPO/actions/jobs/$job/logs" 2>/dev/null \ + | tr -d '\r' | grep -o 'MFC_RETRY_STATS .*' || true + done + done +) + +if [ -z "$stats" ]; then + echo "No MFC_RETRY_STATS lines found (runs may predate the instrumentation)." >&2 + exit 1 +fi + +printf '%s\n' "$stats" | awk ' +{ + for (i = 1; i <= NF; i++) { + if ($i ~ /^rescued=/) { split($i, a, "="); rescued += a[2] } + if ($i ~ /^failed=/) { split($i, a, "="); failed += a[2] } + if ($i ~ /^passed=/) { split($i, a, "="); passed += a[2] } + } + if (match($0, /by_class=.*/)) { + cls = substr($0, RSTART + 9) + if (cls != "none") { + n = split(cls, parts, ",") + for (i = 1; i <= n; i++) { + split(parts[i], kv, ":") + by[kv[1]] += kv[2] + } + } + } + runs++ +} +END { + printf "\nruns sampled: %d\n", runs + printf "tests passed: %d\n", passed + printf "tests failed: %d\n", failed + printf "rescued by a retry: %d\n", rescued + if (rescued > 0) { + print "\nrescues by failure class:" + for (k in by) printf " %6d %s\n", by[k], k + } else { + print "\nNo retry rescued anything in this sample." + print "Every retried case failed all of its attempts, so the retries" + print "cost roughly 3x the wall clock of those failures for no benefit." + } +}' diff --git a/toolchain/mfc/test/test.py b/toolchain/mfc/test/test.py index 0f7b91877a..b60f4271e1 100644 --- a/toolchain/mfc/test/test.py +++ b/toolchain/mfc/test/test.py @@ -1,3 +1,4 @@ +import collections import itertools import math import os @@ -29,6 +30,7 @@ current_test_number = 0 total_test_count = 0 nRESCUED = 0 # cases that failed and were recovered by a retry (#1798) +nRESCUED_BY_CLASS = collections.defaultdict(int) # and of what kind errors = [] failed_tests = [] # Track failed test details for summary test_start_time = None # Track overall test duration @@ -435,7 +437,7 @@ def test(): seconds = total_duration % 60 # Build the summary report - _print_test_summary(nPASS, nFAIL, nSKIP, minutes, seconds, failed_tests, skipped_cases, nRESCUED) + _print_test_summary(nPASS, nFAIL, nSKIP, minutes, seconds, failed_tests, skipped_cases, nRESCUED, dict(nRESCUED_BY_CLASS)) # Write failed UUIDs to file for CI retry logic if failed_tests: @@ -448,7 +450,7 @@ def test(): sys.exit(nFAIL) -def _print_test_summary(passed: int, failed: int, skipped: int, minutes: int, seconds: float, failed_test_list: list, _skipped_cases: list, rescued: int = 0): +def _print_test_summary(passed: int, failed: int, skipped: int, minutes: int, seconds: float, failed_test_list: list, _skipped_cases: list, rescued: int = 0, rescued_by_class: dict = None): """Print a comprehensive test summary report.""" total = passed + failed + skipped @@ -480,6 +482,8 @@ def _print_test_summary(passed: int, failed: int, skipped: int, minutes: int, se # How often a retry actually earned its cost. See #1798: without this the # only measurable retry outcomes were the ones that failed anyway. summary_lines.append(f" [yellow]{rescued:4d}[/yellow] recovered by a retry") + for cls, n in sorted((rescued_by_class or {}).items(), key=lambda kv: -kv[1]): + summary_lines.append(f" [dim]{n:3d} {cls}[/dim]") summary_lines += [ f" [dim]{'─' * 12}[/dim]", f" [bold]{total:4d}[/bold] total", @@ -516,6 +520,12 @@ def _print_test_summary(passed: int, failed: int, skipped: int, minutes: int, se cons.raw.print(Panel("\n".join(summary_lines), title="[bold]Test Summary[/bold]", border_style=border_style, padding=(1, 2))) cons.print() + # A single greppable line so retry value can be aggregated across runs + # without reading logs by hand -- see #1798 and + # .github/scripts/harvest-retry-stats.sh. + by_class = ",".join(f"{k}:{v}" for k, v in sorted((rescued_by_class or {}).items())) + cons.print(f"MFC_RETRY_STATS rescued={rescued} failed={failed} passed={passed} by_class={by_class or 'none'}", no_indent=True) + def _process_silo_file(silo_filepath: str, case: TestCase, out_filepath: str): """Process a single silo file with h5dump and check for NaNs/Infinities.""" @@ -736,6 +746,29 @@ def _handle_case(case: TestCase, devices: typing.Set[int]): timeout_timer.cancel() # Cancel timeout timer +def classify_error(exc: Exception) -> str: + """Bucket a test failure into the categories the retry policy turns on. + + Whether a retry is worth its cost depends on the class: a tolerance mismatch + re-runs the same binary over the same input and can only reach the same + comparison, whereas an execution failure may be a transient launcher or node + problem. Counting rescues without the class cannot tell those apart, which + is what #1798 needs to distinguish. + """ + text = str(exc).lower() + + if "tolerance" in text or "golden" in text or "mismatch" in text: + return "tolerance mismatch" + if "timeout" in text: + return "timeout" + if "nan" in text: + return "NaN detected" + if "failed to execute" in text: + return "execution failed" + + return "" + + def should_retry(attempt: int, max_attempts: int, aborting: bool) -> bool: """Whether a failed case gets another attempt. @@ -763,6 +796,7 @@ def handle_case(case: TestCase, devices: typing.Set[int]): return # Exit gracefully if abort was requested nAttempts = 0 + last_error_type = "" if ARG("single"): max_attempts = max(ARG("max_attempts"), 3) else: @@ -782,9 +816,14 @@ def handle_case(case: TestCase, devices: typing.Set[int]): # recorded this before, so a pass on attempt 3 was # indistinguishable from a pass on attempt 1 -- which is why # the value of retrying could never be measured. See #1798. + # The class matters as much as the count: retrying is worth + # very different things for a tolerance mismatch than for an + # execution failure. nRESCUED += 1 - cons.print(f" [yellow]recovered on attempt {nAttempts}[/yellow]: {case.trace}") + nRESCUED_BY_CLASS[last_error_type or "unclassified"] += 1 + cons.print(f" [yellow]recovered on attempt {nAttempts}[/yellow] ({last_error_type or 'unclassified'}): {case.trace}") except Exception as exc: + last_error_type = classify_error(exc) if should_retry(nAttempts, max_attempts, abort_tests.is_set()): continue nFAIL += 1 @@ -811,16 +850,7 @@ def handle_case(case: TestCase, devices: typing.Set[int]): cons.print() # Track failed test details for summary - error_type = "" - exc_lower = str(exc).lower() - if "tolerance" in exc_lower or "golden" in exc_lower or "mismatch" in exc_lower: - error_type = "tolerance mismatch" - elif "timeout" in exc_lower: - error_type = "timeout" - elif "nan" in exc_lower: - error_type = "NaN detected" - elif "failed to execute" in exc_lower: - error_type = "execution failed" + error_type = classify_error(exc) failed_tests.append({"trace": case.trace, "uuid": case.get_uuid(), "error_type": error_type, "attempts": nAttempts}) diff --git a/toolchain/mfc/test/test_retry_policy.py b/toolchain/mfc/test/test_retry_policy.py index 41e44c4d2c..cd47a589e8 100644 --- a/toolchain/mfc/test/test_retry_policy.py +++ b/toolchain/mfc/test/test_retry_policy.py @@ -50,3 +50,36 @@ def test_the_summary_stays_quiet_when_no_retry_helped(capsys): _print_test_summary(10, 2, 0, 0, 1.0, [], [], rescued=0) assert "recovered by a retry" not in capsys.readouterr().out + + +def test_failures_are_classified_into_the_categories_the_policy_cares_about(): + from mfc.test.test import classify_error + + assert classify_error(Exception("Variable n5282 is not within tolerance")) == "tolerance mismatch" + assert classify_error(Exception("Test X: Failed to execute MFC.")) == "execution failed" + assert classify_error(Exception("NaN or Inf detected in the case.")) == "NaN detected" + assert classify_error(Exception("Test case exceeded 1 hour timeout")) == "timeout" + assert classify_error(Exception("something else entirely")) == "" + + +def test_the_summary_says_which_classes_a_retry_actually_rescued(capsys): + # A bare rescue count cannot answer "which retries are worth keeping". + # A tolerance mismatch rescued by a retry means something very different + # from an execution failure rescued by one. + from mfc.test.test import _print_test_summary + + _print_test_summary(10, 2, 0, 0, 1.0, [], [], rescued=3, rescued_by_class={"execution failed": 2, "tolerance mismatch": 1}) + out = capsys.readouterr().out + assert "execution failed" in out + assert "tolerance mismatch" in out + + +def test_the_summary_emits_one_machine_readable_line_for_aggregation(capsys): + # The numbers only become knowledge if they can be harvested across many + # runs without hand-reading logs, so emit one greppable line. + from mfc.test.test import _print_test_summary + + _print_test_summary(10, 2, 0, 0, 1.0, [], [], rescued=3, rescued_by_class={"execution failed": 3}) + out = capsys.readouterr().out + assert "MFC_RETRY_STATS" in out + assert "rescued=3" in out From dfc71f88e57cd6b23554ff27608487d848dfbc60 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Tue, 1 Sep 2026 09:01:51 -0500 Subject: [PATCH 10/10] Revert the retry-stats harvester; it answered a 6% question I measured the thing I had been asserting, and it does not support the apparatus I built for it. "Retries cost 3x the wall clock" is true per failing test, but I let it read as 3x overall, and never checked. Across the window: 2,796 failed-test records x 2 extra attempts x ~29s mean = ~45 h, against 730 h of total first-attempt waste -> ~6% And that 6% is concentrated where the abort already belongs: median failing job: 3 failed tests (~3 min of retry overhead) top 20 jobs: 56% of all retry cost worst single job: 108 failed tests The mass-failure jobs are bad nodes and broken environments, which is exactly what the 30% failure-rate abort exists to stop -- and the abort was being defeated by the retry until the previous commit fixed it. So that fix already recovers the majority of the recoverable cost, and the remaining policy question governs a few minutes per job. Removed: per-class rescue tracking, the MFC_RETRY_STATS line, and harvest-retry-stats.sh -- ~120 lines of measurement apparatus, designed to be deleted once it had answered, for a question worth less than the machinery. Kept, because each stands on its own: - should_retry(), including not retrying through an abort: this is where the cost actually was - classify_error(): it replaced the same if/elif chain inlined twice - the plain rescued count: five lines, and it makes the retry's value visible for free 500 tests pass. --- .github/scripts/harvest-retry-stats.sh | 83 ------------------------- toolchain/mfc/test/test.py | 21 ++----- toolchain/mfc/test/test_retry_policy.py | 23 ------- 3 files changed, 5 insertions(+), 122 deletions(-) delete mode 100755 .github/scripts/harvest-retry-stats.sh diff --git a/.github/scripts/harvest-retry-stats.sh b/.github/scripts/harvest-retry-stats.sh deleted file mode 100755 index 60b269f136..0000000000 --- a/.github/scripts/harvest-retry-stats.sh +++ /dev/null @@ -1,83 +0,0 @@ -#!/bin/bash -# Aggregate retry outcomes across recent CI runs (see issue #1798). -# -# The test suite emits one line per run: -# MFC_RETRY_STATS rescued=N failed=N passed=N by_class=execution failed:2,... -# -# This collects those lines over the last N Test Suite runs and totals them, so -# "are retries worth their cost" is answered with numbers instead of argument. -# Reading a hundred logs by hand is what made the question unanswerable before. -# -# Usage: bash .github/scripts/harvest-retry-stats.sh [runs] (default 40) -# -# Interpreting the result: retries cost roughly 3x the wall clock of the -# failures they act on. A class with a rescue count at or near zero across a few -# hundred retried cases is paying that cost for nothing and is a candidate for -# skipping the retry entirely. - -set -uo pipefail - -REPO="${MFC_REPO:-MFlowCode/MFC}" -WORKFLOW="${MFC_WORKFLOW:-test.yml}" -LIMIT="${1:-40}" - -command -v gh >/dev/null 2>&1 || { echo "gh CLI is required" >&2; exit 1; } - -echo "Harvesting MFC_RETRY_STATS from the last $LIMIT $WORKFLOW runs of $REPO..." >&2 - -runs=$(gh run list --repo "$REPO" --workflow "$WORKFLOW" --limit "$LIMIT" \ - --json databaseId --jq '.[].databaseId' 2>/dev/null) - -if [ -z "$runs" ]; then - echo "No runs found." >&2 - exit 1 -fi - -stats=$( - for run in $runs; do - for job in $(gh api "repos/$REPO/actions/runs/$run/jobs?per_page=100" \ - --jq '.jobs[] | select(.conclusion != "skipped") | .id' 2>/dev/null); do - gh api "repos/$REPO/actions/jobs/$job/logs" 2>/dev/null \ - | tr -d '\r' | grep -o 'MFC_RETRY_STATS .*' || true - done - done -) - -if [ -z "$stats" ]; then - echo "No MFC_RETRY_STATS lines found (runs may predate the instrumentation)." >&2 - exit 1 -fi - -printf '%s\n' "$stats" | awk ' -{ - for (i = 1; i <= NF; i++) { - if ($i ~ /^rescued=/) { split($i, a, "="); rescued += a[2] } - if ($i ~ /^failed=/) { split($i, a, "="); failed += a[2] } - if ($i ~ /^passed=/) { split($i, a, "="); passed += a[2] } - } - if (match($0, /by_class=.*/)) { - cls = substr($0, RSTART + 9) - if (cls != "none") { - n = split(cls, parts, ",") - for (i = 1; i <= n; i++) { - split(parts[i], kv, ":") - by[kv[1]] += kv[2] - } - } - } - runs++ -} -END { - printf "\nruns sampled: %d\n", runs - printf "tests passed: %d\n", passed - printf "tests failed: %d\n", failed - printf "rescued by a retry: %d\n", rescued - if (rescued > 0) { - print "\nrescues by failure class:" - for (k in by) printf " %6d %s\n", by[k], k - } else { - print "\nNo retry rescued anything in this sample." - print "Every retried case failed all of its attempts, so the retries" - print "cost roughly 3x the wall clock of those failures for no benefit." - } -}' diff --git a/toolchain/mfc/test/test.py b/toolchain/mfc/test/test.py index b60f4271e1..05720522d3 100644 --- a/toolchain/mfc/test/test.py +++ b/toolchain/mfc/test/test.py @@ -1,4 +1,3 @@ -import collections import itertools import math import os @@ -30,7 +29,6 @@ current_test_number = 0 total_test_count = 0 nRESCUED = 0 # cases that failed and were recovered by a retry (#1798) -nRESCUED_BY_CLASS = collections.defaultdict(int) # and of what kind errors = [] failed_tests = [] # Track failed test details for summary test_start_time = None # Track overall test duration @@ -437,7 +435,7 @@ def test(): seconds = total_duration % 60 # Build the summary report - _print_test_summary(nPASS, nFAIL, nSKIP, minutes, seconds, failed_tests, skipped_cases, nRESCUED, dict(nRESCUED_BY_CLASS)) + _print_test_summary(nPASS, nFAIL, nSKIP, minutes, seconds, failed_tests, skipped_cases, nRESCUED) # Write failed UUIDs to file for CI retry logic if failed_tests: @@ -450,7 +448,7 @@ def test(): sys.exit(nFAIL) -def _print_test_summary(passed: int, failed: int, skipped: int, minutes: int, seconds: float, failed_test_list: list, _skipped_cases: list, rescued: int = 0, rescued_by_class: dict = None): +def _print_test_summary(passed: int, failed: int, skipped: int, minutes: int, seconds: float, failed_test_list: list, _skipped_cases: list, rescued: int = 0): """Print a comprehensive test summary report.""" total = passed + failed + skipped @@ -482,8 +480,6 @@ def _print_test_summary(passed: int, failed: int, skipped: int, minutes: int, se # How often a retry actually earned its cost. See #1798: without this the # only measurable retry outcomes were the ones that failed anyway. summary_lines.append(f" [yellow]{rescued:4d}[/yellow] recovered by a retry") - for cls, n in sorted((rescued_by_class or {}).items(), key=lambda kv: -kv[1]): - summary_lines.append(f" [dim]{n:3d} {cls}[/dim]") summary_lines += [ f" [dim]{'─' * 12}[/dim]", f" [bold]{total:4d}[/bold] total", @@ -520,12 +516,6 @@ def _print_test_summary(passed: int, failed: int, skipped: int, minutes: int, se cons.raw.print(Panel("\n".join(summary_lines), title="[bold]Test Summary[/bold]", border_style=border_style, padding=(1, 2))) cons.print() - # A single greppable line so retry value can be aggregated across runs - # without reading logs by hand -- see #1798 and - # .github/scripts/harvest-retry-stats.sh. - by_class = ",".join(f"{k}:{v}" for k, v in sorted((rescued_by_class or {}).items())) - cons.print(f"MFC_RETRY_STATS rescued={rescued} failed={failed} passed={passed} by_class={by_class or 'none'}", no_indent=True) - def _process_silo_file(silo_filepath: str, case: TestCase, out_filepath: str): """Process a single silo file with h5dump and check for NaNs/Infinities.""" @@ -796,7 +786,7 @@ def handle_case(case: TestCase, devices: typing.Set[int]): return # Exit gracefully if abort was requested nAttempts = 0 - last_error_type = "" + last_error = None if ARG("single"): max_attempts = max(ARG("max_attempts"), 3) else: @@ -820,10 +810,9 @@ def handle_case(case: TestCase, devices: typing.Set[int]): # very different things for a tolerance mismatch than for an # execution failure. nRESCUED += 1 - nRESCUED_BY_CLASS[last_error_type or "unclassified"] += 1 - cons.print(f" [yellow]recovered on attempt {nAttempts}[/yellow] ({last_error_type or 'unclassified'}): {case.trace}") + cons.print(f" [yellow]recovered on attempt {nAttempts}[/yellow] ({classify_error(last_error) or 'unclassified'}): {case.trace}") except Exception as exc: - last_error_type = classify_error(exc) + last_error = exc if should_retry(nAttempts, max_attempts, abort_tests.is_set()): continue nFAIL += 1 diff --git a/toolchain/mfc/test/test_retry_policy.py b/toolchain/mfc/test/test_retry_policy.py index cd47a589e8..ce3d1218b7 100644 --- a/toolchain/mfc/test/test_retry_policy.py +++ b/toolchain/mfc/test/test_retry_policy.py @@ -60,26 +60,3 @@ def test_failures_are_classified_into_the_categories_the_policy_cares_about(): assert classify_error(Exception("NaN or Inf detected in the case.")) == "NaN detected" assert classify_error(Exception("Test case exceeded 1 hour timeout")) == "timeout" assert classify_error(Exception("something else entirely")) == "" - - -def test_the_summary_says_which_classes_a_retry_actually_rescued(capsys): - # A bare rescue count cannot answer "which retries are worth keeping". - # A tolerance mismatch rescued by a retry means something very different - # from an execution failure rescued by one. - from mfc.test.test import _print_test_summary - - _print_test_summary(10, 2, 0, 0, 1.0, [], [], rescued=3, rescued_by_class={"execution failed": 2, "tolerance mismatch": 1}) - out = capsys.readouterr().out - assert "execution failed" in out - assert "tolerance mismatch" in out - - -def test_the_summary_emits_one_machine_readable_line_for_aggregation(capsys): - # The numbers only become knowledge if they can be harvested across many - # runs without hand-reading logs, so emit one greppable line. - from mfc.test.test import _print_test_summary - - _print_test_summary(10, 2, 0, 0, 1.0, [], [], rescued=3, rescued_by_class={"execution failed": 3}) - out = capsys.readouterr().out - assert "MFC_RETRY_STATS" in out - assert "rescued=3" in out