diff --git a/.github/scripts/run_parallel_benchmarks.sh b/.github/scripts/run_parallel_benchmarks.sh index 104bd87da..2f262692d 100755 --- a/.github/scripts/run_parallel_benchmarks.sh +++ b/.github/scripts/run_parallel_benchmarks.sh @@ -1,9 +1,15 @@ #!/bin/bash # Run PR and master benchmarks and verify outputs. -# Both SLURM jobs are submitted up front so they run concurrently on -# compute nodes (fair comparison under the same cluster load), but -# monitoring happens sequentially to stay within the per-user cgroup -# memory limit on login nodes (4 GB on Phoenix shared by 7 runners). +# +# Phoenix GPU runs both trees in ONE single-node job (see bench-pair.sh): same +# node, same GPUs, benchmarked back-to-back -- identical hardware with no +# contention, and it only needs one node so it backfills across a partition list. +# +# Other clusters (Frontier) and Phoenix CPU keep the two-job model: both SLURM +# jobs are submitted up front so they run concurrently on compute nodes (fair +# comparison under the same cluster load), but monitoring happens sequentially to +# stay within the per-user cgroup memory limit on login nodes (4 GB on Phoenix +# shared by 7 runners). # Usage: run_parallel_benchmarks.sh set -euo pipefail @@ -24,96 +30,119 @@ echo "====================" echo "Starting benchmark jobs..." echo "====================" -# For Phoenix GPU benchmarks, select a consistent GPU partition so PR and -# master always land on the same GPU type. -if [ "$device" = "gpu" ] && [ "$cluster" = "phoenix" ]; then - echo "Selecting Phoenix GPU partition for benchmark consistency..." - # Require 2 nodes so both jobs can run concurrently on compute. - GPU_PARTITION_MIN_NODES=2 source "${SCRIPT_DIR}/select-gpu-partition.sh" - BENCH_GPU_PARTITION="$SELECTED_GPU_PARTITION" - export BENCH_GPU_PARTITION -fi - -# The bench script must come from the PR tree (master may not have it). -PR_BENCH_SCRIPT="$(cd "${SCRIPT_DIR}/../workflows/common" && pwd)/bench.sh" -# Must match the slug computed by submit-slurm-job.sh: -# basename("bench.sh") → "bench" → "bench-${device}-${interface}" +# job_slug names the benchmark YAML each tree writes; bench.yml's bench_diff requires +# exactly this name, and it is the same whether one job or two produced the results. +# The SLURM .out/.slurm_job_id basename is NOT always the same string: submit-slurm-job.sh +# derives that from the submitted script's basename, so the single-node path logs to +# bench-pair-- instead. Keep the two apart. job_slug="bench-${device}-${interface}" +log_slug="$job_slug" -# --- Phase 1: Submit both SLURM jobs (no monitoring yet) --- -echo "Submitting PR benchmark..." -(cd pr && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster") -pr_job_id=$(cat "pr/${job_slug}.slurm_job_id") -echo "PR job submitted: $pr_job_id" - -echo "Submitting master benchmark..." -(cd master && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster") -master_job_id=$(cat "master/${job_slug}.slurm_job_id") -echo "Master job submitted: $master_job_id" - -echo "Both SLURM jobs submitted — running concurrently on compute nodes." -echo "Monitoring sequentially to conserve login node memory." - -# --- Phase 2: Monitor sequentially (one at a time on login node) --- -# On Phoenix 'embers' a long benchmark job can be preempted (PreemptMode=CANCEL, -# so it is killed rather than requeued). On preemption (run_monitored exit 76) -# resubmit a fresh job in the same tree and re-monitor, bounded by -# MAX_PREEMPT_RESUBMITS (the 480m job timeout is the real backstop). Note: a -# resubmitted job no longer overlaps its counterpart, slightly reducing -# same-load fairness -- still preferable to failing the run on an infra preempt. -: "${MAX_PREEMPT_RESUBMITS:=10}" -monitor_bench_with_resubmit() { # arg: (pr|master); sets BENCH_MON_RC - local dir="$1" - local out="${dir}/${job_slug}.out" - local jobid attempt=0 rc - jobid=$(cat "${dir}/${job_slug}.slurm_job_id") - while :; do - rc=0 - bash "${SCRIPT_DIR}/run_monitored_slurm_job.sh" "$jobid" "$out" || rc=$? - if [ "$rc" -ne 76 ]; then - BENCH_MON_RC="$rc" - return - fi - if [ "$attempt" -ge "$MAX_PREEMPT_RESUBMITS" ]; then - echo "::error::${dir} benchmark preempted ${MAX_PREEMPT_RESUBMITS}x without completing; giving up." - BENCH_MON_RC=1 - return - fi - attempt=$((attempt + 1)) - echo "::warning::${dir} benchmark job $jobid was preempted; resubmitting (attempt ${attempt}/${MAX_PREEMPT_RESUBMITS})." - rm -f "$out" - ( cd "$dir" && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster" ) - jobid=$(cat "${dir}/${job_slug}.slurm_job_id") - echo "${dir} benchmark resubmitted as job $jobid" - done -} - -echo "" -echo "=== Monitoring PR job $pr_job_id ===" -monitor_bench_with_resubmit pr -pr_exit=$BENCH_MON_RC -if [ "$pr_exit" -ne 0 ]; then - echo "PR job exited with code: $pr_exit" - tail -n 50 "pr/${job_slug}.out" 2>/dev/null || echo " Could not read PR log" - # The PR benchmark run genuinely failed (cases crashed/hung/SIGTERM'd, not a - # monitor false-positive -- run_monitored_slurm_job.sh re-checks sacct). Fail - # the job instead of falling through to the YAML-exists check, which would let - # a broken PR pass green as long as a partial YAML was written. Scoped to PR - # only: a master/baseline infra flake stays a warning and does not red-cross. - exit 1 +if [ "$device" = "gpu" ] && [ "$cluster" = "phoenix" ]; then + # --- Phoenix GPU: build + bench BOTH trees in ONE single-node job --- + # bench-pair.sh builds master and PR, then benchmarks them back-to-back on + # the same node and GPUs: identical hardware, no cross-run contention. Needing + # only one node (-G2) lets submit-slurm-job.sh submit to a partition list and + # backfill, instead of the old model's two-idle-nodes-in-one-partition wait. + # Launched from the PR tree so ../master resolves and mfc.sh load finds a tree. + # NOT SUBMIT_ONLY: submit-slurm-job.sh monitors and handles preemption (76) + # and node faults (77) itself, so this even gains node-exclude-on-fault, which + # the old two-job bench path never had. + PAIR_SCRIPT="$(cd "${SCRIPT_DIR}/../workflows/common" && pwd)/bench-pair.sh" + log_slug="bench-pair-${device}-${interface}" + echo "Phoenix GPU: building and benchmarking master and PR in one single-node job." + pair_rc=0 + ( cd pr && bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PAIR_SCRIPT" "$device" "$interface" "$cluster" ) || pair_rc=$? + if [ "$pair_rc" -ne 0 ]; then + # One job builds+benches both trees, so a failure is not attributable to + # one side. Fail hard rather than fall through to the YAML-exists check, + # which would let a broken run pass green on a stale/partial YAML. + echo "::error::single-node bench-pair job failed (exit ${pair_rc})." + exit 1 + fi + pr_exit=0 + master_exit=0 + echo "Single-node bench-pair job completed successfully." else - echo "PR job completed successfully" -fi + # --- Other clusters / Phoenix CPU: two concurrent jobs, monitored serially --- + # The bench script must come from the PR tree (master may not have it). + PR_BENCH_SCRIPT="$(cd "${SCRIPT_DIR}/../workflows/common" && pwd)/bench.sh" + + # Phase 1: Submit both SLURM jobs (no monitoring yet) + echo "Submitting PR benchmark..." + (cd pr && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster") + pr_job_id=$(cat "pr/${log_slug}.slurm_job_id") + echo "PR job submitted: $pr_job_id" + + echo "Submitting master benchmark..." + (cd master && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster") + master_job_id=$(cat "master/${log_slug}.slurm_job_id") + echo "Master job submitted: $master_job_id" + + echo "Both SLURM jobs submitted — running concurrently on compute nodes." + echo "Monitoring sequentially to conserve login node memory." + + # Phase 2: Monitor sequentially (one at a time on login node) + # On Phoenix 'embers' a long benchmark job can be preempted (PreemptMode=CANCEL, + # so it is killed rather than requeued). On preemption (run_monitored exit 76) + # resubmit a fresh job in the same tree and re-monitor, bounded by + # MAX_PREEMPT_RESUBMITS (the 480m job timeout is the real backstop). Note: a + # resubmitted job no longer overlaps its counterpart, slightly reducing + # same-load fairness -- still preferable to failing the run on an infra preempt. + : "${MAX_PREEMPT_RESUBMITS:=10}" + monitor_bench_with_resubmit() { # arg: (pr|master); sets BENCH_MON_RC + local dir="$1" + local out="${dir}/${log_slug}.out" + local jobid attempt=0 rc + jobid=$(cat "${dir}/${log_slug}.slurm_job_id") + while :; do + rc=0 + bash "${SCRIPT_DIR}/run_monitored_slurm_job.sh" "$jobid" "$out" || rc=$? + if [ "$rc" -ne 76 ]; then + BENCH_MON_RC="$rc" + return + fi + if [ "$attempt" -ge "$MAX_PREEMPT_RESUBMITS" ]; then + echo "::error::${dir} benchmark preempted ${MAX_PREEMPT_RESUBMITS}x without completing; giving up." + BENCH_MON_RC=1 + return + fi + attempt=$((attempt + 1)) + echo "::warning::${dir} benchmark job $jobid was preempted; resubmitting (attempt ${attempt}/${MAX_PREEMPT_RESUBMITS})." + rm -f "$out" + ( cd "$dir" && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster" ) + jobid=$(cat "${dir}/${log_slug}.slurm_job_id") + echo "${dir} benchmark resubmitted as job $jobid" + done + } -echo "" -echo "=== Monitoring master job $master_job_id ===" -monitor_bench_with_resubmit master -master_exit=$BENCH_MON_RC -if [ "$master_exit" -ne 0 ]; then - echo "Master job exited with code: $master_exit" - tail -n 50 "master/${job_slug}.out" 2>/dev/null || echo " Could not read master log" -else - echo "Master job completed successfully" + echo "" + echo "=== Monitoring PR job $pr_job_id ===" + monitor_bench_with_resubmit pr + pr_exit=$BENCH_MON_RC + if [ "$pr_exit" -ne 0 ]; then + echo "PR job exited with code: $pr_exit" + tail -n 50 "pr/${log_slug}.out" 2>/dev/null || echo " Could not read PR log" + # The PR benchmark run genuinely failed (cases crashed/hung/SIGTERM'd, not a + # monitor false-positive -- run_monitored_slurm_job.sh re-checks sacct). Fail + # the job instead of falling through to the YAML-exists check, which would let + # a broken PR pass green as long as a partial YAML was written. Scoped to PR + # only: a master/baseline infra flake stays a warning and does not red-cross. + exit 1 + else + echo "PR job completed successfully" + fi + + echo "" + echo "=== Monitoring master job $master_job_id ===" + monitor_bench_with_resubmit master + master_exit=$BENCH_MON_RC + if [ "$master_exit" -ne 0 ]; then + echo "Master job exited with code: $master_exit" + tail -n 50 "master/${log_slug}.out" 2>/dev/null || echo " Could not read master log" + else + echo "Master job completed successfully" + fi fi # --- Phase 3: Verify outputs --- @@ -146,7 +175,7 @@ if [ ! -f "$pr_yaml" ]; then echo "ERROR: PR benchmark output not found: $pr_yaml" ls -la pr/ || true echo "" - tail -n 100 "pr/${job_slug}.out" 2>/dev/null || echo " Could not read PR log" + tail -n 100 "pr/${log_slug}.out" 2>/dev/null || echo " Could not read PR log" exit 1 fi @@ -154,7 +183,7 @@ if [ ! -f "$master_yaml" ]; then echo "ERROR: Master benchmark output not found: $master_yaml" ls -la master/ || true echo "" - tail -n 100 "master/${job_slug}.out" 2>/dev/null || echo " Could not read master log" + tail -n 100 "master/${log_slug}.out" 2>/dev/null || echo " Could not read master log" exit 1 fi diff --git a/.github/scripts/select-gpu-partition.sh b/.github/scripts/select-gpu-partition.sh deleted file mode 100644 index b86c03098..000000000 --- a/.github/scripts/select-gpu-partition.sh +++ /dev/null @@ -1,49 +0,0 @@ -#!/bin/bash -# Select the best available Phoenix GPU partition using sinfo. -# Sources into caller: exports SELECTED_GPU_PARTITION. -# -# Priority order prefers partitions most likely to have availability. -# V100 is last due to slower performance near the test time limit. -# Falls back to gpu-a100 if no partition meets the idle node threshold. -# -# gpu-l40s is out of rotation: it has been failing jobs for weeks, and it was -# also the partition CI kept selecting and then starving on. -# -# Only fully idle nodes count. A "mix" node is partially allocated and may have -# no free GPU, so counting it overstates availability: run 33553417354 picked -# gpu-l40s on "1 idle/mix nodes", then sat in the queue until the 3-5.5h job -# timeout without ever starting. -# -# The match is anchored at both ends. sinfo's %t suffixes a state to flag it -- -# "*" not responding, "$" reserved for maintenance, "~" powered down -- and this -# cluster does emit them (drain*, down*, alloc$, drain$ are all live right now). -# A bare "^idle" would count idle* and idle$ as available and starve the job on -# nodes that cannot take it. -# RTX 6000 nodes are excluded (too slow for the test suite time limit). -# -# Optional: set GPU_PARTITION_MIN_NODES before sourcing to require a minimum -# number of idle nodes (e.g. GPU_PARTITION_MIN_NODES=2 for parallel bench jobs). -# -# Usage: source .github/scripts/select-gpu-partition.sh - -_GPU_PARTITION_PRIORITY="gpu-h200 gpu-h100 gpu-a100 gpu-v100" -_GPU_PARTITION_FALLBACK="gpu-a100" -_GPU_PARTITION_MIN_NODES="${GPU_PARTITION_MIN_NODES:-1}" - -SELECTED_GPU_PARTITION="" -for _part in $_GPU_PARTITION_PRIORITY; do - _idle=$(sinfo -p "$_part" --noheader -o "%t" 2>/dev/null | grep -cE "^idle$" || true) - if [ "${_idle:-0}" -ge "$_GPU_PARTITION_MIN_NODES" ]; then - SELECTED_GPU_PARTITION="$_part" - echo "Selected GPU partition: $SELECTED_GPU_PARTITION ($_idle idle nodes)" - break - fi -done - -if [ -z "$SELECTED_GPU_PARTITION" ]; then - echo "WARNING: No idle GPU partition found; falling back to $_GPU_PARTITION_FALLBACK (may queue)" - SELECTED_GPU_PARTITION="$_GPU_PARTITION_FALLBACK" -fi - -export SELECTED_GPU_PARTITION -unset _GPU_PARTITION_PRIORITY _GPU_PARTITION_FALLBACK _GPU_PARTITION_MIN_NODES _part _idle diff --git a/.github/scripts/submit-slurm-job.sh b/.github/scripts/submit-slurm-job.sh index 28f476a64..1a39ca7a6 100755 --- a/.github/scripts/submit-slurm-job.sh +++ b/.github/scripts/submit-slurm-job.sh @@ -126,30 +126,16 @@ elif [ "$device" = "gpu" ]; then # Determine GPU partition gpu_partition="batch" if [ "$gpu_partition_dynamic" = "true" ]; then - if [ "$job_type" = "bench" ]; then - # Benchmarks compare PR against master, so both jobs must land on the - # SAME GPU type or the comparison is meaningless. That rules out a - # partition list (SLURM could place PR and master on different - # hardware); instead a single partition is picked and pinned across - # both jobs via BENCH_GPU_PARTITION. See run_parallel_benchmarks.sh. - if [ -n "${BENCH_GPU_PARTITION:-}" ]; then - gpu_partition="$BENCH_GPU_PARTITION" - echo "Using pre-selected bench partition: $gpu_partition (PR/master consistency)" - else - source "${SCRIPT_DIR}/select-gpu-partition.sh" - gpu_partition="$SELECTED_GPU_PARTITION" - fi - else - # Tests (and build+test) don't compare across hardware, so submit to a - # partition LIST and let SLURM start on whichever frees first instead - # of pinning one partition and queueing behind it. This restores the - # multi-partition backfill that #1299 dropped when it unified test and - # bench onto the single-partition bench selector. gpu-l40s (bad - # hardware) and gpu-rtx6000 (too slow for the test time limit) are - # intentionally omitted. - gpu_partition="gpu-h200,gpu-h100,gpu-a100,gpu-v100" - echo "Using GPU partition list for test job: $gpu_partition" - fi + # Submit to a partition LIST and let SLURM start on whichever frees first, + # instead of pinning one partition and queueing behind it. Both tests and + # benchmarks run on a single node now: benchmarks build and bench BOTH the + # master and PR trees in one job on the same GPUs (see bench-pair.sh), so + # neither needs the old single-partition bench selector (which required two + # idle nodes in the SAME partition at once -- the main bench queue-starver). + # gpu-l40s (bad hardware) and gpu-rtx6000 (too slow for the time limit) are + # intentionally omitted. + gpu_partition="gpu-h200,gpu-h100,gpu-a100,gpu-v100" + echo "Using GPU partition list: $gpu_partition" fi case "$cluster" in @@ -311,8 +297,9 @@ while :; do exit 1 fi 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. + # The in-allocation preflight found this node unusable. Exclude it and draw + # another node. Note bench-pair.sh probes only after building both trees, so + # a fault there discards those builds and the resubmit repeats them. 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)) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 097b55dcc..473f0988c 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -159,8 +159,9 @@ jobs: - name: Print Logs if: always() run: | - cat pr/bench-${{ matrix.device }}-${{ matrix.interface }}.* 2>/dev/null || true - cat master/bench-${{ matrix.device }}-${{ matrix.interface }}.* 2>/dev/null || true + # bench*- also catches the single-node bench-pair--.out job log. + cat pr/bench*-${{ matrix.device }}-${{ matrix.interface }}.* 2>/dev/null || true + cat master/bench*-${{ matrix.device }}-${{ matrix.interface }}.* 2>/dev/null || true - name: Print Per-Case Logs if: always() diff --git a/.github/workflows/common/bench-pair.sh b/.github/workflows/common/bench-pair.sh new file mode 100755 index 000000000..9d69e4a2d --- /dev/null +++ b/.github/workflows/common/bench-pair.sh @@ -0,0 +1,99 @@ +#!/bin/bash +# Build and benchmark BOTH the master and PR trees inside ONE Phoenix SLURM job, +# on the same node and the same GPUs, so PR-vs-master is measured on identical +# hardware. Builds run first (compile only, no GPU contention); the two benchmark +# runs are then executed strictly back-to-back so they never share the GPUs. +# +# This replaces the old two-job model (one SLURM job per tree, both pinned to one +# partition via BENCH_GPU_PARTITION). That model needed two idle nodes in the +# SAME partition at once -- the main source of benchmark queue starvation. A +# single node needs only -G2, so the job submits to a partition LIST (see +# submit-slurm-job.sh) and backfills onto whichever partition frees first. +# +# Same-node is a strictly stronger fairness guarantee than the old "same GPU +# type": the two runs share the exact silicon, and by building first and +# benchmarking back-to-back they also run adjacently in time. +# +# The build/bench commands mirror .github/workflows/common/bench.sh. They are +# duplicated rather than folded in behind a phase flag because the pair +# orchestration (two trees, one node probe, shared GPUs) does not map cleanly +# onto that single-tree script. +# +# Runs inside the SLURM allocation, launched from the PR tree; master is a +# sibling at ../master. The module environment is already loaded by +# submit-slurm-job.sh (modules are cluster-level, so both trees build against +# them). Expects env: job_device, job_interface, job_cluster. + +set -euo pipefail + +pr_dir="$(pwd)" +master_dir="$(cd "${pr_dir}/../master" && pwd)" + +# Output name the downstream comparison expects in each tree (see bench.yml +# "Generate & Post Comment": bench--.yaml). +bench_yaml="bench-${job_device}-${job_interface}.yaml" + +# Cap parallel compile jobs (see common/bench.sh: GNR nodes have 192 cores but +# nproc is too aggressive for the build). +n_jobs=$(( $(nproc) > 64 ? 64 : $(nproc) )) + +# $gpu_opts (e.g. "--gpu acc") from $job_device/$job_interface. +source "${pr_dir}/.github/scripts/gpu-opts.sh" + +tmpbuild=/storage/project/r-sbryngelson3-0/sbryngelson3/mytmp_build + +# Per-tree scratch dir, unique to this job so concurrent matrix jobs don't race. +tree_tmpdir() { echo "${tmpbuild}/run-$(basename "$1")-${SLURM_JOB_ID:-$$}"; } + +# Remove this job's scratch dirs however the script exits. set -e means a failed build +# or bench would otherwise skip cleanup and leak them on project storage -- and failures +# are when they accumulate. bench.sh traps its scratch dir the same way. +trap 'rm -rf "${tmpbuild}/run-"*"-${SLURM_JOB_ID:-$$}" 2>/dev/null || true' EXIT + +build_tree() { # + local dir="$1" + echo "====================" + echo "BUILD: $dir" + echo "====================" + ( cd "$dir" + export TMPDIR="$(tree_tmpdir "$dir")"; mkdir -p "$TMPDIR" + # Always nuke stale builds: Phoenix compute nodes are heterogeneous, so a + # binary left by another node risks a SIGILL microarchitecture mismatch. + source .github/scripts/clean-build.sh; clean_build + source .github/scripts/retry-build.sh + retry_build ./mfc.sh build -j "$n_jobs" $gpu_opts ) +} + +bench_tree() { # + local dir="$1" + echo "====================" + echo "BENCH: $dir" + echo "====================" + ( cd "$dir" + export TMPDIR="$(tree_tmpdir "$dir")"; mkdir -p "$TMPDIR" + # $ngpus / $gpu_ids from the allocation (a -G2 job sees 2 GPUs: "0 1"). + source .github/scripts/detect-gpus.sh + ./mfc.sh bench --mem 4 -o "$bench_yaml" \ + -- -c phoenix-bench $gpu_opts -g $gpu_ids -n "$ngpus" ) +} + +# --- Build both trees (GPUs idle; order/timing here does not affect fairness) --- +build_tree "$master_dir" +build_tree "$pr_dir" + +# --- Probe the node once before spending the allocation on the benchmarks --- +# (exit 77 => submit-slurm-job.sh excludes this node and resubmits elsewhere.) +preflight_rc=0 +bash "${pr_dir}/.github/scripts/preflight.sh" "$job_cluster" "$job_device" || preflight_rc=$? +if [ "$preflight_rc" -ne 0 ]; then + exit "$preflight_rc" +fi + +# --- Benchmarks: strictly back-to-back on the same GPUs (fair, no contention) --- +bench_tree "$master_dir" +bench_tree "$pr_dir" + +# Let NFS settle before the EXIT trap removes this job's scratch dirs. +sleep 10 + +echo "bench-pair complete: master and PR benchmarked on $(hostname -s)"