Skip to content

Commit e7da4f7

Browse files
sbryngelsonsbryngelson
authored
Merge branch 'master' into flame-mixlyr-3D
2 parents bc2f20a + b44c811 commit e7da4f7

40 files changed

Lines changed: 791 additions & 213 deletions

‎.github/scripts/run_parallel_benchmarks.sh‎

Lines changed: 121 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,15 @@
11
#!/bin/bash
22
# Run PR and master benchmarks and verify outputs.
3-
# Both SLURM jobs are submitted up front so they run concurrently on
4-
# compute nodes (fair comparison under the same cluster load), but
5-
# monitoring happens sequentially to stay within the per-user cgroup
6-
# memory limit on login nodes (4 GB on Phoenix shared by 7 runners).
3+
#
4+
# Phoenix GPU runs both trees in ONE single-node job (see bench-pair.sh): same
5+
# node, same GPUs, benchmarked back-to-back -- identical hardware with no
6+
# contention, and it only needs one node so it backfills across a partition list.
7+
#
8+
# Other clusters (Frontier) and Phoenix CPU keep the two-job model: both SLURM
9+
# jobs are submitted up front so they run concurrently on compute nodes (fair
10+
# comparison under the same cluster load), but monitoring happens sequentially to
11+
# stay within the per-user cgroup memory limit on login nodes (4 GB on Phoenix
12+
# shared by 7 runners).
713
# Usage: run_parallel_benchmarks.sh <device> <interface> <cluster>
814

915
set -euo pipefail
@@ -24,96 +30,119 @@ echo "===================="
2430
echo "Starting benchmark jobs..."
2531
echo "===================="
2632

27-
# For Phoenix GPU benchmarks, select a consistent GPU partition so PR and
28-
# master always land on the same GPU type.
29-
if [ "$device" = "gpu" ] && [ "$cluster" = "phoenix" ]; then
30-
echo "Selecting Phoenix GPU partition for benchmark consistency..."
31-
# Require 2 nodes so both jobs can run concurrently on compute.
32-
GPU_PARTITION_MIN_NODES=2 source "${SCRIPT_DIR}/select-gpu-partition.sh"
33-
BENCH_GPU_PARTITION="$SELECTED_GPU_PARTITION"
34-
export BENCH_GPU_PARTITION
35-
fi
36-
37-
# The bench script must come from the PR tree (master may not have it).
38-
PR_BENCH_SCRIPT="$(cd "${SCRIPT_DIR}/../workflows/common" && pwd)/bench.sh"
39-
# Must match the slug computed by submit-slurm-job.sh:
40-
# basename("bench.sh") → "bench" → "bench-${device}-${interface}"
33+
# job_slug names the benchmark YAML each tree writes; bench.yml's bench_diff requires
34+
# exactly this name, and it is the same whether one job or two produced the results.
35+
# The SLURM .out/.slurm_job_id basename is NOT always the same string: submit-slurm-job.sh
36+
# derives that from the submitted script's basename, so the single-node path logs to
37+
# bench-pair-<device>-<interface> instead. Keep the two apart.
4138
job_slug="bench-${device}-${interface}"
39+
log_slug="$job_slug"
4240

43-
# --- Phase 1: Submit both SLURM jobs (no monitoring yet) ---
44-
echo "Submitting PR benchmark..."
45-
(cd pr && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster")
46-
pr_job_id=$(cat "pr/${job_slug}.slurm_job_id")
47-
echo "PR job submitted: $pr_job_id"
48-
49-
echo "Submitting master benchmark..."
50-
(cd master && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster")
51-
master_job_id=$(cat "master/${job_slug}.slurm_job_id")
52-
echo "Master job submitted: $master_job_id"
53-
54-
echo "Both SLURM jobs submitted — running concurrently on compute nodes."
55-
echo "Monitoring sequentially to conserve login node memory."
56-
57-
# --- Phase 2: Monitor sequentially (one at a time on login node) ---
58-
# On Phoenix 'embers' a long benchmark job can be preempted (PreemptMode=CANCEL,
59-
# so it is killed rather than requeued). On preemption (run_monitored exit 76)
60-
# resubmit a fresh job in the same tree and re-monitor, bounded by
61-
# MAX_PREEMPT_RESUBMITS (the 480m job timeout is the real backstop). Note: a
62-
# resubmitted job no longer overlaps its counterpart, slightly reducing
63-
# same-load fairness -- still preferable to failing the run on an infra preempt.
64-
: "${MAX_PREEMPT_RESUBMITS:=10}"
65-
monitor_bench_with_resubmit() { # arg: <dir> (pr|master); sets BENCH_MON_RC
66-
local dir="$1"
67-
local out="${dir}/${job_slug}.out"
68-
local jobid attempt=0 rc
69-
jobid=$(cat "${dir}/${job_slug}.slurm_job_id")
70-
while :; do
71-
rc=0
72-
bash "${SCRIPT_DIR}/run_monitored_slurm_job.sh" "$jobid" "$out" || rc=$?
73-
if [ "$rc" -ne 76 ]; then
74-
BENCH_MON_RC="$rc"
75-
return
76-
fi
77-
if [ "$attempt" -ge "$MAX_PREEMPT_RESUBMITS" ]; then
78-
echo "::error::${dir} benchmark preempted ${MAX_PREEMPT_RESUBMITS}x without completing; giving up."
79-
BENCH_MON_RC=1
80-
return
81-
fi
82-
attempt=$((attempt + 1))
83-
echo "::warning::${dir} benchmark job $jobid was preempted; resubmitting (attempt ${attempt}/${MAX_PREEMPT_RESUBMITS})."
84-
rm -f "$out"
85-
( cd "$dir" && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster" )
86-
jobid=$(cat "${dir}/${job_slug}.slurm_job_id")
87-
echo "${dir} benchmark resubmitted as job $jobid"
88-
done
89-
}
90-
91-
echo ""
92-
echo "=== Monitoring PR job $pr_job_id ==="
93-
monitor_bench_with_resubmit pr
94-
pr_exit=$BENCH_MON_RC
95-
if [ "$pr_exit" -ne 0 ]; then
96-
echo "PR job exited with code: $pr_exit"
97-
tail -n 50 "pr/${job_slug}.out" 2>/dev/null || echo " Could not read PR log"
98-
# The PR benchmark run genuinely failed (cases crashed/hung/SIGTERM'd, not a
99-
# monitor false-positive -- run_monitored_slurm_job.sh re-checks sacct). Fail
100-
# the job instead of falling through to the YAML-exists check, which would let
101-
# a broken PR pass green as long as a partial YAML was written. Scoped to PR
102-
# only: a master/baseline infra flake stays a warning and does not red-cross.
103-
exit 1
41+
if [ "$device" = "gpu" ] && [ "$cluster" = "phoenix" ]; then
42+
# --- Phoenix GPU: build + bench BOTH trees in ONE single-node job ---
43+
# bench-pair.sh builds master and PR, then benchmarks them back-to-back on
44+
# the same node and GPUs: identical hardware, no cross-run contention. Needing
45+
# only one node (-G2) lets submit-slurm-job.sh submit to a partition list and
46+
# backfill, instead of the old model's two-idle-nodes-in-one-partition wait.
47+
# Launched from the PR tree so ../master resolves and mfc.sh load finds a tree.
48+
# NOT SUBMIT_ONLY: submit-slurm-job.sh monitors and handles preemption (76)
49+
# and node faults (77) itself, so this even gains node-exclude-on-fault, which
50+
# the old two-job bench path never had.
51+
PAIR_SCRIPT="$(cd "${SCRIPT_DIR}/../workflows/common" && pwd)/bench-pair.sh"
52+
log_slug="bench-pair-${device}-${interface}"
53+
echo "Phoenix GPU: building and benchmarking master and PR in one single-node job."
54+
pair_rc=0
55+
( cd pr && bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PAIR_SCRIPT" "$device" "$interface" "$cluster" ) || pair_rc=$?
56+
if [ "$pair_rc" -ne 0 ]; then
57+
# One job builds+benches both trees, so a failure is not attributable to
58+
# one side. Fail hard rather than fall through to the YAML-exists check,
59+
# which would let a broken run pass green on a stale/partial YAML.
60+
echo "::error::single-node bench-pair job failed (exit ${pair_rc})."
61+
exit 1
62+
fi
63+
pr_exit=0
64+
master_exit=0
65+
echo "Single-node bench-pair job completed successfully."
10466
else
105-
echo "PR job completed successfully"
106-
fi
67+
# --- Other clusters / Phoenix CPU: two concurrent jobs, monitored serially ---
68+
# The bench script must come from the PR tree (master may not have it).
69+
PR_BENCH_SCRIPT="$(cd "${SCRIPT_DIR}/../workflows/common" && pwd)/bench.sh"
70+
71+
# Phase 1: Submit both SLURM jobs (no monitoring yet)
72+
echo "Submitting PR benchmark..."
73+
(cd pr && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster")
74+
pr_job_id=$(cat "pr/${log_slug}.slurm_job_id")
75+
echo "PR job submitted: $pr_job_id"
76+
77+
echo "Submitting master benchmark..."
78+
(cd master && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster")
79+
master_job_id=$(cat "master/${log_slug}.slurm_job_id")
80+
echo "Master job submitted: $master_job_id"
81+
82+
echo "Both SLURM jobs submitted — running concurrently on compute nodes."
83+
echo "Monitoring sequentially to conserve login node memory."
84+
85+
# Phase 2: Monitor sequentially (one at a time on login node)
86+
# On Phoenix 'embers' a long benchmark job can be preempted (PreemptMode=CANCEL,
87+
# so it is killed rather than requeued). On preemption (run_monitored exit 76)
88+
# resubmit a fresh job in the same tree and re-monitor, bounded by
89+
# MAX_PREEMPT_RESUBMITS (the 480m job timeout is the real backstop). Note: a
90+
# resubmitted job no longer overlaps its counterpart, slightly reducing
91+
# same-load fairness -- still preferable to failing the run on an infra preempt.
92+
: "${MAX_PREEMPT_RESUBMITS:=10}"
93+
monitor_bench_with_resubmit() { # arg: <dir> (pr|master); sets BENCH_MON_RC
94+
local dir="$1"
95+
local out="${dir}/${log_slug}.out"
96+
local jobid attempt=0 rc
97+
jobid=$(cat "${dir}/${log_slug}.slurm_job_id")
98+
while :; do
99+
rc=0
100+
bash "${SCRIPT_DIR}/run_monitored_slurm_job.sh" "$jobid" "$out" || rc=$?
101+
if [ "$rc" -ne 76 ]; then
102+
BENCH_MON_RC="$rc"
103+
return
104+
fi
105+
if [ "$attempt" -ge "$MAX_PREEMPT_RESUBMITS" ]; then
106+
echo "::error::${dir} benchmark preempted ${MAX_PREEMPT_RESUBMITS}x without completing; giving up."
107+
BENCH_MON_RC=1
108+
return
109+
fi
110+
attempt=$((attempt + 1))
111+
echo "::warning::${dir} benchmark job $jobid was preempted; resubmitting (attempt ${attempt}/${MAX_PREEMPT_RESUBMITS})."
112+
rm -f "$out"
113+
( cd "$dir" && SUBMIT_ONLY=1 bash "${SCRIPT_DIR}/submit-slurm-job.sh" "$PR_BENCH_SCRIPT" "$device" "$interface" "$cluster" )
114+
jobid=$(cat "${dir}/${log_slug}.slurm_job_id")
115+
echo "${dir} benchmark resubmitted as job $jobid"
116+
done
117+
}
107118

108-
echo ""
109-
echo "=== Monitoring master job $master_job_id ==="
110-
monitor_bench_with_resubmit master
111-
master_exit=$BENCH_MON_RC
112-
if [ "$master_exit" -ne 0 ]; then
113-
echo "Master job exited with code: $master_exit"
114-
tail -n 50 "master/${job_slug}.out" 2>/dev/null || echo " Could not read master log"
115-
else
116-
echo "Master job completed successfully"
119+
echo ""
120+
echo "=== Monitoring PR job $pr_job_id ==="
121+
monitor_bench_with_resubmit pr
122+
pr_exit=$BENCH_MON_RC
123+
if [ "$pr_exit" -ne 0 ]; then
124+
echo "PR job exited with code: $pr_exit"
125+
tail -n 50 "pr/${log_slug}.out" 2>/dev/null || echo " Could not read PR log"
126+
# The PR benchmark run genuinely failed (cases crashed/hung/SIGTERM'd, not a
127+
# monitor false-positive -- run_monitored_slurm_job.sh re-checks sacct). Fail
128+
# the job instead of falling through to the YAML-exists check, which would let
129+
# a broken PR pass green as long as a partial YAML was written. Scoped to PR
130+
# only: a master/baseline infra flake stays a warning and does not red-cross.
131+
exit 1
132+
else
133+
echo "PR job completed successfully"
134+
fi
135+
136+
echo ""
137+
echo "=== Monitoring master job $master_job_id ==="
138+
monitor_bench_with_resubmit master
139+
master_exit=$BENCH_MON_RC
140+
if [ "$master_exit" -ne 0 ]; then
141+
echo "Master job exited with code: $master_exit"
142+
tail -n 50 "master/${log_slug}.out" 2>/dev/null || echo " Could not read master log"
143+
else
144+
echo "Master job completed successfully"
145+
fi
117146
fi
118147

119148
# --- Phase 3: Verify outputs ---
@@ -146,15 +175,15 @@ if [ ! -f "$pr_yaml" ]; then
146175
echo "ERROR: PR benchmark output not found: $pr_yaml"
147176
ls -la pr/ || true
148177
echo ""
149-
tail -n 100 "pr/${job_slug}.out" 2>/dev/null || echo " Could not read PR log"
178+
tail -n 100 "pr/${log_slug}.out" 2>/dev/null || echo " Could not read PR log"
150179
exit 1
151180
fi
152181

153182
if [ ! -f "$master_yaml" ]; then
154183
echo "ERROR: Master benchmark output not found: $master_yaml"
155184
ls -la master/ || true
156185
echo ""
157-
tail -n 100 "master/${job_slug}.out" 2>/dev/null || echo " Could not read master log"
186+
tail -n 100 "master/${log_slug}.out" 2>/dev/null || echo " Could not read master log"
158187
exit 1
159188
fi
160189

‎.github/scripts/select-gpu-partition.sh‎

Lines changed: 0 additions & 49 deletions
This file was deleted.

‎.github/scripts/submit-slurm-job.sh‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -126,14 +126,16 @@ elif [ "$device" = "gpu" ]; then
126126
# Determine GPU partition
127127
gpu_partition="batch"
128128
if [ "$gpu_partition_dynamic" = "true" ]; then
129-
# Use pre-selected bench partition if available, otherwise query sinfo
130-
if [ -n "${BENCH_GPU_PARTITION:-}" ]; then
131-
gpu_partition="$BENCH_GPU_PARTITION"
132-
echo "Using pre-selected bench partition: $gpu_partition (PR/master consistency)"
133-
else
134-
source "${SCRIPT_DIR}/select-gpu-partition.sh"
135-
gpu_partition="$SELECTED_GPU_PARTITION"
136-
fi
129+
# Submit to a partition LIST and let SLURM start on whichever frees first,
130+
# instead of pinning one partition and queueing behind it. Both tests and
131+
# benchmarks run on a single node now: benchmarks build and bench BOTH the
132+
# master and PR trees in one job on the same GPUs (see bench-pair.sh), so
133+
# neither needs the old single-partition bench selector (which required two
134+
# idle nodes in the SAME partition at once -- the main bench queue-starver).
135+
# gpu-l40s (bad hardware) and gpu-rtx6000 (too slow for the time limit) are
136+
# intentionally omitted.
137+
gpu_partition="gpu-h200,gpu-h100,gpu-a100,gpu-v100"
138+
echo "Using GPU partition list: $gpu_partition"
137139
fi
138140

139141
case "$cluster" in
@@ -295,8 +297,9 @@ while :; do
295297
exit 1
296298
fi
297299
if [ "$monitor_rc" -eq 77 ]; then
298-
# The in-allocation preflight found this node unusable before any real
299-
# work started. Exclude it and draw another node.
300+
# The in-allocation preflight found this node unusable. Exclude it and draw
301+
# another node. Note bench-pair.sh probes only after building both trees, so
302+
# a fault there discards those builds and the resubmit repeats them.
300303
faulted_node=$(bash "$SCRIPT_DIR/node-exclude.sh" node-from "$output_file")
301304
if [ "$node_attempt" -lt "$MFC_MAX_NODE_RESUBMITS" ]; then
302305
node_attempt=$((node_attempt + 1))

‎.github/workflows/bench.yml‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,23 @@ concurrency:
1010
group: ${{ github.workflow }}-${{ github.ref }}${{ github.event_name == 'pull_request_review' && format('-review-{0}', github.run_id) || '' }}
1111
cancel-in-progress: true
1212

13+
# Least-privilege default: no job in this workflow writes to the repo.
14+
permissions:
15+
contents: read
16+
1317
jobs:
1418
file-changes:
1519
name: Detect File Changes
1620
if: >
1721
github.event_name != 'pull_request_review' ||
1822
github.event.review.user.type != 'Bot'
23+
# Job-level permissions replace the workflow default outright rather than
24+
# merging with it, so contents must be restated here. paths-filter reads the
25+
# PR's changed-file list via pulls.listFiles; this is the only job that needs
26+
# it, so it is granted here instead of workflow-wide.
27+
permissions:
28+
contents: read
29+
pull-requests: read
1930
runs-on: 'ubuntu-latest'
2031
outputs:
2132
checkall: ${{ steps.changes.outputs.checkall }}
@@ -148,8 +159,9 @@ jobs:
148159
- name: Print Logs
149160
if: always()
150161
run: |
151-
cat pr/bench-${{ matrix.device }}-${{ matrix.interface }}.* 2>/dev/null || true
152-
cat master/bench-${{ matrix.device }}-${{ matrix.interface }}.* 2>/dev/null || true
162+
# bench*- also catches the single-node bench-pair-<d>-<i>.out job log.
163+
cat pr/bench*-${{ matrix.device }}-${{ matrix.interface }}.* 2>/dev/null || true
164+
cat master/bench*-${{ matrix.device }}-${{ matrix.interface }}.* 2>/dev/null || true
153165
154166
- name: Print Per-Case Logs
155167
if: always()

‎.github/workflows/claude-code-review.yml‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -58,26 +58,17 @@ jobs:
5858
exit 1
5959
fi
6060
61-
PR_HEAD_REF="$(gh pr view "$PR_NUMBER" --repo "${{ github.repository }}" --json headRefName --jq .headRefName)"
62-
6361
echo "pr_number=$PR_NUMBER" >> "$GITHUB_OUTPUT"
64-
echo "pr_head_ref=$PR_HEAD_REF" >> "$GITHUB_OUTPUT"
6562
63+
# Base repo only. This job is privileged (pull_request_target / issue_comment:
64+
# it holds secrets and a write-capable token), so the PR head is never checked
65+
# out or fetched here. The diff and per-file context are pulled through the gh
66+
# API in later steps and treated purely as data.
6667
- name: Checkout base repo
6768
uses: actions/checkout@v5
6869
with:
6970
fetch-depth: 0
7071

71-
- name: Fetch PR head
72-
shell: bash
73-
env:
74-
PR_NUMBER: ${{ steps.mode.outputs.pr_number }}
75-
run: |
76-
set -euo pipefail
77-
# Fetch the PR merge ref — works for both same-repo and fork PRs
78-
# (fork branches don't exist on origin, but pull/<n>/head always does)
79-
git fetch origin "pull/${PR_NUMBER}/head"
80-
8172
- name: Resolve review state
8273
id: state
8374
shell: bash

0 commit comments

Comments
 (0)