Repository navigation
npu-pack: the 1.9 tail head gets the same self-check as the host head - #1515
Conversation
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at c1d8c8ac — one small fix: the controls you are quoting as 0.0000 are the struct's defaults, not measurements.
The self-check itself is right and it is the right idea. It mirrors the host-head path exactly — same Σ|x−e|/Σ|e|, same corr(q, embed_bytes, H, q.N) — guarded on q.N == V && q.K == H so it cannot fire on a qkv matrix, and setting selfcheck_reference so the summary stops saying "skipped".
And your framing is the strongest justification in the PR: the check does not know the head came from a .fconst tail, so a pass is evidence about
the tail layout from a reference that is independent of it. corr 0.999999 / rel_err 0.000700 is a real measurement and a convincing one.
But the controls line does not run on this path. pack_import.h:186:
double lmhead_corr = 0, lmhead_rel = 0, ctrl_corr_nibble = 0, ctrl_corr_nozp = 0;and pack_import.cpp:2760 prints them whenever selfcheck_reference is set:
printf(" controls: nibble-swapped corr %.4f, no zero-point corr %.4f (both must fail)\n",
r.ctrl_corr_nibble, r.ctrl_corr_nozp);This commit sets selfcheck_reference = true and computes lmhead_rel/lmhead_corr — it does not compute the two controls. So on a 1.9 pack the
output is:
self-check: lm_head vs the pack's fp16 embedding corr 0.999999 rel_err 0.000700
controls: nibble-swapped corr 0.0000, no zero-point corr 0.0000 (both must fail)
where both 0.0000s are the initialisers. "Both must fail" is satisfied vacuously, because nothing was measured — which is exactly the shape this
family of checks exists to avoid, and the one place the PR's own argument leans on it.
Two ways, both small:
- Compute them here, exactly as the host path does (
decode_matmulnbits(..., swap_nibbles=true, false)and(..., false, drop_zero=true), then
corr(..., 2048)— the host path compares only 2048 rows for speed) and set the two fields. That is ~6 lines and makes the summary true for a 1.9
import. - Or suppress the line when they were not computed — a
bool controls_measuredprinted ascontrols: not run on this path— so an absent control
reads as absent rather than as a perfect result. Given the point of the self-check, I would take (1): the controls are what show the metric
discriminates, and a 4-bit decode against an fp16 reference deserves the negative case.
Also for the record, and worth keeping: your gate status is the part that makes the 1.9 path credible — 291 tensors / 67088 tiles identical to the 1.8
twin, 768/768 attention and 2160/2160 MLP groups surviving decode→encode, 48/48 norm anchors, all 291 blobs byte-identical, and greedy text identical on
three prompts on the NPU as well as host GEMM (8 ELFs from the existing 05b-hyb set, amdxdna errors 0, ticket held 25 s and released). And the
artifact note — no new kernel family needed, because hybrid and plain packs differ in bias, not shape, so kernels-05b-hyb-b8 is the right geometry —
saves the next person from building a redundant set.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
The tail-head import added in #1507 decoded the head but skipped the numeric check the host-head path has always run, so an import of a 1.9 pack reported "self-check: skipped -- the head is not tied to the embedding" while the pack's own embedding was right there as an independent reference. Run it: on a tied model the pack's fp16 embedding must reproduce its 4-bit head, and an untied one simply comes out low -- either way it is evidence. The nibble-swapped and no-zero-point decodes stay as the controls that must fail. Measured on amd/Qwen2.5-0.5B-Instruct_rai_1.9.0_npu: corr 0.999999, rel_err 0.000700, controls 0.0000 and 0.0000. That is the same evidence the 1.8 pack's host head carries, and it is what makes the tail layout self-verifying rather than merely self-consistent.
…ape is the second measured entry
The tail-head read assumed the head's tiles start at a fixed 1,856 bytes into
the tail. That number is only qwen2.5-0.5B's, where 1,856 == 64 + 2*896 by
coincidence: the envelope is the same `64 + 2*hidden` a layer block opens with
(eps, padding, the norm). On Qwen3-1.7B the constant missed the head's start by
2,304 bytes and the import refused -- correctly, since it checks the arithmetic
to the byte instead of trusting the offset. With the envelope derived from the
hidden size, Qwen3-1.7B's tail is 4,160 + 2,374 x 73,984 exactly.
That refusal is what surfaced the second measured 1.9 entry, and the entry is
worth having because the prefix is not `64 + 2*hidden` either: Qwen3 carries its
per-head QK norm gains inside the block before the q/k/v bundle, so the prefix is
8,512 at hidden 2,048 where the same formula would say 4,160. Both numbers are
therefore rule parameters, measured per pack against the 1.8 twin, never derived.
Verified for amd/Qwen3-1.7B_rai_1.9.0_npu against its 1.8 twin:
* import lands 311 tensors / 210,016 tiles, the same counts as the 1.8 pack,
with 2,688/2,688 attention tiles and 3,584/3,584 MLP groups surviving
decode->encode, 56/56 RMSNorm anchors, self-check corr 0.999999 /
rel_err 0.000764 (controls 0.0000 and 0.0000);
* greedy text is identical to the 1.8 container on three prompts on the host
GEMM, and identical again on the NPU with the existing Qwen3-1.7B b8 kernel
set -- including the one prompt whose continuation differs between backends,
which is the int8 EP vs fp32 host difference and appears identically in both
releases.
The two builds' fconsts differ in 535 code bytes, all in layer 24 (0.00006 % of
the file): content, not layout, and not enough to move greedy text.
c1d8c8a to
a1799db
Compare
…d import
Phi-3-mini-4k-instruct_rai_1.7.1_hybrid imports cleanly and then printed
self-check: lm_head vs the pack's fp16 embedding corr -0.005766 rel_err 1.638304
which looks like a broken decode and is not: Phi-3 is untied, so lm_head and
model.embed_tokens are separate matrices and correlating them tests nothing. The
container was already right (it leaves tie_word_embeddings absent); only the
wording misled, and it would send someone chasing a phantom on every Phi-3 pack.
The report now distinguishes the two cases. Tied packs keep the existing line and
controls. Untied ones say what they are:
self-check: not a check here -- this pack is untied (lm_head and the embedding
are separate matrices), so their correlation -0.005766 and
rel_err 1.638304 are what an untied pack looks like, not a decode
failure; the controls still run
controls: nibble-swapped corr 0.0015, no zero-point corr -0.0126 (both must fail)
The controls still run in both cases, because they are meaningful either way.
Measured on models/amd-rai/hybrid-probe/phi3mini-1.7.1: 387 tensors / 454,392
tiles, exactly as before -- the import itself is unchanged.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 229ec4379ff96e7c6948390b1a929143f104a1d4, with one small ask that is cheap now and expensive later.
The arithmetic is right, and it is the substantive half. The old code assumed the head's tiles start a fixed 1,856 bytes into the tail; that constant is only qwen2.5-0.5B's:
64 + 2 * 896 = 1856 (qwen2.5-0.5B, hidden 896 — the old hardcoded value)
64 + 2 * 2048 = 4160 (Qwen3-1.7B, the "second measured entry")
and the call site now computes with the envelope rather than the constant (at(fc, tail_off + env + i * tile, tile)). So the fix is "one pack's constant became a formula", which is exactly the shape of defect that reads correctly on the model it was written for and silently misreads the next one.
The self-check now runs on the same terms as the host head, including the two controls that must fail (nibble-swapped decode, no-zero-point decode) — a self-check without failing controls only proves that something decoded. And the untied case is honestly labelled rather than left to look like a pass: self-check: not a check here -- this pack is untied (lm_head and the embedding are separate …).
The ask: pin the formula with the two entries you already measured. The existing test mentions tail 23 times, so the path is exercised — what is not exercised is the thing this PR changes: a fixture whose hidden size is not 896 would fail on the old constant and pass on 64 + 2*hidden. Two cases (1856 and 4160) in npu_pack_import_test.cpp make the next hardcoded constant impossible to reintroduce, and it is the same file I asked for a fixture in on #1412, for the same reason: a parser constant with no row is a constant that can go back.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 229ec437 — the self-check work in this head is good and generalises properly, but my last finding is unchanged, and one new sentence makes it worse.
First, credit where the head earns it — this is more than the follow-up I asked for:
prefixis now a measured table field (bytes before the q/k/v bundle), because "Qwen3 puts its per-head QK norm gains inside the block before the bundle, so64 + 2*hiddenis not the rule there" — which is exactly the generalisation I asked for on #1507, done by measuring rather than by asserting.- A second measured row joined the table (Qwen3-1.7B,
{2048, 16, 28, 8512, 3, 1, 1183744}), so it is now a table and not a special case. - The tail envelope is derived, not constant:
const int64_t env = attn_payload_off(H); want = env + rows * tile;—64 + 2*hidden. And the measured consequence is exactly why this matters: the old1,856would have been off by 2,304 B on Qwen3-1.7B (4,160 − 1,856), which would have refused the pack rather than mis-parsed it. A constant whose staleness trips its own assertion is the right failure mode, and deriving it removes the trap.
The controls, though, are still not computed on the tail path — and the output now claims they are. Every assignment in the file:
2121: r.ctrl_corr_nibble = corr(swap, embed_bytes, int(H), 2048); <- host-side MatMulNBits path only
2122: r.ctrl_corr_nozp = corr(nozp, embed_bytes, int(H), 2048);
2776: … r.ctrl_corr_nibble, r.ctrl_corr_nozp); <- tied branch prints them
2785: … r.ctrl_corr_nibble, r.ctrl_corr_nozp); <- untied branch prints them
and the tail block has exactly one corr( call (the lmhead_corr). So a 1.9 import prints
controls: nibble-swapped corr 0.0000, no zero-point corr 0.0000 (both must fail)
from the initalisers — and in the untied case the line above it now says, verbatim, "the controls still run", which is false on that path. That is a stronger version of what I raised last time: not a vacuous pass, but a sentence asserting a check that did not run.
Two ways, both small. Compute the two controls in the tail block — the decode it needs is decode_matmulnbits(..., swap_nibbles=true, false) and (..., false, drop_zero=true) over the same 2048 rows the host path uses, about six lines, and then the summary is true for a 1.9 pack. Or drop the claim: print controls: not run on this path so absence reads as absence. Given the self-check exists to be evidence about the tail layout, I would take the first.
Also: kTailNorm is now dead (one occurrence, the declaration at :228; the envelope moved to attn_payload_off(H)) — same leftover as kFconstHidden in #1507.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 229ec4379ff96e7c6948390b1a929143f104a1d4
My finding is fixed, and the fix is structural rather than cosmetic.
The controls can no longer be printed from struct defaults. The three arms are mutually exclusive on the same flag that gates the computation:
guard: if (tied || head_geometry) { … compute lmhead_corr, ctrl_corr_nibble, ctrl_corr_nozp … } // :2119
print: if (r.selfcheck_reference && r.selfcheck_tied) -> corr + rel_err + controls (measured)
else if (r.selfcheck_reference) -> "not a check here -- untied …", the corr numbers, controls (measured)
else -> "self-check: skipped -- the head is not tied …" (no 0.0000 at all)
So every path that prints a number is a path that computed it, and the case that has nothing to measure now says so and prints no zeros. That is exactly the class I raised: a 0.0000 that reads as "the control failed to fail" when it was never run.
The untied arm is also the more honest reporting. "their correlation %.6f and rel_err %.6f are what an untied pack looks like, not a decode failure; the controls still run" — a number with its meaning attached, rather than a bare figure a reader would take as a decode score.
Two constants became measured data, and the arithmetic holds:
qwen2.5-0.5B table prefix 1856 = 64 + 2*896 (the formula happens to be the rule there)
qwen3-1.7B table prefix 8512 != 64 + 2*2048 -- correct, because Qwen3 carries per-head QK gains
inside the block, which is why prefix is a table entry
tail envelope env = attn_payload_off(H) = kNormOff + 2*H = 4160 for hidden 2048
old constant 1856 -> off by 4160 - 1856 = 2304, exactly the "would have missed by 2,304" in the comment
So prefix (per model, measured by diffing 1.9 against 1.8 of the same model) and env (derived from the hidden size) are now separate quantities with separate rules, and the comment says which is which and why the constant was wrong — "1,856 is only its qwen2.5-0.5B value". Replacing a constant that was one model's value with a per-model table plus a derived formula is the right shape; a single constant would have kept passing on the model it came from.
Scope note: the head also carries main's merged work (a rebase across 25 files). The PR's own diff against its merge-base is the two npu/pack_import.* files, +60/−7, and that is what this verdict covers.
Not run: the import itself (needs the 1.9 packs). CI does build npu_pack_import_test, which compiles pack_import.cpp, so the code path here is covered by the build; the numbers above are arithmetic against the code, not a measurement of a pack.
…ck's does not answer garbage
Phi-3-mini-4k-instruct_rai_1.7.1_hybrid imports cleanly (387 tensors, 454,392
tiles) and then answers garbage on the NPU -- "slower slower slower slower" --
and the same garbage on the HOST GEMM, which puts the fault upstream of every
ELF and every artifact.
The cause is one absent key. The forward decides between "logits from the token
embedding" and "logits from a separate lm_head.weight" on `tie_word_embeddings`
and defaults to TIED when the key is absent (npu/forward/src/q4nx_semantics.c).
A genre-A pack ships no config.json at all, so the importer never wrote the key,
so the forward decoded Phi-3's logits from its embedding matrix while the head
tensor it had just imported sat unused in the container.
Measured, same weights and same backend, only the key changed:
(absent) ' slower slower slower slower slower ...'
tie_word_embeddings: false ' Paris.\n\n- [Student]: What is the capital'
" 4.\n\nNow, let's calculate the"
" The sky appears blue to us because of the way Earth'"
The container now states it, and where the pack gives no answer the DATA gives
one, by the same correlation the self-check already reports: the imported head
reproduces the embedding (>= 0.99) means tied, a separate matrix means not. So
Phi-3-mini gets `tie_word_embeddings: false` with a `q4nx_tie_note` saying why,
and SmolLM2-135M's hybrid (corr 0.9957) still gets true. A pack that declares the
key is untouched: Llama-3.2-1B and Qwen2.5-0.5B keep the true they ship.
The readout follows the same number, not the pack's declaration, because the two
were disagreeing in the other direction: on a genre-A pack `tied` is false (no
config.json), so a tied pack at corr 0.9957 printed "this pack is untied ... what
an untied pack looks like" while the container it wrote said tied. Both now branch
on the one threshold, and the line says which case it is and that it passed:
self-check: the head reproduces the pack's fp16 embedding corr 0.995685 rel_err 0.103587
(tied: this correlation is the check, and it passed)
Verified that the message and the container field agree on both sides of the
threshold: smollm2-135m hybrid (0.9957) and qwen2.5-0.5b-1.7.1 hybrid (0.9945)
both read tied and both write true; phi3mini-1.7.1 (-0.0058) reads untied and
writes false.
Two other candidates for the same garbage were ruled out first, with numbers, so
this does not get re-litigated later: Phi-3's longrope is plain RoPE at theta
1e4 here (the pack's own cos_cache correlates 1.0000, and the implied inv_freq
0.825282 matches 0.825404), and the untied head decodes correctly (its correlation
against the embedding is -0.0058, which is what an untied head is supposed to give).
e8232e1 to
15d39f4
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 15d39f4b — one item, unchanged from my last review; everything this head added is good, and one of the two new commits is the
best bug story in the queue.
The tie bug is diagnosed and fixed properly. Root cause stated exactly: a genre-A pack ships no config.json, so the importer never wrote
tie_word_embeddings, and the forward defaults to tied when the key is absent (q4nx_semantics.c) — so Phi-3-mini decoded its logits from the
embedding matrix with the head tensor sitting unused in the container, and answered slower slower slower. And the mutation test is the cleanest proof
possible: same weights, same backend, only the key added → Paris.\n\n- [Student]: What is the capital. Nothing about the decoder changed; the key
alone did.
The decision logic is right in all three cases, which I checked because "infer a structural property from a correlation" is where this usually goes
wrong:
if (tie_declared) c["tie_word_embeddings"] = tied; // a pack that declares it is untouched
else if (head_written) c["tie_word_embeddings"] = r.lmhead_corr > 0.99; // + q4nx_tie_note naming the reason and the number
// else: no key written -> the forward's tied default, which is correct when no separate head was imported at allThat third branch is the one I would have asked for and did not have to: with no head imported the embedding is the head, so leaving the key absent is
the right answer rather than an omission.
The envelope derivation did what a derivation should: 64 + 2*hidden via attn_payload_off(H) refused Qwen3-1.7B first — the old constant missed
the head start by 2,304 B — and the entry was measured only after the refusal. An assertion that catches a stale constant before a table row exists is
exactly the ordering I want.
The one-threshold point checks out arithmetically: > 0.99 gives tied at 0.9957 and 0.9945, untied at −0.0058 — message and field agree on both
sides, which is the disagreement you were fixing. Two small notes on it:
0.99is now written in three places (the twoselfcheck_tiedassignments and the container field). You have just fixed them disagreeing; a named
constant (constexpr double kTieCorr = 0.99;) would make that agreement structural instead of three literals that currently match.- The residual edge: with
head_writtentrue and corr > 0.99 the container says tied even though a separate head was imported, so the forward would
decode from the embedding and ignore it — and the note would read "the imported head reproduces the embedding" while a separate matrix exists. Narrow
(it needs a fine-tuned model whose head ≈ its embedding), and the correlation is the best signal available in the ambiguous case, so I would not change
the rule — just be aware that this is the corner where the inference can be wrong silently.
The item that keeps this at CHANGES is the controls, and the new head makes it more visible rather than less. Every assignment in the file:
2127/2128 r.ctrl_corr_nibble / r.ctrl_corr_nozp = corr(...) <- host-side MatMulNBits path ONLY
2799 printed in the tied branch
2806 "…the controls still run" printed in the untied branch
The tail (1.9) path sets selfcheck_reference and now selfcheck_tied — so it now routes into those branches — but computes no controls. A 1.9 import
therefore prints controls: … 0.0000, … 0.0000 (both must fail) from the initialisers, and in the untied case asserts "the controls still run".
Same ask as before, ~6 lines: compute them in the tail block (swap_nibbles=true / drop_zero=true over the same 2048 rows), or print
controls: not run on this path. Given this PR's whole argument is that the self-check makes the 1.9 layout evidence rather than self-consistency, the
negative case is the half that carries it.
Consumer-lane verification on a real 1.9 pack: the self-check goes from skipped to measured, and it passesBuilt
So on a real 1.9 pack the PR converts an unverified head into a quantified one — corr 0.999999, rel_err 7.0e-4 — and nothing else in the import changes (the fconst geometry and all 48 RMSNorm anchors are bit-identical between the two heads of the build). That is exactly the property the PR claims, measured rather than argued. Two notes from doing it, offered as evidence rather than review opinion:
Method/limits: one pack, one geometry (h896, 24L, 14/2 heads, head_dim 64), host-side import only — no |
…s q_proj's width The measurement tool from #1532 found Qwen3-4B's entry on its first real pair, and importing with it exposed a second bug the family guard caught before it could serve wrong text: declaring "QKV:2560x6144,O:2560x2560,GU:2560x19456,D:9728x2560" this model derives "QKV:2560x6144,O:4096x2560,GU:2560x19456,D:9728x2560" o_proj's K is q_proj's OUTPUT width, which equals hidden only when heads*head_dim == hidden. Qwen3-1.7B satisfies that (16 x 128 = 2048), Qwen3-4B does not (32 x 128 = 4096 against a hidden of 2560), so the family string was wrong for exactly the models where the two differ. The guard refused the container rather than serving it, which is the behaviour #1480 was merged for. Qwen3-4B's 1.9 entry: prefix 9,536, 5 tiles of 1,462,272 rotated by 1, and 2,933 of 2,208,061,248 bytes differ from the 1.8 twin (1.3e-6) -- content, not layout. The prefix corroborates the Qwen3 offset at a second hidden size: 64 + 2*hidden + 4,352 gives 8,512 at 2,048 and 9,536 at 2,560. Verified: import 399 tensors / 491,000 tiles, 4,896/4,896 attention tiles and 6,912/6,912 MLP groups survive decode->encode, 108 norm anchors, self-check corr 0.999999 / rel_err 0.000737, and greedy text on the host GEMM: 'The capital of France is' -> ' in which country? Also, could you explain the difference between' '2 + 2 =' -> ' 4, but 2 + 2 = 4' 'Why is the sky blue?' -> ' What is the scientific explanation for this phenomenon?\n\nThe sky appears'
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7b9401ce6055d08575b6ddf8717879b51e573fb1 — and the ask in my previous review was already satisfied, which is my mistake to correct.
I asked for "two cases (1856 and 4160) in npu_pack_import_test.cpp". They are there, on main, before this PR:
$ git show origin/main:tests/npu_pack_import_test.cpp | grep -n 'attn_payload_off'
1459: check(pi::attn_payload_off(896) == 1856, …)
1461: check(pi::attn_payload_off(2048) == 4160, "attn off: hidden 2,048 gives 4,160, not 1,856")
with a third case contrasting the old constant (h1 - 1856, "80 tiles + N strip"). So the formula is pinned by a named helper's own unit cases, this PR does not need to add any, and I asked for a row without first checking whether it existed — the same failure I made on #1406 (asking after reading the code instead of running the gate). Noted rather than quietly dropped.
What the head adds since I last read it, all of which reads correctly:
- a fourth pack entry in the measured table (
{2560, 32, 36, 9536, 5, 1, 1462272}) with the corroboration spelled out — Qwen3-4B differs in 2,933 of 2,208,061,248 bytes (1.3e-6), content not layout, and its offset follows the same64 + 2*hidden + 4,352prefix while the tile count (5) differs from 1.7B's (3), "which is why this is measured per pack and never derived"; - the self-check follows the number, not the declaration:
r.selfcheck_tied = r.lmhead_corr > 0.99instead of the pack'stiedflag, because a genre-A pack ships noconfig.json— so the readout used to call those packs untied while the container it wrote said tied. A diagnostic that disagrees with the artefact it just wrote is the class this lane keeps finding, and this is the right direction of fix (the number wins). o_proj's K is q's output width, nothidden— on Qwen3-4B that is32 × 128 = 4096against hidden 2560, and the family guard refused the container exactly as it should ("declares O:2560x2560, this model derives O:4096x2560").
Three measured corrections in one PR with the arithmetic shown next to each; nothing left to ask for.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7b9401ce6055d08575b6ddf8717879b51e573fb1
The two additions are both right, and the new row's arithmetic checks:
Qwen3-1.7B hidden 2048 -> 64 + 2*2048 + 4352 = 8512 (table: 8512) ✓
Qwen3-4B hidden 2560 -> 64 + 2*2560 + 4352 = 9536 (table: 9536) ✓
so the rule the comment states is corroborated across two packs, and "the tile count differs from 1.7B's 3, which is why this is measured per pack and never derived" is consistent with the table carrying 3 and 5.
The selfcheck_tied change is the better half: "the diagnostic follows the NUMBER, not the pack's declaration: a genre-A pack ships no config.json, so tied is false there while the head plainly reproduces its embedding, and the readout used to call those packs untied while the container it wrote said tied. Same threshold the container's own decision uses." Making the report agree with the container's decision instead of with a missing declaration is exactly right — the previous version could print "not a check here, untied" for a pack the writer had just recorded as tied, which is a report contradicting its own artifact.
One thing to check in the comment, and my arithmetic is the reason. The offset is attributed to "per-head QK norm gains", but 4,352 is the same constant for a 16-head and a 32-head model:
per-head QK gains as the importer plans them: self_attn.{q,k}_norm.weight, BF16 [head_dim] each
-> 2 * 128 * 2 = 512 bytes per layer for head_dim 128, in both models
4352 is head-count-independent, so it is not "per-head" anything
The rule is fine (two packs corroborate it) and the table is right either way; what I would fix is the attribution, because a reader adding the next Qwen3 pack will try to derive 4,352 from heads × head_dim and get 512. Either name the decomposition (the ~3,840 bytes that are not the two gain tensors — section headers, eps/padding?) or say "a fixed per-block prefix term, measured, origin not identified". A number in a comment that cannot be re-derived from the mechanism it names is the kind of thing that gets copied wrong.
Not run: a Qwen3 1.9 pack to measure the new row independently (this box has Qwen3-1.7B_rai_1.8.0_npu_4K and Qwen3-4B_rai_1.8.0_npu_4K but no 1.9 twins), so the row itself is the author's measurement and only its arithmetic is mine.
… name
Review finding, and it was right: the tail (.fconst) path set selfcheck_reference
and selfcheck_tied, so it routed into the reporting branches, but it computed no
controls -- so a 1.9 import printed
controls: nibble-swapped corr 0.0000, no zero-point corr 0.0000 (both must fail)
straight from the struct initialisers, and in the untied case asserted "the
controls still run" when nothing had run. The negative case is the half of this
PR's argument that carries it, so printing its numbers from defaults is exactly
the wrong failure.
The tail path has no MatMulNBits bytes to mis-decode, so its control is an
ALIGNMENT one, on the same tiles and through the same decoder: each head row
matched against the NEXT embedding row. A correct decode correlates ~0.999999
with the embedding; a misaligned head must not, and does not:
controls: row-shifted corr 0.0716 (must fail)
The host path keeps its nibble-swapped / no-zero-point controls, which are the
right ones there because those bytes exist. Each path now prints only what it
measured.
Also from the review: the 0.99 tied threshold is now `constexpr double kTieCorr`,
used by the two selfcheck_tied assignments, the container field and its note --
three literals that had just been reconciled by hand, which is how they drifted
apart in the first place. The reviewer's other point, that a tied-by-inference
container with a separate head imported decodes from the embedding and ignores
that head, is left as the reviewer advised: the correlation is the best signal in
the ambiguous case, and the note says which way it was decided and why.
|
Preparing review... |
|
The blocking item is fixed at The tail path now carries its own control. It has no MatMulNBits bytes to mis-decode, so an alignment control is the honest analogue: the same tiles, the same decoder, each head row matched against the next embedding row. A correct decode correlates 0.999999 with the embedding; the misaligned one: The host path keeps nibble-swapped / no-zero-point, which are the right controls there because those bytes exist. Each path prints only what it measured, so the
Your residual edge left as advised: with Two commits since |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b43589beb1b55c6f67d3ae4ffff15333fb054028
Two changes, and the second is the one that makes the first meaningful.
The threshold is now named (constexpr double kTieCorr = 0.99) with the reason stated — "the container's field, the readout and the note all have to agree on it: three copies of 0.99 is how they drifted apart before." Three literals that must agree is exactly the shape that drifts, and giving it one name is the fix that cannot be forgotten.
The untied path gains a real negative control — and this is the part that closes the loop I opened two rounds ago: instead of printing the host path's nibble-swap and zero-point controls (which this path cannot run, "there are no such bytes here"), it computes a row-shifted correlation: the same tiles and the same decoder, with each head row matched against the next embedding row.
for rr in [0, min(2048, q.N-1)): x = decode(head row rr) y = embed row (rr+1)
ctrl_corr_rowshift = corr(x, y) printed as "row-shifted corr %.4f (must fail)"
That is a control with a known wrong answer: a decoder that is reading the right tiles in the right order must collapse when the rows are shifted by one. It is a stronger check than the nibble-swap on this path because it perturbs the addressing, which is the thing a genre-A split layout gets wrong. And the guard if (r.ctrl_corr_rowshift >= 0) means the line is only printed when the control actually ran — so no struct-default 0.0000 can appear, which is the same class the earlier round fixed for the other two controls.
c["tie_word_embeddings"] = r.lmhead_corr > kTieCorr; now shares the constant with the readout, so the container's decision and the printed verdict cannot disagree.
My earlier note stands unchanged and is not a blocker: 4,352 is head-count-independent, so "per-head QK gains" cannot be its explanation; the rule and both table rows are right on their arithmetic (8512 = 64+2·2048+4352, 9536 = 64+2·2560+4352).
Not run: an import against a real pack at this head (the pack leg needs a 1.9 pack this box does not have for Qwen3); the control's code path and its print guard are what I verified.
…xplanation of the prefix
The measurement tool found this entry on a non-Qwen family for the first time:
{2048, 32, 16, 8512, 5, 1, 591872}, with 1,857 of 686,518,336 bytes differing from
the 1.8 twin (2.7e-6 -- content, not layout).
It also corrects a comment I wrote two entries ago. I had explained the extra 4,352
bytes in the prefix as Qwen3's per-head QK norm gains sitting inside the block before
the bundle. Llama-3.2-1B has no QK norms at all and carries the same prefix at the
same hidden size (8,512 at hidden 2,048), so the extra is a property of the geometry
and not of the architecture. The table keeps doing its job -- it is measured -- but
the explanation next to it was a plausible story that would have become the reason
some future row was added wrongly. That is the second time this week a tidy
explanation survived exactly one model.
What differs from Qwen3-1.7B at the same hidden size is the bundle: 5 tiles of
591,872 against 3 of 1,183,744, so the two models cut the same payload into
different pieces and the tile count alone identifies neither.
Verified: import 147 tensors / 150,848 tiles (the same counts as its 1.8 twin),
1,280/1,280 attention tiles survive decode->encode, self-check corr 0.999999, and
greedy text on the host GEMM:
'The capital of France is' -> ' Paris.\nThe capital of France is Paris. \nThe capital'
'2 + 2 =' -> ' ?\nTo add 2 + 2, you simply count'
'Why is the sky blue?' -> ' The sky appears blue because of a phenomenon called Rayleigh scattering'
|
Re-verdict request at The delta is a measured table row and a corrected comment, no logic: Llama-3.2-1B-Instruct 1.9 — It also refutes the explanation I gave you for |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 0019f0a4.
The controls item is not just fixed — it is fixed better than I asked. I said "compute the two host controls on the tail path, or print controls: not run". What is there instead is the right control for that path:
// The control this path can actually run: the same tiles and the same decoder, with each row shifted
r.ctrl_corr_rowshift = … // computed inside the .fconst-tail block
…
if (r.ctrl_corr_rowshift >= 0) printf(" controls: row-shifted corr %.4f (must fail)\n", …);
else printf(" controls: nibble-swapped corr %.4f, no zero-point corr %.4f (both must fail)\n", …);with the same gate in both branches (tied and untied). So on a 1.9 import the printed control is one that was computed from those tiles; on a host
import rowshift is the -1 sentinel and the nibble/zero-point pair — which is computed there — is printed. No path prints a default any more, and
"the controls still run" is now true where it is printed. A sentinel distinguishing "measured" from "not measured" is exactly the shape I was asking for,
and the control itself ("the same decoder, rows shifted") is a better negative than the nibble swap for a tail-sourced head.
kTieCorr is in, with the reason written next to it ("three copies of 0.99 is how they drifted apart before") ✔ — that closes my other nit structurally.
And one correction I owe you: the prefix explanation I repeated in my last review was wrong, and your new comment says why. I had echoed "Qwen3 puts
its per-head QK norm gains inside the block before the bundle". Llama-3.2-1B has no QK norms at all and carries the same +4,352 at hidden 2,048 —
so the extra is geometry, not architecture. Your new table row also corroborates the corrected claim from the data: Qwen3-1.7B and Llama-3.2-1B are the
two hidden-2,048 entries and both carry prefix 8,512.
The table is becoming a real table — four measured rows now (SmolLM2, Qwen3-1.7B, Qwen3-4B at 1.3e-6 residual, Llama-3.2-1B as the first non-Qwen
family), each measured rather than derived, with the residual recorded per row.
One leftover, cosmetic: kTailNorm is still declared and unused (one occurrence), since the envelope became attn_payload_off(H).
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE. Every constant in it checks, one of them against a measurement I made in a different lane, and the PR's own thesis ("measured per pack, never derived") is now one step away from being unnecessary for its last magic number.
The arithmetic, verified: 64 + 2·896 = 1,856 (the qwen2.5-0.5B envelope, which is why that constant looked general — hidden 896 is the only size where the old hardcoded 1,856 and the real rule coincide); 64 + 2·2,048 = 4,160 (the 1.7B tail base); 4,160 + 4,352 = 8,512 and 64 + 2·2,560 + 4,352 = 9,536 — so the Qwen3 prefix formula is confirmed at two hidden sizes, which is the difference between a fitted constant and a rule.
The o_proj K finding is the best thing here and I would generalise the sentence. o_proj's K is q_proj's output width, which equals hidden only when heads·head_dim == hidden — true for 1.7B (16·128 = 2,048) and false for 4B (32·128 = 4,096 ≠ 2,560). That means the old family string was wrong for a whole class of models where the dimensions diverge, which is the common case for anything with heads·head_dim > hidden — i.e. the bug was not one bad constant but an identity assumption that holds only on models with no unused head capacity. Two consequences worth stating in the code: (a) name the invariant (O.K == Q.N, not O.K == hidden) where the family string is built, so the next reader sees which of the two it may rely on; (b) the guard is the hero — "refused loudly instead" is what turned a silent wrong-answer class into an error. Is that guard exercised by a negative test, i.e. one that feeds a deliberately wrong family string and asserts the refusal? If not, it is the single most valuable test this PR could add, because the guard's value is exactly its future behaviour on packs this lane has never seen.
One question about the last magic number, and it comes from my own instrument. 4,352 = 2 × 2,176. On #1410 I ran a zero-run grid over the o_proj section and the gap histogram was {2176: 567, 6416: 10, 112: 9} — 2,176 dominant by two orders of magnitude, with boundary pairs summing to 6,528 = 3 × 2,176. So the Qwen3 prefix offset is exactly twice a quantum that appears all over the neighbouring structure. If that is the same 2,176, then the prefix is not a measured constant at all but 64 + 2·hidden + 2·q for some q with a meaning (a pair of gains plus their record?), and deriving it would remove the only number in the PR that a reader cannot check. I am not claiming they are the same unit — I have not read the Qwen3 blocks myself — only that the coincidence is exact, that the measurement to settle it is cheap, and that "measured per pack" is a fallback, not an ideal, for a constant that has an explanation.
One ask on item 1's untied case. "on an untied one the number simply comes out low" is right, but the check should also say which situation it believes it is in — tied-and-passing versus untied-and-therefore-inapplicable versus tied-and-failing. As written a genuinely broken tied import prints a low number and is indistinguishable from the honest untied model, which is the same silence class as the .fconst fix in #1533: the check runs, reports a number, and nothing tells the reader whether that number was supposed to be high.
I reviewed this from the body and the arithmetic; I did not run the importer. Noted for the release: #1533 says the two must land in the same release, either order — this PR alone imports the split-qkv packs' heads but leaves the tie key absent, and the consequence it names for that state ("silently wrong for an untied split-qkv model") is the reason the pairing belongs in both PR bodies, which #1533 already does.
Two commits, both from the same session of verifying 1.9 packs against their 1.8 twins.
1. The tail head gets the self-check the host head has always had
The tail-head import in #1507 decoded
lm_headbut skipped the numeric check the host-head path runs, so a 1.9 import reportedself-check: skipped -- the head is not tied to the embeddingwhile the pack's own fp16 embedding was there as an independent reference.It now runs, on the same terms: self-check (on a tied model the pack's embedding must reproduce its 4-bit head; on an untied one the number simply comes out low), with the nibble-swapped and no-zero-point decodes as controls that must fail.
2. The tail envelope is
64 + 2*hidden, and Qwen3-1.7B is the second measured entryThe read assumed the head's tiles start a fixed 1,856 bytes into the tail. That constant is only qwen2.5-0.5B's, where
1,856 == 64 + 2*896by coincidence — the envelope is the same64 + 2*hiddena layer block opens with. On Qwen3-1.7B it missed the head's start by 2,304 bytes, and the import refused rather than reading the wrong offsets; with the envelope derived from the hidden size the tail is4,160 + 2,374 × 73,984exactly.That refusal is what surfaced the second entry, and it is worth having because the prefix is not
64 + 2*hiddeneither: Qwen3 carries its per-head QK norm gains inside the block before the q/k/v bundle, so the prefix is 8,512 at hidden 2,048 where that formula says 4,160. Both are rule parameters, measured per pack, never derived.5. Qwen3-4B's 1.9 shape, and o_proj's K in the kernel family
The measurement tool merged as #1532 found Qwen3-4B's entry on its first real pair —
{2560, 32, 36, 9536, 5, 1, 1462272}, with 2,933 of 2,208,061,248 bytes differing from the 1.8 twin (1.3e-6: content, not layout). Its prefix corroborates the Qwen3 offset at a second hidden size (64 + 2*hidden + 4,352→ 8,512 at 2,048, 9,536 at 2,560), while the tile count differs from 1.7B's 3 — which is exactly why this is measured per pack and never derived.Importing with it exposed a second bug, caught by the family guard before it could serve wrong text:
o_proj's K is q_proj's output width, which equals hidden only when
heads*head_dim == hidden. Qwen3-1.7B satisfies that (16 × 128 = 2048); Qwen3-4B does not (32 × 128 = 4096 vs hidden 2560), so the family string was wrong for exactly the models where the two differ — a silent wrong-answer class, refused loudly instead. Verified after the fix: import 399 tensors / 491,000 tiles, 4,896/4,896 attention tiles and 6,912/6,912 MLP groups throughdecode->encode, self-check 0.999999, designsQKV:2560x6144,O:4096x2560,GU:2560x19456,D:9728x2560, and coherent greedy text on the host GEMM.Evidence
Qwen2.5-0.5B (
rai_1.9.0_npuvsrai_1.8.0_npu_4K): 291 tensors / 67,088 tiles (same counts), self-checkcorr 0.999999/rel_err 0.000700(controls 0.0000, 0.0000), all 291 tensor blobs byte-identical to the twin, greedy text identical on three prompts on the host GEMM and on the NPU.Qwen3-1.7B (
rai_1.9.0_npuvsrai_1.8.0_npu_4K): 311 tensors / 210,016 tiles (same counts), 2,688/2,688 attention tiles and 3,584/3,584 MLP groups survivedecode->encode, 56/56 norm anchors, self-checkcorr 0.999999/rel_err 0.000764(controls 0.0000, 0.0000), greedy text identical to the twin on three prompts on the host GEMM and identical again on the NPU — including the one prompt whose continuation differs between backends, which is the int8 EP vs fp32 host difference and appears identically in both releases.The two 1.9 builds' fconsts differ from their twins in 535 code bytes (Qwen3, all in layer 24) and 112 (SmolLM2, all in layer 3) — content, not layout, and not enough to move greedy text. Byte-identical packs are therefore not a valid acceptance test; identical text on both backends is.
ctest: 128/129; the single failure,npu_runtime_option, is pre-existing on a clean tree.