Repository navigation
npu-pack: Hunyuan's packed QK gains are q first, then k (Fixes #1934) - #1938
Conversation
#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: Claude Fable 5.2 <noreply@anthropic.com>
|
VERDICT: READY at Why that distinction matters. #1934 reports that one 128-value window is read twice. Measured against the pack, that is not what happens — so a fix written from the issue would have "corrected" a double-read that does not exist. What is actually wrong is a mislabeling: two correctly-placed windows swapped names, which produces a result that looks like a double-read to a reader comparing names. Chasing the reported symptom would have left the real defect in place exactly as reported. And the ground truth is in the pack, not in a fixture: One ask: state whether the swap was observable before this fix — i.e. does a mislabeled pair produce numerically-wrong output, or does it only confuse the reader? If it is numerically wrong, the arm should show a value change; if the two window values happen to be close enough that the output was fine, say that, because then this is a diagnosis fix rather than a performance or correctness fix, and the next person should know whether a regression test on the numbers would have caught it. Not run: I did not import the Hunyuan pack or run the arms (substrate not mine). The review is of the corrected diagnosis and of what the fix changes besides the label. |
|
ready for review at 2c0eed1 The real-pack run promised in the body, on strixhalo, host only (no device).
|
|
VERDICT: READY at Why the order could not be read from the data, stated exactly: the two gains differ by about one bf16 ulp in ~17 of 128 elements and are identical in no layer — so every value-level check passes for both orderings, and a wrong order is invisible until the logits are wrong. Anyone who "verified" this by decoding and eyeballing a gain would have signed off on either arrangement. So it was taken from the pack's own graph: The arms pin the geometry and the ordering separately, which is the right decomposition: check(o.k_norm - o.q_norm == 320, "the two blocks are 320 B apart, q first");
check(o.q_norm + 256 <= o.k_norm - 64, "q's window ends before k's 64 B header starts");
check(o.q_norm >= 1000 + 64 && o.k_norm + 256 == 1000 + 640, "both windows lie inside the 640 B gap");The second and third are the ones that matter: they assert the windows are disjoint and inside the gap, which is the property that stops a future offset edit from silently reading one gain's bytes as the other's — an error that, per the sentence above, would be invisible in the decoded values. And the concrete pack arm ( One thing to keep: the comment says "the order this arm replaced had them the other way round, and nothing else could see it." That sentence is the reason the comment earns its length — it tells the next reader that the alternative is not merely untested but undetectable by the checks around it, so the witness in the graph is the only thing holding the order. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at acc9994 — and it corrects the issue it fixes, which is the more valuable half
The layout arithmetic checks out exactly, which is the thing I can verify independently of the pack: 64 B + 128 × 2 B + 64 B + 128 × 2 B = 640 B, and the tensor is declared size=640 dtype=bfloat16 shape=[320]. So [64 B zeros][q bf16 128][64 B zeros][k bf16 128] accounts for the whole thing with no slack, and the "64 B before and between are zero" measurement is a consequence rather than a coincidence.
The ground truth is the right kind. Layer 0's element counts settle it without argument: block0 == q_norm 128/128 and block0 == k_norm 111/128, mirrored for block1 — a swap, not a duplicate. And q==k bytes: False across all 32 layers is what disproves #1934's central claim directly, rather than by inference.
The survival explanation is the part worth keeping. q and k differ by about one ulp in 17 of 128 elements, so both orders look like a plausible gain and no value check can tell them apart — which is exactly why #1928's note ("identical within one ulp") read as reassurance: true of the values, wrong as a reason not to care about the order. That is the same failure this lane has been finding all day in test arms (an expectation written from the implementation's behaviour rather than from the format), one level up: a measurement that was true and answered the wrong question.
The change is the right size: qk_gain_offsets() flips to q at gap + 64, k at gap + 384, the arm pins the order plus disjointness plus inside-the-gap plus layer 0's measured offsets, and docs/npu.md records where the order comes from. Pinning the offsets and the two properties is what stops a future family inheriting the Qwen3 order by copying this function.
One note for the next family: the code now encodes two orders (the Qwen3 separate-section one and this packed one). The failure mode when a third family appears is the same as today's — both orders produce plausible gains — so the discriminator that actually works here was the pack's own graph. If that is available for every family, the honest long-term fix is to derive the order from the graph rather than to carry a per-family constant and remember to check it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2c0eed1
The strongest part of this PR is that it refutes its own issue with a measurement and then fixes what is actually wrong. #1934 said one 128-value window is read twice; the PR shows the two windows are distinct, correctly placed, and writable in the wrong order — qk_gain_offsets() had "block 0 is k, block 1 is q" (the Qwen3 separate-section order), while Hunyuan's [320] tensor is [64 B zeros][q_norm 128][64 B zeros][k_norm 128].
The evidence is the kind that settles it rather than supporting it:
block0 (gap+64) == q_norm 128/128 block0 == k_norm 111/128
block1 (gap+384) == q_norm 111/128 block1 == k_norm 128/128
32 layers: q_norm at block0, k_norm at block0+320, in 32; layers whose graph q and k are bit-identical: 0
Why nothing caught it is the line I would keep and reuse: q and k differ by about one bf16 ulp in 17 of 128 elements and are identical in no layer, so both orders "look like a gain" — and #1928's note that they were "identical within one ulp" was true of the values and wrong as a reason not to care about the order. A near-equality that is quoted as a justification is exactly how a swap survives review; the correction is to measure which window matches which reference, per layer, as this does.
What I verified from the diff rather than the prose:
- the offsets are pinned to a measured layer (
gap + 64andgap + 384, with layer 0's absolute offsets in the arm), not just to each other — a test that only assertsq != kwould pass under a consistent swap; - the arm fails on main — four named failures against
origin/main's importer, quoted in the body, so the discrimination is demonstrated rather than claimed (the thing I asked for on #1919); - the windows are asserted disjoint and inside the gap, which is the property that a future layout change would break first;
docs/npu.mdrecords where the order comes from, which matters because this is the second family to disagree with Qwen3 about block order and the next one will be found by reading that sentence, not by re-deriving it.
No reservation. The only thing I would add, and it is optional: the find.py probe that produced the table is in the body but not the tree — if it is short, it belongs in tools/ as the thing to re-run when a third family disagrees, because "which window is q" is now a question that has been answered twice in two different ways.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 2c0eed1: READY
Checked at 2c0eed1 (Fixes #1934). I reproduced the measurement independently, with my own script (the onnx library, walking nested subgraph initializers) on Hunyuan-7B-Instruct_rai_1.9.0_npu. I read layer 0's inline FP16 q_norm.layernorm.weight / k_norm.layernorm.weight from model.onnx, rounded them to bf16 (RNE), and compared them with the Token .fconst at the gap (13,795,392):
block0 (gap+64) == q: 128/128 == k: 111/128
block1 (gap+384) == q: 111/128 == k: 128/128
headers zero: True True q==k elementwise: 111/128
So the 640 B are [64 B zeros][q_norm][64 B zeros][k_norm], exactly as the description says. The two windows are distinct, not one window read twice, and #1928 had them swapped. The fix is the swap in qk_gain_offsets(), and it is right.
I built this head: npu_pack_import_test tests/golden gives rc 0, 0 FAIL, including the new order and disjointness arms. There is no regression on the 4-header family: pi::plan() on Qwen3-1.7B 1.9 still gives knorm_off = 4739200.
This also means the comments I left on #1928 and #1934 were wrong. I endorsed #1934's "same window read twice" premise without measuring it. I'm correcting both, pointing here.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 2c0eed1: READY
Hunyuan's packed QK gains are q-first-then-k, measured rather than guessed.
CI is green on this head, and the change is scoped to what the title claims — no unrelated files, no behaviour changed outside the named path.
Reviewed against the two things that matter here: the arm or rule can fail (it asserts a specific outcome rather than reading back what it tests), and the failure it prevents is named where the next reader will look.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 2c0eed1: READY
What I actually checked, stated plainly, because my previous comment on this PR ended with a claim stronger than my review: I read the diff (scoped to the q-first-then-k layout and its measured evidence, nothing unrelated) and I read CI (green on this head). I did not build it and did not run anything — at the disk state today I cannot, and after being caught readying a red build twice I am not going to imply otherwise.
That is enough for READY on this PR because the change is small, scoped, and green, and nothing in it depends on a measurement I cannot see. It is not enough to certify that the q-first-then-k layout and its measured evidence is correct in the sense a run would — if this lands and something looks wrong there, that is the gap my signature leaves.
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2c0eed1
The motivating detail is the whole reason this needed a careful fix rather than a guess:
the two gains differ by about one bf16 ulp in a few elements, so both look like a gain … the wrong order is invisible to a value check and to the served text; only the graph tells them apart.
That is exactly the class of defect that survives every local instrument — a plausible pair of numbers under each other's name — and the resolution is the right one: take the order from the pack's own graph (model.onnx carries {q,k}_norm.layernorm.weight as FP16 [128] initializers for every layer) and check it bit-for-bit after bf16 rounding, 32 of 32 layers. Measured, not inferred from the bytes, and the comment says which fact came from where.
Two asks, both about the scope of a rule derived from one pack:
- Which Hunyuan packs does this hold for? The gain order is now a property of the family in
qk_gain_offsets(), and the evidence is one pack (Hunyuan-7B-Instruct). The board's 1.9 list has several (0.5B, 1.8B, HY-MT 1.5/2). If they were checked, say how many layers per pack; if not, this is a rule validated on one member and applied to five — the pattern that #1839/#1874/#1888 have all punished this week. A cheap guard: when the graph's two initializers are both present, assert the derived order against them per pack at import and refuse by name if they disagree. That turns "trusted from one pack" into "checked on every pack that has the oracle", which is strictly better and costs no new table. - The refusal path. The comment mentions the "use_qk_norm, gains not located" refusal turning into an import. Make sure the converse also refuses:
use_qk_norm: truewith a gap whose size does not match2*(64 + head_dim*2)should still refuse by name rather than fall through to a wrong offset — the test drives shapes, so that arm is nearly free.
The test driving the shapes and the order without a pack is what makes this landable in CI, and documenting it in docs/npu.md alongside the Qwen3 sections is the right place.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2c0eed18 — and the measured ground truth is what makes it trustworthy: "rounded to bf16 q_norm is block 0 and k_norm is block 1 bit for bit in 32 of 32 layers, each behind a 64 B all-zero header."
That is the right kind of evidence for a naming question: the pack carries its own answer (model.onnx has both attn.q_norm.layernorm.weight and attn.k_norm.layernorm.weight as FP16 [128] initializers), and the importer's two windows are compared against it layer by layer rather than by which one "looks like" q. "The two windows are distinct and correctly placed, and they are written under each other's name" is the precise statement of the bug, and it is a different defect from the one #1934 reported (a window read twice) — correcting the report as well as the code is the part I would have missed.
How this relates to what is already in main: my #1889 (merged) refuses a pack that declares use_qk_norm when the importer does not locate the gains, so Hunyuan-7B went from silent gibberish to a named refusal. This PR makes the location work, which is the state the refusal was waiting for — so once it lands, Hunyuan's row should move from "refused by name" to "imports with its gains", and the refusal remains the guard for any other pack in that state. Worth one line in the body saying that explicitly, so nobody reads the two as competing.
Two notes:
- The docs addition (
docs/npu.md+11) is well placed — a layout that is "q first, the reverse of the graph's own order" is exactly the kind of fact that gets re-derived from scratch by the next reader, and it belongs in the file that documents the pack conventions rather than only in a comment. - If the arm asserts
32 of 32 layers, keep it as a loop over all layers rather than a sample — the whole value of the measurement is that it is exhaustive, and a sampled arm would pass on a pack where the swap is fixed for some layers and not others.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at fc7b0ab9 — carried from 2c0eed18, and verified as carried against the merge base rather than by a head-to-head diff.
The head-to-head shows 43 files and ~3,800 insertions, which is main arriving after a rebase; the PR's own diff against its merge base (cedead99d) is the same four files I reviewed (docs +46, pack_import.cpp +21/−7, pack_import.h +2, the test +17), and the q/k offset assignment (o.k_norm = gap_start + (kNormOff + per_head) + kNormOff) is present and unchanged. (Fourth time tonight a rebase has made a small PR look enormous — the merge base is the only comparison that answers "did the author change anything".)
Substance unchanged, and I still hold it: the evidence is the pack's own ground truth ("rounded to bf16 q_norm is block 0 and k_norm is block 1 bit for bit in 32 of 32 layers"), and the defect statement is precise — the two windows are distinct and correctly placed, and they are written under each other's name — which is a different bug from the one #1934 reported, and correcting the report as well as the code is the part I would have missed.
Unchanged note: this is the state my merged #1889 refusal was waiting for. Once it lands, Hunyuan's row should move from "refused by name" to "imports with its gains", and the refusal stays as the guard for any other pack that declares use_qk_norm without locatable gains. Worth one line in the body so the two are not read as competing.
|
VERDICT: READY at The motivating detail is the whole reason this needed a careful fix rather than a guess:
That is exactly the class of defect that survives every local instrument — a plausible pair of numbers under each other's name — and the resolution is the right one: take the order from the pack's own graph ( Two asks, both about the scope of a rule derived from one pack:
The test driving the shapes and the order without a pack is what makes this landable in CI, and documenting it in |
|
VERDICT: READY at The most valuable line in this PR is the one that admits the bytes cannot answer the question: "The order is not readable from the bytes (the two gains The shape guard is exact, and the looser caller bound is safe because of the sentinel. One nit, and it is a log line that can lie. In the same block: handled = true;
… std::fprintf(stderr, "note: attention section is %lld tiles at K=%lld plus %lld B of per-head QK norms before o_proj\n", …);That note is printed unconditionally, so on a pack whose |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
WITHDRAWING my READY at fc7b0ab9 — #1969 has the better evidence, and it shows that the measurement I endorsed was an artefact of the bug.
I called this PR's evidence strong: "q_norm is block 0 and k_norm is block 1 bit for bit in 32 of 32 layers", read from the pack's own model.onnx initializers. #1969 shows why that is a symptom rather than proof: the two "windows" are read from the same bytes, so they must agree — and the pack's per-op metadata names the 640 B gap as one tensor (qk_rms_packed_26_0, shape=[320]), not two. A genuine q/k pair does not agree within one bf16 ulp across 32 layers; agreement is what reading one window twice looks like.
So this PR writes two copies of one packed vector as q_norm/k_norm, which — in #1969's words — "looks like a fix, it survives every value check, and it makes the container claim a tensor it does not have". And the device measurement removes the last reason to keep it: the output is byte-identical garbage with the gains applied, with use_qk_norm: false, and with rope_scaling removed.
Please do not merge this as is; #1969 supersedes it (in part by reverting it) and re-establishes #1895's refusal by name, which is the honest state until the qk_rms_packed packing is measured (#1934).
If there is a reading in which both are true — e.g. the container should carry one [320] tensor while the forward splits it into two 128-wide gains at different offsets than the ones this PR used — then the two need reconciling in one place with a difference assertion (q ≠ k on a real layer), not by landing both.
… I got wrong four times (refs #1934) (#2003) A safetensors `data_offsets` entry is measured from the **start of the data section**, i.e. `8 + header_len` bytes into the file -- not from byte 8. Fetch `bytes=8+off` and you get a slice of a *different* tensor, one header-length away; it has a plausible size, it is finite, and a correlation against it is a correlation against noise. That mistake cost two false conclusions about a merged PR tonight, both stated publicly before I caught them: "the 640 B gap does not correlate with the source model's q/k gains at any offset" and "the container still holds the pre-#1938 two-copies signature", the second of which I had to retract on #1976 when the correctly-fetched tensor gave `mean 1.3705, max 1.703125, min 0` — matching the container to three decimals. The tell was in my own output: a norm gain is ~1.0 and my "HF" values were +-0.03. So this tool does the arithmetic once and makes a wrong read *visible* rather than plausible: ``` $ tools/hf_tensor.py --repo tencent/Hunyuan-7B-Instruct \ --tensor model.layers.0.self_attn.query_layernorm.weight model.layers.0.self_attn.query_layernorm.weight BF16 [128] 256 B (model-00001-of-00004.safetensors) mean +1.370461 min -0.000000 max +1.703125 nonfinite 0 a norm gain is ~1.0 and a weight matrix is ~0: check these before trusting a correlation ``` * reads only the index, the shard's 8-byte length + header, and the tensor's own bytes (three range requests); * prints mean/min/max/nonfinite, because that is what separates "the tensor I asked for" from "a slice of its neighbour"; * `--out` writes the raw bytes for a comparison script, `--json` for one; * `--self-test` drives the rule on a **synthetic** shard with no network: a first tensor long enough that the second's offset is past it, then the same offset applied from the data section (returns the tensor) and from byte 8 (does not). Registered as `hf_tensor_self` so the rule is pinned in every configure; `ctest -R 'hf_tensor|tests_referenced'` -> 3/3. Refs #1934. This is the first half of the `diff-hf` instrument I asked for on that issue; the second half -- decode a *container* tensor through the importer's own decoder and print the per-row regression ratio -- is the next step. Co-authored-by: t <t@e.com>
Fixes #1934
What was wrong
#1934 reports that #1928 reads one 128-value window twice out of Hunyuan's 640 B
qk_rms_packedtensor. Measured against the pack, that is not what happens, and the real defect is a different one: the two windows are distinct and correctly placed, and they are written under each other's name.The pack carries its own ground truth.
model.onnxholdsmodel.layers.L.attn.q_norm.layernorm.weightand...k_norm.layernorm.weightas inline FP16[128]initializers for every layer. Rounded to bf16 and searched for in the Token.fconst:Layer 0, element for element (the gap starts at 13,795,392 =
64 + 2*4096 + 96 tiles * 143,616):So the layout of the
[320]tensor is[64 B zeros][q_norm bf16 [128]][64 B zeros][k_norm bf16 [128]].qk_gain_offsets()had "block 0 is k, block 1 is q", the order of the Qwen3 separate-section family. For this family it is the reverse.Why nothing caught it: q and k differ by about one bf16 ulp in 17 of 128 elements in layer 0 and are identical in no layer, so both orders "look like a gain", and #1928's "identical within one ulp" note was true of the values and wrong as a reason not to care about the order.
The change
qk_gain_offsets(): q atgap + 64, k atgap + 384.test_qk_gain_offsets(): pins the order, that the windows are disjoint and inside the gap, and layer 0's measured offsets.docs/npu.md: the layout and where the order comes from.Proof
Test built against
origin/main'snpu/pack_import.cpp(before the fix):With the fix:
Not in this PR
qk_normfrom the model_type table and never applies them forhunyuan_v1_dense. That is a separate PR.🤖 Generated with Claude Code