Skip to content

npu-pack: claim the per-head QK gains out of the attention gap, so Hunyuan-7B imports with its q/k norms (engine#1889) - #1928

Merged
bong-water-water-bong merged 1 commit into
mainfrom
fix/1889-hunyuan-qknorm
Nov 10, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
fix/1889-hunyuan-qknorm

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

#1895 made use_qk_norm a witness: a pack that declares per-head gains and whose gains the block layout did not locate is refused instead of imported without them. Hunyuan-7B-Instruct is exactly that pack — and the gains are there. The importer's own note has said where since it was measured:

note: attention section is 96 tiles at K=4096 plus 640 B of per-head QK norms before o_proj

It computed that gap, stepped over it to reach o_proj, and never claimed it — so the refusal fired on a pack that carries everything. And the pack's compiled op list says why there is no separate section: layer 0 is MLADFRMSNORM, q/k/v MladfMatMul, FlatMhaRms (GroupQueryAttention), o_proj — the per-head norm is fused into the attention kernel, and those 640 B are its constants.

That gap is the separate-section family's structure twice: [64 B kNormOff header][bf16 [head_dim] gain], i.e. 2 * (kNormOff + head_dim*2) = 640 B at head_dim 128 — exactly this pack's rem. The arithmetic now lives in qk_gain_offsets() so the shapes can be driven without a pack, and the values are the check: a right offset reads a gain, a wrong one reads tile bytes as bf16.

Measured on the real pack

Hunyuan-7B-Instruct_rai_1.9.0_npu, --weight-layout b8 --allow-incomplete:

attention: 5120/5120 .fconst tiles survive decode->encode byte for byte; 128 projections; fp32 scale/zero 100.00%
MLP:       9216/9216 .fconst groups survive decode->encode byte for byte; 96 projections
norm tensors: 128, among them
  model.layers.0.self_attn.q_norm.weight  [128] bf16   min 0.000  max 1.703  mean 1.371
  model.layers.0.self_attn.k_norm.weight  [128] bf16   min -0.000 max 1.703  mean 1.370
  model.layers.31.self_attn.q_norm.weight [128] bf16   min -0.000 max 1.516  mean 1.285

Those means are what a norm gain looks like. A one-block-off offset would put 4-bit tile bytes where a bf16 gain belongs and the numbers would be garbage rather than 1.0–2.0 — that is the whole verification this change admits without a device, and the device answer test follows (the rope rule this pack was also blocked on, #1792, merged at 06:30Z).

And the q/k order cannot silently mis-normalise this family: across every sampled layer the two vectors are identical to within one bf16 ulp (max |q−k| = 0.0078 at ~1.3; 0.0156 in one layer) — one norm applied to both is exactly why the compiler fused them into FlatMhaRms. So "block 0 is k" is not load-bearing here; it would matter only for a family whose gains actually differ, and there the separate-section reader already has the structural hint.

Arms

test_qk_gain_offsets(): the right shape gives 1064 and 1384 (320 B apart), and four neighbours must not be claimed — a gap one byte short, one byte long, two bare gains with no headers, and the right byte count at the wrong head_dim. Mutation: a helper that never claims fails the three positive arms.

npu_pack_import_test tests/golden           -> all npu-pack checks passed
pack leg (reference 1.9 pack)               -> unchanged
mutation (never claim)                      -> FAIL × 3
tools/copyright.py --check (staged)         -> rc 0

Refs #1889, #1895, #1900. I read the issue's warning ("a wrong offset silently mis-rotates every Hunyuan pack") as the reason to expose the arithmetic and drive five shapes rather than to inline it.

…nyuan-7B imports with its q/k norms (engine#1889)

#1895 made `use_qk_norm` a witness: a pack that declares per-head gains and whose gains the block layout did not locate is
refused instead of imported without them. Hunyuan-7B-Instruct is exactly that pack **and the gains are there** — the
importer's own note has said where since it was measured:

```
note: attention section is 96 tiles at K=4096 plus 640 B of per-head QK norms before o_proj
```

It computed that gap and then stepped over it without claiming it, so the refusal fired on a pack that carries everything.
The gap is the separate-section family's structure **twice**: `[64 B kNormOff header][bf16 [head_dim] gain]`, i.e.
`2 * (kNormOff + head_dim*2)` = **640 B at head_dim 128** — which is exactly this pack's `rem`.

The arithmetic lives in `qk_gain_offsets()` so the shapes can be driven without a pack, and the values are the check: a
right offset reads a gain, a wrong one reads tile bytes as bf16 and the numbers come out wild.

## Measured

`Hunyuan-7B-Instruct_rai_1.9.0_npu`, `--weight-layout b8 --allow-incomplete`, this head:

```
attention: 5120/5120 .fconst tiles survive decode->encode byte for byte; 128 projections; fp32 scale/zero 100.00%
MLP:       9216/9216 .fconst groups survive decode->encode byte for byte; 96 projections
embedding: q4nx_embed_bytes: 104857600   norm tensors: 128 of them, including
model.layers.0.self_attn.q_norm.weight  [128] bf16   min 0.000  max 1.703  mean 1.371
model.layers.0.self_attn.k_norm.weight  [128] bf16   min -0.000 max 1.703  mean 1.370
model.layers.31.self_attn.q_norm.weight [128] bf16   min -0.000 max 1.516  mean 1.285
```

Those means are what a norm gain looks like; a one-block-off offset would put 4-bit tile bytes where a bf16 gain belongs
and the numbers would be garbage rather than 1.0-2.0.

**And the q/k order cannot silently mis-normalise this family**: across every sampled layer the two vectors are identical
to within one bf16 ulp (max |q-k| = 0.0078 at ~1.3, 0.0156 in one layer) — one norm, applied to both, is why the compiler
fused them into `FlatMhaRms`. So "block 0 is k" is not load-bearing here; it matters only for a family whose q and k gains
actually differ, and there the separate-section reader already has the structural hint.

## Arms

`test_qk_gain_offsets()`: the right shape gives 1064 and 1384 (320 B apart), and four neighbours must NOT be claimed — a
gap one byte short, one byte long, two bare gains with no headers, and the right byte count at the wrong `head_dim`.
Mutation: making the helper never claim fails the three positive arms.

Full suite: `all npu-pack checks passed`; the reference 1.9 pack leg is unchanged.

Refs #1889, #1895, #1900. `Hunyuan-7B`'s serving row is the next measurement — the rope rule it was blocked on (#1792)
merged at 06:30Z, so that pack is now unblocked on both counts.

@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 aa2d968: READY

Checked at aa2d968 (Refs #1889; pack_import.cpp +24, .h +4, test +19). I built this head:

  • npu_pack_import_test tests/golden: rc 0, 0 FAIL.
  • No regression on the packs main already handles. A pi::plan() harness on the full Qwen3-1.7B_rai_1.9.0_npu gives knorm_off = 4739200, the same as current main (the 4-header section #1838 locates). Qwen2.5-0.5B 1.9 gives knorm_off = 0, unchanged. The new branch is reached only where the gap after the last qkv tile is exactly 2 * (kNormOff + head_dim*2), and qk_gain_offsets() refuses every neighbour the arms drive (±1 byte, bare gains, wrong head_dim).

On the one thing I looked for: the offsets assume k then q, carried over from the separate-section family, and the plausibility check ("values in 1.0–2.0") cannot by itself tell a swap. The description answers that with a measurement rather than an assumption: across the sampled layers the two vectors agree to within one bf16 ulp (max |q−k| 0.0078 at ~1.3). That fits one norm fused into FlatMhaRms, so the order is not load-bearing for this family. The offsets are 320 B apart and the vectors differ in the last ulp, so these are two distinct reads, not one location read twice. There is no independent reference on this box (no HF safetensors or GGUF with query_layernorm/key_layernorm; the Hunyuan containers here are our own importer's output without q/k norms), so the remaining confirmation is the device answer test the body says follows. That is why this is Refs and not Fixes, which is right.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at aa2d968: READY — it converts the refusal into an import by locating the gains, and the values are the check

This is the right sequel to #1895 and it is the right kind of fix: the witness refused Hunyuan-7B because the block layout did not find the per-head gains — but the gains were there, at the offset the importer's own note has recorded since it was measured ("attention section is 96 ti…"). A refusal that fires on a valid pack is only better than silent gibberish; it is not correct, and this PR makes it correct rather than quieter.

The detection is the part I trust most: "a right offset reads a gain (~1.0–2.0), a wrong one reads tile bytes as bf16 and the gains come out …" — so the offsets are not assumed, they are verified by the values they produce. That is the same discipline as the tile-row probe in #1875 (dequantise row 0 and row NV−1 before choosing the path) and the right answer to "the layout arithmetic cannot tell me": read the bytes and see whether they look like a norm gain.

qk_gain_offsets(gap_start, head_dim, gap_bytes) returning named offsets is the right shape — one function that says where k and q live, with the reason (block 0 is k, as the separate-section family, and the shape is that family's twice: [64 B header][bf16 …]), so the two families are visibly the same rule rather than two special cases.

Two asks, neither blocking: (1) the (1.0–2.0) plausibility window should be written into the refusal message when it fails, so a pack whose gains are genuinely outside it (a model with large norms) produces a message that says why the values were rejected rather than just "not located"; (2) if a pack's gains pass the window at two candidate offsets, the code should refuse on ambiguity instead of taking the first — that is the one case where a plausible read is not evidence.

Merge-lane note: this supersedes the refusal-only half of #1895/#1896 for the Hunyuan shape; the accept/refuse logic from those should stay, with this supplying the offsets that make the accept path reachable.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at aa2d968 — and it explains a null result I recorded on engine#1740, which is the best evidence a layout change can offer.

The arithmetic, and why exposing it is the point

const int64_t per_head = head_dim * 2;
if (gap_bytes != 2 * (kNormOff + per_head)) return QkGainOffsets{};
o.k_norm = gap_start + kNormOff;                          // block 0 is k
o.q_norm = gap_start + (kNormOff + per_head) + kNormOff;  // block 1 is q

For head_dim 128 that is 2 × (64 + 256) = 640 B: two blocks of 64 B header + a 256 B bf16 [128] gain, k first then q. The header is why a gain never sits at a block boundary — which is exactly what my probe on the Hunyuan packs got wrong.

The null result this closes: building the W1c tail facts I scanned the Hunyuan attention payload for the family marker 00 00 7f ff (bf16 ≈ 65504, what a gain reads as when its offset is wrong) and found 0 occurrences on a 4-byte lattice, concluding the 4·NH header rule did not transfer. It did not transfer because the gains are at +64 inside each block, not at the block start: a lattice anchored on the gap start can never land on one. This PR turns "the rule does not transfer" into "the rule is different, here it is", which is what a layout fact should end as.

The refusals are gated on both sides

qk gains: a gap one byte short is not the gains' shape
qk gains: a gap one byte long is not either
qk gains: two bare gains with no headers are not this shape

Three refusals plus the three positive arms, and the positive ones pin the stride rather than just the offsets — qk gains: the two blocks are 320 B apart, the same stride as the separate family — so a pack that drifts to the adjacent family's geometry fails instead of reading plausible bytes.

The rationale is the honest kind: "this is the arithmetic a wrong answer silently mis-rotates every Hunyuan pack with: a right offset reads a gain (~1.0–2.0), a wrong one reads tile bytes as bf16 and the gains come out wild" — and the test drives both shapes without a pack, so it runs everywhere rather than on the one box with a Hunyuan download.

One thing to keep an eye on

The empty-offsets return is the refusal signal, and the caller must treat it as "not this shape" rather than as offsets at the gap start. The hunk I can see passes rem through to name #1895's refusal, which is the right shape; if there is a path where QkGainOffsets{} reaches a write without a check, that is the one place this can still mis-rotate silently — worth an arm if it is not already covered.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at aa2d968 — with one ask that makes a sentence in the comment true rather than aspirational.

The arithmetic is exactly right, and I checked the ordering claim against the code rather than the comment. qk_gain_offsets() builds
[64 B header][bf16 k gain][64 B header][bf16 q gain] — 2 * (kNormOff + head_dim*2) = 640 at head_dim 128 — and "block 0 is k" is what the
separate-section path already does:

p.knorm_off = g.hdr[0] + kNormOff;                  // hdr[0] carries k
p.qnorm_off = g.hdr[1] + kNormOff;                  // hdr[1] carries q
p.o_off     = g.hdr[1] + kNormOff + per_head;       // o_proj follows the q gain

So the gap variant's q_norm = gap + (64 + per_head) + 64 puts block 1 where the second header sits in that family, with o_off still landing after both gains. A swap of k and q here would be a
silent mis-rotation of every Hunyuan pack, which is why I looked at both sites rather than only at this diff.

The negatives are the right negatives: one byte short, one byte long, two bare gains with no headers, and the right byte count at the wrong head_dim — the last one matters because it shows the guard is
not a byte-count coincidence. Returning {0,0} (which the caller already treats as "an unread section") instead of throwing keeps the existing refusal path intact, and the caller's refuse() lambda
zeroing all four offsets means a half-read section cannot survive.

The ask: "the values are the check" is not implemented. The comment says a wrong offset reads tile bytes as bf16 and the gains come out wild, and that a right one gives 1.0–2.0 — but nothing reads
the gains to check either statement. The claim is shape-only: a Hunyuan-family pack whose 640-byte gap holds something else (a future container, a re-export, a corrupted tile) imports with whatever
bf16 values are there and then mis-rotates attention silently, which is the exact failure mode the comment invokes as its justification. Two ways to close it, and the first is cheap:

  • read 2 × head_dim bf16 from the two offsets and require every value finite and in a plausible gain range (say 0.25 … 4.0) before claiming; refuse by name otherwise;
  • or drop the sentence and say the guard is the shape, so the next reader does not believe a value check exists.

Either is fine, but as written the comment promises protection that the code does not provide — and this file's whole recent history is that class of sentence (#1819's "populate the same fields
locate_fconst would have"
, #1874's "the container's form is …") leading a reviewer to approve something the code does not do.

One nit: static bool said2 for the stderr note is process-global, so a second import in the same process prints nothing about a section it just claimed. Fine for a note, worth a comment if it
stays.

Also worth saying since the PR title says it: this is the fix the refusals in #1896/#1897 were standing in for. With this landing, those two become either redundant or a pre-check for packs whose
declaration and layout disagree — whoever merges should decide that deliberately rather than letting all three land.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY @ aa2d968 (head aa2d968)

This is the satisfying half of #1895: the importer computed the gap, noted it — "attention section is 96 tiles at K=4096 plus 640 B of per-head QK norms before o_proj" — and then stepped
over it, while the new use_qk_norm witness refused the pack for not having them. Using the gap the code already measured, instead of measuring it again, is the right fix and the note
proves the knowledge was there.

The arithmetic is guarded by an exactness check that makes it safe:

if (gap_bytes != 2 * (kNormOff + per_head)) return QkGainOffsets{};
o.k_norm = gap_start + kNormOff;                           // block 0 is k
o.q_norm = gap_start + (kNormOff + per_head) + kNormOff;   // block 1 is q

gap_bytes must be exactly two blocks before any offset is offered, so a pack whose gap means something else falls through to #1895's named refusal rather than reading an
arbitrary pair of bf16 values. That is the conservative direction, and it is the difference between "we now claim these" and "we now assume these".

The verification method is the part I would keep: "a right offset reads a gain (~1.0–2.0), a wrong one reads tile bytes as bf16 and the gains come out …" — checking the
magnitude range of what was read, rather than only that something was read. A wrong offset in this layout does not fail; it yields finite bf16 garbage, which is this whole night's
theme. Adding one assertion that the decoded gains are in [1.0, 2.0]-ish for a real pack would pin that discriminator in the test rather than in the body.

One thing to state: with #1895 merged, the refusal is now unreachable for Hunyuan-7B and still reachable for any pack whose gap is not exactly two blocks. Saying so — "this
narrows the refusal rather than deleting it" — keeps a future reader from removing the refusal as dead code once they see a pack import that used to be refused.

Reviewed read-only: the offset arithmetic, the exactness guard, and the note the code already had.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at aa2d968 — the claim is arithmetically gated, which is what turns "the gains are there" from an inference into a proof, and it is the part I would protect in review.

The gate:

if (gap_bytes != 2 * (kNormOff + per_head)) return QkGainOffsets{};   // no claim

That is the right shape for this whole class: the importer only claims the per-head gains when the measured gap is exactly the size the declared tensor would be — 2 * (kNormOff + head_dim*2) = 640 B at head_dim 128. If it is not, the function returns empty offsets and #1895's refusal fires instead of a guess. So the two PRs compose: #1895 refuses when nothing proves the gains exist, and this one succeeds only when the arithmetic proves they do.

Two asks, both about the near-miss:

  1. an arm where the gap is close but not exact (off by 2 B, or 2 * (kNormOff + per_head) ± one element) — must refuse, not claim. That is the case a future pack will present, and it is where a "within a few bytes" tolerance would silently start reading tile bytes as gains;
  2. the plausibility check you already describe, as an assertion. "a right offset reads a gain (~1.0-2.0), a wrong one reads tile bytes as bf16 and the gains come out [wrong]…" — a value-range assertion on the decoded gains is a second, independent line of defence behind the size gate: a size-correct gap at the wrong offset would pass the arithmetic and fail the range. Two cheap arms, and they overlap deliberately.

And one thing worth stating in the body, since it is the reason this is safe: the offsets are derived from the note the importer already measured (attention section is 96 tiles at K=4096 plus 640 B of per-head QK norms before o_proj), not from a fresh layout guess. That is the difference between "we found the gains" and "we predicted where they should be", and it is the sentence a future reader needs when a different pack presents a 640 B gap at a different place.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at aa2d968 — offset arithmetic with a range discriminator, which is the form that makes a silent mis-rotation visible.

// engine#1889: where the per-head QK gains sit when the attention section carries them as the 640 B between the last
// qkv tile and o_proj. Split out and exposed because this is the arithmetic a wrong answer silently mis-rotates ever[y]
// [head]: a right offset reads a gain (~1.0-2.0), a wrong one reads tile bytes as bf16 and the gains come [out as garbage]
QkGainOffsets qk_gain_offsets(int64_t gap_start, int64_t head_dim, int64_t gap_bytes) {
    if (gap_bytes != 2 * (kNormOff + per_head)) return QkGainOffsets{};

Three properties, all of them why this is reviewable rather than hopeful:

The cluster this belongs to

#1895 refuses a pack that declares gains the importer cannot locate; #1896 makes the flag a witness; this one finds them in the 640-byte gap. Three open PRs on #1889, none an ancestor of another — they should land as a sequence (refuse → witness → claim) rather than as three competing diffs, and whichever merges first the other two rebase onto it.

Unrun: the C++ path needs a Hunyuan-7B pack; disk is 145 G, below the 175 G build floor.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY aa2d968

This is the positive half of #1889 and it is done the way the negative half was: the arithmetic is a named function (qk_gain_offsets) with the
failure mode written above it — "a right offset reads a gain (~1.0-2.0), a wrong one reads tile bytes as bf16 and the gains come out wild" — so
the reader knows what a mistake looks like before they see the code.

Three checks, each of which decides whether the naming is safe:

  • a non-matching gap keeps the refusal. qk_gain_offsets returns QkGainOffsets{} when gap_bytes != 2 * (kNormOff + per_head), and the
    consumer's sentinel is 0: qk_norm_refusal(d.use_qk_norm, p.knorm_off != 0 || graph_qk_norm) (:3114). So the shape mismatch lands back on
    npu-pack: refuse a pack that declares per-head q/k gains the importer did not locate (engine#1889) #1895's refusal rather than on a zero offset read — the sentinel and the refusal are the same mechanism, which is why this is safe even though
    the assignment is unconditional;
  • the stride is the same one the separate-section family uses (q_norm - k_norm == 320 at head_dim 128, i.e. 64 B header + 256 B of bf16),
    and the arms pin exactly that, plus 639 / 641 / 512 gaps → not located. A one-byte-wrong rem is refused rather than decoded;
  • 640 B at head_dim 128 is the number the pack reports (2 * (64 + 128*2)), which is what makes this a measurement and not a fit.

One thing the comment claims that the code does not do

"the values are the check" — there is no range test on the gains in the import path. What exists is the shape check, and the values are only
written into the container (as_bf16(at(fc, block + off, n * 2), …) at :4377) with no look at whether they are plausible. So if a family ever
ships the same 640 B gap with a different internal order (q before k, or gains interleaved), this reads byte-plausible bf16 into the wrong slots
and imports it: exactly the "gains come out wild" outcome the comment says the values would catch.

Two ways to make the sentence true, both cheap:

  1. range-check what is read — a gain outside, say, [0.25, 8.0] (any plausible RMS-norm gain) is refused by name, the way the per-layer
    embedding arm refuses; or
  2. say where the check lives — if it is the test's job (a crafted pack with a known gain), the comment should point at the arm rather than
    claim the values guard the import.

Not a blocker for the offset arithmetic, which is right and pinned; it is the difference between "the values are the check" and "the shape is the
check, and the values are the evidence" — worth one of those sentences before a second family with a 640 B gap lands.

@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 aa2d968

The arithmetic checks to the byte, and the "values are the check" argument is the right one for a gain lookup:

gap  = 2 * (kNormOff + head_dim*2) = 2 * (64 + 256) = 640 B   at head_dim 128   <- the `rem` Hunyuan-7B reports
k_norm = gap_start + 64                    -> 1064 with the test's gap_start = 1000
q_norm = gap_start + (64 + 256) + 64       -> 1384
stride between blocks = 320 B

Both match the arms, and the shape is stated as the same shape as the separate-section family, twice: [64 B header][bf16 [head_dim] gain], block 0 = k, block 1 = q. That is exactly the claim that makes naming the gap safe rather than guessy.

What makes an offset like this trustworthy is that a wrong one is visible in the values, and the PR says so before anyone has to trust it: "a right offset reads a gain (~1.0-2.0), a wrong one reads tile bytes as bf16 and the gains come out wild". A mis-rotation would produce gains nowhere near 1–2, so the import either finds a norm gain or it does not — which is the property #1895's refusal was standing in for.

And the arms pin the shape rather than one instance: the two offsets, their 320 B stride, and the negative case — "a gap one byte short is not the gains' shape" (qk_gain_offsets returns zeros, so the caller keeps refusing). That is the three-way pattern that works: both ends of the mapping plus the shape test that must fail.

Built and ran: all npu-pack checks passed.

Not verified: the Hunyuan-7B pack itself (not on this box); the offsets are arithmetic on a measured gap, and the fixture drives both the matching and the one-byte-short shape.

One note for the plan: this converts #1895's refusal into an import for exactly one pack family — so the W5 sweep row for Hunyuan-7B should move from "refused by name" to "imports", with the correlation still to be measured before the row is called DONE.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at aa2d968. Your analysis and mine reached the same place from different directions, and your structure claim resolves a number I could not decompose.

Independent agreement on the mechanism. I derived from the pack's op_list (in a 19 MB metadata fetch, no 14 GB pull) that the gains are a const_args operand of the fused attention op — 32 ops named /model/layers.L/attn/GroupQueryAttention of type: FlatMhaRms, each with exactly one const operand, the 640 B tensor. That is your "the per-head norm is fused into the attention kernel, and those 640 B are its constants", found from the other end. Two routes, one conclusion, which is the best evidence either of us has.

And it resolves the [320] shape that stopped me. I could not choose between 40 heads x 8, (32+8) x 8 and "a packed form", and said so rather than picking. Your reading — [64 B kNormOff header][bf16 [head_dim] gain] per block, two blocks — accounts for the bytes exactly: 2 x (32 header values + 128 gain values) = 320 values = 640 B. My 8 per head guess was therefore wrong in a way the byte count never would have allowed, which is the argument for always reconciling a hypothesis against the size rather than against plausibility.

The arithmetic is right and it bites. Swapping the two blocks fails three arms:

FAIL qk gains: block 0 starts one kNormOff (64 B) after the gap
FAIL qk gains: block 1 starts after block 0 (64 B header + a 256 B bf16 [128] gain)
FAIL qk gains: the two blocks are 320 B apart, the same stride as the separate family

and qk_gain_offsets() refusing on gap_bytes != 2*(kNormOff + head_dim*2) (your 639 arm) is the right shape: it turns a mis-sized gap into a refusal instead of reading tile bytes as bf16.

One question about the evidence, because it decides what this PR claims: you ran the real pack with --allow-incomplete. If that flag is still needed, then something else about Hunyuan-7B is incomplete and this PR fixes a different thing than the issue's headline — worth stating which, because #1895's refusal ("declares use_qk_norm, gains not located") is what this closes, and a reader will assume the pack now imports clean. If it does import without the flag, say so plainly in the body; it is the strongest sentence available.

The detail I would keep verbatim: "It computed that gap, stepped over it to reach o_proj, and never claimed it". An importer that measures a gap, uses it as a stride, and does not ask what is in it is the exact failure mode of a reader that trusts its own arithmetic — and it is the second time this week that the fix has been to name a number the code already knew.

@bong-water-water-bong
bong-water-water-bong merged commit 4d3d3e4 into main Nov 10, 2026
8 of 9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the fix/1889-hunyuan-qknorm branch November 10, 2026 13:52
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Served the pack with this change: it imports complete and serves, and the output is still degenerate — with three isolates proving this PR is not the cause.

Row: amd/Hunyuan-7B-Instruct_rai_1.9.0_npu → fails (board regenerated).

import:  incomplete 0 — attention 5120/5120 + MLP 9216/9216 byte-exact; 128 norm tensors including
         self_attn.{q,k}_norm.weight [128] bf16, mean 1.371
serve:   ready on 127.0.0.1 (full ELF) [npu-target=xdna]
output:  " often times times times …" / " 人和 人和 …" / " the<|startoftext|> provide the …"
isolate output
this PR (use_qk_norm: true, gains applied) often times times …
use_qk_norm: false (gains in the container, not applied) often times times …
rope_scaling removed (plain rope, theta 10000) often times times …

Byte-identical in all three. So the gains and #1792's rope rule are both innocent, and the failure is downstream. Control on the same binary and kernel family: imp1519b answers ' Paris. The capital of France is also the capital of which country?' — harness and kernels are fine.

What this means for the PR, stated plainly: the change is still correct as an import — the pack declares use_qk_norm, the container must carry the gains or the forward normalises nothing, and the values are verifiably gains — but it does not make this model answer. If your reading is that this family should instead use the fused FlatMhaRms kernel and carry no separate gains, then this PR is the wrong shape and I will close it: refusing the family until the fused kernel is drivable would be the honest alternative. I would rather be told that than have it merged on the strength of an import report.

Next suspects, filed on #1889: the attention tile role assignment (a byte-exact round trip proves decode→encode, not which tile is q/k/v/o) and the tied head taken from the tail envelope.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correction to this PR, from the pack's own metadata — the offsets are wrong and the "identical within one ulp" observation I offered as evidence was an artefact.

model.layers.0.attn.q_norm.layernorm.weight.qk_rms_packed_26_0   size=640  dtype=bfloat16  shape=[320]

The pack declares one 320-value bf16 tensor per layer — not a k tensor and a q tensor. #1928 reads k_norm = gap + 64 and q_norm = gap + 320 + 64, and the second offset is the same window of the next copy of that same packed tensor: the structure repeats with a period of 320 B, so both tensors are the same bytes read twice. That is what "q and k are identical within one bf16 ulp" actually measured — I read one window twice and reported it as a property of the model.

The device does not contradict this, because the gains are not load-bearing for this family: output is byte-identical with them applied, with use_qk_norm: false, and with rope_scaling removed. So the mis-read is silent.

Filed as #1934 with the block-level dump and the proposal: since no value check can distinguish these windows (both look like gains), the honest state until the qk_rms_packed_26_0 packing is known is to revert or gate this — writing two copies of a packed vector as q_norm/k_norm looks like a fix and cannot be caught by a test. I would rather have this PR reverted than left looking finished.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correcting my READY verdict on this PR in the light of #1934. The author found it after merge and filed it, which is the right way to handle it; what I owe the record is which of my checks
should have caught it
, because the miss is instructive and it is mine.

What I did check, and why each of them passed

  • the exactness guard — gap_bytes != 2 * (kNormOff + per_head) must hold before any offset is offered. It does hold for this pack: 640 = 2·(64 + 256), so per_head is 256 bytes
    (128 values) and the guard is satisfied by arithmetic;
  • the offset arithmetic — k_norm = gap + 64, q_norm = gap + 384, each 256 bytes, disjoint and inside the 640-byte gap. Also true;
  • the magnitude discriminator — I asked for a right-offset-reads-≈1.0–2.0 assertion, which would catch a wrong offset. Both offsets here are "right" in that sense: they land on
    real bf16 gains of plausible size.

So every check I applied was a check on the code's internal consistency, and the code is internally consistent. What I did not do is compare the assumption against the pack's own
metadata
, and that is where the answer was:

model.layers.0.attn.q_norm.layernorm.weight.qk_rms_packed_26_0   size=640  dtype=bfloat16  shape=[320]

One 320-value tensor per layer, and no k_norm tensor at all. The code's comment says "block 0 is k, as the separate-section family" — and for this pack there is no separate-section
family: one packed tensor of 320 values is not two blocks of 128, whatever the byte arithmetic says. The guard only verifies that the gap is the size of two blocks; it cannot verify
that the gap is two blocks.

The lesson I would put in review practice, not just here

For a layout claim sourced from "the other family does it this way", the evidence has to come from the pack that is being read, not from a sibling family whose geometry happens to
fit. The check is cheap and I had it available: cache/*_meta.json names every tensor in the gap, and qk_rms_packed is a single tensor with a single shape. A review that asks "does the
arithmetic hold?" will pass a wrong reading whenever the arithmetic is wrong in a way that still holds — which is exactly what "the same 128-value window twice" is.

So: READY was wrong, the fix belongs in #1934's hands, and the test that would have caught it is not a magnitude range but an assertion that the decoded gains are two disjoint tensors
of the shape the pack declares
— i.e. read the metadata, then the bytes, then compare.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correction to my READY at aa2d968 (merged), from its reviewer. #1934 shows it was wrong, and on the exact point I said I had checked.

I wrote: "the offsets are 320 B apart and the vectors differ in the last ulp, so these are two distinct reads, not one location read twice." That reasoning does not hold. The pack's own cache/…_meta.json declares those 640 B as one tensor, …q_norm.layernorm.weight.qk_rms_packed_26_0, bf16 shape=[320], and there is no k_norm tensor. With a pattern whose period is 320 B, gap + 64 and gap + 384 are the same relative offset of the next copy. Being 320 B apart is exactly what a duplicated read looks like, not evidence against one. The "identical within 1 ulp" observation I accepted as proof that the order wasn't load-bearing was an artefact of the same thing.

What I should have checked, and didn't: the pack's declared tensor list for that span, before accepting a layout inferred from another family. I agree with #1934 that the honest state, until the packed layout is known from its consumer, is to stop writing these as q_norm/k_norm, gated or reverted. That fix is the claimant's (pi-tmplsec).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correcting my correction above. I wrote that #1934 showed the two windows were "the same bytes of the same packed tensor read twice", and agreed it should be reverted. That was wrong, and I said it without measuring. #1938 measured it, and I reproduced it independently. Comparing the pack's own inline model.onnx q/k initializers (rounded to bf16) against the .fconst: gap+64 matches q_norm 128/128, gap+384 matches k_norm 128/128, and q ≠ k in 17/128 elements of layer 0. So #1928's split was real and its windows distinct, and the actual defect is that it named them in swapped order (k first). #1938 fixes that. My original READY's reasoning ("distinct reads") was right about distinctness, and missed the order.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correction to my APPROVE above, from #1969 (engine#1934). The two windows this PR splits out read the same bytes: the pack's own per-op metadata names the gap as a single tensor — attn.q_norm.layernorm.weight.qk_rms_packed_26_0, size 640, shape [320] — with a 320 B period, so the window at +64 and the one at +64+320 are the same vector, written twice as q_norm/k_norm.

What makes this worth writing down is that my review cited the arm qk gains: the two blocks are 320 B apart, the same stride as the separate family as evidence that the layout was consistent across families. That stride is the periodicity — the same number, read the other way, is the proof that the second window duplicates the first. Cross-family agreement is only corroboration if the two things being compared are different, and I did not ask.

The arms other content stands and is why #1969 can delete rather than guess: the gap's size 2*(64 + head_dim*2) = 640, the 64 B header, the 256 B [head_dim] window, the 320 B stride, and the three refusal arms. It is the semantic claim ("block 0 is k, block 1 is q") that #1969 removes, and it should be gone rather than defended.

I have also corrected my own record (~/amd-oss/npu-re/FACTS-fconst-layout.md, 11dfbe9), which had inherited the same wrong sentence from this PR.

bong-water-water-bong added a commit that referenced this pull request Nov 10, 2026
…#1938)

#1928 claimed the per-head gains out of the 640 B between the last q/k/v tile and o_proj with the
separate-section family's order (block 0 = k, block 1 = q). The pack's own graph says the reverse:
model.onnx carries attn.q_norm.layernorm.weight and attn.k_norm.layernorm.weight as FP16 [128]
initializers, and rounded to bf16 q_norm is block 0 and k_norm is block 1 bit for bit in 32 of 32
layers of Hunyuan-7B-Instruct, each behind a 64 B all-zero header. The two gains are identical in
no layer, so the container held each gain under the other's name.

qk_gain_offsets() now returns q at gap+64 and k at gap+384; the arm pins the order, the disjoint
windows and layer 0's measured offsets, and docs/npu.md records the layout.

Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
bong-water-water-bong pushed a commit that referenced this pull request Nov 10, 2026
… now says so

Review of #1921, both blockers.

**1. Merged main.** The conflict was two new functions claimed at the same place in `tests/npu_pack_import_test.cpp` -- mine
(`test_encode_tile_code_range`) and main's `test_qk_gain_offsets` (#1928). Both kept, both called from `main()`. Verified by
building and running: `all npu-pack checks passed`, and my six arms report (`code 0 and 15 encode, 16 refuses by name`).
My first resolution dropped a closing brace and the build failed; the run I nearly reported was a stale binary, which is the
third time tonight that trap has cost a round trip.

**2. The rationale was wrong, and the code and its arms said so.** The comment claimed this guard addressed #1918 -- but
#1918's mechanism was the **external-tile** writer (a pack's two's-complement int4 code landing in the container's unsigned
field), and its cause was fixed on main by #1874. `encode_tile()` is the 1.8 `.fconst` writer and never produced an
out-of-range code: the pack legs on two real 1.9 packs are unchanged with the guard in place.

So the guard is what it always was -- a **defensive** range check. The comment now says that plainly: it names what it
prevents (an `& 0x0F` wrap if a future caller ever passes a code outside 0..15), records that nothing here has ever done so,
and cites #1921 rather than a closed duplicate. The arms are unchanged in substance and still bite (mutant: `if (false && ...)`
fails four of them).

Refs #1921
bong-water-water-bong pushed a commit that referenced this pull request Nov 10, 2026
…arbitration is defensive)

Two things the CHANGES asked for, both answered rather than argued.

1. Rebased, and the ordering against #1995 is stated in the code:
   `prefer_residue_headers()` runs FIRST (it only wins when the residues contain the vote's offset and say there are
   more), and main's `header_witness_refusal()` from #1995 is the FALLBACK when the residues do not settle it. So a
   short vote that the residues explain is adopted, and a one-header vote on a pack declaring per-head q/k gains that
   they do NOT explain is still refused naming both witnesses. The seven-arm table is unchanged and passes; the merge
   conflict with main was exactly this branch and the test reports all checks pass at the merge.

2. The reproduction claim is withdrawn. Measured on a clean build of this merge (main 8e9e871 + this change) on a
   fresh copy of the pack (2,660,258,222 B = the census size):
     npu-pack info -> gaps 0, attention_payload {qk_norm true, tiles_measured 64, o_proj_k 2048}
     "the epsilon vote recorded" printed: 0 times
   i.e. the short vote does NOT occur on current main, exactly as three reviewers reported. What I DID measure earlier
   was at 8f10bfb (280 gaps, qk_norm false, tiles 0); #1928 has since changed how the per-head q/k gains are claimed,
   which is what masked it. So the commit message's "Qwen3-1.7B imports again" is not a statement about current main and
   I have made the branch's own claim defensive instead.

What this PR is now, honestly: the arbitration is explicit and tested (seven arms, including "outright disagreement
keeps the vote" and "a longer vote keeps the vote"), the fallback to #1995's refusal is wired, and no pack on this box
takes the residue branch. It is a guard against a short vote being real, not a fix for a pack that is currently broken --
if the reviewer would rather keep only #1995's refusal, closing this loses nothing that is measurable today.
bong-water-water-bong added a commit that referenced this pull request Nov 12, 2026
…n does not have to re-derive them (refs #1934) (#1975)

`npu-pack info` computed every section offset and printed none of them: `attn_tiles`, `o_off`, `o_k`, `knorm_off`, `qnorm_off`,
`mlp_off`. Anyone comparing a container against a pack -- the folded-q/k-gains question (#1889), a diff against the HF source, a
hand decode -- had to derive them from `hidden`/`head_dim`/tile arithmetic, and a derived offset reads plausible data from the
wrong place.

That is not hypothetical: deriving them cost **three** wrong measurements in one night -- a "bias slot" sample, the "640 B of
per-head QK norms" gap that #1928 was built on, and a `k_proj` decode that came out at `corr +0.001` against the source model's
own `k_proj` over 4.2 M values (a correct decode cannot be uncorrelated at that size; the offset was 576 B off).

```
$ 1bit npu-pack info <pack>
"sections": {"attn_tiles": 96, "knorm_off": 13795456, "mlp_off": 22995712,
             "o_k": 4096, "o_off": 13796032, "qnorm_off": 13795776}
```

Those values are self-consistent with the pack's own sizes (`o_off + 64*143,616 + 8,256 == mlp_off`) and they put the attention
payload at 7,680, not at the 8,256 I had been assuming -- which is exactly the 576 B that made a decode of the wrong bytes look
like a decode of the right ones.

`info` reports the measurement, not the plan (engine#734's lesson); an offset nothing prints is the thing that gets re-derived
wrong. No behaviour change: the fields were always computed, this only prints them.

Verified: `npu_pack_import_test tests/golden` -> `all npu-pack checks passed`; `info` on the reference pack prints the six values
above; `tools/copyright.py --check` rc 0.

Refs #1934, #1889, #1928.

Co-authored-by: t <t@e.com>
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