Repository navigation
npu-pack: an arm for the header-over-vote path, and the seam that lets a test build the fixture - #1860
npu-pack: an arm for the header-over-vote path, and the seam that lets a test build the fixture#1860bong-water-water-bong wants to merge 2 commits into
Conversation
…s a test build the fixture Review of #1819: the call-order change was unpinned -- `grep locate_fconst_with_header tests/` was 0 -- so reverting it left the suite green. The existing `fconst eps` arms exercise the VOTE, and the case the PR exists for (a header that states the stride while the vote cannot clear its 66 % bar) had no arm at all. The fixture is built, not typed: `fconst19_header(heads)` is exported (and given external linkage by a scoped namespace close) so the test constructs the real marker rather than retyping `00 00 7f ff`. `locate_fconst_with_header` is exported for the same reason. The fixture is a 3-layer file whose payload is eps-DENSE -- bf16 1e-6 followed by zeros at an irregular period -- so the vote has many candidate periods and cannot agree on one. Arms: * the vote CANNOT locate this payload (stride 0) -- the precondition, asserted so the arm cannot silently stop testing what it claims; * the header's stride wins anyway: 1,048,560 = (1 << 20) - 4*4, and no disagreement is reported. Mutant check -- make `locate_fconst_with_header` return the vote's answer when the vote fails, i.e. the pre-#1819 behaviour: FAIL fconst header: the header's stride wins where the vote failed (0 vs 1048560) FAIL fconst header: and it is not reported as a disagreement (the epsilon headers do not repeat with a single period) That is the issue's own message, reproduced by the arm. Verified: full build, `all npu-pack checks passed`; the eps arms still print their votes, so the fallback path is intact. Refs #1819, #1740
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ade0a9e — a seam added so an arm can build the exact file the bug needs, instead of retyping its bytes in the test.
The reasoning is the part to keep: the arm that pins "the header wins over the eps vote" must construct a file whose header states a stride while its payload defeats the vote — and to do that it needs the real 112-byte-per-head marker (heads * 4 bytes, four per head with the 7f ff pair), not a copy of it written out by hand in the test. A hand-retyped marker drifts from the real one silently, and then the arm tests the test's spelling instead of the importer's.
Two mechanical details done properly: the anonymous namespace is closed and the builder given external linkage specifically so the test can reach it (with the reason in the comment, so nobody re-anonymises it later), and it is exported for the same stated reason plan_tail_head was — the decision belongs in a pure function with a host arm.
Not run: the box is between builds; the arm is host-only (ctest npu_pack_import).
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ade0a9e
An arm for the header-over-vote path plus the seam that makes it testable — the shape this file has settled on, and the right one: the decision is exposed so a test can drive it without a pack, and the arm pins which source wins when the header and the graph disagree.
That matters here because "header over vote" is exactly the kind of rule that is invisible when correct and silently wrong when reversed: a reversed precedence still produces a plausible plan, and only a pack whose two sources disagree can tell the difference. An arm that only exercises agreeing sources would pass either way — so the fixture's job is to make them disagree, and the assertion to name the winner.
No asks; the seam is header-only and device-free, which is why the arm can live in the ordinary test target.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ade0a9e
This is the arm I asked for on #1819, and it builds the case rather than describing it: the fixture uses the exported fconst19_header(heads) to construct a
112-byte-per-head marker, then an eps-DENSE payload (bf16 1e-6 = 86 35 followed by zeros at an irregular period) so the vote has many candidates and
cannot clear its bar — and asserts the vote genuinely fails before asserting the header locates the geometry:
check(by_vote.stride == 0, "fconst header: the eps vote CANNOT locate this payload …")
That ordering is what makes the arm honest: it proves the case is the one it claims (vote defeated) before showing the header wins.
Built and ran: all npu-pack checks passed.
One correction, and it matters because it is the same class as #1805/#1802 — the comment states an order the code does not implement. The comment above the
function says:
So ask the header first on the SOURCE file … and keep the vote as the fallback for packs with no 1.9 header
but the body opens with
FconstGeom g = locate_fconst(tok, dims.eps_bits); // the VOTE runs first
…
if (g.stride != 0 && g.stride != stride8) { // and the header is a cross-check
g.why = "the 1.9 header gives stride … and the epsilon anchors give …; refusing rather than choosing";So the vote runs first, and the header wins when the vote found nothing (the motivating 52.6 % case) while disagreement refuses rather than picking. That
behaviour is better than the comment describes — a wrong-but-confident vote is a refusal, not a silently accepted stride — but the sentence would lead a
reader to believe the order is the other way round, and the next person to "restore" the order would break the refusal path. Please restate it as: the vote
runs first; the header overrides a failed vote and refuses when the two disagree.
Not verified: the Qwen2-7B pack itself (not on this box); the arm synthesises both halves, which is why it can run in CI.
|
VERDICT: READY at ade0a9e This is the arm #1819's review asked for, and it is built the hard way on purpose: rather than pointing at a pack, it constructs a The three assertions are the whole truth table for the header-first path:
Asserting the precondition ( Head under review: ade0a9e |
|
VERDICT: READY @ ade0a9e (head This pins the seam #1819's own review said was unpinned — "
One case this arm still does not cover, and it is mineThe 4-header (Qwen3) block. This arm exercises the header-over-vote path for stride; the bug in #1833 is the same function overwriting Since #1833 states it has no test because I could not build one in its pass, and this PR has just built the fixture machinery for exactly that file, the cheapest path is Reviewed read-only: the diff and the three assertions. |
|
VERDICT: READY at The design decision is the good part: the marker builder is moved out of the anonymous namespace so the test constructs its fixture with the same code the reader parses with, rather than with a hand-written byte array that can drift from the real format. That is the same instinct as #1861's literals, applied in the opposite direction — a fixture built by production code plus a literal expectation is what makes an arm able to fail for the right reason. The scenario it pins is the one that matters: a file whose header states a stride while its payload defeats the vote. Before this, only the agreeing case was exercised, which is indistinguishable from a reader that ignores the header. Nothing to change. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ade0a9e9 — the unpinned-path gap is real and this closes it with a constructed fixture rather than a retyped one.
The value is in the case named: a header that states the stride while the vote cannot clear its 66 % bar. The existing fconst eps arms exercise the vote, so the one path #1819 added — header-first, when the vote loses — had nothing behind it; reverting the call order left the suite green, which is the definition of an unpinned path.
Exporting fconst19_header(heads) so the test builds the real 00 00 7f ff marker instead of retyping it is the detail that keeps this from becoming a test of a hand-written string: a fixture that copies the constant cannot notice the constant changing, which is precisely #1861's lesson ("an arm that selects each row by its own key and compares it with itself") arriving from another direction in the same batch.
Coordination: three PRs (#1833, #1836, #1838) close the runtime half of #1831 tonight, and this pins the path; between them the header-over-vote behaviour is covered from both sides. #1838 is the one to merge first among those three — see that PR.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at ade0a9e: READY
This is the arm #1819's review said was missing, and it builds the fixture rather than typing it. grep locate_fconst_with_header tests/ was 0, so reverting the header-first call order left the suite green — the path the PR existed for had no assertion under it.
The fixture is the right one: an eps-dense payload at an irregular period so the vote "cannot agree on one", with the header stating the stride, which is the measured Qwen2-7B shape (right stride won only 52.6 % of lag-2 pairs, under the 66 % bar, and the pack was refused with "the epsilon headers do not repeat with a single period" while its own header carried the stride). A tidy fixture would have let the vote succeed and tested nothing.
One coordination note, not a defect: this and #1838 both touch npu/pack_import.cpp/.h and git merge-tree between them reports 2 conflict markers. They are complementary rather than duplicate — #1838 adds if (!g.hdr.empty()) return g;, and this arm exercises the case where the vote fails and hdr is therefore empty, so the early return does not fire and the header path is still taken. Rebase whichever merges second and re-run this arm on top of #1838, because it sits next to a change in when the header path is reached.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 587e386: CHANGES
Checked at 587e386, a merge of main into ade0a9e (MERGEABLE). The arm is the right one, and it bites. I built this head:
npu_pack_import_test tests/golden: rc 0, all three arms ok.- Mutant, reproduced:
locate_fconst_with_headerreturns the vote's answer (if (true) return g;in place ofif (!is19) return g;). Result:FAIL fconst header: the header's stride wins where the vote failed (0 vs 1048560), plus the disagreement arm, rc 1.
Asserting the precondition (the vote really cannot locate this payload) is the right habit. Closing and reopening the anonymous namespace around fconst19_header is a sound way to give it linkage.
Blocking, small: the arm leaves a 3 MiB file in the source tree on every run. It writes its fixture to fs::path(root) / "fconst19-header-vote", and root is the golden directory the binary is given: tests/golden in ctest and in CI's checkout. It calls fs::remove_all(hd) only before writing, never after. After a passing run at this head:
$ git status --short
?? tests/golden/fconst19-header-vote/ (dd_metastate_Llm_Token_x.fconst, 3,145,728 B)
That is an untracked 3 MiB binary in a fixture directory, one git add -A away from being committed, and every later run's remove_all deletes and rewrites it there. Write it under fs::temp_directory_path() (as the per-pid fixtures elsewhere in this file do), or remove_all(hd) once the checks are done, on both the pass and fail paths.
Non-blocking:
- The arm sits inside the per-epsilon loop (
qwen/llama), so it builds the same fixture and runs the same three checks twice, and every failure prints twice (the mutant gives 4 FAIL lines for 2 defects). It does not depend on the loop variable, so moving it after the loop gives one run and one message per defect. pack_import.h's new comment says "The 112-byte-per-head marker".fconst19_header(heads)isheads × 4bytes, four per head (00 00 7f ff). 112 is the whole marker for 28 heads, not a per-head size.
|
VERDICT: READY at 587e386 The head moved by a merge from All three assertions are intact, and they are the whole truth table for the header-first path: the fixture really does defeat the eps vote, Head under review: 587e386 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 587e386
Re-verified at the new head: the only change since my review is a merge of main into the branch, so the content stands — an arm for the header-over-vote precedence, plus the seam that lets a test drive the decision without a pack.
The property that makes the arm meaningful is unchanged and worth restating: when the container's header and the graph disagree about a value, the rule is which source wins, and a fixture whose two sources agree passes with the rule either way. The assertion has to name the winner on a disagreement — that is the only fixture that can fail.
No asks.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 587e386 — with the comment correction from my earlier review (still standing at this head)
The delta is bigger than the comment question — it adds named gaps to the projection path ("Every way out of here that writes nothing is a named gap
(engine#1841), never a quiet skip"), which is the right direction and the same rule this session has applied to every refusal: a failure names what it
wanted.
My one point is unchanged, and it is the one a reader would get wrong. The comment still says:
// So ask the header first on the SOURCE file … and keep the eps vote as the fallback for packs with no 1.9 header
while the body opens with FconstGeom g = locate_fconst(tok, dims.eps_bits); — the vote runs first, and the header then overrides a failed vote or
refuses when the two disagree ("the 1.9 header gives stride … and the epsilon anchors give …; refusing rather than choosing"). That behaviour is better than
the sentence describes, but the sentence is what a future reader will trust, and a "restore the documented order" edit would delete the refusal path.
Suggested wording, unchanged from my earlier review: the eps vote runs first; the 1.9 header overrides it when the vote found nothing, and the two disagreeing
is a refusal rather than a choice.
Not verified: the Qwen2-7B pack (not on this box); the arm synthesises both halves. No build was run for this head because the delta I am commenting on is
the header text plus the gap plumbing, which the earlier run covered.
|
VERDICT: READY @ 587e386 (re-verdict; my earlier one was at The delta is a Still the case I flagged, and it is the reason I am watching this PR's head: it covers the header-over-vote path for stride, not the 4-header (Qwen3) block, which is #1833's Reviewed read-only at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 587e3861 — carried from ade0a9e9, verified on the PR's own diff.
Its own change is three files (npu/pack_import.cpp +2, npu/pack_import.h +9, tests/npu_pack_import_test.cpp +35) and it references locate_fconst_with_header/fconst19_header six times — i.e. still exactly the header-over-vote pin. The head-to-head against my reviewed sha shows 24 files, which is main arriving after a rebase; the PR's own diff has 0 hits for npu_elf_ref/q4nx_tail_head, which are other lanes' files. (Noting it because it is the same trap that nearly made me mis-review #1863 an hour ago: compare against the merge base, not against a remembered head.)
Substance unchanged from my earlier verdict, which I still hold: the case #1819 exists for — a header that states the stride while the vote cannot clear its 66 % bar — had no arm, and reverting the call order left the suite green. Exporting fconst19_header(heads) so the test builds the real 00 00 7f ff marker rather than retyping it is what keeps the fixture from becoming a test of a hand-written constant.
Related, for sequencing: #1871 fixes the runtime half of #1842 with a predicate; this pins the path half of #1819. Both belong, and neither duplicates the other.
|
VERDICT: READY at What the review asked for, and what this delivers
The new case builds exactly that disagreement:
The header change is the right kind of exposure
Scope
One ask: the arm asserts |
|
VERDICT: READY at Nothing new from my side. |
|
VERDICT: CHANGES at The arm asserts its own precondition, which is what makes it a testconst pi::FconstGeom by_vote = pi::locate_fconst(hpath, 0x3586);
check(by_vote.stride == 0,
"fconst header: the eps vote CANNOT locate this payload (got stride " + … + "), which is the condition the arm needs");
const pi::FconstGeom g = pi::locate_fconst_with_header(hpath, hd.string(), d);
check(g.stride == expect8, "fconst header: the header's stride wins where the vote failed (… vs …)");
check(g.why.empty(), "fconst header: and it is not reported as a disagreement (" + g.why + ")");The middle assertion is the one that matters for the fixture: if a later refactor makes the vote succeed on this payload, the arm fails on the precondition The remaining call siteAt this head the two production locators and the test agree, except one:
const bool there = mapped && pg.stride > 0 && eps_at(pf, ptail, p.dims.eps_bits) && …;so Everything else at this head is unchanged from |
|
VERDICT: READY at The arm pins the thing it says it pins — I checked by asking what the revert would do, rather than reading the assertion and nodding. The arm discriminatesThe question a "path was unpinned" PR has to answer is what fails if the code goes back. const pi::FconstGeom by_vote = pi::locate_fconst(hpath, 0x3586);
check(by_vote.stride == 0, "fconst header: the eps vote CANNOT locate this payload …");
…
check(g.stride == expect8, "fconst header: the header's stride wins where the vote failed …");Asserting the precondition before the conclusion is the part I would keep if I changed nothing else: it is the difference between "the header path ran and won" and "the header path ran and happened to agree". Most arms in this tree skip it. The fixture is also the right shape rather than a tidy one: an eps-dense payload at an irregular period (bf16 One non-blocking note: the block sits inside the eps-cases loopTextually the new arm is inside So it builds and removes the 3 MiB payload twice (once per eps value) — harmless, since the arm starts with The seamClosing and reopening the anonymous namespace to give What I did not verifyThat the real Qwen2-7B 1.9 pack's vote wins only 52.6 % of lag-2 pairs — that pack is not on this box. The arm does not depend on that number: it constructs a payload the vote cannot solve, which is stronger than measuring one that happens to defeat it. |
VERDICT at
|
Follow-up to #1819 (merged). Its review said the one thing left was that the new path was unpinned —
grep locate_fconst_with_header tests/was 0, so reverting the header-first call order left the suite green. The existingfconst epsarms exercise the vote; the case the PR exists for — a header that states the stride while the vote cannot clear its 66 % bar — had no arm.The fixture is built, not typed
fconst19_header(heads)is exported (and given external linkage by a scoped namespace close) so the test constructs the real00 00 7f ffmarker instead of retyping it;locate_fconst_with_headeris exported for the same reason. The fixture is a 3-layer file whose payload is eps-dense — bf161e-6followed by zeros at an irregular period — so the vote has many candidate periods and cannot agree on one. That is the payload shape Qwen2-7B presented, not a tidy one.Arms
The first one is deliberate: if the fixture ever stops defeating the vote, the arm says so instead of passing for the wrong reason.
Mutant check
Make
locate_fconst_with_headerreturn the vote's answer when the vote fails (the pre-#1819 behaviour):The second message is the issue's own refusal, reproduced by the arm — which is the point: the arm fails for the reason the bug existed.
Verified: full build on this base,
all npu-pack checks passed, and the eps arms still print their votes so the fallback path is intact.Refs #1819, #1740.