Fix/389 Harness baseline/opt comparison direction and timeout - #416
Conversation
…ble copy
The isolated unittest timed the candidate against baseline_src/ -- a second
copy of the source INSIDE the task dir, selected by a meta.baseline_callable
string the generated harness bound by hand. Both trees held byte-identical,
identically-named code, so "which one is the baseline" was a coin flip that
failed SILENTLY: both legs are correct implementations, so correctness still
passed and only the ratio inverted (a real 1.055x candidate was reported as
0.96x and rejected). It also let mode=author time optimized-HIP against its own
naive-HIP seed (15.7x isolated, ~0% e2e).
Both legs now run the SAME leg_runner.py + the SAME cases.py, differing only in
the overlay on PYTHONPATH:
baseline <task>/baseline_overlay frozen CURRENT_OVERLAY = install + every
already-accepted kernel (== the e2e gate's
ref leg)
candidate <task>/_cand_overlay that same stack + exactly ONE entry built
from kernel_src/ (meta.candidate_bind)
Direction becomes a property of the environment rather than of a name, so
speedup = baseline_ms/optimized_ms cannot invert. assert_legs_differ refuses to
measure until both legs provably resolve target_callable to different code AND
the baseline resolves outside the task dir.
Also fixes the ~8% loader gap from comparing a from-file loader against the
installed package: both legs now import through the identical mechanism.
Side effects: the extractor finally receives CURRENT_OVERLAY (every other role
already did), so isolated speedup and e2e_delta_pct share a denominator and the
Amdahl cross-check is meaningful; and two accepted kernels in the same module
COMPOUND instead of the second silently reverting the first, because kernel_src/
now starts from the module as the current stack resolves it.
Tests: test_leg_runner.py (the four leg_runner modes against a stub task dir)
plus the two-leg section of test_harness_lib.py (_run_leg PYTHONPATH order,
build_candidate_overlay, assert_legs_differ's two refusals). test_leg_runner.py
is added to the explicit pytest list in ci-l0-checks.yml -- that list is what
decides whether a test file ever runs.
NOT YET VALIDATED ON GPU -- the torch-dependent paths (measure_legs timing,
oracle .pt roundtrip) have never executed.
Refs #389
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…udget
Shape capture for the MiniMax-M3 MoE kernel died at "Server not healthy within
360s", so the most valuable editable target in the run was never authored.
bench_e2e.sh graded its /health budget by "is an overlay set" (72*5s = 360s),
a budget that exists to reject a WEDGED CANDIDATE overlay -- but shape capture
sets OVERLAY_PYTHONPATH too, so a legitimate slow TP4 cold start inherited a
fail-fast deadline.
Grading the budget by purpose (capture -> 1800s) would still guess a constant
per model, and would still treat "slow" as a proxy for "wedged". Wedged shows
up as SILENCE, not as elapsed time. Judge on that evidence instead:
* keep waiting while the server log keeps growing; declare death only after
STALL_WINDOW_SEC (600) with no new bytes. Both backends print continuously
through dist init, shard load and graph capture, so no-new-bytes IS wedged.
This rejects a wedged candidate SOONER than 360s and lets a legitimately
slow TP4 cold start run as long as it keeps moving.
* the absolute ceiling is now DERIVED from the checkpoint size
(600 + 8*GB/TP, clamped 900..7200; 1800 for an HF id) and is only a
backstop against a server that spins printing forever. Not a prediction.
SERVER_STARTUP_TIMEOUT_SEC overrides it and is NAMED in the failure.
* retry ONCE, and only for unambiguously transient faults
(engine_core_timeout, dist_init_fail). OOM is never retried: lowering
MEM_FRACTION would hide "this candidate does not fit", which is a result.
* always write OUT_DIR/server_start.json {status, attempts, reason,
phase_hint, wait_sec, ceiling_sec, ...} so a failed cold start is a
readable reason downstream, not an empty task dir that reads as
"authored and found no gain".
OVERLAY_KIND / CAPTURE_HEALTH_TRIES / OVERLAY_HEALTH_TRIES no longer select a
budget -- OVERLAY_KIND survives as a provenance label only. That also fixes the
callers that never set it (director.md's final validation leg silently got the
360s fail-fast budget).
Verified without a GPU via a throwaway stub adapter (ADAPTER= override),
STALL_WINDOW_SEC=5:
prints every 1s, healthy at 25s -> status=ok attempts=1 wait_sec=25
(any fixed budget under 25s kills it)
fully silent -> reason=stalled attempts=1 after 5s
"EngineCore initialization timed out" -> reason=engine_core_timeout attempts=2
"HIP out of memory" -> reason=oom attempts=1 (no retry)
ceiling_sec derived correctly from a 3GB fake checkpoint at TP4 (606 -> 900).
NOT yet exercised on a real GPU cold start.
Refs #389
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cated prose - bench_e2e.sh: replace the checkpoint-size-derived CEILING (a du -sbL on the model dir plus clamps) and the dead HEALTH_TRIES alias with a single 7200s backstop overridable by SERVER_STARTUP_TIMEOUT_SEC. The progress-based wait is what decides liveness now, so the backstop no longer has to be tuned. - bench_e2e.sh: drop the SERVER_START_ATTEMPTS relaunch loop. #389 does not ask for retry, and the misclassification it papered over is what the stall window fixed. Failure CLASSIFICATION stays: engine_core_timeout / dist_init_fail are still written to server_start.json for the caller to act on. - kernel_extractor.md: fold two restatements of the fake-15.7x rationale into references to the TWO LEGS section that already states it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two conflicts, both from the teardown-contract work that landed on main in parallel: * ci-l0-checks.yml -- both sides appended to the explicit pytest list. Kept both: test_leg_runner.py plus test_server_teardown.py and test_bench_e2e_teardown_lookup.py. * bench_e2e.sh -- main still carried the OLD fail-fast health loop (HEALTH_TRIES=180, OVERLAY_HEALTH_TRIES=72) that this branch replaces, and had ALSO added `server_record_identity "$SERVER_PID"` right before it. Took this branch's progress-based wait and kept main's identity freeze ahead of it -- the two are orthogonal: one decides WHEN the server is declared dead, the other decides WHO teardown is allowed to signal. One silent break the merge did NOT flag: main renamed cleanup() to server_teardown() and moved it into server_teardown.sh, so this branch's startup-failure branch was left calling a function that no longer exists. Changed to `server_teardown; SERVER_PID=""`, which is the intended pairing -- server_teardown early-returns on an empty or already-dead SERVER_PID, so clearing the pid afterwards makes the EXIT trap a no-op. Merged tree passes all four affected test files (256 tests): test_bench_e2e_teardown_lookup 4, test_server_teardown 18, test_leg_runner 25, test_harness_lib 209. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment cleanup: remove the bug-archaeology narration that restated why the old baseline_callable design was wrong (harness_lib._run_leg banner, the two test-file banners, leg_runner's module docstring) and keep only what the reader needs to use the mechanism. Drop comments that restate the code they sit above (bench_e2e.sh progress-token / reason-refinement, overlay_setup --from). Scope: delete the dead OVERLAY_KIND variable (write-only leftover of the removed per-purpose startup budget) along with its server_start.json field and the kernel_extractor.md call site; drop the unrelated ISL/OSL/CONC guidance and the unverified "two accepted kernels COMPOUND" claim; reduce engineer.md rule 1 to the filenames it needed to add. Translate a stray Chinese token in an English role prompt. No behavior change. bash -n / py_compile / node --check pass; the L0 pytest suite was not re-run locally (no pytest available in this environment). Refs #389 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous cleanup dropped these as "scope", but on review both describe a failure mode the agent walks into on its own rather than background rationale: - kernel_extractor: the ISL/OSL/CONC rule sits next to "REPEATS=0 drives a short window", which invites shrinking OSL to speed the capture up. That freezes an out-of-regime oracle and every downstream speedup is then measured on shapes the deployment never serves. - engineer rule 1: the consequence clause is the anti-reward-hacking half of the rule — an agent with an incentive to move the denominator needs to know edits outside kernel_src/ are both futile (pathspec-filtered) and fatal (discarded at the e2e gate), not just forbidden. The other five prompt trims stay out: their imperatives are intact and only the rationale was removed. Refs #389 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
iraj465
left a comment
There was a problem hiding this comment.
Reviewed the code, not just the description. The core design is right and I'd like to see this land. Making leg identity a property of the environment rather than of a name is the correct fix, and assert_legs_differ is the part that makes it stick — refusing to measure until both legs provably resolve to different code, and the baseline resolves outside the task dir, turns "don't invert the ratio" from a convention into an invariant.
Worth stating: measure_legs gives serving_weighted_speedup its first production producer. That function has been in harness_lib.py with zero non-test Python callers; the per-bucket per_case this emits is what finally feeds it.
Five things before merge, one of which I think is blocking.
1. Blocking — measure_legs takes n=1 per leg per bucket
b = _run_leg(task, base, "time", bucket=sig, ...)["cases"]
o = _run_leg(task, cand, "time", bucket=sig, ...)["cases"]One measurement per leg, and — correctly — in a fresh process each time. But fresh-process variance is exactly the variance you cannot average away inside the process.
I measured this today on MI355X gfx950, 5 fresh-process replicates of the same GEMM task:
| bucket | CV across fresh processes |
|---|---|
| prefill M=8192 | 0.09 – 0.47% |
| decode M=64 | 0.12 – 2.42% |
| decode M=64, aiter | 13.7% |
The motivating case in the description is a 1.055x candidate. At ~2.4% per-leg CV, a single unpaired B-then-C pair puts 1.055x at roughly 2 sigma — recoverable, but not reliably, and the tail backend sits at 13.7%. The direction bug is fixed; the resolution is not yet enough to trust the number that bug was corrupting.
Suggest reps (default 3) and interleaving B,C,B,C,B,C per bucket rather than one B then one C, then reporting median plus the spread. Interleaving matters more than the count — it makes any drift during the sweep common-mode.
2. _run_leg timeout can leave a process on the GPU
subprocess.run(..., timeout=timeout) kills only the direct child. If the leg is mid-kernel or has spawned any helper, the GPU allocation survives the raise, and the next leg then measures against a busy device — which reads as a slowdown attributable to nothing.
start_new_session=True plus os.killpg on TimeoutExpired closes it. Note os.kill(pid, 0) will report a zombie as alive, so check reaping too.
3. oracle mode assumes the op returns a bare tensor
out = call(shape["make_inputs"](rng))
blob[...] = out.detach().clone().cpu()Attention kernels commonly return a tuple (out, lse). This raises AttributeError and, via _run_leg, surfaces as a generic non-zero-exit RuntimeError rather than "this op returns a tuple". A small normalize over tuple/list would cover it.
4. unittest.py is declared IMMUTABLE but is still agent-authored
kernel_extractor.md:30 calls the driver immutable, and :314 tells the agent to make exactly one measure_legs call. That is the right contract — but it is enforced by prompt. Since the file is now fully generic (it only needs TASK and meta), shipping it as a real file next to leg_runner.py and copying it in would make the contract structural, and would remove the last place the direction bug could be reintroduced by generation drift.
5. Minor
- Log-byte growth as the liveness token is defeated by a retry loop printing the same error forever. The 7200s ceiling does backstop it, so this is a note, not an objection — but
stalledvsceiling_exceededwill then misreport which one happened. - The
FATALin the fatal-marker grep is broad enough to match a log-level legend line. Anchoring it would avoid a falsefatal_marker.
Relationship to the other open PRs
This is complementary to #415 (issue #417), not overlapping: #416 fixes which leg wins, #415 fixes at what shape it is timed. #416 does not touch op_bench.py, and #415 does not touch the isolated-unittest path. I trial-merged them — the only conflict is an append-collision at the end of tests/test_harness_lib.py, trivially resolved.
Sequenced with #384 (measured m_buckets rather than guessed), the three are one chain: right shapes → timed at those shapes → credited to the right leg. None is sufficient alone.
… grep Three of the five points from @iraj465's review. The measurement-resolution point (reps/interleaving) is not here — see the PR thread. 1. A timed-out leg could leave a process on the GPU. `subprocess.run(timeout=)` kills the direct child only, so anything the leg spawned survived the raise still holding its allocation, and the NEXT leg then timed against a busy device — a slowdown attributable to nothing, in the one code path whose whole job is to compare two legs fairly. The leg now runs in its own session and a timeout SIGTERMs then SIGKILLs the whole group. Death is confirmed by `proc.wait()`, not `os.kill(pid, 0)`, which a zombie still answers. 2. The oracle assumed the op returns ONE bare tensor. Attention entries commonly return `(out, lse)`. The review found the `out.detach()` in `leg_runner`, but the same assumption sat on the consumer side too, and there it was worse: `correct()` catches its own exceptions, so a tuple-returning candidate was not erroring — it was being reported INCORRECT and silently rejected. Fixing only the recorder would have moved the failure, not removed it. So both ends now handle tensor / tuple / list / dict / namedtuple: `_snapshot` records the whole structure, `correct` compares component-wise (all must pass, worst error reported, count mismatch is a failure), and the recorded oracle is moved onto the candidate's device component-wise. Scalars riding along (`(out, n_tokens)`) are carried but not compared. `kernel_extractor.md` now tells the agent to hand the entry point's output back WHOLE — unwrapping to the first tensor drops the rest from every gate, silently. 3. The startup fatal-marker grep matched a bare `FATAL`, which a `--log-level` legend line also matches — killing a healthy cold start. Anchored to an emitted record: `[FATAL]` or a `FATAL:` prefix. Tests: +9 in test_leg_runner (snapshot shapes + oracle end-to-end), +21 in test_harness_lib (process-group kill incl. a real-grandchild survival test, component-wise correctness, recorded-tuple parity). The fake subprocess module grew a `Popen`. 226 + 34 pass; `bash -n` clean. Refs #389 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The blocking point from @iraj465's review: `measure_legs` took n=1 per leg per bucket, and fresh-process variance is exactly the variance `time_op`'s in-process repeats cannot average away. Their gfx950 replicates put a decode bucket at ~2.4% CV and an aiter one at 13.7% — so the 1.055x candidate this PR exists to recover sits at roughly 2 sigma of a single unpaired pair. Direction was fixed; the resolution behind it was not. Two changes, only one of which costs anything: - INTERLEAVE. A rep is now a B,C PAIR, never all Bs then all Cs. Drift over the sweep lands on both legs and cancels instead of accruing to whichever ran later. Free — identical subprocess count. This is the half that matters most, and the review said so. - REPEAT ONLY WHERE IT CHANGES THE ANSWER. `max_reps=3` is a ceiling, not a plan. After each pair the RUNNING median ratio is tested against `undecided` (0.95–1.10 by default); once outside, the verdict cannot plausibly flip and the bucket stops. A 2.3x bucket costs one pair exactly as before; a 1.05x bucket spends the budget. Preferred over the flat `reps=3` the review suggested: fresh-per-bucket is deliberate here, so every extra rep pays another cold torch import. Flat reps would have tripled the cost of every bucket to buy resolution on the few in doubt — on a 17 model x backend batch that is not a rounding error. The adaptive form spends the same tokens where the review's data says the noise actually is (small-M decode), and nothing where it isn't (prefill, ~0.1-0.5% CV). `baseline_ms`/`optimized_ms` are now the median over the pairs run, so one unlucky cold process cannot carry a bucket. `reps` and `speedup_spread` ride along in per_case so a self-disagreeing bucket is visible rather than averaged into silence; `serving_weighted_speedup` reads per_case by .get() and is unaffected. Tests: +11 in test_harness_lib — decisive costs one pair (both directions), an undecided bucket spends the budget, B,C,B,C ordering, median not first, spread reported, mid-sweep decisiveness releases the budget, per-bucket not shared, max_reps=1 restores the old shape, band is caller-controlled. The pre-existing measure_legs tests pass UNCHANGED, which is itself the evidence that the common decisive path did not get more expensive. Refs #389 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — 1, 2, 3 and 5 are fixed in 1 (blocking). Took the interleaving — it's free and makes drift common-mode. Didn't take flat 2. 3. You found the recorder; the same assumption was on the consumer side and worse there — 5. Anchored to 4. Agree with the direction, but not this PR: |
|
Re-reviewed at Three things I found on the closer pass. None blocks merge.
Merge order with #415I trial-merged both into current Whichever lands second picks up that one-file resolution. Happy for #416 to go first. |
…-and-startup # Conflicts: # e2e_workflow/scripts/tests/test_harness_lib.py
Fix issue #389
Bug 1 — the isolated speedup could point the wrong way
The isolated unittest timed the candidate against
baseline_src/, a second copy of thesource inside the task dir, selected by a
meta.baseline_callablestring that thegenerated harness bound by hand. Both trees held byte-identical, identically-named code,
so "which one is the baseline" was a coin flip — and it failed silently: both legs are
correct implementations, so correctness still passed and only the ratio inverted. A real
1.055x candidate was reported as 0.96x and rejected. The same hole let
mode=authortimeoptimized-HIP against its own naive-HIP seed: 15.7x isolated, ~0% e2e.
Both legs now run the same
leg_runner.pyover the samecases.py, differing onlyin the overlay on
PYTHONPATH:<task>/baseline_overlayCURRENT_OVERLAY= install + every already-accepted kernel (identical to the e2e gate's ref leg)<task>/_cand_overlaykernel_src/(meta.candidate_bind)Direction becomes a property of the environment rather than of a name, so
speedup = baseline_ms / optimized_mscannot invert.assert_legs_differrefuses to measureuntil both legs provably resolve
target_callableto different code and the baselineresolves outside the task dir.
This also closes the ~8% loader gap from comparing a from-file loader against the installed
package — both legs now import through the identical mechanism.
Two consequences worth calling out:
CURRENT_OVERLAY(every other role already did), soisolated speedup and
e2e_delta_pctshare a denominator and the Amdahl cross-check meanssomething.
reverting the first, because
kernel_src/starts from the module as the current stackresolves it.
Bug 2 — startup budget graded by purpose, not by evidence
Shape capture for the MiniMax-M3 MoE kernel died at
Server not healthy within 360s, so themost valuable editable target in the run was never authored.
bench_e2e.shgraded its/healthbudget by "is an overlay set" (72 × 5s = 360s) — a budget that exists to reject awedged candidate overlay. Shape capture sets
OVERLAY_PYTHONPATHtoo, so a legitimateslow TP4 cold start inherited a fail-fast deadline.
Raising the constant per purpose (capture → 1800s) would still be a guess per model, and
would still treat slow as a proxy for wedged. Wedged shows up as silence, not as
elapsed time. So judge on that:
STALL_WINDOW_SEC(600) with no new bytes. Both backends print continuously through distinit, shard load and graph capture, so no-new-bytes is wedged. This rejects a wedged
candidate sooner than the old 360s while letting a legitimately slow cold start run as
long as it makes progress.
SERVER_STARTUP_TIMEOUT_SECand named in the failure message. It is a backstop against aserver that spins printing forever, not a prediction — the progress wait is what decides
liveness.
OUT_DIR/server_start.json(status,reason,phase_hint,wait_sec,ceiling_sec, …), so a failed cold start is a readable reason downstream instead of anempty task dir that reads as "authored and found no gain". Classification
(
engine_core_timeout,dist_init_fail,oom,died_early,stalled) is preserved forthe caller to act on.
is exactly what the stall window fixes. OOM in particular must not be retried at a lower
MEM_FRACTION— "this candidate does not fit" is a result.OVERLAY_KIND/CAPTURE_HEALTH_TRIES/OVERLAY_HEALTH_TRIESno longer select a budget;OVERLAY_KINDsurvives as a provenance label. That incidentally fixes callers that never setit —
director.md's final validation leg was silently getting the 360s fail-fast budget.Unit tests
test_leg_runner.py(new, 423 lines) — the fourleg_runnermodes against a stub task dir.test_harness_lib.pytwo-leg section —_run_legPYTHONPATHordering,build_candidate_overlay, and both ofassert_legs_differ's refusals.test_leg_runner.pyis added to the explicit pytest list inci-l0-checks.yml; that listis what decides whether a test file ever runs.
e2e validation on real hardware
Both fixes have since driven a full multi-model e2e batch on gfx950, across both vLLM and
sglang — 17 model × backend workflows run to completion.
Bug 2 — startup
427
server_start.jsonrecords were written (378ok, 49failed):wait_sec > 360reason=stalledat 155s on a candidate overlay — wedged rejected sooner than the old 360soomdied_earlyphase_hintline lifted from the server logBug 1 — leg direction
The two-leg path is what actually ran:
baseline_overlay+_cand_overlayare present acrossthe batch and zero
baseline_src/directories remain.leg_runner.pywas staged into 196task dirs and
--mode oraclewas invoked 843 times. 353 measurement records carry realbaseline_ms/optimized_mspairs, someasure_legstiming and the oracle.ptroundtripare both exercised (including a 1.7 GB
reference_io.ptreplayed for MiniMax-M3).The direct evidence that direction is now sound is the Amdahl cross-check — every accepted
kernel's isolated speedup and its measured e2e delta agree in sign and rough magnitude. The
pathology this PR fixes (15.7x isolated against ~0% e2e) does not appear anywhere in the batch:
kernel_gemm_xdl_cshuffle_v3(dense fp8 a8w8)dense_fp8_a8w8_blockscale_gemmfp8_a8w8_blockscale_bpreshuffle_GEMM(qkv/o)matmul_ogs_mxfp4_grouped_fused_moeEnd-to-end throughput
Workflows that reached a validated win with output parity:
The remaining workflows completed as
validated_no_win— the harness ran, measured, andcorrectly declined to claim a gain, which is the outcome this PR is meant to make trustworthy.