[CI] Add opt-in paired performance check for pull requests (ci:perf) - #2152
Draft
zufayu wants to merge 12 commits into
Draft
[CI] Add opt-in paired performance check for pull requests (ci:perf)#2152zufayu wants to merge 12 commits into
zufayu wants to merge 12 commits into
Conversation
Adds a label-gated, non-blocking performance check. On `ci:perf` it measures the merge-base and the PR head back to back and reports the delta as a PR comment; it never fails the PR and is deliberately not part of `ci:full`. Why a paired measurement rather than a historical baseline: per-image baselines do not exist. Across 52 deduplicated perf runs on the public dashboard, 30 of 39 container images were benchmarked exactly once and only 3 reached three runs, while cross-image comparison spreads 11-21% against 0.57% median CV within one image. Running both commits in the same job holds the machine, the image and the container instance fixed, so the commit is the only thing that differs. Both halves share one container. setup-gpu-container bind-mounts the workspace (-v "$GITHUB_WORKSPACE":/workspace), so switching commits is a host-side checkout the running container picks up immediately -- no second container and no new script duplicating benchmark-tmpl.yml, which cannot be reused here because it runs one job per concurrency level and Actions cannot pin two jobs to one machine. Judging criterion is family linkage rather than a per-configuration threshold: independent concurrency levels moving together is far less likely than any one of them moving, and unlike a sigma gate it needs no estimate of the noise band -- which matters because per-image history is too sparse to estimate one. Levels below c=64 are measured and displayed but never judged; low concurrency manufactures false positives (an entry whose large-concurrency levels sat at their historical peak was flagged at -4.2% purely on c4..c32) and dilutes real ones (an all-levels median of -7.1% was -12.0% restricted to large concurrency), yet hiding them removes the evidence a reader needs to tell a small-batch path problem from noise. Incomplete data never reports clean. An entry that lost a level is `insufficient`, a matrix where entries produced nothing is `partial`, and a shape the median cannot describe is `unclear` -- each says why rather than defaulting to a pass. The family-median threshold is provisional: it was tightened one notch from the value used for nightly time-series monitoring, and is not yet calibrated against a repeated same-commit measurement. Both the code and the step summary say so. This commit ships the smoke configuration (GLM-5.2-FP8, three judging levels, 3 jobs) to prove the mechanics on real hardware before widening to the full five-entry matrix. Reuses check_heavy_ci_gate.sh, catalog.py, atom-bench-container, atom_test.sh and summarize.py; no existing benchmark code is modified.
Contributor
🏷️ CI GuideRuns automatically on every eligible PR before approval:
Heavy model tests:
|
zufayu
marked this pull request as draft
September 7, 2026 08:35
The BASE half checks out the merge-base, which predates this feature and
therefore does not contain perf_check_half.sh. Invoking it as
`.github/scripts/perf_check_half.sh` means the first half deletes the script
before the second half can start:
base: collected 1 result file(s)
...
.github/scripts/perf_check_half.sh: No such file or directory
exit 127
Stage it in RUNNER_TEMP and run it from there, where no checkout reaches.
The BASE half itself was fine -- single-quoted JSON in server_args survived
the stdin hand-off, the server launched, the benchmark ran, GPUs released in
12s, and the result file was collected under the expected name. Only the
second invocation was missing its own entry point.
Adds a regression test that reproduces the exact shape: a repository whose
base commit has no .github/scripts at all, with the script staged outside the
tree, running both halves.
Contributor
Inconclusive -- not enough database
Concurrency is how many requests are in flight at once. Only levels at or above 64 decide the verdict. The rows marked (not judged) were measured and are shown, but excluded from every calculation: at low concurrency the numbers swing enough to both invent regressions and hide real ones. They are here because dropping them entirely would leave no way to tell a small-batch problem from ordinary low-concurrency noise. Main-side context — not caused by this PR. The baseline above was measured on main, and these models have been sliding there. A paired comparison sits on top of that: the delta is honest and the absolute level is not.
Read from the nightly history on the dashboard, over the concurrency levels this check judges. Reported when several levels of one model slide together and TPOT mirrors the move -- a single level's slope carries no information, and across 414 configurations the 14-day change is symmetric enough that amplitude alone cannot separate signal from noise. No model reported enough judged levels. This is not a pass -- treat it as no signal. Coverage gaps: 1 model(s) reported too few judged levels. This check does not block merge. It reports a measured delta between the merge-base and the head commit; deciding whether it is an acceptable trade-off is the reviewer's call. |
The container runs as root against a bind-mounted workspace, so anything it
writes there -- __pycache__, build output -- ends up owned by root. Two things
then fail as the runner user:
- the checkout and `git clean` between the two halves, and
- the *next* job's actions/checkout, which is where it actually surfaced:
##[error]File was unable to be removed
Error: EACCES: permission denied, unlink '.../ATOM/atom/__pycache__/...'
That failure lands on an unrelated job on the same runner, which makes it
easy to misread as flaky infrastructure.
perf_check_half.sh now chowns the tree back through the already-running
container before each checkout, and Clean Up repeats it through a throwaway
container as a safety net for a job that died before either half ran. The
latter mirrors benchmark-tmpl.yml's Clean Up, which removes the same
root-owned directories for the same reason.
Also creates perf-pair/ before writing provenance.json: the step runs with
if: always(), so on a job that skipped both halves it was writing into a
directory that did not exist.
The previous fix chowned the tree from inside perf_check_half.sh, which never
runs when the debris is already there: actions/checkout is the first step to
touch the workspace, it runs as the runner user, and it fails outright.
##[error]File was unable to be removed
Error: EACCES: permission denied, unlink '.../atom/__pycache__/...'
Every downstream step is then skipped, including the cleanup meant to prevent
exactly this. A job that dies early therefore leaves debris that outlives it
and lands on whoever schedules on that runner next -- the failure surfaces on
an unrelated job, one run later, on whichever machines happened to inherit it.
Reclaim ownership before checkout as well. The in-job chown stays: it keeps the
two halves working, and this one keeps the job from failing before they start.
Also defaults the image digest to "unknown" when no provenance file was
produced. `xargs -r` with no input succeeds, so the `|| echo unknown` never
fired and the PR comment rendered an empty pair of backticks.
The first real paired run showed head faster than base on identical inference
code -- +5.9% at c=64, +9.1% at c=128, with TPOT and TTFT agreeing:
c=64 base 9422.63 head 9978.01 tput +5.89% TPOT -5.43% TTFT -9.67%
c=128 base 11834.09 head 12915.19 tput +9.14% TPOT -8.30% TTFT -11.29%
Larger than the -3% the criterion is asked to resolve, and pointing the wrong
way: it hides regressions rather than inventing them. A 6% regression would
have read as flat.
The cause is the shared container. Sharing it is what holds machine and image
fixed across the two halves, but it also carries the JIT/autotune caches the
first run populates -- the same "cold autotune cache" that makes a round
untrustworthy in nightly monitoring. Server startup fell 572s -> 481s and the
benchmark itself 670s -> 636s between the halves, which is the throughput
difference. The per-run warmup already in atom_test.sh (--num-warmups) cannot
reach this: it warms a server, not the caches that outlive one.
Two changes:
- A discarded warmup pass before either measurement, at a tenth of the prompt
count via NUM_PROMPTS_OVERRIDE, so the caches are populated before anything
is recorded. Its results are deleted rather than collected -- a warmup that
reached the judge would be indistinguishable from a measurement.
- The base commit is measured again after head. The distance between the two
readings is whatever drifted while the pairing ran; the baseline becomes
their mean, which cancels drift to first order, and drift beyond the
resolvable threshold yields a new `untrustworthy` verdict rather than a
number nobody should act on.
Warmup addresses the cache; the second base reading bounds everything else
that changes with time, including whatever the warmup fails to cover. Whether
a tenth-length warmup is enough is exactly what the next run measures: the
reported drift is the answer.
The comment showed throughput and TPOT as per-entry summaries; TTFT never
appeared, and the per-level numbers a reader needs to judge the shape were
only in the step summary.
Now one row per concurrency level, with Tput, TTFT and TPOT each in their own
column, a median row per entry, and judging vs reference levels labelled in
place rather than split into separate tables.
Three metrics, not five. Output throughput is total throughput divided by a
constant -- with --ignore-eos and fixed ISL/OSL the token count per request is
fixed, so both are the same measurement scaled by (ISL+OSL)/OSL. Measured on
the first successful pairing:
c=64 output +5.89% total +5.89% ratio 9.035
c=128 output +9.14% total +9.14% ratio 9.003
c=256 output +2.77% total +2.77% ratio 9.036 (ISL+OSL)/OSL = 9.0
ITL and E2EL are likewise recoverable from TTFT and TPOT. Listing them would
spend width without adding a fact, and a reader scanning six columns of which
three are restatements has a harder time finding the one that moved.
The step summary keeps the raw base / base2 / head values alongside the
percentages, so the deltas can be checked against what was actually measured.
atom_test.sh reads ISL, OSL, CONC and RANDOM_RANGE_RATIO from its own
environment, under `set -u`. perf_check_half.sh was not passing them, relying
instead on the container already carrying them -- which it does in CI, because
atom-bench-container injects them via container-env when the container starts.
Started any other way, the benchmark aborts after the model has finished
loading:
.github/scripts/atom_test.sh: line 447: ISL: unbound variable
Passing them on the docker exec is redundant in CI and removes the dependency
on how the container was created. They also join the required-variable checks
at the top, so a missing one names itself instead of surfacing as an unbound
variable from somewhere downstream.
Adds a test asserting the four reach the container: the existing suite could
not catch this, because the docker shim does not read them under `set -u` the
way atom_test.sh does.
Three unrelated fixes, all found by running the check for real. Report columns. `Entry`, `c`, `role` and `reference` were terms invented here; a reader had to reach the legend to learn that a row did not count toward the verdict. Now `Model` and `Concurrency`, with the excluded rows marked `4 (not judged)` in place of a separate column, and `median of judged` instead of a bare `median`. The legend states each metric's direction rather than assuming the reader supplies it. GPU preflight. A container that dies without stopping its server leaves KFD processes holding VRAM -- atom_test.sh's pkill runs inside the container, so once the container is gone nothing reaches them. This workflow starts four servers in a row, so an abort leaves more behind than native ATOM CI, which is why it opts into GPU_PREFLIGHT_KILL_KFD where native CI does not. Observed locally: four workers held 664GB across four cards after an aborted run, and only a host-side kill cleared them. Without this the next job fails to allocate and the failure reads as a slow load rather than a collision. Walrus operators. perf_judge.py parses on 3.6 again. CI runs 3.12, so this never showed there, but the script also has to run wherever someone points it at a pair of result directories -- it failed with a SyntaxError at the end of an hour-long local run, after every measurement had already been taken. Also ignores the artifacts a local pairing run leaves in the tree.
An A/A run -- every phase on one commit, so every delta is noise -- measured
what the warmup leaves behind, on MI308 with DeepSeek-V4-Flash at tp=8:
base 5424.64 tok/s
head 5497.71 tok/s +1.35% (was +5.9% / +9.1% / +2.8% with no warmup)
base2 5493.00 tok/s +1.26%
The warmup removes about 80% of the bias. base2 lands where head does, so what
remains is drift over the run rather than anything specific to the second half:
later readings are faster than earlier ones by roughly that much, whichever
commit they carry. TPOT agrees to within 0.01 points (-1.36% vs -1.37%).
Averaging the two base readings halves the residual, to +0.71%. In threshold
terms that moves the trip point from about -4.35% to -3.71% -- 0.65 percentage
points of sensitivity for a third full measurement, 33% more GPU time per job.
Against regressions worth catching, which run 5% and up, that is not worth
paying on every PR, so the base2 phase goes.
perf_judge.py keeps --base2-dir and its drift handling: adding the phase back
is a workflow edit when a run needs its drift quantified, and the code path is
covered by tests either way.
The threshold constant is no longer described as provisional, because it is
now measured -- but the number that matters is not the threshold. The bias is
systematic and favours head, so it subtracts from any regression rather than
inventing one, and the check trips near -4.35% rather than -3%. Both the code
and the step summary say so; overstating the resolution would be worse than
having a coarse one.
Caveat recorded with it: one run, one shared machine, and drift varied visibly
within it -- the benchmark slowed from 1.07 to 1.65 s/it partway through and
recovered. 1.35% is an order of magnitude, not a constant.
A paired comparison answers "did this change make it worse" and is structurally blind to "main was already broken". If base and head are both 30% down, the delta is zero and the run reports clean -- the one shape where a confident green is exactly wrong. Adds a check on the base measurement itself, against main's recent level from the dashboard. Three gates, all required, following the shape the nightly monitor uses for its highest-confidence class: - magnitude: at least 25% under the median of the last 8 nightly runs - variance: and clear of 2.5x that configuration's own spread, because a fixed percentage condemns a naturally jumpy configuration for behaving normally - corroboration: on at least two judged levels, because one alone is the shape a scheduling blip takes Crimson magnitude only, and the data says why: across 372 configurations the 8-run spread on the dashboard is 7.7% at the median, 13.7% at P75 and 25.1% at P90, all ordinary cross-image variation. Anything finer is indistinguishable from noise. Slow drift on main -- the 5% kind that accumulates over weeks -- needs a time series and a trend criterion, which is a nightly-side job. When it fires it outranks every other verdict, including a trip: a regression measured against a bad baseline is not a regression, and a clean result against one says nothing at all. Also stops the judge reaching the network on its own. It fetched the dashboard whenever --history was absent, which put an outbound request behind an omitted flag and made the failure mode silent. The workflow now fetches it in a named step and passes the file; without it the primary criterion still works, since levels moving together needs no noise estimate, and the summary says which checks are off rather than degrading quietly.
A paired comparison cannot see a level that has been sliding for weeks: base
and head both sit on top of it, so the delta is honest and the absolute number
is not. A reviewer told only "no significant change" reads that as "fine".
Ports the nightly monitor's trend criterion, constants intact, and runs it on
the dashboard file this check already fetches for its noise band. Judged per
family -- one model at one input/output shape, across its concurrency levels --
because a single configuration's slope carries no information: measured across
414 configurations, the 14-day change is symmetric, 18 below -10% against 22
above +10%. Amplitude alone cannot separate signal from noise; several
independent levels sliding together, with TPOT mirroring the move, can.
Against the current dashboard it finds four families, all 3/3 levels and all
mirrored:
GLM-5.2-MXFP4 MTP3 8192/1024 14d -11.6% TPOT +13.2%
GLM-5.2-FP8 MTP3 8192/1024 14d -7.8% TPOT +8.3%
GLM-5.2-MXFP4 8192/1024 14d -7.7% TPOT +8.7%
GLM-5.2-FP8 8192/1024 14d -5.1% TPOT +6.0%
GLM-5.2-FP8 is the model this check measures, so every run of it has been
comparing against a baseline that is itself two weeks into a slide.
Reported in its own section, labelled as main-side and not caused by the PR,
and deliberately outside the verdict: folding it in would blame an author for
something they did not do. Scoped to the models the run measured, so the
reader is not left picking their model out of a list of everything drifting in
the repository.
The weekly anchor the monitor also uses is not ported -- it needs state across
runs, which a per-PR job does not have. The 7- and 14-day horizons work from
the dashboard file alone, and found all four of the above.
Two jobs of one run died on a runner that had filled up:
GLM-5.2-FP8 c=256 System.IO.IOException: No space left on device
'.../action_runner_g42/_diag/Worker_...log'
GLM-5.2-FP8 c=128 The self-hosted runner lost communication with the server
Neither reported a failing step -- every step up to Bench HEAD was green and
the job was killed from outside, which is what a full disk looks like from the
job's side. Only the annotations name it.
This workflow is heavier on disk than the accuracy suites it shares runners
with: pull-policy always fetches a ~47GB image, and one job starts three
servers. atom-test.yaml already guards for this and prunes Docker resources
older than 30 days when the Docker root drops under 500 GiB free; that guard
is mirrored here.
It is not sufficient on its own, though. The disk ran out in the runner's own
work tree rather than Docker's, on accumulated diagnostic logs that nothing
prunes, so old files under _diag are cleared as well. Both figures are printed
before and after, so a job that dies this way next time says why in its own
log rather than only in an annotation.
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.
Adds a label-gated, non-blocking performance check. On
ci:perfit measures the merge-base and the PR head back to back and reports the delta as a PR comment. It never fails the PR, sets no required status check, and is deliberately not part ofci:full.This PR is currently configured for smoke — one entry (
GLM-5.2-FP8, the only target that is not whole-node) across the three judging levels, so 3 jobs instead of 25. It exercises the same runner, the same catalog and the same criterion as the full configuration. Once the mechanics are proven on real hardware, widening is a change to theenv:block at the top of the workflow and nothing else.Why a paired measurement rather than a historical baseline
Per-image baselines do not exist. Across 52 deduplicated perf runs on the public dashboard, 30 of 39 container images were benchmarked exactly once and only 3 reached three runs, while cross-image comparison spreads 11-21% against 0.57% median CV within one image. Running both commits in the same job holds the machine, the image and the container instance fixed, so the commit is the only thing that differs.
Why one container, and why not
benchmark-tmpl.ymlsetup-gpu-containerbind-mounts the workspace (-v "$GITHUB_WORKSPACE":/workspace), so switching commits is a host-side checkout the running container picks up immediately. That removes any need for a second container or for a new script duplicating the launch/benchmark logic —atom_test.shalready does all of it.benchmark-tmpl.ymlcannot be reused: it runs one job per concurrency level, and Actions offers no way to pin two jobs to the same machine.Judging criterion
Family linkage rather than a per-configuration threshold. Independent concurrency levels moving together is far less likely than any one of them moving, and unlike a sigma gate it needs no estimate of the noise band — which matters, because per-image history is too sparse to estimate one.
Levels below
c=64are measured and displayed but never judged. Low concurrency pollutes a verdict in both directions: it manufactures false positives (an entry whose large-concurrency levels sat at their historical peak was flagged at -4.2% purely on c4..c32) and dilutes real ones (an all-levels median of -7.1% was -12.0% restricted to large concurrency). Hiding them from the display as well would remove the evidence a reader needs to tell a small-batch path problem from noise, so they are reported alongside and labelled.Incomplete data never reports clean
An entry that lost a level is
insufficient; a matrix where entries produced nothing ispartial; a shape the median cannot describe isunclear. Each says why rather than defaulting to a pass, and a level that dropped is spelled out even when its family could not be judged.Provisional threshold
The family-median threshold was tightened one notch from the value used for nightly time-series monitoring and is not yet calibrated against a repeated same-commit (A/A) measurement. Both the code and the step summary say so. Calibrating it is a one-constant follow-up.
Scope
New files only; no existing benchmark code is modified. Reuses
check_heavy_ci_gate.sh,catalog.py,atom-bench-container,atom_test.shandsummarize.py.29 unit tests cover the criterion and the pairing mechanics (shimmed
docker, no GPU) and run inpre-checks.yaml.