Skip to content

npu-pack: the header-derived layout keeps every in-block header, and never overwrites the vote's (#1831) - #1838

Merged
bong-water-water-bong merged 2 commits into
mainfrom
cl-review/1831-keep-header-list
Nov 10, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
cl-review/1831-keep-header-list

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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 wrote g.hdr.assign(1, best_off); g.period = 2; unconditionally. Two consequences, both on the Qwen3 shape:

  • The vote's own list was overwritten. plan() reads the block layout from hdr.size() (1 = 2-header Qwen2.5/Llama, 3 = 4-header Qwen3), and FconstGeom::post_attn_hdr() is hdr.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.
  • 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, which is k_norm's.

The change (npu/pack_import.cpp, npu/pack_import.h)

  • The residue step runs only when the vote left hdr empty (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 least min_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) sets hdr / h1 / period exactly as locate_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 new test_fconst_block_headers covering 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.
  • Negative controls, both caught:
    • writing the old shape in the setter (hdr.assign(1, …), period = 2) fails "set as a 4-header block whose post-attention header is the last one, not k_norm's";
    • keeping only the most common residue in fconst_block_headers fails both 4-header arms.
  • Pack leg on 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) and copyright.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

…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 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 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"
    , with g.stride = 0 so 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.

  1. 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.
  2. 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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
#1819 asked what happens when the header and the vote disagree and did not ask what the residue step does when they agree.

The arms are the right ones for a layout decision: 1-inner yields period 2, h1 = 400; 3-inner yields period 4, h1 = 300 with the
three offsets in order; two inner headers is refused with nothing changed (the shape that must not be guessed); a zero stride yields
nothing; and a degenerate repeat set is empty rather than a wrong answer. Asserting that the refusal leaves g untouched
(g.hdr.empty() && g.h1 == 7 && g.period == 0) is the detail that makes it safe to call on a live structure.

Head under review: 15666a0

@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 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 — "requiring layers/2 was 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).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correction to this PR's "Not verified here: a Qwen3 1.9 pack. None is on this box": that was wrong. amd/Qwen3-1.7B_rai_1.9.0_npu is a measured kFconst19Rules shape and downloads in about 1 GB (configs plus the Token .fconst). I measured it with a harness that includes pack_import.cpp (stride 27,931,456 in every row):

main  @ef865f9  vote         period=4 hdr=[4739136,4739456,7107264] post_attn_hdr=7107264
                with_header  period=2 hdr=[4739136]                  post_attn_hdr=4739136   (#1831 on a real pack)
#1838 @15666a0  with_header  period=4 hdr=[4739136,4739456,7107264] post_attn_hdr=7107264

#1833 @0777cc6 gives the same result for this pack. On it, the vote wins, so the residue path (fconst_block_headers) is not exercised here. Harness and pack are in ~/scratch-packs/cl-review-q3hdr/.

@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 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 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 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
bong-water-water-bong merged commit e9a7bf1 into main Nov 10, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the cl-review/1831-keep-header-list branch November 10, 2026 11:04

@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 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 meeting min_repeat, and std::map iteration makes the result sorted, so inner.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 why and stride = 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() == 3 and post_attn_hdr() == 340, which the old assign(1, …) code cannot satisfy; the
    "two inner headers" case asserts nothing is changed (h1 == 7 preserved), 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.

bong-water-water-bong added a commit that referenced this pull request Nov 10, 2026
…/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>
@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1831 - Partially compliant

Compliant requirements:

  • Fix the issue where locate_fconst_with_header() overwrites the per-block header list for every 1.9 pack that has a 1.9 header, including when locate_fconst()'s epsilon vote already found the layout
  • Ensure that the header-derived layout keeps every in-block header instead of only the most common residue
  • Prevent the misreading of Qwen3's 4-header blocks as 2-header blocks where the k_norm header is incorrectly used as the post-attention header
  • Apply the fix to ensure that when the vote fails, all residues that repeat are kept rather than just the most common one
  • Add a host test to verify the residue rule

Non-compliant requirements:

  • None

Requires further human verification:

  • None
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Header List Overwrite

The previous implementation of locate_fconst_with_header() would overwrite the header list determined by locate_fconst()'s epsilon vote, causing Qwen3 4-header blocks to be misread as 2-header blocks. This led to incorrect interpretation of the post-attention header as k_norm's header.

if (!g.hdr.empty()) return g;
Incorrect Residue Handling

The original code only kept the most frequent residue when determining block headers, which caused a 4-header block with three equally frequent inner headers to incorrectly pick k_norm's header instead of preserving all three.

const std::vector<int64_t> inner = fconst_block_headers(pos, stride8, 3);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

1 participant