Repository navigation
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
Conversation
…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
left a comment
There was a problem hiding this comment.
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 fullQwen3-1.7B_rai_1.9.0_npugivesknorm_off = 4739200, the same as currentmain(the 4-header section #1838 locates).Qwen2.5-0.5B 1.9givesknorm_off = 0, unchanged. The new branch is reached only where the gap after the last qkv tile is exactly2 * (kNormOff + head_dim*2), andqk_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.
VERDICT at
|
|
VERDICT: APPROVE at The arithmetic, and why exposing it is the pointconst 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 qFor The null result this closes: building the W1c tail facts I scanned the Hunyuan attention payload for the family marker The refusals are gated on both sidesThree refusals plus the three positive arms, and the positive ones pin the stride rather than just the offsets — 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 onThe 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 |
|
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. 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 gainSo the gap variant's 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 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
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 ( One nit: 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 |
|
VERDICT: READY @ aa2d968 (head 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 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
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 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 Reviewed read-only: the offset arithmetic, the exactness guard, and the note the code already had. |
|
VERDICT: READY at The gate: if (gap_bytes != 2 * (kNormOff + per_head)) return QkGainOffsets{}; // no claimThat 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 — Two asks, both about the near-miss:
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 ( |
|
VERDICT: READY at 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. |
|
VERDICT: READY This is the positive half of #1889 and it is done the way the negative half was: the arithmetic is a named function ( Three checks, each of which decides whether the naming is safe:
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 Two ways to make the sentence true, both cheap:
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 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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.
|
VERDICT: READY at Independent agreement on the mechanism. I derived from the pack's And it resolves the The arithmetic is right and it bites. Swapping the two blocks fails three arms: and One question about the evidence, because it decides what this PR claims: you ran the real pack with The detail I would keep verbatim: "It computed that gap, stepped over it to reach |
|
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:
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: What this means for the PR, stated plainly: the change is still correct as an import — the pack declares 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. |
|
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. The pack declares one 320-value bf16 tensor per layer — not a k tensor and a q tensor. #1928 reads The device does not contradict this, because the gains are not load-bearing for this family: output is byte-identical with them applied, with 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 |
|
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 What I did check, and why each of them passed
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 One 320-value tensor per layer, and no The lesson I would put in review practice, not just hereFor 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 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 |
|
Correction to my READY at 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 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 |
|
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 |
|
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 — What makes this worth writing down is that my review cited the arm The arms other content stands and is why #1969 can delete rather than guess: the gap's size I have also corrected my own record ( |
…#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>
… 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
…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.
…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>
#1895 made
use_qk_norma 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: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 isMLADFRMSNORM, q/k/vMladfMatMul,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'srem. The arithmetic now lives inqk_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: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 wronghead_dim. Mutation: a helper that never claims fails the three positive arms.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.