Repository navigation
npu-window11: take the tolerance from a measured control and name the statistic (closes #834) - #880
Conversation
… statistic (closes #834) The 2 % ms/launch tolerance had no measurement behind it: this box moves the same binary against itself by up to 7.7 % per prof row (engine#834's control: 8 alternating rounds, sha256-identical binary, medians spanning 1.008-1.077 with the two summary statistics disagreeing in sign on 5 of 9 rows). A tolerance nobody can defend is worse than none, because closes have been citing 10 % while the gate asserted 2 %. --compare now refuses to run without a tolerance source: either --tol PCT (a number you can defend, printed as such) or --control C1.log C2.log, the same-binary pair whose own spread becomes the tolerance, rounded up to the next whole percent. Every row prints both statistics and the header names the one the tolerance applies to: ratio-of-medians (median of each side, what the script printed before) or median-of-ratios (paired per round, which is the honest one for a no-regression claim and what the tolerance uses when both logs carry rounds). A control whose rows all move the same way is reported as a POSITION BIAS - the second serve of a round paying for the BO/ELF warm-up - and the fix for that is the driver alternating A B / B A inside a round, which is now said in the output and in docs/npu.md rather than left to be rediscovered. --self-test writes the fixtures itself and checks the policy end to end: refusal without a tolerance, an explicit tolerance honoured and still failing a 30 % row, a control pair deriving 8 % from its own 7.7 % worst row, and the paired statistic being named when the logs carry rounds. Registered as ctest npu_window11_policy (no build target, so the CI target list does not need to change).
PR Reviewer Guide 🔍(Review updated until commit d3b15a5)Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
Closes #834 properly: the tolerance is no longer a constant the script can pretend to check, and I exercised all six
behaviours with my own fixture logs rather than through its self-test (which also passes, 6/6).
| fixture | what the script did |
|---|---|
--compare c1 c2 with no --tol/--control |
exit 2, message quoting the #834 measurement ("the SAME binary against itself moving up to 7.7 % per row") and naming both acceptable inputs |
--control c1 c2 (worst row 7.7 %) |
prints the control's per-row spread (+7.7 / +1.0 / -1.0), then tolerance 8% from control (worst row 7.7%, rounded up) and statistic in force: ratio-of-medians |
| control whose rows all move the same way (+6.0 % each) | prints the POSITION BIAS note — "not noise, and more rounds will not average it out: alternate the order inside a round" — and still derives the tolerance |
| two rounds per row in both logs | statistic in force: median-of-ratios, with both columns printed per row |
| a 30 % row against the control's 8 % | exit 1 (outside tolerance) |
| same prof rows, one prompt's text differs | exit 1 — so the transcript half still gates when the tolerance half passes |
The two things I most wanted to see are both real: the refusal has no silent default (grep for a 2 % constant finds only
the comment explaining why it cannot exist), and the paired statistic is actually selected by the data shape rather than
announced. add_test(NAME npu_window11_policy … --self-test) is registered in CMakeLists.txt, so CI runs the policy
check itself, and it needs no build target (the comment says so, which is the trap this repo has hit three times).
docs/npu.md says the same thing the code does, including the half the script cannot do for you — "alternate the order
inside a round" — which is the honest split, since the A/B driver owns that.
One note for the next pass rather than a gate: the position-bias detector needs ≥3 rows and sd/|mean| < 0.5; a control
with two rows, or with one clean row and two biased ones, will not trip it and will report a tolerance inflated by the
warm-up. Nothing in the output hides that (the per-row spread is printed), but the note is the only thing that turns it into
an explanation.
|
Persistent review updated to latest commit e9e864b |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
At e9e864b5d (merge of main into the head I reviewed; both files blob-identical, so the content is the content I verified).
Build success at this head.
Evidence: exercised all six behaviours with my own fixture logs, not through the self-test — no --tol/--control → exit 2 quoting the #834 numbers; --control → per-row spread plus tolerance 8% from control (worst row 7.7%, rounded up); a uniform +6 % control → the POSITION BIAS note; two rounds per row → statistic in force: median-of-ratios; a 30 % row → exit 1; a text-only mismatch → exit 1. No silent 2 % default remains, and npu_window11_policy is a registered ctest.
|
Persistent review updated to latest commit fbedbd0 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
At fbedbd072 (merge of main into the head I reviewed; 3 of 3 files blob-identical, so the content is what I verified).
Build success at this head.
Evidence stands: all six behaviours exercised with my own fixtures — refusal with no --tol/--control (exit 2, #834 numbers in the message), control-derived tolerance 8% from a 7.7 % worst row, the POSITION BIAS note on a uniform control, median-of-ratios chosen when both logs carry rounds, exit 1 for a 30 % row and for a text-only mismatch; npu_window11_policy is ctest-registered.
|
Persistent review updated to latest commit d3b15a5 |
… embedding load it caught on main The gate (#803, scripts/npu-window11.sh) ran on one `rm` Qwen3-0.6B container, which has no folded basis, no ternary LM head and no split full-attention QKV. So the parts of the forward the containers this box actually serves (bonsai2-27b-pb16i, bonsai2-27b-t2s2) depend on were never executed by it: three defects lived exactly there (engine#808) while the gate produced 108/108 identical transcripts, 9/9 identical prof rows and 452 bit-identical logprob tokens. Text equality cannot see them -- one refuses to answer, one keeps the text plausible with a signed permutation of the true Hadamard, one kills the process. `--folded [--tap-layer N]` runs the same three greedy prompts against a folded container with the container's own debug taps on (NPU_PB16I_TRACE / NPU_QKV_DUMP, both already in the forward, so neither arm is instrumented differently) and records, per launch of that layer, the raw activation, the rotated activation, the int8 codes the kernel was given, its int32 accumulator, the dequantised output and the side-table eref. `--compare` requires those lines to be identical as well as the text. A folded log may only be compared with a folded log at the same layer, and `--folded` gives a 27B thinking model 32 tokens by default (at the 0.6B row's 8 every reply is the same preamble stub, so the text signal compares nothing). Measured on ~/models/npu/bonsai2-27b-pb16i, one npu-hold ticket, arms built from this tree (main, and main with each defect injected alone -- ~/p4-gate/876/inject.py): main vs main PASS 3/3 transcripts, 611 tap lines identical (570 launches + 41 qkv dumps) main vs DA (no instance resolved for the ternary-only role split) FAIL "the sealed ELF has no instance for role QKVFAQ", 0/3 transcripts, 18 taps vs 611 main vs DB (the branch's xor/strided butterfly rotation, verbatim) FAIL tap 0: same activation (as[0]=0.0128398435), different int8 codes (A[0..7] -71 -70 66 -83 -59 12 6 -72 vs 1 78 -92 84 10 10 -77 6), and the text main vs DC (the verbatim `const bool head_pb16i = head_pb16i && ...`) FAIL the serve dies in the LM head launch, 0/3 transcripts, 18 taps vs 611 The row's first run on main also caught a live regression: #854 read `kpad_` at the embedding dequant (embed_f_.resize(NV * kpad_) and again in dequant_any) while assigning it ~190 lines later, so the resize was zero-length and every container with a non-BF16 embedding failed to load at all -- "forward init failed: embedding dtype I8 but dequant failed" on 23d6972, where 692cfa2 loads the same container (1271398400 floats, 4.74 GiB, then ready). The 0.6B rm container has BF16 embeddings, which is why the old row could not see it. That fix is NOT in this commit any more: main landed the same root cause ahead of it (#884, engine#881, which assigns the grid before the first dequant), so this PR is the gate row only and the forward is untouched. Rebased onto 7690a8c, where the gate script's comparison had become tolerant-and-paired (#880, engine#834). Two resolutions rather than a textual merge, because the two changes disagree by design: a comparison is still refused with no measured tolerance, and on a folded row the ms/launch columns are reported and not asserted unless the caller supplies the same-binary `--control` pair -- with a measurement they are scored like any other row, which is what #834 wanted and what "not scored at 2 %" was standing in for. The test's accept/refuse cases now pass `--tol 10` explicitly for the same reason. Device-free half: tests/npu_window11_compare.sh (ctest npu_window11_compare, 10/10) pins what the comparison must accept and refuse -- folded against folded passes, one differing rotated activation fails, a folded log against a plain one fails, a folded row's 73 % ms move passes unscored but fails once a control pair measures the tolerance, the plain row keeps its tolerance, and `--folded` really reaches the serve's environment. `scripts/npu-window11.sh --self-test` stays green (6/6). closes #876
… embedding load it caught on main The gate (#803, scripts/npu-window11.sh) ran on one `rm` Qwen3-0.6B container, which has no folded basis, no ternary LM head and no split full-attention QKV. So the parts of the forward the containers this box actually serves (bonsai2-27b-pb16i, bonsai2-27b-t2s2) depend on were never executed by it: three defects lived exactly there (engine#808) while the gate produced 108/108 identical transcripts, 9/9 identical prof rows and 452 bit-identical logprob tokens. Text equality cannot see them -- one refuses to answer, one keeps the text plausible with a signed permutation of the true Hadamard, one kills the process. `--folded [--tap-layer N]` runs the same three greedy prompts against a folded container with the container's own debug taps on (NPU_PB16I_TRACE / NPU_QKV_DUMP, both already in the forward, so neither arm is instrumented differently) and records, per launch of that layer, the raw activation, the rotated activation, the int8 codes the kernel was given, its int32 accumulator, the dequantised output and the side-table eref. `--compare` requires those lines to be identical as well as the text. A folded log may only be compared with a folded log at the same layer, and `--folded` gives a 27B thinking model 32 tokens by default (at the 0.6B row's 8 every reply is the same preamble stub, so the text signal compares nothing). Measured on ~/models/npu/bonsai2-27b-pb16i, one npu-hold ticket, arms built from this tree (main, and main with each defect injected alone -- ~/p4-gate/876/inject.py): main vs main PASS 3/3 transcripts, 611 tap lines identical (570 launches + 41 qkv dumps) main vs DA (no instance resolved for the ternary-only role split) FAIL "the sealed ELF has no instance for role QKVFAQ", 0/3 transcripts, 18 taps vs 611 main vs DB (the branch's xor/strided butterfly rotation, verbatim) FAIL tap 0: same activation (as[0]=0.0128398435), different int8 codes (A[0..7] -71 -70 66 -83 -59 12 6 -72 vs 1 78 -92 84 10 10 -77 6), and the text main vs DC (the verbatim `const bool head_pb16i = head_pb16i && ...`) FAIL the serve dies in the LM head launch, 0/3 transcripts, 18 taps vs 611 The row's first run on main also caught a live regression: #854 read `kpad_` at the embedding dequant (embed_f_.resize(NV * kpad_) and again in dequant_any) while assigning it ~190 lines later, so the resize was zero-length and every container with a non-BF16 embedding failed to load at all -- "forward init failed: embedding dtype I8 but dequant failed" on 23d6972, where 692cfa2 loads the same container (1271398400 floats, 4.74 GiB, then ready). The 0.6B rm container has BF16 embeddings, which is why the old row could not see it. That fix is NOT in this commit any more: main landed the same root cause ahead of it (#884, engine#881, which assigns the grid before the first dequant), so this PR is the gate row only and the forward is untouched. Rebased onto 7690a8c, where the gate script's comparison had become tolerant-and-paired (#880, engine#834). Two resolutions rather than a textual merge, because the two changes disagree by design: a comparison is still refused with no measured tolerance, and on a folded row the ms/launch columns are reported and not asserted unless the caller supplies the same-binary `--control` pair -- with a measurement they are scored like any other row, which is what #834 wanted and what "not scored at 2 %" was standing in for. The test's accept/refuse cases now pass `--tol 10` explicitly for the same reason. Device-free half: tests/npu_window11_compare.sh (ctest npu_window11_compare, 10/10) pins what the comparison must accept and refuse -- folded against folded passes, one differing rotated activation fails, a folded log against a plain one fails, a folded row's 73 % ms move passes unscored but fails once a control pair measures the tolerance, the plain row keeps its tolerance, and `--folded` really reaches the serve's environment. `scripts/npu-window11.sh --self-test` stays green (6/6). closes #876
… embedding load it caught on main (#886) The gate (#803, scripts/npu-window11.sh) ran on one `rm` Qwen3-0.6B container, which has no folded basis, no ternary LM head and no split full-attention QKV. So the parts of the forward the containers this box actually serves (bonsai2-27b-pb16i, bonsai2-27b-t2s2) depend on were never executed by it: three defects lived exactly there (engine#808) while the gate produced 108/108 identical transcripts, 9/9 identical prof rows and 452 bit-identical logprob tokens. Text equality cannot see them -- one refuses to answer, one keeps the text plausible with a signed permutation of the true Hadamard, one kills the process. `--folded [--tap-layer N]` runs the same three greedy prompts against a folded container with the container's own debug taps on (NPU_PB16I_TRACE / NPU_QKV_DUMP, both already in the forward, so neither arm is instrumented differently) and records, per launch of that layer, the raw activation, the rotated activation, the int8 codes the kernel was given, its int32 accumulator, the dequantised output and the side-table eref. `--compare` requires those lines to be identical as well as the text. A folded log may only be compared with a folded log at the same layer, and `--folded` gives a 27B thinking model 32 tokens by default (at the 0.6B row's 8 every reply is the same preamble stub, so the text signal compares nothing). Measured on ~/models/npu/bonsai2-27b-pb16i, one npu-hold ticket, arms built from this tree (main, and main with each defect injected alone -- ~/p4-gate/876/inject.py): main vs main PASS 3/3 transcripts, 611 tap lines identical (570 launches + 41 qkv dumps) main vs DA (no instance resolved for the ternary-only role split) FAIL "the sealed ELF has no instance for role QKVFAQ", 0/3 transcripts, 18 taps vs 611 main vs DB (the branch's xor/strided butterfly rotation, verbatim) FAIL tap 0: same activation (as[0]=0.0128398435), different int8 codes (A[0..7] -71 -70 66 -83 -59 12 6 -72 vs 1 78 -92 84 10 10 -77 6), and the text main vs DC (the verbatim `const bool head_pb16i = head_pb16i && ...`) FAIL the serve dies in the LM head launch, 0/3 transcripts, 18 taps vs 611 The row's first run on main also caught a live regression: #854 read `kpad_` at the embedding dequant (embed_f_.resize(NV * kpad_) and again in dequant_any) while assigning it ~190 lines later, so the resize was zero-length and every container with a non-BF16 embedding failed to load at all -- "forward init failed: embedding dtype I8 but dequant failed" on 23d6972, where 692cfa2 loads the same container (1271398400 floats, 4.74 GiB, then ready). The 0.6B rm container has BF16 embeddings, which is why the old row could not see it. That fix is NOT in this commit any more: main landed the same root cause ahead of it (#884, engine#881, which assigns the grid before the first dequant), so this PR is the gate row only and the forward is untouched. Rebased onto 7690a8c, where the gate script's comparison had become tolerant-and-paired (#880, engine#834). Two resolutions rather than a textual merge, because the two changes disagree by design: a comparison is still refused with no measured tolerance, and on a folded row the ms/launch columns are reported and not asserted unless the caller supplies the same-binary `--control` pair -- with a measurement they are scored like any other row, which is what #834 wanted and what "not scored at 2 %" was standing in for. The test's accept/refuse cases now pass `--tol 10` explicitly for the same reason. Device-free half: tests/npu_window11_compare.sh (ctest npu_window11_compare, 10/10) pins what the comparison must accept and refuse -- folded against folded passes, one differing rotated activation fails, a folded log against a plain one fails, a folded row's 73 % ms move passes unscored but fails once a control pair measures the tolerance, the plain row keeps its tolerance, and `--folded` really reaches the serve's environment. `scripts/npu-window11.sh --self-test` stays green (6/6). closes #876 Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Why
scripts/npu-window11.sh --compareasserted that no prof row's ms/launch moves by more than a fixed 2 %, andengine#834 showed that assertion cannot be evaluated on this box: the same binary against itself (8 alternating
rounds, sha256-identical, one NPU ticket) moved up to 7.7 %, the two obvious summary statistics disagreed in sign
on 5 of 9 rows, and the worst single round pair was off by ±33–43 %. Closes have meanwhile been citing "inside the
10 % tolerance" while the gate asserted 2 %, so the number in a close had no relation to what the gate checked.
What changed
The tolerance now comes from a measurement, or the comparison refuses to run.
With neither,
--compareexits 2 and says why. With--control, the control's own worst row becomes the tolerance,rounded up to the next whole percent (7.7 % → 8 %), and the control's per-row spread is printed so the number is
auditable.
The statistic is named. Every row prints both, and the header says which one the tolerance applies to:
ratio-of-mediansmedian(ms_B) / median(ms_A)— what the script printed beforemedian-of-ratiosmedianover rounds ofms_B / ms_A— paired; needs both logs to carry the same roundsThe paired one is used when both logs carry ≥2 rounds per row, because the box's noise is largely a level shift over a
round (someone else's traffic, a BO/ELF warm-up) that pairing cancels and a ratio of medians turns into a fake movement
— precisely the sign disagreement in the issue. A log with rounds carries the same
prof/textlines once per round,in round order.
A biased control is called what it is. If every control row moves the same way by a similar amount, the output says
it is a position bias — the second serve of a round paying for the warm-up — and that alternating
A BthenB Ainside a round is the fix, because no number of rounds averages a systematic bias out. That is the half of the issue the
script cannot do itself (the driver of the A/B pair owns the order), so it is stated in the output and in
docs/npu.md.Measured
The policy, end to end, device-free —
scripts/npu-window11.sh --self-test, also registered as ctestnpu_window11_policy(a shell test needs no build target, so ci.yml's list is untouched):On the real control logs from the issue's own session (
~/p4-gate/out-ava-20261102T125310, same binary, 8 rounds —tolerance derived from rounds 1–4, applied to rounds 5–8):
Two things fall out of that run and are the reason the change is more than a flag: the control is not at 7.7 %
once the four rounds are paired (it is +12–27 %, all positive — the fixed
ORDER="A B"bias), and the script now saysso instead of quietly recommending a 27 % tolerance as if it were noise. With the order alternated in the driver, that
systematic term disappears and the tolerance the control suggests drops to the noise floor.
python3 tools/copyright.py --checkrc 0. No device claim is needed for this change; no NPU ticket was taken for it.Closes #834.
Note on ownership: the issue's triage assigned it to pi-mosaic; the orchestrator reassigned it to this lane for the
overnight run. The alternating-order half lives in whatever drives the A/B pair, so if pi-mosaic's driver already does
it, the only thing to keep is the policy — and the
--controlnumber it prints is the one to quote in a close.