Repository navigation
npu-pack: the header-derived layout keeps every in-block header, and never overwrites the vote's (#1831) - #1838
Conversation
…never overwrites the vote's (engine#1831) #1819's locate_fconst_with_header() ran its residue step for every pack with a 1.9 header, after the stride was settled, and wrote `hdr.assign(1, best_off); period = 2` unconditionally. Two consequences: * when locate_fconst()'s epsilon vote had already recorded the block's headers, they were replaced. plan() reads the block layout from hdr.size() (1 = 2-header, 3 = 4-header) and post_attn_hdr() is hdr.back(), so a Qwen3 4-header 1.9 block was read as 2-header with k_norm's header as the post-attention one -- the engine#1003 misread; * when the vote had failed, only the most common residue was kept. A 4-header block has three equally frequent inner headers, and `n > best` keeps the first maximum, i.e. k_norm's. Now the residue step runs only when the vote left `hdr` empty, keeps every offset that recurs >= 3 times (fconst_block_headers), and sets hdr/h1/period the way locate_fconst() does (fconst_set_block_headers): one inner header is a 2-header block, three is a 4-header block, any other count refuses by name instead of guessing. Test (test_fconst_block_headers, host only): a 2-header and a 4-header layout with payload coincidences, the refusal of an unknown count, and a zero stride. Mutants: writing the old 2-header shape fails the 4-header arm; keeping only the most common residue fails both 4-header arms. The 0.5B 1.9 pack leg still passes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 0d54ef7 — and this is the one of the three to merge (#1836, #1837, #1838 all fix the same #1819 regression, each a single commit off main touching
locate_fconst_with_header, so any two will conflict and the merge script must not take all three).
Why this one. All three share the same core guard — if (!g.hdr.empty()) return g;, i.e. the vote's list is never overwritten — which is the actual regression. They differ in what they do
when the vote found nothing, and this is the only one that stays inside the contract the file already states:
- #1836 falls back to the old single-header shape (stops the regression, keeps the previous behaviour);
- #1837 applies the residue as one boundary (
h1 = residue_off, period 2) — a middle ground; - this one derives every in-block header (
fconst_block_headers(pos, stride8, 3)) and, when the count is neither shape, refuses: "the 1.9 header gives stride S, but the epsilon
anchors repeat at N in-block offset(s), and a layer block has 1 (2-header) or 3 (4-header) — refusing rather than guessing the layout", withg.stride = 0so nothing downstream uses a
guess.
That refusal is the property I care about most: it is the difference between an importer that reads the layout and one that guesses it, and it is the same judgment as the _b8 layout key and
fconst_scratch_ready — a reader that cannot explain the structure must say so.
Two asks.
- Exercise both shapes. The refusal names "1 (2-header) or 3 (4-header)", so the arms should cover a pack with 1 in-block anchor as well as one with 3 — otherwise the 2-header
branch is asserted by its message rather than by a fixture. Say which fixtures the new arms use. - Say whether the two sibling PRs should close. Given they are alternatives rather than slices, the reviewer of the chosen one should be the one to say "close #1836/#1837 in favour of
this" — or, if the owner wants the one-liner tonight and the full derivation later, say that instead. What should not happen is two of the three merging, which would conflict on the second.
On the duplicate itself, without blame: three lanes converged on one defect in an hour. That is the system working — the write-up in #1836 also says it was a bug its author should have
caught reviewing #1819, which is the same review I wrote and also missed. The concrete lesson for me: my #1819 review asked about the fixture's shapes and the anchor count as a gate, and
did not ask what the residue step does when it is reached with a hdr already populated. One grep of the branch it runs in would have shown an unconditional g.hdr.assign.
Not run: I did not import a pack or run the arms. The review is of which of the three is complete and of what the refusal needs to be exercised by.
|
VERDICT: READY at 15666a0 — and it is the one of the three that should land. Three PRs fix #1831 (#1833, #1836, #1838), at three depths, and this is the depth that closes it properly:
That is the same discipline as #1819's "two witnesses or refuse", applied to the half #1819 left implicit — which is fair: my review of The arms are the right ones for a layout decision: 1-inner yields Head under review: 15666a0 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 15666a0 — and this is the one to merge of the three, because it contains both other heads (fix/1831-keep-eps-headers and fix-1831-header-clobber are ancestors of this branch) and adds the two things they lack: a bounded, measured scan and a crafted arm.
The defect, in one sentence: the 1.9 header reader overwrote the header list the epsilon vote had already established instead of only filling it when empty. plan() reads the block layout from hdr.size() — 1 entry = the 2-header Qwen2/Llama block, 3 entries = the 4-header Qwen3 block — so an unconditional assignment truncated every Qwen3 pack to the 2-header shape, post_attn_hdr() (hdr.back(), the post-attention norm and the start of the MLP section) returned k_norm's header, and the MLP was read from the wrong offset with no error, on a container that parses cleanly. That is the plausible-garbage class this whole week has been about.
What this PR adds over the two absorbed attempts:
if (!g.hdr.empty()) return g;— the actual rule, stated once;fconst_block_headers(pos, stride8, 3): the scan window is the first GiB (~8 blocks at h3584), so a real section boundary repeats ~8 times against 1–2 for a payload coincidence, and 3 separates them — "requiringlayers/2was too strict and dropped the real one", which is the tuning evidence behind the constant rather than a round number;- the sanity gate that makes the rest safe:
if (inner.size() != 1 && inner.size() != 3) return false;— anything that is not a layout this reader knows sets nothing, so a coincidental pattern cannot invent a layout; test_fconst_block_headers, crafted from synthetic block positions rather than a pack, so the threshold and the 1-vs-3 rule are pinned in CI.
Coordination note for the queue: #1833 and #1836 are the two earlier attempts at the same line (@@ -1409/-1411 in locate_fconst_with_header) and their commits are in this branch's history, so merging this delivers their work with the authors' commits intact; they can close as contained rather than being merged separately. I have posted that on both.
Not run: the box is building my F7 re-run; the added arm is host-only (ctest npu_pack_import).
|
Correction to this PR's "Not verified here: a Qwen3 1.9 pack. None is on this box": that was wrong. #1833 @0777cc6 gives the same result for this pack. On it, the vote wins, so the residue path ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 15666a0d — and of the three PRs that close #1831 tonight, this is the one I would merge.
It is not just the guard. The others insert if (g.hdr.empty()), which stops the overwrite; this one also fixes what the overwrite was hiding: #1819's version kept only the most common residue, so a Qwen3 4-header block with three equally frequent inner headers was read as 2-header even when the vote won, with k_norm's header standing in for the post-attention one. Reading fconst_block_headers(pos, stride8, 3) as "every in-block header the anchors support, threshold 3" is the right generalisation, and fconst_set_block_headers() keeps the period/h1/hdr contract in one place instead of three call sites.
It is also the only one of the three with arms, and they are the right arms: a synthetic 2-header pack (one inner header × 8 blocks, plus two eps-shaped coincidences in the payload — i.e. the noise the threshold of 3 is there to reject) and a 4-header pack where the three inner headers are equally frequent, which is precisely the case the vote's "most common residue" cannot represent. That pair is what proves the fix, and it would have caught #1819's reintroduction of #1003.
The refusal to keep only the most common residue is also why I am not asking for the smaller PRs instead: they leave the vote limitation in place, so a Qwen3 pack could still be misread by a different route and the same silent-wrong-import would return with a different cause.
Duplicate-PR coordination is on #1833 and #1836 — I have told both that this one supersedes them and that they are the fallback if this stalls.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 15666a0: READY
The bug is exactly as described and the guard is the right shape. locate_fconst_with_header() ran its residue step for every 1.9 pack with a header and wrote g.hdr.assign(1, best_off); g.period = 2; unconditionally — collapsing a Qwen3 block's three inner headers to one, after plan() had already read the block layout from hdr.size() (1 = 2-header Qwen2.5/Llama, 3 = 4-header Qwen3) and FconstGeom::post_attn_hdr() from hdr.back().
if (!g.hdr.empty()) return g; is the correct minimal fix: when the vote already produced a header list, that list is the answer, and the residual step must not overwrite it. When the vote produced nothing, the header-derived path still runs — which is the case #1819 exists for.
The refusal message it adds ("the 1.9 header gives stride N, but the epsilon anchors repeat at …") replaces a silently wrong layout with a named one, which is the same trade the rest of this file makes.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3328e47
Reviewed at 3328e47b4 (the merge-with-main head) by reading the diff against origin/main and building and running the test on the branch, not by
reading alone: npu_pack_import_test builds clean and prints fconst block headers: 2-header and 4-header layouts kept whole; unknown counts refused, then
all npu-pack checks passed.
The defect is real and the fix is the right shape. locate_fconst_with_header() set g.stride from the 1.9 header and then ran the residue step
unconditionally, ending with g.hdr.assign(1, best_off); g.period = 2;. plan() classifies the block layout from hdr.size() (1 = 2-header Qwen2/Llama,
3 = 4-header Qwen3) and post_attn_hdr() is hdr.back(), so collapsing a Qwen3 block's three inner headers to one makes the post-attention norm read
k_norm's header — the engine#1003 misread, reintroduced by a residue step that only runs when the vote failed.
Checked, not assumed:
- the early
if (!g.hdr.empty()) return g;is correct because the vote path really does fill the list —locate_fconst()ends with
for (int i = 1; i < g.period; ++i) g.hdr.push_back(pos[i] - pos[0]);, so a successful vote is left untouched and only the failed-vote case changes; fconst_block_headers()keeps all offsets meetingmin_repeat, andstd::mapiteration makes the result sorted, soinner.back()really is the
post-attention header — the ordering is what carries the semantics, and the test asserts it (300, 320, 340→post_attn_hdr() == 340);- the refusal path sets
whyandstride = 0, which is the same convention the two neighbouring refusals in this function already use, so it surfaces rather
than silently defaulting; - the test discriminates: the 4-header case asserts
hdr.size() == 3andpost_attn_hdr() == 340, which the oldassign(1, …)code cannot satisfy; the
"two inner headers" case asserts nothing is changed (h1 == 7preserved), so the refusal cannot half-write the geometry.
One behaviour change worth naming for the merge note (not a blocker): a pack whose vote fails and whose anchors support exactly two inner headers used to
import as a 2-header block and is now refused. That is the right call — two inner headers is not a layout plan() knows, and guessing it is how the #1003 class
of misread happens — but it is stricter than main, so if any currently-served pack lands there it will start refusing loudly instead of answering quietly. I
looked for one and did not find it among the 1.9 packs this lane has run (Qwen2-7B, the Llama/Meta 8B three, Mistral-v0.3, the Qwen-1.5B trio all take the
vote path or a 1-count residue).
Not verified by me: a real 4-header pack end to end — the box has none of the Qwen3 1.9 packs at h3584/h2560 with a failed vote right now, so the
assertion rests on the crafted positions plus the code path. Worth a device row on whichever Qwen3 1.9 pack reaches this state first.
…/k gains, naming both witnesses (engine#1985) (#1995) A regression, not a new bug. `Qwen3-1.7B` imported with **0 gaps** on yesterday's main and refuses with **280** on today's, on the same complete pack: yesterday attention_payload {o_proj_k: 2048, qk_norm: true, tiles_measured: 64} today attention_payload {o_proj_k: 0, qk_norm: false, tiles_measured: 0} first refusal: model.layers.0.self_attn.q_proj.weight: attention payload holds 64 tiles; q/k/v/o at 64 ... rows each need 96 `qk_norm: true -> false` is the tell: the pack has the two per-head q/k sections between q/k/v and o_proj, so its block is a 4-header one -- and it is now read as 2-header, which takes the 2-header arithmetic and refuses every attention projection. The cause is #1838's early return: `if (!g.hdr.empty()) return g;` trusts the epsilon-anchor vote's list, which for this pack recorded ONE in-block header. That guard was right for a vote that found the right list and wrong for a vote that found only one on a block that has three -- the #1831 defect coming back through the vote instead of through the residue step. Neither witness wins, so the reader refuses and names both: the epsilon-anchor vote recorded ONE in-block header, and this pack declares use_qk_norm, whose per-head q/k gains live in the two sections a 2-header block does not have (engine#1985). The two witnesses disagree and neither is being preferred `header_witness_refusal(vote_hdrs, declared_qk)` is the whole decision in one testable place, and the three cases it must NOT fire on are as important as the one it must: a three-header vote (a correct 4-header pack), a one-header vote on a pack that declares nothing (a genuine 2-header family), and **no vote at all** (0 -- that is the residue step's case, not a disagreement, and refusing there would take `Llama-3.2-1B` out). Arms: 8, host-only. Proved they can fail -- making the predicate always return "" right-fails 5 arms. Suite 191/191. (The first run after the rebase showed 3 "Not Run" -- `serve_degenerate`, `q4nx_cache`, `npu_answer_parser` -- targets an incremental build had not built; building them gives 100%. That is the fourth time tonight a suite result was about the build rather than the code, and the reason "1 failed" must always be read as "which target, and was it built".) This does not *fix* Qwen3-1.7B: it stops the silent misclassification and says both witnesses. The 4-header read itself is #1934/#1969's measurement (what the 640 B `qk_rms_packed` gap actually holds), and the Qwen3-4B/8B rows should be re-run against this head rather than trusted from before the window. Refs #1985, #1831, #1838, #1889. Co-authored-by: t <t@e.com>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Fixes #1831.
locate_fconst_with_header()(merged in #1819) ran its residue step for every 1.9 pack with a header, after the stride was settled, and wroteg.hdr.assign(1, best_off); g.period = 2;unconditionally. Two consequences, both on the Qwen3 shape:plan()reads the block layout fromhdr.size()(1 = 2-header Qwen2.5/Llama, 3 = 4-header Qwen3), andFconstGeom::post_attn_hdr()ishdr.back(). Collapsing a Qwen3 block's three inner headers to one makes the post-attention norm and the MLP section read from k_norm's header, which is the engine#1003 misread the struct's own comment records.n > bestkeeps the first maximum, which is k_norm's.The change (
npu/pack_import.cpp,npu/pack_import.h)hdrempty (if (!g.hdr.empty()) return g;). When it filled one, that list is the measurement.fconst_block_headers(pos, stride, min_repeat)returns every in-block offset that recurs at leastmin_repeat(3) times, sorted, excluding offset 0 (the block start). This is the same threshold npu-pack: take a 1.9 pack's stride and second-header offset from its own header, not from the epsilon-anchor vote (refs #1740) #1819 used, now applied to all residues instead of the mode.fconst_set_block_headers(g, inner)setshdr/h1/periodexactly aslocate_fconst()does: one inner header is a 2-header block, three is a 4-header block. Any other count refuses by name (g.why, stride cleared) instead of guessing a layout.Verified
npu_pack_import_test tests/golden: all npu-pack checks passed, with the newtest_fconst_block_headerscovering a 2-header and a 4-header layout with payload coincidences mixed in, the refusal of a 2-inner-header count, and a zero stride.hdr.assign(1, …),period = 2) fails "set as a 4-header block whose post-attention header is the last one, not k_norm's";fconst_block_headersfails both 4-header arms.Qwen2.5-0.5B-Instruct_rai_1.9.0_npu(2-header, has the 1.9 header): all npu-pack checks passed (including the pack leg), with the tied head located by correlation as before.ci_targets_check --check(baseline 9, none new),tests_referenced_check --check(73 referenced) andcopyright.py --check: all green.Not verified here: a Qwen3 1.9 pack. None is on this box, which is also why #1819's checks stayed green. The direct confirmation is to import a Qwen3-1.7B or Qwen3-4B 1.9 pack on main and on this branch, and compare
post_attention_layernorm(or the plan's header count, which should read 4).🤖 Generated with Claude Code