ci(rccl): make the perf-regression gate refuse to answer when it did not measure - #310
Draft
speriaswamy-amd wants to merge 17 commits into
Draft
ci(rccl): make the perf-regression gate refuse to answer when it did not measure#310speriaswamy-amd wants to merge 17 commits into
speriaswamy-amd wants to merge 17 commits into
Conversation
Fix the group-by/key bug in the RCCL regression pipeline where results were bucketed by message size alone, silently collapsing the (data type, inPlace) dimensions and hiding regressions: - add group_rccl_results() canonical grouping by (type, inPlace) + size sort - convert_to_graph_dict no longer overwrites in-place/out-of-place/dtype rows - check_bw_dip / check_lat_dip / check_bus_bw group + sort before comparing, eliminating dtype-boundary false positives and cross-dimension mixups Add a paired A/B regression detector (regression_lib.py) that compares a candidate build against a reference build run back-to-back on the same nodes, so common-mode noise cancels. Triple-gated for CI stability: size-tiered relative threshold + non-parametric separation (p75(B) < p25(A)) + adjacency confirmation, plus on-hardware threshold derivation from a control run. Validated on 4-node MI350X: 0 false positives on A=A control, real regressions caught on ref-vs-candidate. Add rccl_ab_regression.py orchestration, optional per-side librccl path + -d data-type support in rccl_regression, plm_rsh_args for multi-node ssh launch, and unit tests (47 passing). Co-authored-by: Cursor <cursoragent@cursor.com>
Implement the remaining ticket items, keeping pure decision logic separate from
cluster orchestration and unit-testing it (14 new tests, 61 total):
- ci_robustness_lib.py:
- run_with_retries(): retry transient sweep failures with linear backoff and a
pre-retry hook; classify_failure() never retries data-corruption/schema
failures (so real bugs aren't papered over).
- build_gpu_cleanup_script()/parse_gpu_pids(): kill stale RCCL/MPI processes
(self-match-safe pkill via the [x]yz trick), optionally GPU-holding PIDs from
rocm-smi and stale docker/podman containers.
- rccl_lib.cleanup_gpus_on_nodes(): best-effort cluster-wide cleanup wrapper.
- rccl_ab_regression.py: run cleanup first (test_00) and between sweep retries;
wrap each A/B sweep in run_with_retries (transparent on healthy runs).
Validated on 4-node MI350X: cleanup runs per node, retry is transparent on
success, control verdict unchanged (0 regressions).
Co-authored-by: Cursor <cursoragent@cursor.com>
Add a dedicated, low-noise run record separate from the verbose parallel-ssh logging. For each rccl-tests run we append a section to cvs_params.rccl_command_log containing the MPI launch command normalized to a single copy/paste-able line (for reproducibility) followed by the raw rccl-tests output (perf table; plus NCCL INFO when NCCL_DEBUG=INFO). - rccl_lib.format_run_command_log_entry() (pure, unit-tested) + _maybe_write_run_command_log() - rccl_regression appends the record on a successful run - rccl_ab_regression auto-defaults the path to <output_dir>/rccl_runs.log Co-authored-by: Cursor <cursoragent@cursor.com>
…uto-load - Wrap remote mpirun in coreutils timeout so a wedged collective (e.g. an alltoall deadlock) is killed on the node instead of leaking ranks/GPUs indefinitely (rccl_regression, rccl_perf in rccl_lib.py). - Tolerate legacy rccl-tests runs that append duplicate JSON result blobs by parsing only the first valid value. - Make the NCCL-knob regression matrix optional in rccl_ab_regression.py so a plain perf-regression run doesn't require a knob sweep. - Add ab_regression.skip_keys to exclude known-broken upstream (collective, dtype) combos from the gate instead of hard-failing. - Auto-load hardware-calibrated thresholds (ab_derived_thresholds.json) in detect mode, with use_derived_thresholds: false as an escape hatch. - Add unit tests for the new parametrization behavior.
Relocates the RCCL CI gate's build/submit/poll/report scripts here from an untracked NFS location, addressing review feedback on rocm-systems#8351 that these scripts weren't version controlled. Placed under cvs (rather than a new repo, rocm-systems, or cvs-sbatch) since these bash wrappers around sbatch/squeue are a stopgap for scheduler-submission capability CVS is expected to gain natively; see ci/rccl_perf_gate/README.md. All scripts honor an RCCL_CI_ROOT override so they aren't tied to one cluster's NFS layout.
Workspace isolation (RCCL_CI_WORKSPACE=1, default off): run build and detect out of runs/<run_key>/ with their own cvs worktree, cvs-sbatch copy, builds, artifacts and logs, so concurrent runs cannot clobber each other. Prerequisite for adding more GitHub runners. Legacy shared-tree behaviour is byte-identical when the flag is off. Builds are seeded from a shared rev+recipe-keyed cache via hardlinks so isolation does not cost a cold ~30min LTO build per run. ROCm dist layout invariant: fail the build fast if the SDK has several inodes per SONAME. The dist is a pip wheel and wheel packaging flattens versioned-library symlink chains into independent files; glibc dedups loaded objects by inode, so a dlopen() by unversioned name returns a second, uninitialised copy. For ROCr that silently disabled DMA-BUF and hung multi-node collectives for the full rccl_timeout.
ws_prune_build ran du over build/, stage/ and src/ unconditionally. stage/ is not always created and the candidate side builds in place so it has no src/; du then returns non-zero, pipefail propagates that out of the assignment, and set -e aborted a build whose libraries were already built, cached and templated. Via the afterok dependency that also silently cancelled the detect job behind it. Measure only paths that exist, and treat cache-publish and prune as non-fatal at the call sites.
… them
run_rccl_build.sh and run_rccl_ab.sh source workspace.sh and exec
normalize_rocm_dist.sh from ${RCCL_CI_ROOT}/sbatch/lib -- an unversioned
NFS directory. So the committed scripts depended on code that was not in
the repo: a fresh checkout of ROCm/cvs would hard-fail on the `source`,
and normalize_rocm_dist.sh would be silently skipped, which reintroduces
the flattened-dist DMA-BUF hang the invariant check exists to catch.
Bring all three into the tree next to their callers. The callers now
resolve lib/ relative to their own location and only fall back to the
NFS path, so both the cvs checkout and the hand-maintained
/it-share/rccl-ci/sbatch copies keep working.
check_dmabuf.sh is new: it probes both A/B sides for ROCr resolution and
DMA-BUF support. Beyond liveness it enforces symmetry, because a pair
where reference has DMA-BUF off and candidate has it on does not merely
run slowly -- the sides use different transports, so the candidate scores
a large fake improvement and the gate reports a false negative.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y default Capability gate (run_rccl_ab.sh -> lib/check_dmabuf.sh): Before committing the pinned 4-node allocation to a ~25min detect, verify in ~10s that both A/B sides resolve ROCr and end up with DMA-BUF. Catches two otherwise-silent failures: a side without DMA-BUF hangs every sweep for the full rccl_timeout (a 25min run became 2h39m), and an asymmetric pair produces an invalid comparison that reads as a large improvement. It runs in the batch-script process, which holds the job GPU cgroup -- verified, the .batch step is allocated gres/gpu=8 under plain --exclusive. Wrapping it in srun would NOT work: an srun step without its own --gres gets no /dev/kfd. A hard fail aborts; an inconclusive probe only warns, so a flaky pre-flight can never fail a good PR. Workspace isolation default ON: Introduced opt-in so it could be validated against the live pipeline; that validation passed (build + 4-node detect, 224 sweeps, verdict PASS, prod config md5 verifiably unchanged). Leaving it opt-in would mean the shared-path clobbering returns the moment a second runner is added. RCCL_CI_WORKSPACE=0 still selects the legacy shared trees. workspace.sh query CLI: The workflow must locate THIS run artifacts to render the report. In workspace mode that path derives from the run key; re-deriving it in YAML would be a second source of truth that drifts. --print-artifacts answers it, and returns the legacy shared path when the flag is off. Getting this wrong is not cosmetic -- the shared ab_artifacts still holds an older run report, so a workflow reading the wrong path would post a months-old verdict on a PR as if it were current. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The gate could report a green PASS in situations where it had not actually
compared anything. "0 confirmed regressions" is only a PASS if the detector
looked, and several paths let it not look while still reaching the happy exit:
* A sweep that died mid-run left the group with fewer repeats than configured;
the surviving samples were scored anyway.
* Sizes present on the reference side but missing from the candidate were
dropped from the comparison instead of counted, so a run that truncated
two-thirds of the way through looked clean.
* `set -e` aborted run_rccl_ab.sh at the pytest invocation, so the verdict
block that recounts confirmed regressions never ran.
* In detect mode both sides can resolve to the same build (the prod config
ships identical ld_library_paths); A=A trivially finds nothing.
* The rccl_changed path filter failed open: if the diff against the base ref
errored, the gate skipped the detector and reported success.
Every one of these now produces an explicit NO VERDICT -- neither a pass nor a
regression -- carried end to end: `summary.trustworthy` from the detector, a
top-level `trustworthy` + `untrustworthy_reasons` in the report JSON, an
exit-2 branch in format_report.py, and a distinct message in the PR comment.
Detector accuracy:
* derive_thresholds used 2*p95 of the per-key CV distribution, an outlier
statistic on top of an outlier statistic -- two flaky keys pushed the
large-tier threshold to 12.9%, roughly 53x the measured median noise, wide
enough to pass a real 10% regression. Now median + k*MAD, clamped by a
hand-set max_thresholds ceiling, with a per-collective table so one noisy
collective no longer sets the bar for the rest.
* An uncalibrated collective fell back to the loosest threshold; it now falls
back to the tightest.
* min_bandwidth_floor was a scalar 0.5 GB/s, which excluded the entire small
message band -- the keys most likely to expose a latency regression. Now
per-tier.
Infrastructure:
* submit_and_poll.sh exported the full runner environment to Slurm, including
the Actions token. Now an explicit allow-list via --export=NONE, verified
against planted-secret probe jobs 16126/16127.
* Add a queue budget, an empty-poll confirmation before declaring a job gone,
and a scancel trap so an interrupted poller releases its nodes.
* The build cache keyed only on git rev, so a toolchain or recipe change was a
cache hit. Fold ROCm dist, gfx targets, build_rccl.sh, hipcc and cmake into
the key, memoised.
* Real GC: LRU build cache, .active-aware workspace reaping that ages out
stale markers, orphaned per-run config and Slurm log pruning, and a
--janitor entry point wired to cron. The GC pass runs even with
RCCL_CI_WORKSPACE=0, since the rollback switch is exactly when nobody is
watching the disk.
* The workspace query modes invented a local-<ts>-<pid> run key when no run
identity was in the environment, handing callers a confident-looking path
for a run they could not name. They now refuse with exit 3.
* Right-size rccl_timeout 1800 -> 360 and add a circuit breaker, so a broken
environment fails in minutes instead of burning the full wall clock
retrying. This is the multi-hour runtime complaint.
* umask 002 and EXIT INT TERM HUP traps so a scancel still runs cleanup.
Also version the two configs that decide verdicts. They lived only on NFS, so
the gate's decision boundary had no history.
Detector self-checks in ci/rccl_perf_gate/tests/test_regression_lib.py, kept out
of cvs/tests/ so they do not consume a GPU allocation. 8/8 pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… checks _publish_derived_thresholds ran before the per-group trustworthiness loop, so a control run that tripped the circuit breaker, lost a group to incomplete repeats, or came back mostly inconclusive still wrote its numbers to the shared configs/ab_derived_thresholds.json -- and every later detect run reads that file. A single bad calibration could quietly widen the gate for everything that followed, and the run that did it would report NO VERDICT, so nobody would think to go back and check what it had published. Derive as before, but hold the publish until the checks have run, then write the artifact copy either way and the shared copy only if the run came back clean. Also retune ci_control.json to match ci_detect_prod.json. It had never been updated alongside it, so calibration was measuring a different experiment than the one being gated: min_bandwidth_floor was still a scalar 0.5 GB/s, which excludes the entire small tier -- detect would have gated the small band against thresholds calibrated on no small-band data at all. Timeouts were still 1800s, and mad_k / max_thresholds / require_balanced_samples / max_inconclusive_frac / circuit_breaker_failures were absent. The two configs now differ only in control_mode and the scratch result path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rccl_build.sbatch pinned --nodelist=mia1-p01-g28. That node is one of the four
in the rccl_ci detect reservation (mia1-p01-g[22,26,28,32]), and the workflow
additionally passed --reservation=rccl_ci to the build submit, so the build was
confined to the detect pool twice over.
The build takes its node --exclusive and is submitted with `sbatch --wait`. The
detect job then requests all four. Within a single pipeline run those two are
sequential regardless -- detect `needs: build` -- so the cost is not that a run
serialises against itself. The cost is that for the length of every build a
quarter of the detect pool is held by a job with no business being in it: a
single-node CPU compile, no RDMA, no MPI. sacct shows builds sitting on g28 for
11-13 minutes routinely and 32 minutes once, and during that window anything
that wants four nodes waits -- another PR's detect job, a manual detect run,
rccl-gin.
It is also a hard blocker for the second runner. The entire point of adding one
is to have two pipeline runs in flight; with the build pinned inside the detect
reservation, run A's build would sit on a node run B's detect is waiting for,
and the concurrency we just bought would go straight back out again.
The fix is in two halves:
- rccl_build.sbatch drops the hard --nodelist entirely. Node choice is the
caller's business now, expressed as a reservation; Slurm picks a free
machine from it. Pass --nodelist at submit if you need a specific one.
- the workflow gains SLURM_BUILD_RESERVATION (rccl_dev), separate from
SLURM_RESERVATION (rccl_ci). An empty value means no reservation rather
than inheriting the detect one, so the flag is built into an array and
omitted when unset -- sbatch rejects --reservation="".
Verified with sbatch --test-only: the build now places on mia1-p01-g30 under
rccl_dev, and on mia1-p01-g36 in the general partition with no reservation.
Neither is in the detect set. The runner submits as arravikum@amd.com, who is
in the rccl_dev user list, so the reservation move does not cost the pipeline
its access.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The four timeouts guarding a detect run were 8h (Slurm wall clock), 8h (MAX_WAIT_SEC), 24h (MAX_QUEUE_SEC) and 9h (the GitHub job). That is not a nesting. The 24h queue budget sat outside the 9h Actions timeout, so a job starved in the queue was always killed by Actions first, with a bare "exceeded the maximum execution time" -- and submit_and_poll.sh's own diagnostic, the one that names the starvation and cancels the allocation, was unreachable code. Sized against what the job actually does rather than against a round number. A healthy full-matrix run is ~25min: jobs 16092, 16108 and 16131 took 00:25:29, 00:25:23 and 00:25:34. The slow path is bounded too -- 8 active (collective,dtype) groups, a 360s per-collective timeout and a circuit breaker that abandons a group after 2 consecutive failures cap the all-timeouts case at ~3.2h. So: rccl_ab.sbatch --time 8h00m -> 4h00m Slurm kills; state becomes TIMEOUT MAX_WAIT_SEC 8h00m -> 4h10m backstop if slurmctld does not MAX_QUEUE_SEC 24h00m -> 4h00m never started -> report starvation detect timeout-minutes 9h00m -> 8h30m > 4h00 + 4h10, so it fires last 8h was not a safety margin, it was a hostage window: the job holds four nodes --exclusive and the runner polling it holds one of only two runner slots, so a wedged run took the pinned pool and the whole pipeline down with it for a working day. Anything slower than 4h is a fault to report, not to wait out. The build side had the mirror-image bug: timeout-minutes was 60, exactly equal to rccl_build.sbatch's own --time=01:00:00, leaving zero room for queue time -- a build that waited 20min for a node and then compiled for 13 was killed by Actions while Slurm was perfectly happy. Now 120, so Slurm decides when a build has run too long. And the build step leaked its allocation. `sbatch --wait` does not cancel the job when the client dies, so a cancelled run, a timeout or a runner restart left the build compiling on an --exclusive node until its wall clock expired, with nobody left to read the result. submit_and_poll.sh already had a scancel trap for the detect job; the build now has the same one. --parsable makes sbatch print the job id at submission and then block as usual, so it runs in the background, the id is read out of its output, and `wait` recovers the real exit status -- keeping the exit-code mapping that is the whole reason to use --wait while giving the trap something to cancel. Covered by .work/test_build_trap.sh (fake sbatch/scancel on PATH): build succeeds -> no cancel; build fails -> exit code propagates, no cancel; sbatch cannot submit -> no bogus cancel; step killed with SIGTERM or SIGHUP mid-build -> the allocation is scancelled and the step does not exit 0. 8/8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both GC paths logged only when they deleted something:
(( reaped > 0 )) && ws_log "gc: removed ${reaped} old workspace(s)"
so a janitor run that reclaimed nothing produced no line at all. In the log
file that is indistinguishable from a GC that did not run -- which is the one
question you are reading janitor.log to answer. The same silence would cover a
keep-threshold that has quietly stopped reclaiming anything, a runs/ directory
that moved, and a glob that stopped matching.
Log unconditionally now, with the counts on both sides and the thresholds in
force, so a line is evidence and the numbers are checkable:
gc: workspaces: removed 0 (older than 14d beyond the newest 20), 7 kept
gc: build cache: removed 0, 4 kept (limit 40)
janitor: slurm logs: removed 0 older than 30d, 230 kept
The missing-directory cases say "nothing to reclaim" and name the path they
looked at, rather than returning silently.
(The `&&` construct was not also a set -e hazard, for the record: bash exempts
a failing left operand of &&, and both callers already guard with `|| true`.)
Adds two test files next to test_regression_lib.py. Neither needs a GPU, an
allocation or a cluster, and they live outside cvs/tests/ so the CVS harness
does not collect them into a real perf run:
test_workspace_gc.sh (13 assertions) builds a throwaway RCCL_CI_ROOT under
mktemp for every case, which matters more than usual because the code under
test is `rm -rf` in a loop. Covers: populated root with nothing old enough
(must still report, must not delete); genuinely stale entries (right ones
reaped, right ones kept); empty root (reports, exits 0, does not crash);
workspace mode off (still a no-op, since the janitor forces the mode on for
its own GC pass).
The .active assertion needed care to be worth anything. Marking the run and
then ageing the other directories left the marked one newest, so it was
spared by keep-newest-N and the guard was never exercised -- the test passed
for the wrong reason. It is now old *and* outside the N window, so only the
marker can be saving it.
test_build_submit_trap.sh (8 assertions) fakes sbatch/scancel on PATH and
kills the step mid-build with SIGTERM and SIGHUP, asserting the allocation is
scancelled and the step does not exit 0. Also checks the quiet paths: a
successful build cancels nothing, a failed build propagates its exit code,
and a submit that never got a job id does not scancel something imaginary.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A broken run-block in rccl_perf_regression.yml is invisible until the job
executes it, which on this pipeline means burning a GPU allocation to find a
typo. This parses the YAML, strips the ${{ }} templating that Actions expands
before bash ever sees it, and runs bash -n over each block.
Usage: python3 cvs/ci/rccl_perf_gate/tests/lint_workflow.py path/to/workflow.yml
All 11 run-blocks currently pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The design doc was last touched on 2026-07-17 and predates the whole robustness
pass, so it documented a gate that no longer exists. The corrections, not just
additions:
- threshold estimator was described as safety_factor x p95(CV). It is now
safety_factor x (median + mad_k x MAD), derived per collective rather than
globally, and clamped by a hand-set max_thresholds ceiling. p95 of a CV
distribution is itself an outlier statistic; the doc presented it as if it
were a robust one.
- min_bandwidth_floor was documented as the scalar 0.5 GB/s. It is per tier
now, and the doc did not say that a flat 0.5 excludes the entire small tier.
- the worked example quoted MI350X p95 CV and adopted thresholds of
20/15/7.5%. Replaced with the MI355X calibration actually in force
(derived 12.5/6.1/3.0%, under the 15/8/6% ceiling), naming the job it came
from.
- "61 unit tests" did not match anything countable. Replaced with the
collected numbers: 34 pytest cases in the two lib test files, 8 more in
ci/rccl_perf_gate/tests, 21 shell assertions in the two .sh tests.
- section 12 listed per-collective thresholds as future work. They shipped.
New material: the trustworthiness model (a groups checks, how they propagate to
speriaswamy-amd
added a commit
to ROCm/rocm-systems
that referenced
this pull request
Aug 12, 2026
47 of the last 60 runs of this workflow ended `cancelled`, not `failure` and
not `success`. Only 5 succeeded. That is not a flaky gate, it is a gate that
mostly does not run, and the cause is in this file.
A GitHub Actions concurrency group holds at most ONE pending job. When a third
run arrives, the pending one is cancelled to make room. Both jobs here used a
single global group, so every PR competed for that one slot and evicted
whoever was in it. In the two hours before this was written, six different
authors hit it:
31538101800 queued 21:29:09, cancelled 21:29:44 <- evicted 1s after
31538146420 arrived at 21:29:43
31538542916 queued 21:34:44, cancelled 22:28:10 <- waited 53 minutes to
be told nothing
From the author's side there is no error, no comment and no explanation. The
run simply disappears, and the only recourse is to push again and hope.
Key the groups by PR instead. Cross-PR contention goes away entirely, and the
useful half of the old behaviour survives within a PR: push three times and
the newest still evicts your own older queued run, because they share a key.
Serialising the actual hardware moves to where it belongs -- Slurm, which owns
the pinned nodes and whose queue is unbounded. Slurm makes you wait; it does
not cancel you.
`cancel-in-progress` stays false deliberately. Cancelling a GitHub job does not
cancel the Slurm job it submitted, so a true here would orphan allocations that
keep holding nodes.
Note this trades cancellation for queueing, not for throughput. With one runner
(1 active + 1 queued) and ~25-40min per run against ~2.4 runs/hour arriving,
capacity is roughly break-even and bursts will build a backlog. Real
parallelism needs a second runner with a build-only label; the build needs no
GPUs and no RDMA, just a Slurm submit host.
Also in this commit, because they are all load-bearing for the above:
- Resolve this run's artifact dir by asking `workspace.sh --print-artifacts`
instead of hardcoding the shared ab_artifacts/. The cluster-side detector
now writes to runs/<run_key>/artifacts, so the hardcoded path currently
resolves to a report from an earlier run -- the first run that survives
long enough to render would post a stale verdict on a PR. Reading the
wrong path here is not a cosmetic bug; a stale green is the worst failure
mode a gate has.
- SLURM_BUILD_RESERVATION=rccl_dev, separate from SLURM_RESERVATION. The
build is a single-node CPU compile but takes its node --exclusive, so
running it inside the 4-node detect reservation held a quarter of the
detect pool for its duration. An empty value means no reservation rather
than inheriting the detect one, so the flag is built into an array and
omitted when unset -- sbatch rejects --reservation="".
- A scancel trap around the build submit. `sbatch --wait` does not cancel
its job when the client dies, so a cancelled run or a timeout left the
build compiling on an --exclusive node until its wall clock expired, with
nobody left to read the result.
- Timeouts renested: build 60 -> 120 (60 was equal to the sbatch --time, so
a build that queued for 20min and compiled for 13 was killed here even
though Slurm was happy), detect 540 -> 510 so submit_and_poll.sh's own
budgets fire first and can report *why* rather than being killed with a
bare "exceeded the maximum execution time".
- The PR comment moves into its own always() job. It used to be a step
inside detect, so every earlier failure -- build failing, runner dying,
the lock cancelling the run -- left the sticky comment showing the
PREVIOUS attempt's verdict with no indication it was stale.
- Fail closed on the changed-paths diff. The old form ended in
`|| echo "rccl_changed=0"`, which swallowed a shallow clone or a missing
base ref and reported "RCCL didn't change" -- so detect compared the
prebuilt lib to itself, found nothing, and posted green on a PR that had
never been measured.
- Exclude docs-only churn from the paths filter, and namespace the uploaded
artifact per run+attempt.
The Slurm-side scripts these steps call are in ROCm/cvs#310.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alltoall_perf has been excluded from A/B comparison since 2026-07-13. Job 12389 measured ~5-6x swings between identical A=A repeats in its 128MB-4G band, and with a single shared large-tier threshold that inflated the derived value 0.126 -> 8.41 -- a gate that loose would have missed real large-message regressions in all five collectives, not just alltoall. The recorded unblocking condition was "root-caused or per-collective thresholds implemented". Per-collective thresholds now exist, and the instability does not reproduce. Measured on the 4-node rccl_ci reservation, 7 repeats, 10 groups, 460 keys: job 16368 exploratory, publish disabled 0 A=A false positives job 16382 authoritative, published 0 A=A false positives The 128MB-4G band that caused the exclusion now runs 54-56 GB/s flat, max/min 1.08-1.14 per size, cv_median 0.022. alltoall is no longer the noisiest gated collective: broadcast_perf-d=float has a higher small-tier cv_median (0.085 vs 0.031) and more keys spanning >2x. What changed between July and August is not isolated; the SDK dist was normalised (DMA-BUF symlink chain restored) in that window, which is a plausible but unproven cause. Derived thresholds are stable across the two runs, and alltoall comes in under the ceiling on the authoritative one: AllGather s 0.1000 m 0.0638 l 0.0300 AllReduce s 0.1029 m 0.0586 l 0.0377 AlltoAll s 0.1490 m 0.0699 l 0.0527 Broadcast s 0.1500 m 0.0500 l 0.0300 ReduceScatter s 0.1500 m 0.0500 l 0.0300 Caveat recorded in both config _comment fields: alltoall derived above the max_thresholds ceiling on 16368 (mid 0.0927 vs 0.08, large 0.0748 vs 0.06) and under it on 16382. It sits close enough that if its noise grows the gate will emit false positives rather than silently pass regressions. That is the right failure direction, but it is the signal to re-exclude. Control mode now scores itself with the per-collective table -------------------------------------------------------------------------- Found while checking the above. Detect mode resolves thresholds_by_collective, but control mode applied derived["thresholds"] -- the pooled per-tier dict -- to its own A=A sanity check. So the run whose entire job is to be a faithful dry run of the gate was scoring against numbers production never uses, looser than production for some collectives (large tier 0.0498 pooled against 0.0300 per-collective) and tighter for others. No wrong conclusion was drawn: 16368 was replayed offline through all three gates against the per-collective table and still returned 0. 16382 ran with the fix and its report now carries a distinct threshold per collective per tier (all_gather large 0.0300, all_reduce 0.0377, alltoall 0.0527) where every key previously read 0.0498. Budgets resized for 10 groups -------------------------------------------------------------------------- The circuit breaker abandons a group after 2 consecutive failures at a 360s per-collective timeout, so the all-timeouts bound is groups x 2 sides x 2 failures x 360s. At 8 groups that is 3.2h, inside the 4h wall clock. At 10 it is exactly 4.0h -- not a bound at all, since Slurm would kill the job at the moment the breaker was about to name the group that wedged. rccl_ab.sbatch --time 4h00m -> 5h00m MAX_WAIT_SEC 4h10m -> 5h10m detect timeout-minutes 8h30m -> 9h20m (rocm-systems, separate commit) A healthy 10-group run is ~35min (16368 34:56, 16382 34:52), up from ~25min at 8 groups, so 5h is still ~8x headroom. The nesting table in submit_and_poll.sh and the rationale block in rccl_ab.sbatch are updated to match; both were written to be kept true. The pre-alltoall published thresholds are kept at configs/ab_derived_thresholds.json.pre-alltoall-16132 in case this needs reverting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
speriaswamy-amd
added a commit
to ROCm/rocm-systems
that referenced
this pull request
Aug 12, 2026
alltoall_perf is being re-enabled in the A/B gate (ROCm/cvs#310). That takes the matrix from 8 (collective, dtype) groups to 10, and the circuit-breaker worst case with it: the breaker abandons a group after 2 consecutive failures at a 360s per-collective timeout, so the all-timeouts bound is groups x 2 sides x 2 failures x 360s At 8 groups that is 3.2h, comfortably inside the 4h sbatch --time. At 10 it is exactly 4.0h, which is not a bound -- Slurm would kill the job at the moment the breaker was about to report which group wedged, and the diagnostic would be unreachable code. So rccl_ab.sbatch --time goes to 5h and MAX_WAIT_SEC to 5h10m, and this number has to move with them or it becomes the layer that fires first: rccl_ab.sbatch --time 5h00m Slurm kills; job state becomes TIMEOUT MAX_WAIT_SEC 5h10m backstop if slurmctld never enforces it MAX_QUEUE_SEC 4h00m never started -> report a queue starvation detect timeout-minutes 9h20m > 4h00 queue + 5h10 run, so it fires last A healthy 10-group run is ~35min (Slurm jobs 16368 and 16382, both 00:34:5x), up from ~25min at 8 groups. This is a fault ceiling, not an expected duration. Still a draft: on.pull_request.branches remains pointed at this PR's own branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Robustness pass over the RCCL paired A/B perf-regression gate, from a gap audit of
the whole pipeline. 11 commits. The organising principle throughout:
Several of the bugs fixed here had the same shape — the gate reported green on a run
that had not measured anything. That is worse than no gate, because a reviewer acts on it.
Important
These changes are already live.
/it-share/rccl-ci/cvson the tensorwave clusteris simultaneously this git checkout and the deploy target the GitHub workflow calls,
and it is sitting on this branch. This PR is for review and durability, not to switch
anything on. The companion PR in
ROCm/rocm-systemscarries the workflow half andis required — the two are currently out of sync (see "Companion PR" below).
What changed
The gate can now refuse to answer
A group is trustworthy only if it actually measured: enough repeats, balanced A/B
samples, no excess inconclusive keys, no missing keys silently dropped. That flag
propagates all the way out — per-group →
summary.trustworthy→ top-leveltrustworthy/untrustworthy_reasons→format_report.pyexit 2 and a⚠️ NO VERDICTrender → the workflow's gate step → the PR comment."No usable verdict" is treated as an infra failure, not an advisory finding. The
advisory carve-out exists so a real measured regression doesn't block a PR while the
gate earns trust; it was never meant to cover a run that failed to measure.
Threshold estimation is now robust, per-collective, and bounded
safety_factor × p95(CV). p95 of a CV distribution is itself anoutlier statistic. Now
safety_factor × (median + k·MAD)(MAD scaled ×1.4826).longer inflate the thresholds protecting the others.
max_thresholdsceiling, so calibration can only ever tighten.min_bandwidth_floorwas a scalar0.5 GB/s, which excluded the entire small tierfrom gating. It is per-tier now.
Calibration can no longer poison itself
A control run that fails its own trustworthiness checks does not publish thresholds.
Previously any control run overwrote the shared calibration, so one bad night silently
set the sensitivity of every subsequent PR.
Node contention
rccl_build.sbatchpinned--nodelist=mia1-p01-g28, which is inside the 4-noderccl_cidetect reservation, and the workflow additionally passed--reservation=rccl_ci— confined to the detect pool twice over. The pin is gone;node choice is the caller's business, expressed as a reservation.
Timeouts that nest
The budgets were 8h / 8h / 24h / 9h, which is not a nesting — the outer layer fired
first and the inner diagnostic was unreachable code. Now:
rccl_ab.sbatch --timeMAX_WAIT_SECMAX_QUEUE_SECtimeout-minutesHealthy runs are ~25 minutes.
Per-run workspace isolation
Every run gets its own
runs/<run_key>/: an immutablecvs/worktree snapshot, a realcvs-sbatch/copy, build output, config, artifacts and logs. Nothing writes to sharedstate. This is what makes a second runner safe, and it is on by default.
Other
fails loudly instead of quietly producing slow numbers on both sides.
removed 0and silence lookidentical in a log file, and the reason you read
janitor.logis to find out whetherthe GC ran at all.
Tests
All run on a login node — no GPU, no allocation, no cluster:
test_workspace_gc.shbuilds a throwawayRCCL_CI_ROOTundermktempfor every case,which matters more than usual because the code under test is
rm -rfin a loop.Hardware validation
Slurm job 16132, A=A control on the real 4-node reservation, 25m16s:
Zero false positives across all 8 collective×dtype groups. Derived thresholds
(
median + 3·MAD× 2.0, under a 15/8/6% ceiling):Two hit the ceiling, which is the ceiling working as intended but also worth knowing:
AllGather large and Broadcast small are the noisiest things on this cluster, and real
regressions below those numbers will not be caught.
Companion PR
ROCm/rocm-systems—.github/workflows/rccl_perf_regression.yml. Required. Itcarries the per-PR concurrency fix (47 of the last 60 gate runs were cancelled, not
run) and the per-run artifact path. Without it the workflow still reads the legacy
shared
ab_artifacts/while the detector here writes per-run, so the first run thatsurvives long enough to render would post a stale report.
Known gaps
sbatch --waitfor the detect job is deliberately deferred; the polling loop stays.reservation, not by anything the detector does.