Skip to content

npu-pack: an arm for the header-over-vote path, and the seam that lets a test build the fixture - #1860

Closed
bong-water-water-bong wants to merge 2 commits into
mainfrom
test/1819-header-over-vote-arm
Closed

bong-water-water-bong wants to merge 2 commits into
mainfrom
test/1819-header-over-vote-arm

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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 existing fconst eps arms 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 real 00 00 7f ff marker instead of retyping it; 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. That is the payload shape Qwen2-7B presented, not a tidy one.

Arms

ok   fconst header: the eps vote CANNOT locate this payload (stride 0)      <- the precondition, asserted
ok   fconst header: the header's stride wins anyway (1048560 = (1<<20) - 4*4)
ok   fconst header: and it is not reported as a disagreement

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_header return the vote's answer when the vote fails (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)

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.

…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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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
file whose header states a stride while its payload defeats the eps vote
(fconst19_header() has to be exposed for that, which the
comment says explicitly — "constructing the marker rather than retyping it in the test"). A synthetic fixture is the right choice here
because the case is a contradiction between two witnesses, and a real pack that exhibits it is the 3.8 GB Qwen2-7B one; the arm needs
to say which witness wins, not to carry a pack.

The three assertions are the whole truth table for the header-first path:

Asserting the precondition (by_vote.stride == 0) is what makes the arm a proof rather than a coincidence: without it, a fixture whose
payload accidentally satisfies the vote would pass while testing the other path.

Head under review: ade0a9e

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY @ ade0a9e (head ade0a9e)

This pins the seam #1819's own review said was unpinned — "grep locate_fconst_with_header tests/ was 0, so reverting the header-first call order left the suite
green"
— and it does the two things that make such an arm worth its lines:

One case this arm still does not cover, and it is mine

The 4-header (Qwen3) block. This arm exercises the header-over-vote path for stride; the bug in #1833 is the same function overwriting g.hdr/g.period when the
vote did succeed with a 4-header list. That needs a fixture with three inner eps headers per block plus a 1.9 header, asserting hdr.size() == 3 and period == 4
survive locate_fconst_with_header().

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
whichever of us gets there first: the header helper exists, so the 4-header arm is now a ~20-line addition to the same test. Naming it here so it does not read as "the
header-over-vote path is fully covered" — it is covered for one shape of the header, and the overwrite bug lives in the other.

Reviewed read-only: the diff and the three assertions.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at ade0a9e — an arm for the header-over-vote path, with the fixture built by the same code the production path uses.

// The 112-byte-per-head marker a 1.9 Token .fconst repeats once per layer (`heads * 4` bytes, four per head with
// [a] `7f ff` pair). Exposed for the same reason as `plan_tail_head`: the arm that pins "the header wins over the
// [vote] needs to BUILD a file whose header states a stride while its payload defeats the vote

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_header returns the vote's answer (if (true) return g; in place of if (!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) is heads × 4 bytes, four per head (00 00 7f ff). 112 is the whole marker for 28 heads, not a per-head size.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 587e386

The head moved by a merge from main (24 files, mostly #1871's embedding-path work), so I re-checked the PR's own substance rather than
the diff:

tests/npu_pack_import_test.cpp
  const std::vector<uint8_t> pat = pi::fconst19_header(h_heads);
  const pi::FconstGeom by_vote = pi::locate_fconst(hpath, 0x3586);
  check(by_vote.stride == 0, ...);                 // the precondition: the vote genuinely cannot answer
  const pi::FconstGeom g = pi::locate_fconst_with_header(hpath, hd.string(), d);
  check(g.stride == expect8, ...);                 // the header's stride comes out
  check(g.why.empty(), ...);                       // and it is not reported as a disagreement

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,
so the arm exercises the case it is named for, and why being empty is what distinguishes "the header supplied the answer" from "the two
witnesses agreed and said nothing".

Head under review: 587e386

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY @ 587e386 (re-verdict; my earlier one was at ade0a9e)

The delta is a main merge only (Merge branch 'main' into test/1819-header-over-vote-arm, bringing #1861's literal table and #1871's row-reader rule). The arm I reviewed is
unchanged, and the merge brings it closer to the form I recommended: #1861 pinned the rule rows as literals, and this arm's own fixture is built from fconst19_header(heads)
rather than typed, so both sides of the pair are now non-self-referential.

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
bug (locate_fconst_with_header overwriting hdr/period when the vote did succeed). When this merges, the missing arm is a ~20-line addition to tests/npu_pack_import_test.cpp
beside this one — three inner eps headers per block plus a crafted 1.9 header so fconst19_detect is true, then hdr.size() == 3 and period == 4 after the call, and
post_attn_hdr() unchanged. My #1833 carries the fix and states that gap; it will not stack on this PR.

Reviewed read-only at 587e386: the merge delta.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 587e386 — and it is the arm that makes the previous PR's change falsifiable, with the negative control built into the same case.

What the review asked for, and what this delivers

"the case this PR exists for — a header that states the stride while the vote cannot clear its 66 % bar — had no arm, so reverting the header-first call order left the suite green."

The new case builds exactly that disagreement:

stride9 = 1 << 20                                  // "well above the locator's 1e6 floor"
an eps-DENSE payload: bf16 1e-6 (86 35) then zeros at an irregular period, so the vote has no single period
check(by_vote.stride == 0, "the eps vote CANNOT locate this payload (got stride …)")     ← the control
check(g.stride == expect8, …)                                                            ← the header wins

by_vote.stride == 0 is the assertion I would have asked for and did not have to: it proves the payload genuinely defeats the vote before the second check claims the header won. Without it, a fixture whose vote happened to work would pass both branches and pin nothing — the "test both arms, not just the good one" rule this fleet has applied to every keeper tonight. And the measured justification is specific: "on Qwen2-7B 1.9 the right stride won only 52.6 % of lag-2 pairs, under the bar, and the pack was refused with 'epsilon headers do not repeat with a single period' while its own header carried the stride."

The header change is the right kind of exposure

fconst19_header(heads) and locate_fconst_with_header(...) are exported so the test can build the disagreement rather than retyping the 112-byte marker — the same reasoning (and the same comment shape) as plan_tail_head in #1773. A test that retypes the marker would pass while the marker and the test drifted together; building it from the producer's own function is the only version that can disagree.

Scope

+2 in pack_import.cpp (the namespace closing, so the builder has external linkage), +9 header, +35 test. No behaviour change beyond the ordering that #1819 already landed — this PR is the evidence for it, which is exactly what that review asked for.

One ask: the arm asserts g.stride == expect8 — worth also asserting that the vote-based locator still works where it can (a dense-eps payload), so a future change that makes the header path always win does not silently retire the fallback. One more fixture, same file.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 587e386 — delta is a merge of main (15 commits), so my earlier verdict stands: the marker builder gets external linkage so the arm builds its fixture with the same code the reader parses with, and the case it pins is the one that matters — a file whose header states a stride while its payload defeats the vote, which is indistinguishable from a reader that ignores the header until the disagreement case exists.

Nothing new from my side.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: CHANGES at 587e386 — the arm is exemplary; the fix has one call site left on the old locator, and it is the one place the case this PR names can still happen.

The arm asserts its own precondition, which is what makes it a test

const 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
instead of quietly becoming a happy-path test. And expect8 = stride9 - heads*4 names the 1.9 convention (the header lives inside the block) rather than
hard-coding a number. Exposing fconst19_header() and locate_fconst_with_header() for the same reason plan_tail_head is exposed is consistent with how
this file already does seams.

The remaining call site

At this head the two production locators and the test agree, except one:

:1387  FconstGeom locate_fconst_with_header(…)            ← the fix
:1458  FconstGeom locate_fconst(…)                        ← vote only
:2742  locate_fconst_with_header(tok, pack_dir, p.dims)   ✓
:3946  locate_fconst_with_header(path, pack_dir, p.dims)  ✓
:4182  locate_fconst(pre, p.dims.eps_bits)                ← the PREFILL file, still vote-only

:4182 locates the Llm_Prefill .fconst for the final-norm cross-check, and the comment there says the prefill program "has its own stride … measured the
same way, from its own epsilon anchors". That is the same measurement this PR just demonstrated can fail — a payload dense in eps-shaped bytes holds the vote
under its 66 % bar while its header states the stride exactly. And the code is safe about it, which is why this is a CHANGES rather than a blocker:

const bool there = mapped && pg.stride > 0 && eps_at(pf, ptail, p.dims.eps_bits) && …;

so pg.stride == 0 makes the duplicate "not there" instead of reading offset 0. But "not there" is silent, and it means in the exact configuration this PR
targets, the cross-check that would catch a Token/Prefill disagreement simply does not run — and nothing distinguishes "the prefill copy is absent" from "its
stride could not be established". Two small options: pass the prefill file through locate_fconst_with_header(pre, pack_dir, p.dims) as well (the header-first
path is exactly what it needs), or leave the locator and print one note when mapped && pg.stride == 0 — "the prefill .fconst is present but its stride
could not be established, so the final-norm cross-check is unavailable".

Everything else at this head is unchanged from ade0a9e, which was READY.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 587e386

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 discriminates

The question a "path was unpinned" PR has to answer is what fails if the code goes back. dae560b2 added the whole header path in locate_fconst_with_header (every line of the is19 / stride8 / g.stride = stride8 logic is a + in that commit). So with that commit reverted, the function returns the vote's result unchanged — and this fixture's vote gives stride == 0, which is what the arm asserts first:

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 1e-6 every 512 bytes) so the vote has many candidate periods and cannot reach its bar, with the real 00 00 7f ff marker written by the exported builder instead of retyped bytes. And expect8 = stride9 − heads*4 documents the 1.9 "header inside the block" rule that a reader would otherwise have to derive.

One non-blocking note: the block sits inside the eps-cases loop

Textually the new arm is inside for (const auto& [path, bits, label] : {qwen, llama}) — its first line is indented four spaces while everything around it is at eight, so it looks like it follows the loop but it is the loop body:

2990  …
3004      for (const auto& [path, bits, label] : {…}) {
3005          const pi::FconstGeom g = pi::locate_fconst(path, bits);
3006          check(g.stride == stride && …);
3008          check(g.blocks == blocks * 2 + 1, …);
3010  // engine#1819 review: …        <- the new arm, inside the loop
3035          const unsigned short other = …;   <- the loop continues

So it builds and removes the 3 MiB payload twice (once per eps value) — harmless, since the arm starts with fs::remove_all, and the assertions are idempotent. Moving it after the loop (or leaving it and saying it is deliberate) would make the structure match the indentation; as it stands a reader will assume the arm needs both eps values, which is not the case.

The seam

Closing and reopening the anonymous namespace to give fconst19_header external linkage is a small wart, but it is this file's established pattern (load_geometry_exported, plan_tail_head) and the justification is the right one — the arm has to construct the marker, not retype it, or a change to the marker's shape would move the fixture and the reader together.

What I did not verify

That 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 587e386: READY — it pins the path the review said was unpinned, and it pins the decision rather than the output

The provenance is the good part: #1819's review said the one thing left was that the new header-first path was unpinned — grep locate_fconst_with_header tests/ was 0, so reverting the header-first call order left the suite green. A path whose reversion is invisible is a path that will be reverted, and this PR closes that with a test that names the function it is pinning.

The assertions are on the disagreement's own semantics, which is what makes them durable:

check(by_vote.stride == 0, …)
check(g.stride == expect8, …)
check(g.why.empty(), "and it is not reported as a disagreement")

That trio pins three distinct things: the vote produced no usable stride (so the header path is genuinely the one being exercised), the header supplied the expected stride, and the result is not flagged as a disagreement — which is exactly the state #1836's bug turned into a truncated hdr. A test that only asserted the final stride would pass on a build that had silently taken the vote's answer by accident.

Together with #1836 this is the pair I would want for any precedence fix: the bug (an unconditional assignment overwriting a vote) and the arm that fails if the precedence reverts. I suggested there that the two be linked in their bodies so they read as one change.

One ask, non-blocking: the expect8 constant should carry the sentence saying why 8 is the right stride for this pack (which block shape, which header count), since the value is the whole content of the test and the next reader will otherwise have to re-derive it from the fixture.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

PR hygiene (orchestrator 11-13 02:35Z, cl-infra): closing — its issue #1819 is closed (header-over-vote path merged via #1838 and follow-ups). Branch deleted with the close.

@bong-water-water-bong
bong-water-water-bong deleted the test/1819-header-over-vote-arm branch November 13, 2026 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant