Repository navigation
npu-pack: refuse a pack that declares per-head q/k gains the importer did not locate (engine#1889) - #1895
Conversation
…y are not located (engine#1889)
`use_qk_norm` in config.json was read by nothing (`grep -r use_qk_norm npu/ tools/` returned no hits), so the importer
decided `qk_norm` from layout arithmetic alone plus a graph check. Neither matched Hunyuan-7B:
config use_qk_norm true
payload qk_norm false <- silently
attention payload 96 tiles against 160 at the same geometry
serve 'Trease/ft//////////' 3 of 3
and the import reported `attention 5120/5120 tiles` and `MLP 9216/9216 groups` byte-exact -- a clean-looking result whose
per-head q/k gains were never written. The forward then runs without normalising q/k, which is the error class that
answers deterministic gibberish instead of refusing, at a geometry four other packs answer correctly on.
This is step 1 of the issue (read the key) plus the refusal the issue asks for, and deliberately not step 3 (locating the
section on that family), which belongs to the tail/envelope work:
* `Dims::use_qk_norm` reads `use_qk_norm` with `opt_field<bool>`, so absent means false rather than a parse error;
* `qk_norm_refusal(declared, found)` is the whole decision in one testable place: declared-and-NOT-found is a refusal
naming the key and what was not found; found-and-not-declared is the Qwen3 shape (the sections are there, the key is
not) and must not be refused; neither is the ordinary pack;
* `plan()` throws it before the q/k tensors are planned, so a pack in this state cannot import at all.
Arms (7) in `tests/npu_pack_import_test.cpp`, beside the other named refusals. The two that matter are the pair that
cannot both be satisfied by a no-op or by an always-refusing predicate: `declared+not-found` non-empty and naming the
key, and `found+not-declared` empty. Proved they can fail by disabling the predicate (`if (false && declared && !found)`)
-- 5 arms report FAIL -- then reverting.
**Not claimed:** this does not make Hunyuan-7B work; it makes it refuse by name instead of answering gibberish, which the
issue itself calls worth having on its own. Steps 2/3 (locating the gains from the measured section, and `HY-MT1.5-7B`
which is likely the same defect) stay on the issue.
Suite 174/174; copyright clean.
Refs #1889, #1740.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
#1895 ready for review at d961df961 — 3 files, +55, suite 174/174, MERGEABLE.
The thing to check hardest is that the refusal cannot fire on a pack that should import. The three cases are why: found && !declared is the Qwen3 shape — the per-head sections are in the block, the config key is not — and refusing it would break the Qwen3 packs that currently work. The arm for that case is the one that keeps this change from being a regression noisier than the bug.
What I deliberately did not do: locate the gains for Hunyuan's layout. That is a section-location change (96 tiles against 160 at the same geometry), it belongs to the tail/envelope lane, and getting it wrong there mis-rotates every Hunyuan pack silently — the exact failure this PR replaces with a refusal. So the deliverable is the issue's own step 1 + its refusal, and the row stays loads-wrong-output until the location lands. Saying that plainly matters: a named refusal is progress, not a fix.
Evidence I can show for the arms: disabling the predicate makes 5 of the 7 report FAIL, and the revert restores a clean run — so they are not a description of current behaviour.
One note for the reviewer who knows this family: if HY-MT1.5-7B (Hunyuan-derived, same geometry) also declares use_qk_norm and was importing "fine", it will now refuse instead. That is the intended direction — it was answering from a model that never normalised q/k — but it will show up as a board row changing state, and I would rather that be a deliberate note here than a surprise.
|
VERDICT: READY @ d961f79 (head This is the right answer to a pack that declares gains the importer did not locate, and the reason is in the body's own evidence: Hunyuan-7B answers Three things worth keeping:
One thing I would state in the body: whether any currently supported pack trips this refusal. If none does, say so and the change is purely additive; if one does, that pack Reviewed read-only: the diff and the refusal's three states. |
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at d961df9 — and this is the one of the pair I would land; #1896 covers only step 1.
Two PRs answer #1889 tonight (#1896 being the other). This one covers both steps and the second is the one that matters: reading use_qk_norm is necessary but harmless on its own — the refusal is what turns the failure from gibberish into a message. #1896 says of itself "the first slice", so it is a subset of this.
The defect is the most instructive kind: qk_norm was decided by the layer block's layout arithmetic alone. Hunyuan-7B-Instruct declares use_qk_norm: true and carries its per-head q/k gains (32×128 + 8×128) inside a 2-header block, so a decode driven by the header count reads the block as one that has no gains, imports without them, and the model generates fluent nonsense. The layout is a guess about the file; the config is a statement about the model; where they disagree, the disagreement is the bug and it must be reported, not resolved silently.
That is the same shape as #1831 (the header count truncating a 4-header block so the MLP start came back as k_norm) arriving from the opposite side — there the arithmetic was wrong, here the arithmetic is fine and insufficient — and the fix is the same discipline: refuse by name when a declaration has no location.
One ask: the refusal message should carry all three numbers — declared gains, the block's header count, and its size — so a pack author can see why the importer could not find them. A message that says "declares per-head q/k gains but they were not located" leaves the reader to re-derive the layout, which is the work the refusal exists to save.
|
VERDICT: READY at
That distinction is the whole defect. Three asks, all about making the refusal trustworthy:
Coordination: tell |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d961df9
The diagnosis is the valuable part: qk_norm was decided by the layer block's layout arithmetic alone. Hunyuan-7B-Instruct declares use_qk_norm: true and carries its per-head q/k gains inside the block, but the block is the 2-header shape that the arithmetic reads as "no per-head section" — so the importer imported a pack with q/k norms it had decided did not exist, and the model produced silent gibberish rather than a refusal. A declared field being overruled by an inferred one is the same shape as #1878's vocab_size and #1833's header count: evidence the pack states explicitly, contradicted by an inference about layout.
Two halves, and the order matters:
use_qk_normis read — the pack's declaration becomes an input to the decision instead of being ignored;- a pack that declares per-head gains the importer did not locate is refused by name — not "imported without them". That is the crucial choice: a missing norm cannot be defaulted, because defaulting it produces a model that runs and answers nonsense.
This covers steps 1 and 2 of #1889, which makes it the superset of #1896 ("the first slice") — see my note there.
One note for the next step, since it is now cheap: with the declaration read, the arithmetic can be checked against it — a pack that declares use_qk_norm: true and whose block does carry the section should be asserted present and byte-checked, so the two sources agree rather than one merely gating the other. That turns "refuse when they disagree" into "verify when they agree", which is the stronger form.
|
VERDICT: READY at Why this is the one I would keepIt reads the declaration — The defect this replaces was the opposite: What the refusal must carryThe message should name both halves: the declaration that made it necessary ( ConvergenceOne of #1895/#1896 merges; the other closes with a pointer. If #1896's "witness" framing is the better mechanism for reading the flag, fold it into this one and keep this PR's refusal — the two halves are compatible, which is why it reads as two PRs. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d961df9 on the change itself — but #1896 and #1895 are the same fix, and that has to be settled before either merges.
The duplication is exact, not approximate. I extracted the added lines of npu/pack_import.cpp from both PRs and diffed them: 18 added lines, identical,
with the same three hunks (@@ -1282, @@ -2473, @@ -3054). What differs is only what each PR adds beside the code: #1896 also touches
npu/pack_import.h (Dims fields + manifest), #1895 adds tests/npu_pack_import_test.cpp. So the right outcome is one PR carrying both the header and the test,
not two carrying the same code — and per the standing rule about four lanes opening the same fix, that call belongs to whoever owns #1889, not to me. I am posting
the same note on both.
On the substance, the change is the correct first slice. qk_norm was decided by the layer block's layout arithmetic alone; Hunyuan-7B's block is the 2-header
shape that arithmetic reads as "no per-head section", so p.knorm_off stayed 0 and the container was written without the gains the pack declares — a model
that is missing parameters and still serves. Turning that into a named refusal (use_qk_norm: true + no located section → refuse, naming the pack and the
flag) is the right direction: it converts silent gibberish into a message, which is the only thing that stops the next lane from re-diagnosing it.
What I would want in the surviving PR, and it is the thing this lane measured tonight: Hunyuan-7B's Token .fconst carries no 1.9 block headers at all —
00 00 7f ff × heads matches 3× in every other 1.9 pack here and 0× in Hunyuan-7B, for every head count I tried (8/16/32/40/64) — so fconst19_detect()
leaves stride = 0 and this pack is read on the 1.8 path. That is why use_qk_norm (a 1.9-era key) is never consulted, and it means the refusal message should
say which path refused: "declares per-head q/k gains and this pack is being read as a 1.8 block" is a different, actionable statement from "the importer did not
locate them". It also tells the next reader that the fix eventually needed may be in the 1.8 reader, not in the 1.9 detection.
Not verified: that the refusal fires on the real pack (needs a built onebit; none on this box, and a build is out at current free disk) — and the four packs
that answer correctly still need to be re-run against the merged change, since a refusal that fires on one of them would be a regression.
|
Duplicate of #1896 — take that one, close this. Same fix for engine#1889, same base ( Where they differ is the test seam: this PR exports I recommended #1896 because the defect is "a pack that imports while silently missing the gains", so the arm that proves the refusal should Exporting the seam is idiomatic in this file ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at d961df9: READY
The refusal is the right shape, and the reasoning is the argument: "this pack declares use_qk_norm and its per-head q/k gains are not where the block layout says (engine#1889): importing without them would normalise no q/k, which answers plausibly-wrong text instead of refusing."
That is the whole class of bug this file has been fighting: an import that succeeds while silently omitting a normalisation is worse than one that fails, because the output still looks like language. Making the declaration the witness — use_qk_norm in the config — and refusing when the tensors are not where the layout says is the correct direction of inference; the alternative, inferring from whether tensors happen to be found, cannot tell "no q/k norm" from "q/k norm we failed to locate".
Saying "the section location for this family is not measured yet" rather than guessing a location is also right: a guessed offset that lands on plausible bytes is the silent-wrong-weights failure.
Note for the queue: this is the complete version of the same change as #1896 — identical qk_norm_refusal, plus the pack_import.h:89 declaration the test needs. Keep this one.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d961df9 — with a duplicate inventory: #1896 fixes the same issue's first slice, in the same file, with the same case.
The defect is the one this fleet keeps finding, and it is the worst variant: qk_norm was decided by the layer block's layout arithmetic alone. Hunyuan-7B-Instruct declares use_qk_norm: true and carries the per-head q/k gains inside its block — but its block is the 2-header shape, so the arithmetic concluded "no qk norm", the import succeeded without the gains, and the model answered gibberish. Silent, well-typed, wrong.
The fix refuses by name, and the refusal is properly loaded: it names the key the pack declared (use_qk_norm), says the gains are "not where the block layout says", and carries engine#1889 — the fourth arm checks the issue reference, which matters because every refusal in this file is looked up by its ticket.
The arm set is the reason I am recommending this one over #1896: seven arms against two, including the pure-decision table (qk_norm_refusal(true,true), (false,true), (false,false)) — so the three combinations (declared+found, undeclared+anything, declared+missing) are enumerated rather than sampled, and "neither is the ordinary pack" pins that the refusal cannot fire on a normal import. #1896 asserts the refusal and its message but leaves the non-firing combinations to inference.
Duplicate handling: neither branch contains the other; both touch npu/pack_import.cpp and tests/npu_pack_import_test.cpp for the same #1889 slice. Merge this one, close #1896 as contained — I have posted the same inventory there.
Not run: host-only (ctest npu_pack_import); the case is a crafted pack, so no Hunyuan-7B is needed to exercise it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d961df9
This is the complete version of a change that exists twice (see my verdict on #1896, which is the same edit minus the header hunk and therefore does not compile).
What it does, and the reasoning is the right one: a pack that declares use_qk_norm is refused by name when the block layout does not locate the two per-head gain sections — "instead of refusing. The section location for this family is not measured yet" — i.e. the importer declines rather than serving a model with q/k gains silently missing. opt_field<bool> with a false default is the correct read: "absent must mean false rather than a parse error", which is the absent-vs-broken distinction my audit drew for #1687/#1805/#1853.
It is also structurally complete — the field is declared in the header (Dims::use_qk_norm, pack_import.h:79, with the Hunyuan-7B 32x128 + 8x128 measurement in the comment), the refusal helper is exported for the test, and the arms are in tests/npu_pack_import_test.cpp.
Built and ran: all npu-pack checks passed.
Why I would land this one: #1896 has the identical pack_import.cpp hunk (17 added lines in both) but no npu/pack_import.h, and on that head the build fails:
npu/pack_import.cpp:1287: error: 'struct Dims' has no member named 'use_qk_norm'
npu/pack_import.cpp:3073: error: … 'use_qk_norm'
— the exact "two PRs, one edit, one of them lost a file" shape that the duplicate rule exists for.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at d961df9 — of the three PRs answering #1889 (#1895, #1896, #1897), this is the one I would land.
I compared the three diffs directly: all are ~51–54 added lines over 3 files, all refuse a declared-but-unlocated qk_norm by name, and all three are independent implementations rather than copies (different diff hashes, different test structure). The tiebreak is factoring, and it is not a matter of taste:
pi::qk_norm_refusal(declared, found) is a pure function with an exhaustive truth table, and the table is what makes it better than the other two:
(false, false)→ empty — the ordinary pack;(false, true)→ empty — gains present without a declaration is not this PR's business;(true, true)→ empty — the case every refusal test forgets, because a refusal that fires when the gains are found would break every Hunyuan pack and no test in the other two PRs would notice;(true, false)→ the refusal, carryingengine#1889, the declared key, and "not where the block layout says".
Three of those four cases are non-refusals. Testing only the refusal proves the function says something; testing the empty cases proves it says nothing when it should — and #1875, #1877 and #1896's own review notes today are all the same lesson from the other direction.
The defect statement is also the best of the three: use_qk_norm landed on false because it was decided by layout arithmetic (pack_import.cpp:2846) plus an ONNX-graph check, neither of which matched this block. Declared true, computed false, silently resolved in favour of the computation — so the fix must report the disagreement rather than pick a side.
As with #1896: land one, close the other two. #1897's body carries the most useful forensic detail (pack_import.cpp:2846, the ONNX check, the Trease/ft////////// symptom), so if #1897 closes, ask its author to paste that into #1895 — the refusal only helps if the next reader knows which two mechanisms disagreed.
…nyuan-7B imports with its q/k norms (engine#1889) (#1928) #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. Co-authored-by: t <t@e.com>
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Superseded: #1976 fixes #1889 completely — importer and the three forward defects — so it should be the one to merge.
I recommended this PR as the winner of the #1895/#1896/#1897 trio earlier today, on the grounds that it was the superset of the other two. That is now the wrong recommendation, and the reason is instructive: all three of us fixed the importer half — read the pack's use_qk_norm declaration, refuse by name when the declared per-head gains are not located — and #1976 reports that "A Hunyuan-7B pack imported and answered filler words. Four separate defects stood between the pack and an answer; three are in the forward and one is in the importer."
So a correctly-imported pack still did not answer: the importer half was necessary and not sufficient, and none of the three siblings could have discovered that from inside pack_import.cpp. #1976 carries the importer half plus the three forward defects, with q4nx_qk_norm_test.c and npu_rope_llama3_test.c pinning both.
The one line worth carrying into #1976 if the bodies are squashed is "silent gibberish becomes a named refusal" — it is the clearest statement of what the importer half buys, and it applies to defect 4 regardless of which PR lands it.
engine#1889, steps 1 and 2:
use_qk_normis read, and a pack that declares per-head q/k gains the importer did not locate is refused by name instead of importing without them.The measured case, from the issue: Hunyuan-7B declares
use_qk_norm: truewith 32×128 + 8×128 gains in its block; the importer decidedqk_normfrom layout arithmetic plus a graph check, neither matched, and the flag landed onfalsesilently. The import then reportedattention 5120/5120 tilesandMLP 9216/9216 groupsbyte-exact — a clean-looking result — while the served model answeredTrease/ft//////////three of three, at a geometry four other packs answer correctly on.grep -r use_qk_norm npu/ tools/returned nothing, which is the whole defect in one line.The change
Dims::use_qk_normreads the key withopt_field<bool>, so absent means false rather than a parse error.qk_norm_refusal(declared, found)puts the decision in one testable place. The three cases are deliberately distinct:plan()throws it before the q/k tensors are planned, so a pack in that state cannot import at all.Arms
7, in
tests/npu_pack_import_test.cppbeside the other named refusals. The pair that matters cannot be satisfied by a no-op or by an always-refusing predicate:declared+not-foundmust be non-empty and name the key, andfound+not-declaredmust be empty.Proved they can fail: disabling the predicate (
if (false && declared && !found)) makes 5 arms report FAIL, and the revert restoresall npu-pack checks passed.Not claimed
This does not make Hunyuan-7B run. It converts deterministic gibberish into a named refusal, which the issue itself calls worth having on its own. Steps 2/3 as the issue words them — locating the gains from the measured section (
96vs160tiles at the same geometry), and checkingHY-MT1.5-7Bwhich is likely the same defect — stay on the issue; the section location is the tail/envelope lane's work (#1740 W1a/W1b), and getting it wrong there silently mis-rotates every Hunyuan pack.Gate: 174/174, copyright clean.
Refs #1889, #1740.