Repository navigation
ab.py: refuse to measure a host nobody declared - #2032
Conversation
`ab.py` is the outer harness -- it spans arm A, arm B and the null control --
which makes it the exact process the lock must be held by, and it neither
took the lock nor recorded it. The mandate lived in the README, so obeying it
meant remembering to wrap the invocation. Remembering is what failed three
times in one night: the recorded incidents are a peer sampling `ps` in the
gap *between two arms* of somebody's interleaved A/B, a 45% disagreement
between two identical binaries, and a 10-502% run-to-run spread that could
only be described afterwards as "contention".
The gate is ancestry, not liveness, and that is the load-bearing distinction:
* a lock anchored to this process or one of its ancestors spans every arm;
* a lock anchored to a benchmark *child* is released between arms, so it
certifies each arm and protects none of the comparison -- the shape that
produced the gap incident, and one that looks identical to any check
asking only "is a lock held";
* a lock anchored to anyone else is a reason to stop rather than to start.
Refusal is exit 3, before a single arm is launched, and the message carries
the wrapping command rather than pointing at a doc. Every unprotected state
is distinguishable (`free`, `stale:`, `expired:`, `unusable`, `unknown`), so
"nobody has taken it" cannot be confused with "the holder died under it" --
the distinction #1989 added to the tool itself. An unreadable or missing
`hostlock.sh` refuses too: fail-closed, because a gate that opens when its
instrument breaks is not a gate.
Rows now carry the declaration: `host_lock`, `lock_owner`, `lock_anchor_pid`,
`runnable_at_start`, `contended`. The label covers the whole window -- the
lock is read again at the end and a run that changed hands is stamped
`changed` rather than named after whoever held it last -- so a contaminated
CSV is self-identifying weeks later instead of depending on someone
remembering the night it was taken.
`--unlocked` is the escape hatch and it marks what it produces
(`unlocked:free`), because the failure mode worth preventing is not an
unlocked smoke test, it is an unlocked smoke test whose numbers are quoted
later as if they were protected.
Seven mutations are each killed by a named cell, including the two that
survived the first battery: renaming the `host_lock` column (the assertion
matched a substring of the renamed one) and dropping the end-of-window
re-read (the logic was inline in `main`, so nothing could reach it -- now
`window_label`). The end-to-end cells drive the real `hostlock.sh` against a
stub arm that prints a result line and exits, so they cost no measurable CPU
and need no quiet host, which is what lets them run in CI.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… column that is always unknown Three findings from review of the A/B driver's lock gate. A run whose second provenance read came back empty was stamped `changed`. That asserts a specific fact -- custody moved -- about a run that may have been protected end to end, and it makes a real handoff indistinguishable from a timeout on the final read. It now says `unverified-end`: not evidence of a handoff, not evidence against one. `contended` was emitted on every row as the literal string `unknown`, because hostlock only computes it against `--expect-runnable N` and there is no honest N on a shared host (#1802). A column that is constant is not data, and a column named `contended` that always says `unknown` invites someone to read the absence of a `yes` as a `no`. Dropped rather than shipped as furniture. `--unlocked` exists so an unprotected run cannot be quoted later as a protected one -- reasoning entirely about our own labels. It does not extend to a box a peer has declared, where the damage lands on their measurement and no label of ours can reach it. It now refuses `foreign:`. The mapping from provenance key to column name is now a pure function with its own table-driven test, using distinct values so that a column reading its neighbour's key is caught; the reviewer noted a field swap survived the old suite, which asserted presence rather than value. Also recorded, in `lock_verdict`, why comparing pids without start times is safe here: the test sits behind `state == "HELD"`, which hostlock reports only after matching pid *and* /proc field-22 start time, so a recycled pid arrives as `STALE` and is refused. That coupling is load-bearing and invisible in the expression. Refs #1803 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks — all three actionable items landed in SHOULD-1 — a failed end-read was stamped SHOULD-2 — NIT-3 — The eighth mutation — NIT-1 — the pid/start-time coupling. Documented in NIT-2 — workflow home. Left as is for now, and I'd rather say why than quietly skip it: making Battery re-run after the edits — 7 mutations including the two new ones, no survivors. Suite 20/20.
No |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2032 +/- ##
==========================================
+ Coverage 80.24% 80.94% +0.69%
==========================================
Files 409 425 +16
Lines 189791 209135 +19344
Branches 189791 209135 +19344
==========================================
+ Hits 152307 169292 +16985
- Misses 32059 34183 +2124
- Partials 5425 5660 +235
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
… share (#2047) `73c76458c` made the host lock mandatory for saturating runs, with the outer harness holding it across every arm. #2032 mechanised that for `ab.py`. Every other driver still depended on somebody remembering to type the wrapper — the same class of guarantee as checking `ps` for a running bench binary, and it fails the same way. That is what #1803 was filed about: a peer looked at a quiet host and started a sweep in the gap between two arms of an interleaved A/B. Nobody was careless; the check could not give the answer it was being asked for. **`sweep_decode.py` is the one that mattered most**, and its own docstring says why. It aggregates with the min of the per-run min *"because on a shared machine the min is the only statistic that is not partly a measure of the other tenants."* That was honest when it was written — before the lock existed — but it is a way to **survive** contention rather than exclude it, and it is blind to an SMT sibling and to steady external load in exactly the way the docs say per-run CPU efficiency and A/A nulls are. It is also the most saturating thing we run: a thread sweep to 16 or 32 is the least defensible thing to put beside somebody else's measurement. **What changed** - New `scripts/ort_ab/hostlock_gate.py` — the gate from #2032, moved unchanged. All 20 of `test_ab_lock.py`'s existing cells pass against the extracted version without edits, which is the evidence that the move is behaviour-preserving. - `require(command, unlocked=False)` takes the **caller's own** command, so the remedy prints the line to paste. A wrapper pasted for the wrong script does not span the arms, which is the failure the whole gate exists to prevent. - `sweep_decode.py` gains the gate, a `--unlocked` escape hatch, a `host_lock=` banner and a `host_lock` column on every data row. **Two drivers, two different honest answers at the end of the window.** `ab.py` buffers its rows, so a custody change is stamped onto them. `sweep_decode.py` prints as it goes and cannot relabel what is already on the screen — so it exits **4** and says the rows above span the change and should be discarded. A sweep whose lock changed hands is not half-good data: the thread counts above and below the change were compared across it. **Tests — 23 cells.** New: a deterministic mid-run handoff (the stub bench rewrites `owner=` in the scratch lock while the sweep runs, which is what a takeover looks like to the end-of-window read), a locked run asserting the label reaches the **data row** and not only the banner, and a refusal that must name `sweep_decode.py` rather than `ab.py`. Eight mutations, no survivors: | mutation | outcome | |---|---| | sweep never gates | killed | | sweep always `unlocked=True` | killed | | label dropped from the data row | killed | | mid-run handoff silently tolerated | killed | | refusal returns instead of exiting | killed | | `remedy()` ignores its caller's command | killed | | `--unlocked` overrides `foreign:` | killed | | ancestry test dropped from admission | killed | **Not in scope:** `crates/onnx-runtime-ep-cpu/benches/acc0_*.py` are Sebastian's and are still unwired — the audit is in #2043, and the module is now available for them. `gen_gqa.py` appeared in that audit's first pass and is a fixture generator, not a harness; corrected there. Local: 23/23, `py_compile` clean, workflow YAML parses. No `--admin`, no bypass — `merge_when_green.sh` waits for the required contexts. Refs #2043 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Why
scripts/ort_ab/ab.pyis the outer harness: it runs arm A, arm B and the null control in an interleaved loop. That makes it the exact process the host lock must be held by — and it neither took the lock nor recorded one.The requirement lived in the README, so complying meant remembering to wrap the invocation. Remembering is what failed, three times in one night:
ps, saw no benchmark process, and started a sweep in the gap between two arms of somebody else's A/B;matmul/smallshowed a 10–502% run-to-run spread that could only be characterised afterwards as "contention".Since #1806 the lock is mandatory for saturating runs. This makes the mandatory thing mechanical.
The gate is ancestry, not liveness
This is the whole design, and the distinction is not cosmetic:
mine:<owner>A check that asked only "is a lock held" would pass the middle row, which is the one that already caused an incident.
Refusal is exit 3, before a single arm launches, and the message carries the wrapping command instead of pointing at a doc.
Fail closed, and distinguishably
Every unprotected state gets its own label —
free,stale:,expired:,unusable,unknown— so "nobody took it" cannot be read as "the holder died under it". A missing or brokenhostlock.shreads asunknownand refuses: a gate that opens when its instrument breaks is not a gate.The row carries the declaration
host_lock,lock_owner,lock_anchor_pid,runnable_at_start,contendedon every CSV row. The label covers the whole window: the lock is read again at the end, and a run that changed hands is stampedchangedrather than named after whoever happened to hold it last. A contaminated CSV is then self-identifying weeks later, instead of depending on someone remembering the night it was taken.--unlockedruns anyway and stampsunlocked:<state>. The failure worth preventing is not an unlocked smoke test — it is an unlocked smoke test quoted later as if it had been protected.Anti-vacuity
Seven mutations, each killed by a named cell. Two survived the first battery and are the reason the tests look the way they do:
test_a_declaration_held_by_a_peer_stops_the_runtest_an_unlocked_matrix_is_refused_before_any_arm_runstest_an_unreadable_lock_is_refused_rather_than_assumed_freehost_lockcolumntest_the_wrapped_invocation_is_admitted_and_stamps_every_row— survived at first: the assertion was"host_lock" in header, which is a substring ofhost_lock_unused. Nowcsv.DictReader, by column name and valuetest_a_lock_that_changed_hands_is_not_reported_as_held_throughout— survived at first: the logic was inline inmain, so no cell could reach it. Extracted aswindow_label--unlockedrowstest_unlocked_by_request_runs_but_marks_the_rowsCost
The end-to-end cells drive the real
hostlock.shagainst a stub arm that prints a result line and exits — no benchmark, no quiet host, ~0.4 s for all 16 cells. That is what makes it safe to add to theHost lockworkflow (path-filtered, non-required, same as the conformance suite).Validation
python3 scripts/ort_ab/test_ab_lock.py— 16 pass.scripts/ort_ab/ab.pyand the test.