Repository navigation
npu-pack: read the post-attention norm from the block's last epsilon header (engine#1003) - #1018
Conversation
…header (engine#1003) import_pack took the block's SECOND epsilon header as the post-attention one. That is right on a 2-header block (Qwen2.5, Llama) and wrong on a Qwen3 block, where the second and third headers open the per-head k_norm and q_norm sections -- so every Qwen3 import carried the k_norm gains plus the padding behind them as post_attention_layernorm in all layers (1,791 of 2,048 values zero, max 68.0 on Qwen3-1.7B, against a real norm that is zero-free and 0.9919 correlated with the upstream checkpoint). plan() already used the last header for the MLP section's start, so plan and import disagreed for exactly the 4-header packs. FconstGeom::post_attn_hdr() = hdr.empty() ? h1 : hdr.back(), shared by the norm read, its epsilon anchor and plan()'s MLP offset, so the two readers cannot diverge again. On a 2-header pack it returns h1, i.e. Qwen2.5/Llama imports are byte-identical to before. The regression test builds a crafted 4-header-per-block .fconst with a distinct recognisable value behind every header and reads the imported weight back out of the container: it fails 4 checks on the old code (both "is the bytes after the block's LAST header" and "is not the k_norm section's marker") and passes here, and it also asserts the 2-header case still resolves to h1. All four shipped packs pass the real-pack leg.
…ock header count (engine#1003)
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
This is a data-correctness fix, so I re-derived the layout from the packs on this box rather than from the table, and
then used the pack's own GGUF sibling as the reference.
The geometry, found by scanning for the 64-byte eps header (86 35 = bf16 1e-6, then zeros) — Qwen3-4B pack,
first 50 MiB, found exactly four per block:
block-relative header offsets, Qwen3-4B pack: h0 = 0 h1 = 8,778,816 h2 = 8,779,136 (= h1+320) h3 = 14,524,096
h2 - h1 = 320 = 64 + 128*2 = the k_norm section's header, then q_norm's (head_dim 128)
Then the bytes behind each header, against Qwen3-4B-Q4_K_M.gguf on this box (norms stay high-precision there):
zeros max corr[blk.0.ffn_norm] (= post_attention_layernorm)
h0+64 0 +0.1089 -0.1313 (it is attn_norm: corr +0.9132)
h1+64 (2nd header) 2207 3.4e33 +0.0123 <- the bug's source
h2+64 (3rd header) 2176 6.1e34 +0.0136
h3+64 (LAST hdr) 0 +0.1011 +0.9698 <- the real post-attention norm
and the identification the PR relies on is exact, not statistical:
pack h1+64 [0:128] vs GGUF blk.0.attn_k_norm : corr +1.0000 max|d| 0.000e+00 (bit-exact)
pack h2+64 [0:128] vs GGUF blk.0.attn_q_norm : corr +1.0000 max|d| 0.000e+00 (bit-exact)
So on a Qwen3 pack the second header opens k_norm — the bytes behind it are the per-head k gains, then padding —
and the last header is the post-attention norm (zero-free, corr +0.97 with the GGUF's ffn_norm). The pre-fix code
imported that k_norm-plus-padding row as post_attention_layernorm in every one of the 28/36 layers. h1 = 8,778,816
and h2 = 8,779,136 also match the knorm_off/qnorm_off I verified byte-for-byte in my #986 review, so the two
readers now agree on one boundary.
Shape of the fix is right: one accessor, FconstGeom::post_attn_hdr(), used by both consumers (import_pack's
norm read and plan()'s MLP-section start), so the two cannot drift apart again — which is what made this bug possible
(plan() already used the last header, import_pack did not). hdr.empty() ? h1 : hdr.back() keeps the 2-header
(Qwen2.5/Llama) packs on exactly the old behaviour, which is why the 0.5B control never showed it.
The new test_post_attn_norm_header is a known-answer test rather than a shape test (a distinct recognisable value
behind each of the four headers, then assert the imported weight is the last one and not the second), which is the
form that would have caught this. Build result for it is below as a follow-up when it lands; the byte evidence above is
independent of the test.
PR Reviewer Guide 🔍(Review updated until commit d58c6a3)Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Build and test result, as promised — the test runs and the pack leg is green on all three packs, including the two
Qwen3 ones this bug affected.
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=gcc-16 -DCMAKE_CXX_COMPILER=g++-16
cmake --build build --target npu_pack_import_test -> links
./build/npu_pack_import_test tests/data ~/models/oga/Qwen3-4B_rai_1.8.0_npu_4K
./build/npu_pack_import_test tests/data ~/models/oga/Qwen3-1.7B_rai_1.8.0_npu_4K
./build/npu_pack_import_test tests/data ~/models/oga/Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K
-> "all npu-pack checks passed (including the pack leg)" x3 (rc 0)
Two things worth recording about how that line reads, because the test itself is careful about it:
- with no pack directory (which is how ctest runs it) the last line is
all npu-pack checks passed -- pack leg SKIPPED, no pack directory given, so a skip is never reported as a pass — theengine#842lesson the comment in
main()cites, - the argument order matters:
argv[1]is the golden dir andargv[2]the pack, sonpu_pack_import_test <pack>puts
the pack into the golden slot and quietly self-skips the pack leg. I hit that once before readingmain(); a usage
line on theskipmessage would save the next person the same ten minutes.
test_post_attn_norm_header runs in all three invocations (it is fed golden_dir, not the pack), so the known-answer
check — a distinct recognisable value behind each of a 4-header block's headers, assert the import takes the last and not
the second — passed against the crafted pack while the real packs imported whole.
That completes what the review left open: the byte evidence for which header is the right one is in the review above
(h1+64 bit-exact attn_k_norm, h3+64 corr +0.97 with ffn_norm), and this is the code path agreeing with it on the
real packs.
Gate numbers and the non-regression proof (added after the PR opened; head is unchanged at
|
| prompt | ours (greedy) | first argmax divergence | KL before it |
|---|---|---|---|
| capital of France | <think>\nOkay, the user is asking for the capital of France. I need |
step 14 | 8e-5 … 6e-3 |
| why the sky is blue | <think>\nOkay, so I need to explain why the sky appears blue. Hmm |
step 4 | 1e-4 … 3e-4 |
| first 10 primes | <think>\nOkay, so I need to figure out the first 10 prime |
none, 16/16 | 1e-5 … 6e-1 |
46 of 48 tokens identical; both flips are rank-1↔2 near-ties with identical top-5 sets (prompt 1: ref 38.461/37.365
vs ours 36.621/37.969; prompt 2: ref 36.243/34.940 vs ours 36.321/36.600), i.e. 4-bit noise on a coin-flip. Control
on the same instrument: our long-validated Qwen2.5-0.5B container flips earlier (steps 0, 5, 1) with the same
rank-1↔2 signature.
Before the fix, on the same instrument, step 0 alone was KL 19.23 nats, corr −0.08, top-5 0/5, first divergence at
step 0.
Non-regression for the 2-header packs, as bytes rather than as an argument: the Qwen2.5-0.5B pack imported with
origin/main's importer and with this branch, in the same tree, produces
4e0fe39e1186b29d10d5be6307435a7c1aedc2d5cfd4610945ba4385051d1fe6 both times — cmp finds no difference.
ctest -R npu_pack 1/1 in the ONEBIT_NPU_HOST-only build (the real-pack leg is not registered there), and the
real-pack leg run by hand on all four shipped packs — Qwen3-1.7B, Qwen2.5-0.5B, Llama-3.2-1B, Qwen3-4B — reports
all npu-pack checks passed (including the pack leg) for each.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 8f513ca3c — merge-only move from d149ef5bb, and the non-regression claim checks out against the
packs' own geometry.
Head: own-diff hash identical at both (e6282502b8ef), own four files unchanged, commits since are main merges
(#1007/#1002/#978). So this verdict carries the d149ef5 review and its gate comment.
The "0.5B is byte-identical" claim, verified by construction rather than accepted — the 64-byte eps headers in each
pack, found by scanning for 86 35 + 62 zero bytes:
Qwen2.5-0.5B : block-0 headers at 0, 1,115,968 (next block at 8,786,048) -> 2 headers per block
Qwen3-1.7B : block-0 headers at 0, 4,739,136, 4,739,456, 7,107,264 -> 4 headers per block
On a 2-header pack hdr holds one entry, so post_attn_hdr() returns hdr.back() == h1 — the old value, exactly.
The import cannot differ by even a byte, which is what you measured; it is also why the 0.5B control passed before the
fix and why it is the right control. On the 4-header pack the same expression returns h3, and my byte comparison
against Qwen3-4B-Q4_K_M.gguf says that is the correct one: h1+64 is bit-exact attn_k_norm,
h2+64 bit-exact attn_q_norm, h3+64 corr +0.97 with ffn_norm (and h1+64 carries the 2207/2560 zeros).
On the gate numbers themselves: 46/48 with both flips rank-1↔2 and identical top-5 sets is the signature of quantisation
noise rather than a wrong tensor, and the before row — step 0 at KL 19.23 nats, corr −0.08, top-5 0/5 — is exactly
what my byte evidence predicts for a norm holding k_norm gains plus 1e33 garbage. The 0.5B flipping earlier (steps
0/5/1) on the same instrument is the part that makes the pair meaningful: an instrument that "passes" the model under test
and fails the control would be worthless, and this one does not.
Not re-run by me: the fp32-checkpoint comparison itself — that needs NPU_INFER_HOST_GEMM=1 on the container plus the
Qwen3-1.7B fp32 reference logits, i.e. the lane's instrument. What I verified is the two ends it depends on: which header
holds the real norm (bytes, above), and that the fix cannot have moved the control pack (geometry, above).
|
Persistent review updated to latest commit d58c6a3 |
npu-pack: read the post-attention norm from the block's last epsilon header, not its second (closes #1003)
Root cause
import_packreadpost_attention_layernorm.weightatbase + g.h1 + 64, i.e. the block's secondepsilon header. That is the post-attention header on a 2-header block (Qwen2.5, Llama — which is why the
0.5B control passed) but on a Qwen3 block the second header opens the k_norm section, so every Qwen3
import carried 128
k_normgains plus the padding behind them aspost_attention_layernorm: 1,791 of2,048 values zero, max 68.0, in all 28 layers.
plan()already used the last header for the MLP section'sstart, so plan and import disagreed for exactly the packs with four headers.
Measured on
Qwen3-1.7B_rai_1.8.0_npu_4Klayer 0 (block-relative offsets,bf16[2048]behind each header):post_attention_layernorminput_layernorm, correct)k_normq_normThe container imported before this fix carried exactly the h1 row (mean +0.2926, max +68.0000, 1,791 zeros),
which is the identification: same bytes, not just a suspicious value.
Fix
FconstGeom::post_attn_hdr()—hdr.empty() ? h1 : hdr.back()— innpu/pack_import.h, used by thepost_attention_layernormread, its epsilon anchor, andplan()'s MLP-section start, so the two readerscannot diverge again. On a 2-header pack it returns
h1, so Qwen2.5/Llama imports are unchanged.Measured before/after
tests/npu_pack_import_test.cpp::test_post_attn_norm_header— a crafted4-header-per-block
.fconstwith a distinct recognisable value behind every header (−3.0 behind thesecond, a 1..H ramp behind the fourth), imported and read back out of the container:
4 check(s) failed— bothmodel.layers.0/7.post_attention_layernorm.weight is the bytes after the block's LAST headerand... is not the k_norm section's marker (the engine#1003 bug);h1, so the packs that worked cannot regress.ctest -R npu_pack: 2/2 (with the real pack leg: one refusal each,model.norm.weight, i.e. npu-pack: import the final norm from the .fconst tail envelope (the pack now converts whole) #844's gapand nothing new).
post_attention_layernormpack/checkpoint ratio median 0.0000(max 31,200) before → 0.4470 (p5 0.3692, p95 0.4640) after, which is the reciprocal of the MLP
input fold (
gate_projt[k]median 0.4440), i.e. the MLP section's input channels are consistentagain.
Scope
Loader only: no new tensor, no container-format change, no kernel change. All four shipped
_rai_1.8.0_npu_4Kpacks import as before; the Qwen3 ones now carry a post-attention norm instead ofk_normpadding. FACTS §7.25 records the header rule and the evidence(
amd-oss/npu-ref0653b7,evidence/oga/post-attn-header-2026-11-04/).Closes #1003.