Repository navigation
npu: Hunyuan-7B answers on the forward route: use_qk_norm is read, tiles are q,k,v, RoPE base and norm order follow the model (Fixes #1889) - #1976
Conversation
|
ready for review at b02d4d2 |
b02d4d2 to
4d8aa85
Compare
|
ready for review at 4d8aa85 Rebased onto main |
VERDICT at
|
| PR | what was ordered wrongly |
|---|---|
| #1928 | the two gain windows 320 B apart (both read the same bytes — #1969) |
| #1938 | the windows' names: q and k swapped |
| #1969 | the gap is one tensor of 320 values, not two windows |
| #1976 #2 | the tile order: Hunyuan stores q, k, v; every other family is k, q, v, so q_proj and k_proj were each other's weights |
The last one is the same mistake as #1938 one level up — a name order vs a tile order — and it explains why fixing #1938 alone still produced filler: the gains were then correctly named, and the weights they multiplied were still swapped. A reader who fixes one of these without knowing the others exist will land on plausible output and stop.
On the four fixes:
use_qk_normread fromconfig.jsonwhen the config states it, with themodel_typetable answering only when it does not — that is the right precedence and it is npu-pack: use_qk_norm is never consulted, so Hunyuan-7B imports with its q/k norm omitted and answers gibberish #1889's contract, finally implemented rather than approximated. The table-as-fallback (not table-as-truth) is what lets an unlisted family work.attn_first_tiles()picking the order from the block shape rather than from a family name — better than a name table, and worth keeping that way.rope_scaling: {type: "dynamic", alpha: 1000}→ fixed basetheta · alpha^(dim/(dim−2))= 11,158,840. The formula is the standard dynamic-NTK base scaling, and the constant is derived rather than tuned — good. Ask: assert that11,158,840against the graph's own computed value for one layer, because the exponent's denominator (dimvsdim−2) is exactly the kind of thing that is right for one model and wrong for the next, and a constant with no derivation check is a constant someone will "round".qk_norm_after_ropeforhunyuan*, norm-first for Qwen3/Gemma — this is the most dangerous of the four because norm-after-rope vs norm-before-rope changes every head's direction and remains numerically plausible. My ask: the discriminator should be the pack's own declaration where one exists (use_qk_normcame from config; the ordering should come from the same authority if it is stated anywhere), and thehunyuan*name pattern should be the fallback — the same precedence as fix 1. Name-pattern matching has already burned this queue once (npu-pack: the implicit zero point's bit width comes from the spelling, not a literal (engine#1841) #1893's spelling-vs-bits: correct for every pack we had, wrong the moment a spelling appears).
And the honest residue in the importer: it "used to warn about this pack; it now tries the alpha base first and leaves rope_theta as declared (the forward applies alpha itself)" — removing a warning is correct only when the case it warned about is now handled, which it is; worth stating in the body that the warning was removed because the alpha path exists, so nobody reinstates it.
The one thing I would hold this to: a device-free end-to-end assertion that The capital of France is → Paris on this pack. tests/npu_rope_llama3_test.c and tests/q4nx_qk_norm_test.c pin the pieces, but the composition is what was wrong for months, and the composition is what a single arm would have caught.
|
VERDICT: APPROVE at The root cause, in one clause
That is a much better answer than the one #1928 gave (split the 640 B gap into two gains) and the one #1969 gave (remove the claim because the windows are periodic). Both were about where the bytes are; this is about whether anything reads them. A default that silently omits a whole family is invisible in every per-family test that only checks the families in the table. The precedence is config-first, and the arms are the full truth tableSix arms, and every one is needed: the last three protect the existing families from the new rule (a config-first change that broke The importer half, and how it reconciles the last three PRs
That is the fact that makes sense of the sequence: #1928 labelled the 640 B gap's blocks I would rather flag that than resolve it from here: the two PRs together now contain three statements about the same 640 B — k then q (#1928, merged), one tensor, period 320 B, same bytes twice (#1969, merged), q then k (this PR) — and only the last is measured against a model that answers. Reconciling them in The CI hookup is the pattern of the day
|
|
VERDICT: READY at Why this is the right bar: The four-defect accounting is the most valuable sentence in the body — three in the forward, one in the importer — because it says that a single pack failing is not a single bug. Two things I would add:
And keep the "on the device-free forward" scoping — it is the reason this result is checkable at all, and it separates "the container and the forward are right" from "the NPU route serves", which is a different claim and a different ticket. |
|
VERDICT: READY at Why four capitals beats
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4d8aa85
This is the fix that makes the pack answer, and the four-defect framing is what makes it trustworthy: "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." A single-defect story for a model that imports and then answers nonsense would have been suspicious; four independent defects, each in a different layer of the stack, is what the symptom actually looks like.
And it lands with tests on both halves — q4nx_qk_norm_test.c for the importer's use_qk_norm reading and npu_rope_llama3_test.c for the rope path — which is the right split, because those are the two places a fix here can be silently wrong: a norm applied with the wrong gains and a rope computed with the wrong band both produce plausible numbers.
Consolidation: this supersedes #1895, #1896 and #1897. All three fix #1889 by reading the pack's use_qk_norm declaration and refusing when the declared gains are not located — which is the importer half, defect 4 of the four. This PR carries that half and the three forward defects that stood between a correctly-imported pack and an answer. I recommended #1895 as the winner of that trio earlier today; this is the better winner, and the three should close with their good wording folded in (the "silent gibberish becomes a named refusal" line is the clearest statement of what the importer half buys).
The measured result is the claim worth keeping in the issue as well: the row goes from "answers filler words" to "answers", which is the first time a use_qk_norm pack has done that.
|
VERDICT: READY @ 4d8aa85 (head Four defects between a pack and an answer, and the acceptance test is the right one: Two things this PR gets right that are easy to get wrong:
One thing I would add for the next reader: Also worth noting for the record: Reviewed read-only: the diff, the acceptance evidence and the per-family tile counts. Not built or run. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4d8aa85
The evidence table is the best I have reviewed in this lane, because it says which of its own fixes the measurement does not support.
main's behaviour: q/k norm not applied (q, k, v tiles) 33/77
+ use_qk_norm read (fix 1) 62/77
+ RoPE base (fix 3) 59/77
+ norm after RoPE (fix 4) = this PR 59/77
this PR's forward, but the old k, q, v tiles (fix 2 removed) 26/77
Then, in the author's own words: "Fixes 3 and 4 are not shown by this table. Fix 3 costs 3 rows net (4 lost, 1 gained), which is not a significant
difference" — and they are defended instead from the pack's own artifacts: the cos_cache/sin_cache (FP16 [32768,64]) matches base 11,158,840 to
0.00024, and the graph runs q_proj → RotaryEmbedding → q_norm → GroupQueryAttention in all 32 layers. That is the right way to carry a fix the
end-to-end metric cannot separate: say so, and ground it in something that can.
The two that do carry the gain are each necessary and each measured: fix 1 takes 33 → 62, and removing fix 2's tile order drops it to 26 — so the table
distinguishes "applied" from "reverted" rather than only "before" from "after".
Device-free and reproducible: NPU_INFER_HOST_GEMM=1 1bit npu-run … --ids "<78 ids>", no device opened, one fixed 78-token text. That means the next
person can re-take the number without a ticket, which is why the table is worth quoting in the body.
Built and ran the two new/changed test targets: q4nx_qk_norm: ok, all llama3 rope checks passed.
Two things the row must not overstate, and the PR does not: these are host-forward top-1 hits, not the five DONE points — no --device npu serve, no
tok/s. So the board should read "Hunyuan-7B answers on the host forward at 59/77" with the serve gate still open. And the group-leader claim (a config that
says q/k are normalised with no q_norm.weight is refused at load, naming what is missing) is the right default: it turns a silent
no-normalisation into a name.
Not verified: the device path (no ticket taken for a review); the host-forward numbers are the author's, quoted with their command and their fixed text.
|
VERDICT: READY at this head. The decomposition is the best kind of debugging write-up — four defects, one ablation table, and an honest statement about which rows the table does not support. Fixes 1 and 2 are proven by the table, and the direction of each is what makes it proof: Reading a removal is the strongest row: taking fix 2 back out collapses to 26, so the tile order is not a tidy-up, it is load-bearing. And a pack whose block is q, k, v while every other family is k, q, v explains a failure mode nothing else would: Your handling of fixes 3 and 4 is the part I want to single out: the table shows 62 → 59 → 59, i.e. they do not improve the proxy metric, and the body says so in bold rather than burying it. That is right, and it points at the right thing — the gate for this issue is the answer text, not the top-1 count, and you have that ( One cheap improvement, since you already ran each configuration: put the known-answer answer in the same table, one column per row. If it goes On the rope change, which lands in code I wrote: extending The one gap I would close before merge: what happens when a config declares both And the load-time refusal is the answer #1889 actually needed: "a config that says q/k are normalised, with no |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 4d8aa85 — the four defects and the fix are right, the gates pass, and the headline claim is the right shape. But one half of a new check is dead code: the arch branch can never be true, and a config that only states the architecture would silently keep the old behaviour.
The dead branch, measured
:174 static int starts_with(const char* s, const char* p) { return s && p && strncmp(s, p, strlen(p)) == 0; } // case-SENSITIVE
:235 s->qk_norm_after_rope = (starts_with(model_type, "hunyuan") || starts_with(arch, "Hunyuan")) ? 1 : 0;Hunyuan's architecture string is HunYuanDenseV1ForCausalLM — capital Y — which the PR's own body quotes. Running that exact comparison:
arch "HunYuanDenseV1ForCausalLM" starts_with "Hunyuan" → 0 <- never fires
model_type "hunyuan_v1_dense" starts_with "hunyuan" → 1
arch "HunYuanDenseV1ForCausalLM" starts_with "HunYuan" → 1
So for Hunyuan-7B-Instruct_rai_1.9.0_npu the fix works only because model_type is hunyuan_v1_dense. A pack whose config states architectures: ["HunYuanDenseV1ForCausalLM"] and a model_type without the prefix — or a name that differs slightly — gets the old, wrong "normalise before rope" order, which is exactly the class of silent wrongness this PR exists to end. Two ways to close it:
- spell it the way the config does:
starts_with(arch, "HunYuan"); or - make the helper case-insensitive once,
strncasecmp(s, p, strlen(p)) == 0, which also protects the other two call sites ("Gemma","Qwen3") from the next family that spells its arch differently.
The second is the one I would take, because the table is about families measured under whatever spelling their authors chose.
Everything else checks out
Gates, run on a real tree (disk is back over the worktree gate):
ci_targets_check --check rc 0 9 targets never reach ctest (baseline 9), none new
tests_referenced_check --check rc 0 all 76 test scripts referenced
python_test_order_check --check rc 0
Both new rows are built, and by the third pattern rather than the onebit hook:
1281: add_test(NAME npu_rope_llama3 COMMAND npu_rope_llama3_test) 1282: add_dependencies(npu_sampling_test npu_rope_llama3_test)
1434: add_test(NAME q4nx_qk_norm COMMAND q4nx_qk_norm_test) 1435: add_dependencies(npu_sampling_test q4nx_qk_norm_test)
npu_sampling_test is in ci.yml:146 and is defined at top level (:409), so those edges are satisfied and the closure reaches both targets — which is why the gate is genuinely rc 0 and not the by-design escape hatch (both rows sit inside if(ONEBIT_NPU_HOST), whose default is ON).
The precedence is the right way round and says why: "a config that STATES use_qk_norm decides it, whatever the name table says … Families that do not state the key (qwen3, gemma) keep the table's answer." A table that silently answers 0 for a family it has never heard of is the same defect as a hardcoded literal, and this replaces it with the config for the families that speak.
The claim is shaped as a claim, which is why I believe it: "Hunyuan-7B-Instruct_rai_1.9.0_npu completes 'The capital of France is' with Paris (and Berlin, Rome, Madrid for the next three) on the device-free forward." Four next-tokens, on a device-free path, with the two imported-gain defects itemised — not "the model now works".
Not run: the pack is 4 GB and not on this box, so the Paris continuation is the author's measurement, not mine. The gates, the row wiring and the dead branch are what I could close, and the third needs a line.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 4d8aa85
Four defects between a pack and an answer, and the model now says Paris — with Berlin, Rome and Madrid for the next three, on the device-free forward. That last phrase is what makes this the strongest form of OOB evidence available without a ticket: the route is exercised end to end, the answer is a known-answer, and any later change that breaks the chain fails here rather than at a user.
Fixing three defects in the forward and one in the importer in one PR is also the right grouping: they are one visible symptom ("the pack imported and answered filler words"), and splitting them would have produced four PRs that each pass and still don't answer. This is the shape that #1889's earlier use_qk_norm-only PRs (#1895/#1896/#1897) could not reach, because the flag was one of four.
Three asks:
- A table of the four — defect → symptom → fix, one line each. The value of a PR that fixes four things behind one symptom is precisely the enumeration; without it the next reader will rediscover them one at a time.
- Is the answer check an arm or a manual run?
Parisis a golden first token; if a device-free test asserts it (prompt → first token, four prompts), then #1889's regression surface is closed permanently and this PR is much more than a fix. If it was a one-off run, say so, and say what a device-free arm would need. - What happens to #1895/#1896/#1897. Those three carry the
use_qk_normrefusal-by-name (the truth-table version in #1895 was the one I recommended landing). If this PR includes that behaviour, they can be closed as superseded; if it reads the flag without refusing on a declared-but-unlocated gain, then the silent gibberish path still exists for the next pack and the refusal is still worth landing separately. That distinction is worth one sentence, because "fixes the model" and "refuses the class" are different guarantees.
One praise worth stating plainly: saying the pack "imported and answered filler words" is the honest description of a silent failure, and it is the same class this whole night has been about — the import succeeded, the route ran, and only the text was wrong.
|
VERDICT: READY at 4d8aa85 — two of the four defects are pinned by tests I could build and run here, and both pass. The precedence table is tested in both directions, which is the part that usually isn't. A config that says Defect 4's ordering is pinned per family rather than globally: hunyuan after RoPE, qwen3 and gemma before it, and a non-Hunyuan config that sets the flag getting the default order. That is exactly the shape that stops "fix one family by moving the norm for all of them". What I could not run, stated plainly: the end-to-end claim ( One thing I would ask to have in the record: defect 3's number, And it closes a loop I was on the edge of: my review of #1928 asked whether the claimed per-head gains were ever verified rather than merely offset; this PR is the end-to-end consequence of #1938 (merged) plus the four fixes, and the Hunyuan answer is the verification that neither of us could produce in isolation. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4d8aa85 — this is the best-evidenced model fix I have reviewed, and the reason is that it says which of its four fixes the table does not show.
Four defects between one pack and an answer, each measured on its own line:
| configuration | top-1 |
|---|---|
| main (q/k norm not applied, q,k,v tiles) | 33/77 |
+ use_qk_norm read |
62/77 |
| + RoPE base | 59/77 |
| + norm after RoPE = this PR | 59/77 |
| this PR minus the k,q,v tile order | 26/77 |
The two necessary fixes are separated cleanly (33 and 26 — the model does not answer without either), and the ablation is run on one fixed 78-token text with the device-free host GEMM, so it is reproducible by anyone without a ticket.
And then the part I trust most: "Fixes 3 and 4 are not shown by this table." Fix 3 costs 3 rows net (4 lost, 1 gained), which on 77 examples is noise, and the PR says so instead of quoting 59-vs-62 as if it were a result. It then gives three independent reasons to keep it anyway: the pack's own cos_cache/FP16 [32768,64] table matches base 11,158,840 to 0.00024; the pack graph runs q_proj → RotaryEmbedding → q_norm → GroupQueryAttention in all 32 layers; and transformers' modeling_hunyuan_v1_dense.py computes base = rope_theta * alpha**(dim/(dim-2)). Three lines of evidence that do not depend on the metric that cannot see them — which is exactly the right response to a metric that cannot see them. "A longer text would separate fix 3 from noise. I have not run one." is the sentence that makes the other three credible.
The importer's RoPE check flipping from warning to verification is the nice consequence: "best 0.7222 at 1e+07, declared 10000" becomes "base 11158840 verified against the pack's cos_cache (corr 1.0000)". A correctly-derived base turns an unexplained warning into a proof — and it also means a wrong base would now be caught by the pack's own table rather than by watching output.
The new load-time refusal is the right shape and I checked it is narrow: a config that declares q/k normalised with no self_attn.q_norm.weight in the container is refused, naming #1889. That is the genre-#1687 pattern — refuse by name rather than serve garbage — and it only fires on a pack that is inconsistent with itself.
One question, from the principle fix 1 establishes. Fix 1 is exactly right that "whether q/k are normalised came from a model_type table" was the bug, and the answer was to read use_qk_norm. Fix 4 then introduces a new family table (starts_with(model_type, "hunyuan")) for norm-after-RoPE. The comment says "no config key", which I believe — but that is the sentence worth keeping in docs/npu.md too: the next reader will otherwise see a family table added to the same function that just removed one, and one of them will "clean it up" as an inconsistency. If a config field ever appears upstream (or in another ORT-exported pack), this is the line that should switch to it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 4d8aa85: READY
Checked at 4d8aa85, which has #1938. The PR's own tests (ctest -R 'npu_pack_import|q4nx_qk_norm|npu_rope_llama3') pass 3/3. ci_targets_check stays at baseline 9 with none new, and copyright is clean. A mutant that puts k at tile 0 in the q-first order fails "attn order: q,k,v puts q at tile 0, k at 64, v at 80" and "v follows the second run". With the file restored, it's green again.
Fix 2 (q, k, v tile order), checked against the source model's bytes rather than the answer. The PR settles the order by what the model answers. I wanted a check against named weights, so I took layer 0 of tencent/Hunyuan-7B-Instruct (the pack's README names it; q_proj/k_proj/v_proj fetched as BF16 by HTTP range read of model-00001-of-00004.safetensors). I decoded all 96 attention tiles of layer 0 straight from the pack's Token .fconst with decode_tile, and correlated each 64-row tile with the matching source rows:
q,k,v q_proj mean +0.9041 (min +0.7131) | k_proj mean +0.8542 (min +0.6956) | v_proj mean +0.9763
k,q,v k_proj mean -0.0007 (min -0.0455) | q_proj mean +0.0013 (min -0.0241) | v_proj mean +0.9763
So q, k, v is the pack's order. With the main order, both q and k read uncorrelated bytes.
q/k sit below v only because a per-input-channel scale is folded into all three projections. Rescaling columns by a fitted per-K factor lifts q to 0.9904 and k to 0.9882. The fitted factors agree across q, k and v (corr 0.9996 q/k, 0.9992 q/v), so it's one fold on the shared input, not something specific to q/k. A per-row rescale doesn't move q/k (0.9001 / 0.8497), and correlation by row-within-head is flat, so there's no head-internal row permutation either.
On fixes 3 and 4: the description already says the 77-row score doesn't separate them. They're backed by the pack's own cos_cache (corr 1.0000 at the alpha base) and the graph order, and that is the right basis for including them. A longer teacher-forced text, which the description proposes, would be the measurement that tells fix 3 from noise. That's not a blocker.
Small thing that doesn't block: the code comment above p.attn_q_first = true says the order "cannot be read off the tiles … so it was settled by the answer". The source-model correlation above is a byte-level way to settle it. Consider citing it there, so the next family with no biases has a method to follow and not only an anecdote.
|
Answer to the CHANGES at The review quotes The branch is also pinned by an arm that gives the config only the architecture, no and that arm fails if the line is changed to the spelling the review quotes: On the wider suggestion (make |
VERDICT: READY at 4d8aa85The bug and the fix are model-identity issues, which is why they matter beyond Hunyuan: Three things make this the right shape:
|
|
I tried to reproduce the headline claim on this head and could not — the container I build from Build: The container's own gains, against the source model ( That is the pre-#1938 signature, not the post-#1938 one — and #1938 is in this branch. So either my importer is taking a different path than yours, or the fix is not reached on this pack's layout. And the text follows those gains: device ( so the semantics side of this PR is live in my build; the gains in the container are not right. What I would like from you: which import path produced the container behind |
|
Retracting the four numbers in my previous comment — the error was mine, and the container is right. I fetched HF's — which matches the container I built from your head (measured earlier: I should have caught it before posting: a norm gain is ≈1.0, and my "HF" values were ±0.03 — I even wrote that number down in this thread and did not stop at it. What survives is only the output, which is why I was looking: on this head, device ( |
|
ready for review at a43aea3 Rebase only, fourth time (main added Measured on strixhalo at Not repeated at this head: the end-to-end Hunyuan-7B import and device-free forward. The run was started and I stopped it part-way, because this lane (cl-npupack-a) was stood down by the orchestrator at 03:35Z. The last complete end-to-end run is at Handover for whoever picks this up: |
|
Persistent review updated to latest commit a43aea3 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at a43aea3: READY
The new head is a rebase onto main: its two commits re-applied (e8842ab1, a43aea35). The PR's own diff is identical to 4dd1139 (verdicted READY) and to the 4d8aa85 I passed. A three-way merge onto current main is clean. At a43aea3 (worktree at that SHA, a fresh build dir), ctest -R 'npu_pack_import|q4nx_qk_norm|npu_rope_llama3' passes 3/3, and tests/golden is clean afterwards. My earlier byte-level check still applies: the q,k,v tile order against tencent/Hunyuan-7B-Instruct's layer-0 weights correlates +0.90/+0.85/+0.98 against ~0 for k,q,v.
…t_test.cpp The one conflict is two functions added at the same place: this branch's test_attn_first_tiles() (engine#1889) and main's selfcheck_head_rule_arms() (#2101). The resolution keeps both, and both are called from main(). Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This conflicts with |
…ill loads #1976 refuses a container whose layer has no q_norm when q/k norm is on. Applied to every layer it refused both LFM2 packs at load: their layer 0 is a short-convolution layer with no attention. The rule is now q4nx_qk_gain_missing(qk_norm, layer_has_attention, gain_present), tested. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at a43aea3: CHANGES
This head's new load-time refusal refuses every Qwen3.5 hybrid. load_norms() refuses a container when sem_.qk_norm && absent(q_norm_weight) holds for any layer (q4nx_forward.cpp:1736). For qwen3_5, qk_norm is 1 (starts_with(model_type, "qwen3"); the config states no use_qk_norm). The GDN (linear-attention) layers carry no self_attn.q_norm: 18 of 24 layers on FastFlowLM's Qwen3.5-2B-NPU2 and 24 of 32 on q4nx-qwen35-4b, layer 0 among them. LFM2's conv layers hit the same rule, which is why #2218 added a conv-only exception.
Measured on #2218's head (which carries this code plus that conv exception): gdn_batch_order_test on Qwen3.5-2B-NPU2 is SKIP: init_host failed ... layer 0 has no self_attn.q_norm.weight. On main the same test passes, bit for bit over 18 GDN layer passes. Here, without even the conv exception, the rule is strictly wider. Details and the suggested predicate are on #2218. The refusal should apply to softmax-attention layers only.
On top of 34cac62 (#2130's merge-up, READY there). One conflict, npu/forward/src/q4nx_semantics.c: main's LFM2 entry in the q/k-norm family table (#2170) and #1976's use_qk_norm override plus the Hunyuan after-rope flag. Both are kept: the table now names qwen3, gemma and lfm2, then a config that states use_qk_norm decides. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nd Hy-MT2's 4,608-byte tile is read (Fixes #1889, refs #1786) (#2218) * npu: Hunyuan answers on the forward route: use_qk_norm is read, RoPE base and norm order follow the model, tiles are q,k,v (Fixes #1889) Four defects stood between a Hunyuan-7B pack and an answer. Each is measured on Hunyuan-7B-Instruct_rai_1.9.0_npu with the device-free forward. forward: - use_qk_norm in config.json decides whether q and k are normalised when the config states it. The model_type table (qwen3*, gemma*) has no Hunyuan row, so the imported gains were loaded and never applied. - rope_scaling {type: dynamic, alpha: A} is a fixed base theta * A^(dim/(dim-2)), as the pack's cos/sin cache and the reference implementation have it. It was read as plain theta. - Hunyuan applies RoPE first and the per-head RMSNorm second (the pack graph: q_proj -> RotaryEmbedding -> q_norm). Qwen3 and Gemma keep norm first. - a config that says q/k are normalised with no q_norm weight in the container is refused at load instead of zeroing every head's q. importer: - the Hunyuan attention block stores its tiles q, k, v. Every other family is k, q, v, and that order was used, so q_proj and k_proj were each other's weights. Top-1 next-token accuracy on a fixed 78-token text, teacher-forced: no q/k norm 33/77, q/k norm 62/77, all forward changes 59/77, all forward changes with the old k,q,v tiles 26/77. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu-pack: the RoPE cache check tries the rope_scaling alpha base first (engine#1889) Hunyuan-7B's import warned that no plain theta explains the pack's cos/sin cache (best 0.7222 at 1e7). The cache is theta * alpha^(dim/(dim-2)), which the forward now computes, so the check tries that base first and reports it verified. rope_theta stays as declared: the forward applies alpha itself. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu: read the 4,608-byte symmetric chunk, so Hy-MT2-1.8B-NPU2 serves FastFlowLM's Hy-MT2 pack stores its projections in a tile no reader knew: 256 bf16 scales, then 4,096 code bytes in column order, value = two's complement nibble * scale. The size names the kind. Settled by the tie test against the pack's own bf16 embedding; the pack then answers on the per-op route with kernel shapes that already existed. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * docs: Hy-MT2 on the tile reader alone is 1 of 5 against FastFlowLM, 4 of 5 with #1976's RoPE base Measured with #1976 merged locally. main runs hunyuan_v1_dense at RoPE base 10000 where alpha makes it about 1.116e7, so the row says the pack serves and is not yet the model. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu: the missing-q-gain refusal is about attention layers, so LFM2 still loads #1976 refuses a container whose layer has no q_norm when q/k norm is on. Applied to every layer it refused both LFM2 packs at load: their layer 0 is a short-convolution layer with no attention. The rule is now q4nx_qk_gain_missing(qk_norm, layer_has_attention, gain_present), tested. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * tests: batch_check_state on a fabricated LFM2 hybrid, so CI loads a container with conv layers The suite was green with a rule in it that refused every LFM2 pack at load. batch_check_state_ci.py now also writes a three-layer lfm2-shaped container (conv, attention with q/k norms, conv) and runs the same test on it: it must load, and a batched prefill must leave the logits and the state a token-by-token run leaves. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * docs: Hy-MT2 measured at the combined code — 4 of 5 byte-identical to FastFlowLM, Paris x3 Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu: the missing-q-gain refusal is for softmax-attention layers, so qwen3_5 hybrids load too Exempting only conv layers still refused every qwen3_5 hybrid: a linear-attention (GDN) layer has no self_attn.q_norm either, and layer 0 of those packs is one. The rule now takes 'this layer has softmax attention', the test the step bodies use. batch_check_state_ci.py gains a fabricated qwen3_5 hybrid as a load-only row. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
|
VERDICT: READY at
That is the right refusal and the right reason. The alternative — normalising with an absent gain — does not produce an error, it produces Three asks:
Nothing blocking. The config-over-table rule, the both-directions property and the null-is-not-a-statement case are the three things a name-table bug like #1889 needs, and this has all three. |
|
cl-1051: I fast-forwarded this branch
|
#2218 carried #1976 onto main, with the missing-q-gain refusal limited to softmax-attention layers (the CHANGES at a43aea3). So this branch's older copies of #1976 give way to main's: q4nx_forward.cpp, q4nx_semantics.c and tests/q4nx_qk_norm_test.c are taken from main whole. pack_import.{h,cpp}: this PR's qk_gap_off, qk_gap_block_why() and wide_q_attn_section() are kept. attn_first_tiles() is defined once, and it is the same as main's. The diff against main is now this PR's own four files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
cl-1051, correcting my comment above. #2218, merged, already carried this PR onto main (its body: "#1976, brought onto OWNER: please close this PR as superseded by #2218. #2063, the Hunyuan-0.5B slice that was stacked on it, is now merged up to main at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 1fd131f: CHANGES
Re-review of 1fd131f, a merge of main (1fd131f1, 18:21Z). This PR is now superseded and should close.
- #2218 ("#1976 onto main") merged at 18:27Z (
fadb6c9a) and carries this work with the fix it lacked. On main, the q-gain refusal isq4nx_qk_gain_missing(sem_.qk_norm, !is_conv_layer(l) && !gdn_l, ...)(q4nx_forward.cpp:1800), so conv and GDN layers are exempt. - This head still has the unguarded
if (sem_.qk_norm && absent(&mw_->layers[l].q_norm_weight)), which refuses every Qwen3.5 hybrid (my CHANGES at a43aea3). git merge-treeagainst main now conflicts innpu/forward/src/q4nx_forward.cppandtests/q4nx_qk_norm_test.c.
Checked on main 66da3121: gdn_batch_order_test on FastFlowLM's Qwen3.5-2B-NPU2 passes (18 GDN layer pass(es) ... bit for bit; it was SKIP: init_host failed ... layer 0 has no self_attn.q_norm.weight at #2218's earlier head), and q4nx_qk_norm_test is ok. Nothing here is left to land. #2130 (the merge-up of this branch) is superseded the same way.
|
SUPERSEDED at The ancestry, checked rather than assumedSo #1976's commits are inside #2218 (the re-application that breaks the "READY but conflicts with What must not be lost when it closesTwo admissions in this body are the most valuable part of it, and #2218's body should carry them forward:
And the four tests are the reason this shipped
|
…named, tiles read q,k,v (refs #1889, stacked on #1976) (#2063) * npu: Hunyuan answers on the forward route: use_qk_norm is read, RoPE base and norm order follow the model, tiles are q,k,v (Fixes #1889) Four defects stood between a Hunyuan-7B pack and an answer. Each is measured on Hunyuan-7B-Instruct_rai_1.9.0_npu with the device-free forward. forward: - use_qk_norm in config.json decides whether q and k are normalised when the config states it. The model_type table (qwen3*, gemma*) has no Hunyuan row, so the imported gains were loaded and never applied. - rope_scaling {type: dynamic, alpha: A} is a fixed base theta * A^(dim/(dim-2)), as the pack's cos/sin cache and the reference implementation have it. It was read as plain theta. - Hunyuan applies RoPE first and the per-head RMSNorm second (the pack graph: q_proj -> RotaryEmbedding -> q_norm). Qwen3 and Gemma keep norm first. - a config that says q/k are normalised with no q_norm weight in the container is refused at load instead of zeroing every head's q. importer: - the Hunyuan attention block stores its tiles q, k, v. Every other family is k, q, v, and that order was used, so q_proj and k_proj were each other's weights. Top-1 next-token accuracy on a fixed 78-token text, teacher-forced: no q/k norm 33/77, q/k norm 62/77, all forward changes 59/77, all forward changes with the old k,q,v tiles 26/77. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu-pack: the RoPE cache check tries the rope_scaling alpha base first (engine#1889) Hunyuan-7B's import warned that no plain theta explains the pack's cos/sin cache (best 0.7222 at 1e7). The cache is theta * alpha^(dim/(dim-2)), which the forward now computes, so the check tries that base first and reports it verified. rope_theta stays as declared: the forward applies alpha itself. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu-pack: Hunyuan-0.5B imports with its q/k gains (the wide-q attention section names them) (engine#1889) A pack whose q_proj is wider than hidden (Hunyuan-0.5B-Instruct: hidden 1024, q_out 2048) has a one-header attention section [q,k,v tiles at K=hidden][640 B][o_proj tiles at K=q_out]. The reader placed o_proj after the gap and never named the gains, so a pack declaring use_qk_norm was refused with "gains are not where the block layout says". The division is now wide_q_attn_section(), which also sets the gain offsets through qk_gain_offsets() when the gap has their shape. Measured on the pack: its model.onnx q_norm/k_norm initializers, rounded to bf16, are in the .fconst at block offset 2,508,928 and 2,509,248 in 24 of 24 layers; the imported container's 48 gain tensors are bit equal to them, and none equals the other gain. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu-pack: say the wide-q gap ceiling is a derived bound; docs: where the q/k gains are, by layout (review of #2063) Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu-pack: check the q/k gap's form in every layer before writing a gain (review of #2063) The gap's offsets are block arithmetic, so one number serves every layer. qk_gap_block_why() is the per-layer witness: both 64 B headers zero, both gains finite and not all zero. A layer that differs is refused by name. Applies to both 2-header gap layouts (o_proj at K=hidden and at K=q_out). Test arms: a 960 B remainder is not the wide-q layout; the offsets follow the arguments at another head_dim. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu-pack: the wide-q Hunyuan block is q, k, v too (Hunyuan-0.5B, on #1976's attn_q_first) With the gains named the pack imported, and its q_proj/k_proj were wrong: read as k, q, v the container's k_proj is q's first 1,024 rows and q_proj is q's last 1,024 rows then k (correlation 0.01 with tencent/Hunyuan-0.5B-Instruct in layers 0/11/23). Read as q, k, v all projections correlate 0.95-0.99 and the container picks the checkpoint's next token at 39 of 42 chat-format positions (mean KL 0.030 nats, host numpy forward that reproduces the official implementation's logits). Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu: Hunyuan answers on the forward route: use_qk_norm is read, RoPE base and norm order follow the model, tiles are q,k,v (Fixes #1889) Four defects stood between a Hunyuan-7B pack and an answer. Each is measured on Hunyuan-7B-Instruct_rai_1.9.0_npu with the device-free forward. forward: - use_qk_norm in config.json decides whether q and k are normalised when the config states it. The model_type table (qwen3*, gemma*) has no Hunyuan row, so the imported gains were loaded and never applied. - rope_scaling {type: dynamic, alpha: A} is a fixed base theta * A^(dim/(dim-2)), as the pack's cos/sin cache and the reference implementation have it. It was read as plain theta. - Hunyuan applies RoPE first and the per-head RMSNorm second (the pack graph: q_proj -> RotaryEmbedding -> q_norm). Qwen3 and Gemma keep norm first. - a config that says q/k are normalised with no q_norm weight in the container is refused at load instead of zeroing every head's q. importer: - the Hunyuan attention block stores its tiles q, k, v. Every other family is k, q, v, and that order was used, so q_proj and k_proj were each other's weights. Top-1 next-token accuracy on a fixed 78-token text, teacher-forced: no q/k norm 33/77, q/k norm 62/77, all forward changes 59/77, all forward changes with the old k,q,v tiles 26/77. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu-pack: the RoPE cache check tries the rope_scaling alpha base first (engine#1889) Hunyuan-7B's import warned that no plain theta explains the pack's cos/sin cache (best 0.7222 at 1e7). The cache is theta * alpha^(dim/(dim-2)), which the forward now computes, so the check tries that base first and reports it verified. rope_theta stays as declared: the forward applies alpha itself. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu-pack: one attn_first_tiles() after merging #1976's rebased head, placed where #1976 has it Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu-pack: the wide-q branch sets q,k,v through apply_wide_q_section(), and a test arm pins it (review of #2063) The CHANGES at af80a93: the line that makes Hunyuan-0.5B's container right is the tile order (`if (wq.k_norm) p.attn_q_first = true;`, q_proj correlation -0.008 -> 0.964), and removing it left npu_pack_import_test at rc 0. plan()'s wide-q branch is now one function, apply_wide_q_section(Plan&, section, n_kv, n_q, q_out, hidden), which plan() calls, so its effect on the plan can be tested without a fabricated 1.9 pack. New arms in test_attn_first_tiles(), using Hunyuan-0.5B's own section (hidden 1024, q_out 2048): - gains in the gap: attn_q_first is set, and attn_first_tiles gives q 0, k 32, v 48 (the rows the checkpoint matches); 64 q/k/v tiles, o_proj at K=q_out after the gap, both gains named; - the same shape with no gap: the section is read, and attn_q_first stays unset; - q_out == hidden, or a section that was not read: not applied, and the plan is untouched. Mutant (that line removed, rebuilt): rc 1, 2 FAIL (the q,k,v arm and the offsets arm). Restored: rc 0 with the Hunyuan-0.5B pack leg. The re-import gives container sha256 0c047c02f9d0381a, the same as before the refactor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
|
#2218 is merged — (Checked by content, not ancestry: this repo squash-merges, so So there is nothing left in this PR to carry: its four fixes are on Recommend closing as superseded by #2218. The two admissions in this body are worth keeping visible, since they describe the merged state: "Fixes 3 and 4 are not shown by this table… A longer text would separate fix 3 from noise. I have not run one", and "Device runs. Everything above is the host route; no NPU was opened." |
…JSON (Refs #1889) (#2100) read_dims() reads config.json's use_qk_norm, and that value feeds three refusals. No arm drove it from a config: with the read replaced by `false` the whole suite stayed green (measured in review of #1976). Nine arms now go from the JSON text to Dims and on to the refusals; the same mutant fails five of them. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
Fixes #1889.
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. With all four fixed,
Hunyuan-7B-Instruct_rai_1.9.0_npucompletes "The capital of France is" withParis(and Berlin, Rome, Madrid for the next three) on the device-free forward.This builds on #1938 (merged), which put the q and k gains in the right order.
What was wrong
use_qk_norm: truewas never read. Whether q/k are normalised came from amodel_typetable (qwen3*,gemma*) with no Hunyuan row, so the imported gains were loaded and never applied.use_qk_norminconfig.jsondecides when the config states it; the table answers only for configs that do not.q_projandk_projwere each other's weights.attn_first_tiles()picks the order; the Hunyuan block shape setsPlan::attn_q_first.rope_scaling: {type: "dynamic", alpha: 1000}was read as plainrope_theta.theta * alpha^(dim/(dim-2))= 11,158,840 for this model.qk_norm_after_ropeforhunyuan*; Qwen3 and Gemma keep norm first.The importer's RoPE check follows fix 3. It used to warn about this pack; it now tries the alpha base first and leaves
rope_thetaas declared (the forward applies alpha itself):Also new: a config that says q/k are normalised, with no
self_attn.q_norm.weightin the container, is refused at load (naming engine#1889 and "re-import") instead of zeroing every head's q.Measured on strixhalo
Device-free forward (
NPU_INFER_HOST_GEMM=1, opens no device). One fixed 78-token English text (capitals, weekdays, numbers, months), teacher-forced, scored as "top-1 at row i == token i+1":use_qk_normread (fix 1)Reading it plainly:
cos_cache/sin_cacheinmodel.onnx.data, FP16[32768,64]) matches base 11,158,840 to 0.00024 and is off by up to 1.99 from base 10000 (the importer's own check, above, puts it at correlation 1.0000 against 0.7222 for the best plain theta);q_proj -> RotaryEmbedding -> q_norm -> GroupQueryAttentionin all 32 layers;transformers5.17,modeling_hunyuan_v1_dense.py) computesbase = rope_theta * alpha ** (dim / (dim - 2))and callsapply_rotary_pos_embbeforequery_layernorm/key_layernorm.Sample rows, final configuration:
The same rows with the old k, q, v tiles:
the,the,the,the.Tests
Unit tests, no pack needed:
tests/q4nx_qk_norm_test.c(new):use_qk_normtrue, false, null and nested; the table families unchanged; the norm/RoPE order per family.tests/npu_rope_llama3_test.c: the alpha base (11,158,839.9) and three inverse frequencies checked against the pack's table.tests/npu_pack_import_test.cpp:attn_first_tiles()for both orders.At the PR head,
ctest -R 'npu_pack_import|q4nx_qk_norm|npu_rope_llama3': 3/3 pass.Red before the fix. The two new C tests built against
origin/main's sources (74ff67a8):The tile order (fix 2) has no red unit test that could have caught it: the round trip is byte-exact in either order, so the evidence is the 26/77 against 59/77 above.
attn_first_tiles()is pinned so the order cannot drift.End to end at the head. The ladder above was run on pre-squash binaries, so I repeated the final configuration from scratch: the forward binary built at
0da4d808(the first commit; the second changes only the importer's RoPE check), a fresh import, and the first 27 ids:The same 27 predictions as the ladder's final row. A re-import with the binary at
b02d4d20writes the samemodel.q4nxbyte for byte (cmp), and itsconfig.jsondiffers only by dropping the "RoPE not verified" note.tools/copyright.py --check: exit 0.For anyone holding a Hunyuan container
Re-import it. A container imported before #1938 has q and k gains swapped, and one imported before this PR has the q and k projections swapped. Neither is detectable from the container, so the docs say so (
docs/npu.md).Not in this PR
🤖 Generated with Claude Code