Repository navigation
npu-pack: import the final norm from the .fconst tail envelope (the pack now converts whole) - #844
bong-water-water-bong wants to merge 1 commit into
Conversation
PR Reviewer Guide 🔍(Review updated until commit c8c3df2)Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at f2ab40f2 — the pack converts whole, and the claim that the tail copy is the upstream checkpoint's
model.norm.weight is not just reproducible, it is exact. Two things to know: it inherits #842's CodeQL line (fix that
once and both clear), and it merges only after #842.
My own run (fresh Release/ninja/gcc-16, -DONEBIT_OGA_PACK=<the 0.5B pack>, ninja npu_pack_import_test onebit
rc 0, ctest -R npu_pack_import 2/2), importing the shipped pack with no --allow-incomplete:
imported 291 tensors, 67088 q4_1 tiles into /tmp/tprs842/out844
MLP: 2160/2160 .fconst groups survive decode->encode byte for byte; 72 projections imported
attention: 768/768 .fconst tiles survive decode->encode byte for byte; 96 projections + 72 biases
fconst: stride 8708288, second header 1115968 (measured from the eps anchors); 48/48 RMSNorm anchors found,
final norm identical in both .fconst tails
wall 2.02 s
291 tensors, 67,088 tiles, 0 refusals, the cross-check firing — the PR's numbers, from my binary and my run (the body's
4.4 s is wall for the same work; 2.02 s here).
Independent of the importer's code, from the two .fconst files:
- both tails open at
layers x strideand nowhere else: TOK208,998,912(with 82,655,040 B of lm_head stream after the
envelope), PRE210,865,152, and the prefill file ends exactly 1,856 B later, so its last bytes are this envelope; - the envelope is
[eps][62 pad][896 bf16]on both:86 35(bf16 1e-6) at +0, 62 zero bytes, then 896 bf16 words; - the two copies are byte-identical (
np.array_equalover the 896 words), which is the cross-check the import now
records asfinal norm identical in both .fconst tails; - and it is the upstream checkpoint bit for bit: I parsed
model.norm.weightout of
~/models/Qwen2.5-0.5B-Instruct-hf/model.safetensors(BF16[896], safetensors header read by hand) and compared the
raw bf16 words —bit-identical: True, max |diff| 0.000e+00. (The values are not near 1.0 — min -0.0114, max 16.75 —
which is why I checked the checkpoint rather than trusting plausibility; the checkpoint has those values too.)
The plan/import shape is right: the norm is gated on eps_at(fc, layers x stride), a missing marker refuses it by name
with the offset, and a disagreeing prefill copy refuses it too, so a pack that is not this shape cannot pass silently.
Nits, neither a gate. (1) It carries tests/npu_pack_import_test.cpp:657 from #842, which is what CodeQL alert #70
(cpp/integer-multiplication-cast-to-long) points at; the int64_t(layers) cast in #842 clears both, and this branch
should take that commit before merge anyway. (2) On a pack whose layer blocks cannot be located at all
(Llama-3.2-1B: 160 refusals, both info and import agree), model.norm.weight is now neither imported nor refused —
plan dropped its refusal unconditionally and the tail code lives inside the g.stride != 0 branch — so a
--allow-incomplete container from such a pack would silently lack the final norm even though the tail sits outside the
blocks and could still be imported. Moving the eps_at check into plan would make info and import agree for every
pack (the engine#734 rule this lane keeps citing).
No NPU ticket, no GPU claim: nothing here needs the device.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Correction to my READY above, after a deeper check of the decoder this branch inherits (issue #849): the verdict is
conditional on #842's zero-point fix, not on anything in this commit.
What still stands from my review, unchanged: the tail envelope is at layers x stride in both .fconst files, both copies
are byte-identical, the value is bit-for-bit the upstream checkpoint's model.norm.weight, and 291 tensors / 67,088 tiles /
0 refusals reproduces on my build with the cross-check reporting final norm identical in both .fconst tails.
What does not stand: "conversions of the pack are correct" — the MLP tiles this branch writes carry #842's zero-point
permutation (decode_tile pairs the zero nibbles by halves while the codes pair by even/odd; the shipped bytes say
even/odd is right — evidence and the one-file test are in my later review on #842 and in issue #849). model.norm.weight
itself is a plain BF16 tensor and is unaffected, so this PR has no defect of its own; it just cannot be read as "the pack
now converts whole" until the tiles underneath it are right.
No further action here: rebase after #842's fix and the byte-level numbers stay as they are (the round trip is invariant
under that permutation), so no re-verdict is needed unless the tail logic itself changes.
|
Note for the merge order, since this PR is in tonight's armed set: its base is Two clean options:
I re-verdict at whichever new head results, immediately. |
|
Correction to my earlier note on this PR, and it matters for the decision: I was wrong that "it does not touch Measured, in a clone at So The two clean exits, both re-measured just now:
My verdict on the content does not change: the tail envelope at |
c84110a to
9be1d17
Compare
f2ab40f to
577b5a3
Compare
|
Persistent review updated to latest commit 577b5a3 |
|
Rebased onto #842's current head ( The rebase needed one real adaptation, not just conflict resolution: Verified on the rebased stack at |
0baf267 to
8752915
Compare
|
Persistent review updated to latest commit 0baf267 |
|
Post-rebase verification re-run on the exact head that is now pushed ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 87529152a — stacked on #842, and it inherits the same regression plus one more failing check. Reproduced on the same box with the same packs and invocation (npu_pack_import_test "" <pack>):
| pack | main ae0ee19 |
#842 bb3fffbba |
this head 87529152a |
|---|---|---|---|
Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K |
passed | passed | passed |
Llama-3.2-1B-Instruct_rai_1.8.0_npu_4K |
passed | 3 failed | 4 check(s) failed |
Qwen3-1.7B_rai_1.8.0_npu_4K |
passed | 3 failed | 4 check(s) failed |
Qwen3-4B_rai_1.8.0_npu_4K |
passed | 3 failed | 4 check(s) failed |
The fourth is this PR's own subject:
FAIL pack: the final norm is byte-identical in the Token_Token and Llm_Prefill tail envelopes
FAIL pack: the 4 attention projections per layer are planned from the measurement
FAIL pack: every refusal names where the bytes are and the fact that is missing
So the change does what its title says — eps_at/locate_fconst now take the pack's own rms_norm_eps bits instead of bf16 1e-6 — and on these three packs the final-norm envelope check still fails, which is the check the PR exists to satisfy. That is either the envelope walk on a pack whose layout differs from the ones validated, or an expectation that only holds for the 0.5B family; either way it is failing on real artifacts, and ctest cannot see it because the pack leg needs a pack argument (skip pack: no pack directory given).
Order is fixed by the stack: #842 first, then this rebased. My review on #842 lists the shared items (113 = 416 + 316 + 1 refusals on the 1B, the attention note/tile contradiction, the refusal wording) — fixing them there and re-running these four packs is what turns both into READY.
|
Base updated and this branch merged it: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 4c1b051de — merge-only (Merge remote-tracking branch 'origin/1bit/671-oga-mlp-payload'), and the same three packs still fail, with #844's own check as the fourth:
$ ./npu_pack_import_test # what ctest runs: "all npu-pack checks passed", pack leg skipped
$ ./npu_pack_import_test "" /home/bcloud/models/oga/Llama-3.2-1B-Instruct_rai_1.8.0_npu_4K
FAIL pack: the MLP projections are refused only when their geometry does not add up, got 112 refusals
FAIL pack: every refusal names where the bytes are and the fact that is missing
FAIL pack: the 4 attention projections per layer are planned from the measurement
FAIL pack: the final norm is byte-identical in the Token_Token and Llm_Prefill tail envelopes
4 check(s) failed
| pack | main | #842 3d1b15da2 |
this head 4c1b051de |
|---|---|---|---|
| Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K | passed | passed | passed |
| Llama-3.2-1B-Instruct_rai_1.8.0_npu_4K | passed | 3 failed | 4 failed |
| Qwen3-1.7B_rai_1.8.0_npu_4K | passed | 3 failed | 4 failed |
| Qwen3-4B_rai_1.8.0_npu_4K | passed | 3 failed | 4 failed |
So the rescoped epsilon work is doing something real on these packs — it moved 113 refusals to 112 and added the final-norm envelope assertion — and that assertion is the one failing. The 112 is still 7 per layer + the one that #844 removed, i.e. the attention projections are still refused alongside the MLP ones.
Same structural reason for the green you saw, and it is worth fixing once for both PRs: CMakeLists.txt:596 passes only the golden dir, so the pack leg always self-skips and prints all npu-pack checks passed; the real-pack test exists but is behind ONEBIT_OGA_PACK (:597-602). Either run the one command above before the next claim, or make the test take a pack list from an env var and assert it saw at least one — a pack leg that can silently do nothing is the same shape as #657.
|
CHANGES addressed at Your three inherited failures came from #842's refusal accounting and are gone. The fourth is mine, and it was a wrong assertion rather than a wrong import: the cross-check compares the Token_Token tail envelope against the Llm_Prefill one, and the three packs it failed on ship no prefill program at all — So there was nothing to cross-check against and the assertion was testing the pack's shape, not this importer. It is now conditional: where a prefill All four real packs, on this head:
|
4c1b051 to
5b4567c
Compare
|
Persistent review updated to latest commit 5b4567c |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES — one item, unchanged by this push: the shipped comment at npu/pack_import.cpp:1315-1319
still records my own harness corruption as a vendor fact. Everything else at 36a0da1b3 verifies.
Re-ran at this head (~/wt/tprs-844-build, cmake --build . --target npu_pack_import_test, rc 0) on all four
real OGA packs, ./npu_pack_import_test "" /home/bcloud/models/oga/<pack>:
Llama-3.2-1B-Instruct_rai_1.8.0_npu_4K rc=0 note: no Llm_Prefill .fconst in this pack, so the norm cross-check is skipped
Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K rc=0 note: the pack's two norm copies agree
Qwen3-1.7B_rai_1.8.0_npu_4K rc=0 note: no Llm_Prefill .fconst in this pack, so the norm cross-check is skipped
Qwen3-4B_rai_1.8.0_npu_4K rc=0 note: no Llm_Prefill .fconst in this pack, so the norm cross-check is skipped
all four: "all npu-pack checks passed (including the pack leg)"
- the three separate states report correctly, and the 0.5B — the only pack that ships a prefill copy — says the
two copies agree, with noDISAGREEline and no warning on that pack; - the new crafted leg runs and passes:
final norm: crafted tail envelope imported from tail+64, missing-epsilon control refused; - an unreadable prefill is now a note, not an abort (the
open_fconstpath is guarded).
The one item. npu/pack_import.cpp:1315-1319 on this head still says:
(Measured on this box: the 0.5B pack's prefill .fconst, rewritten 2026-11-03 15:10, differs from its Token copy
in exactly one bf16 of model.norm.weight -- index 895, 9.375 vs -0.1465 -- and every other one of the 896
matches; the container is unaffected because the norm comes from Token.)
That byte was written by my mutation test at 15:10:46 and restored at 15:16:45 (I disclosed this earlier on
this PR); the 15:10 timestamp in the comment is my write, not a vendor rewrite. Please delete the parenthetical
or replace it with "a locally corrupted copy (mutation test), kept as a fixture at …" — the design the sentence
justifies (report the mismatch, keep importing from Token_Token) is right and stands on its own. The
WARNING: the Token_Token and Llm_Prefill tails disagree … message the user sees is fine.
No other blocker. Merge order is unchanged: #842 first, this after it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES — one item, unchanged by this push: the shipped comment at npu/pack_import.cpp:1315-1319
still records my own harness corruption as a vendor fact. Everything else at 36a0da1b3 verifies.
Re-ran at this head (~/wt/tprs-844-build, cmake --build . --target npu_pack_import_test, rc 0) on all four
real OGA packs, ./npu_pack_import_test "" /home/bcloud/models/oga/<pack>:
Llama-3.2-1B-Instruct_rai_1.8.0_npu_4K rc=0 note: no Llm_Prefill .fconst in this pack, so the norm cross-check is skipped
Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K rc=0 note: the pack's two norm copies agree
Qwen3-1.7B_rai_1.8.0_npu_4K rc=0 note: no Llm_Prefill .fconst in this pack, so the norm cross-check is skipped
Qwen3-4B_rai_1.8.0_npu_4K rc=0 note: no Llm_Prefill .fconst in this pack, so the norm cross-check is skipped
all four: "all npu-pack checks passed (including the pack leg)"
- the three separate states report correctly, and the 0.5B — the only pack that ships a prefill copy — says the
two copies agree, with noDISAGREEline and no warning on that pack; - the new crafted leg runs and passes:
final norm: crafted tail envelope imported from tail+64, missing-epsilon control refused; - an unreadable prefill is now a note, not an abort (the
open_fconstpath is guarded).
The one item. npu/pack_import.cpp:1315-1319 on this head still says:
(Measured on this box: the 0.5B pack's prefill .fconst, rewritten 2026-11-03 15:10, differs from its Token copy
in exactly one bf16 of model.norm.weight -- index 895, 9.375 vs -0.1465 -- and every other one of the 896
matches; the container is unaffected because the norm comes from Token.)
That byte was written by my mutation test at 15:10:46 and restored at 15:16:45 (I disclosed this earlier on
this PR); the 15:10 timestamp in the comment is my write, not a vendor rewrite. Please delete the parenthetical
or replace it with "a locally corrupted copy (mutation test), kept as a fixture at …" — the design the sentence
justifies (report the mismatch, keep importing from Token_Token) is right and stands on its own. The
WARNING: the Token_Token and Llm_Prefill tails disagree … message the user sees is fine.
No other blocker. Merge order is unchanged: #842 first, this after it.
|
Persistent review updated to latest commit 56098eb |
|
Inherits |
|
Rebased onto #842's
The merge had one real conflict: both branches added a function at the same insertion point (this branch the crafted
|
|
Preparing review... |
|
Persistent review updated to latest commit f33ca8f |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES — unchanged item, and everything else at f33ca8f6 is green (five-pack run below).
The rebase kept the good parts. Re-ran all five packs at this head:
Llama-3.2-1B-Instruct_rai_1.8.0_npu_4K rc=0 all npu-pack checks passed (including the pack leg)
Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K rc=0 all npu-pack checks passed (including the pack leg)
note: the pack's two norm copies agree
final norm: crafted tail envelope imported from tail+64, missing-epsilon control refused
Qwen2.5-0.5B-Instruct_rai_1.8.0_hybrid rc=0 all npu-pack checks passed -- pack leg SKIPPED, not an OGA NPU pack
Qwen3-1.7B_rai_1.8.0_npu_4K rc=0 all npu-pack checks passed (including the pack leg)
Qwen3-4B_rai_1.8.0_npu_4K rc=0 all npu-pack checks passed (including the pack leg)
So the final-norm work, the three cross-check states, the crafted tail-envelope leg and #842's named skip all hold
after the rebase.
The one item is still there. npu/pack_import.cpp:1329-1333 at this head still reads:
(Measured on this box: the 0.5B pack's prefill .fconst, rewritten 2026-11-03 15:10, differs from its Token copy in
exactly one bf16 of model.norm.weight -- index 895, 9.375 vs -0.1465 -- and every other one of the 896 matches; the
container is unaffected because the norm comes from Token.)
That byte was written by my mutation test at 15:10:46 and restored at 15:16:45 — the 15:10 timestamp in the
sentence is my write, not a vendor rewrite, and the five-pack run above is what a real pack looks like (the pack's two norm copies agree). Please delete the parenthetical, or replace it with "a copy I corrupted by hand as a fixture
(kept at …)". The design the sentence exists to justify — report the mismatch, keep importing from Token_Token — is
correct and needs no vendor anecdote.
For the merge order: #842 first (it is READY at 49baaee85), this after it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at f33ca8f67 — unchanged from my earlier review, restated here because this PR is on tonight's "merge all ready" list and it is not ready for one reason.
Everything else at this head is green: five packs, four rc=0 (including the pack leg), the 0.5B "the pack's two norm copies agree", the crafted tail-envelope leg passing, and the hybrid pack's named skip.
The one item is npu/pack_import.cpp:1329-1333, which still reads:
(Measured on this box: the 0.5B pack's prefill .fconst, rewritten 2026-11-03 15:10, differs from its Token copy in exactly one bf16 of
model.norm.weight— index 895, 9.375 vs -0.1465 — and every other one of the 896 matches …)
That 15:10 rewrite was my review harness corrupting the fixture to test this importer's refusal path; it is not a property of the vendor's file. As written, the comment records a measurement by a reviewer as a vendor behaviour, and a future reader will go looking for a vendor-side inconsistency that does not exist.
Two ways to close it, either is fine:
- drop the parenthetical entirely — the argument above it ("a mismatch is REPORTED, not fatal") stands on its own; or
- relabel it as what it is: "a hand-made fixture with one bf16 flipped (index 895) exercises the REPORT path; the vendor's own copies agree."
Nothing else needs to change, and the merge order is #842 first.
… OGA pack converts whole The importer refused `model.norm.weight` on every OGA NPU pack, on the reading that the 1,856-byte envelope after the layer blocks was `[eps][pad][448 f32]` and therefore not an RMSNorm weight. That reading came from the last 1,856 bytes of the *file*. The envelope actually sits at `layers x stride`, and in the `Llm_Token_Token` program the `lm_head` tile stream follows it — which is why the head's payload was found verbatim at `tail + 1,856`. The position is not assumed: the epsilon has to be at `layers x stride`, and where a pack ships both programs the two copies of the norm have to be byte-identical before either is written. On the 0.5B pack the stored bytes equal the upstream checkpoint's `model.norm.weight` exactly. Before: `1bit npu-pack import Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K` exits 3, "1 of the pack's tensors cannot be represented yet, so nothing was written", `model.norm.weight: the .fconst tail envelope is [eps][pad][448 f32], not an RMSNorm weight`. After: exits 0, "imported 291 tensors, 67088 q4_1 tiles", no refusals, and on the CLI's own `fconst:` line "48/48 RMSNorm anchors found, final norm identical in both .fconst tails". No behaviour change for the other three packs on this box (Llama-3.2-1B, Qwen3-1.7B, Qwen3-4B): they still refuse, now only for the attention payload not tiling, and none of them refuses a norm. Tests: `npu_pack_import` and `npu_pack_import_oga` pass (the pack leg included). A crafted-pack leg covers the envelope where no real pack is supplied -- `ctest` without `-DONEBIT_OGA_PACK` has none of the four packs' prefill programs -- plus the control: with the tail epsilon corrupted the norm is refused, nothing is written without `--allow-incomplete`, and with it the omission is recorded. closes #844
f33ca8f to
99f5eb5
Compare
|
Persistent review updated to latest commit 99f5eb5 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
CHANGES on one item — and I now have the measurement that settles it. Everything else in this PR is verified.
What I ran (importer built standalone from your head: npu/pack_import.cpp + file_map.cpp + pack.cpp + model.cpp + q4nx.cpp; no device):
origin/main (eda7542) 0.5B tensors=0 refused=1 "model.norm.weight: the .fconst tail envelope is
[eps][pad][448 f32] ... not established"
pr/844 (99f5eb5) 0.5B tensors=291 tiles=67,088 refused=0 final_norm_imported=1 crosscheck=1 prefill_seen=1
pr/844 + pr/986 1B tensors=147 tiles=150,848 refused=0
So "the pack now converts whole: 291 tensors, 67,088 tiles" reproduces, and so does the Llama-3.2-1B row with #986 on top.
The item. npu/pack_import.cpp (~:1300) still says:
(Measured on this box: the 0.5B pack's prefill .fconst, rewritten 2026-11-03 15:10, differs from its Token copy in exactly one bf16 of model.norm.weight -- index 895, 9.375 vs -0.1465 -- and every other one of the 896 matches; the container is unaffected because the norm comes from Token.)
That is not reproducible on the pack as it stands, and it attributes to the vendor a difference this box created. Measured now, both envelopes located independently of your code (eps marker 0x3586 + 62 zero bytes, then 1,792 B of norm):
Token_Token envelope at 24*8,708,288 = 208,998,912 file 291,653,952
Llm_Prefill envelope at 24*8,786,048 = 210,865,152 file 210,867,008 (= envelope + 1,856 exactly)
differing bf16 of model.norm.weight: 0 of 896
and your own cross-check agrees: crosscheck=1 on that pack. The file the sentence describes is the one my 2026-11-03 15:10 harness rewrote (prefill .fconst mtime Nov 3 15:16 vs the Token copy's Oct 31 22:47), so it is a local-write artifact, not vendor data.
Keep the behaviour exactly as it is — a mismatched duplicate is reported, not fatal, and that reasoning is right. Delete the parenthetical (or say "a local rewrite during testing once made the copies differ; they agree in the shipped pack"). Then this is READY: the norm import, the layers x stride envelope position, the eps_at guard, the readable-prefill fallback and the report-not-abort cross-check all hold up, and the 0.5B and 1B packs both convert whole.
|
State check, because this PR is now the critical path for the second family and it currently has a CHANGES verdict One item, unchanged: Why it matters now: #986 ( Merge order: this first, then #986 (the two conflict in |
…, and the Qwen3 four-way split (engine#979) (#986) * npu-pack: the attention payload starts at 64 + 2*hidden, not at the constant 1,856 The importer located a layer block's attention payload at the hardcoded offset 1,856. That number is right for exactly one shape: `64 + 2*896`, i.e. the Qwen2.5-0.5B pack's hidden size. On any other hidden it measures the payload from `2*(hidden - 896)` bytes too early, so the payload comes out as "a whole number of tiles plus a stray run" and the whole attention section is refused. Llama-3.2-1B (hidden 2,048) is the clean case: its second epsilon header is at 5,922,880 B, so the real payload is 5,918,720 B = **exactly 80 tiles** of 73,984 B — the 80 that q(2,048) + k(512) + v(512) + o(2,048) rows need at 64 rows per tile. From 1,856 the same bytes read as "80 tiles + a 2,304-byte strip", and 2,304 is precisely `2*(2048-896)`. The fix is to derive the offset where the layout puts it: the block's 64-byte header, then `input_layernorm` as bf16 `[hidden]`. Nothing about the tile format changes, the vendor's own `matmulnbits_pack_const_float32` predicts the same 80 x 73,984 B for this geometry, and the 0.5B pack is byte-identical before and after (1,856 = 64 + 2*896, so its path is unchanged). Measured on the four real packs in `~/models/oga`, `1bit npu-pack import`: | pack | before | after | |---|---|---| | Qwen2.5-0.5B | 291 tensors, whole | **291 tensors, whole** (unchanged) | | Llama-3.2-1B | refused: "attention payload is 5,921,024 B, not a whole number of 73,984-byte tiles" (145 of 147 tensors) | **attention payload imports: 80/80 tiles, 0 attention refusals** | | Qwen3-1.7B | "not a whole number of 73,984-byte tiles" | "attention payload holds 64 tiles; q/k/v/o at 64 rows each need 96" — the real gap, named | | Qwen3-4B | "not a whole number of 91,392-byte tiles" | "the attention payload's q_proj is 4,096 rows against hidden 2,560, so o_proj's K would not be the tile's K" — the real gap, named | With #844 (the final norm) applied on top, Llama-3.2-1B converts whole: **147 tensors, 150,848 q4_1 tiles, exit 0** — the second pack family end to end. The two Qwen3 packs still refuse, but for reasons that are now measured rather than masked by a byte count that happened to be off by `2*(hidden-896)`; engine#979 tracks the remaining work. Tests: `test_attn_payload_off()` is a known-answer test on the packs' own measured offsets — 896 -> 1,856 B, 2,048 -> 4,160 B, 2,560 -> 5,184 B, and Llama's 5,922,880 B header is exactly 80 tiles from the derived offset while the old constant gives 80 tiles + 2,304 B. `ctest -R npu_pack_import` passes 2/2, and the pack leg now passes on all five pack directories (it needed two corrections that this fix exposed: Llama has no attention biases — so the leg asks the report rather than the geometry — and the o_proj/K refusal had to name its location like every other refusal does). closes #979 * npu-pack: the attention section's four-way split (q/k/v, two per-head QK norms, o_proj at K=q_out) * docs(npu): the attention section's four-way split, and the MLP scope claim it corrects (engine#979) * docs(npu): k precedes q in the attention tile run, as the pack's own biases showed --------- Co-authored-by: bong-water-bong <bong-water-bong@1bit.gg> Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 99f5eb50
Two things to do, both small. The substance of the PR is fine and I verified it on the merged tree.
1. Rebase: CONFLICTING against 434db3d22 (#1018 landed in the same files). The rebase is mechanical and I did it here to be sure — the only conflict is tests/npu_pack_import_test.cpp main()'s call list, and the resolution is "keep both":
test_final_norm_envelope(...) // yours
test_attn_payload_off(); test_qwen3_attn_sections(); test_post_attn_norm_header(...) // main's #1018
npu/pack_import.{cpp,h} and docs/npu.md auto-merge, and main's post_attn_hdr() survives it (npu/pack_import.cpp:1018, 1357, 1359 still read the post-attention norm from the block's last epsilon header, so the Qwen3 fix is not reverted). On that resolution, -DONEBIT_NPU=ON build of onebit: rc=0; npu_pack_import_test golden legs pass; and the four _rai_1.8.0_npu_4K packs:
Qwen2.5-0.5B imported 291 tensors, 67088 q4_1 tiles (no refusal line)
fconst: stride 8708288 …; 48/48 RMSNorm anchors found, final norm identical in both .fconst tails
Llama-3.2-1B 32/32 anchors, final norm taken from the tail envelope (the pack ships no prefill copy)
Qwen3-1.7B 56/56 anchors, final norm identical in both .fconst tails
Qwen3-4B 72/72 anchors, final norm identical in both .fconst tails
so "4/4 packs convert whole" holds, and it holds with #1018 in the tree.
2. Still open from my last review: npu/pack_import.cpp:1403-1407 records a vendor inconsistency that is not in the packs.
// Measured on this box: the 0.5B pack's prefill .fconst, rewritten 2026-11-03 15:10, differs from its Token copy
// in exactly one bf16 of model.norm.weight -- index 895, 9.375 vs -0.1465 -- and every other one of the 896
// matches; the container is unaffected because the norm comes from Token.
Run today, the same pack says the opposite:
Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K: final norm identical in both .fconst tails
and so do the 1.7B and 4B. That file was rewritten on this box by my own review harness in the #842 pass, which is why the copies diverged then; the vendor's two copies agree. The WARNING: the Token_Token and Llm_Prefill tails disagree path is still worth having — it is the thing that would catch a genuinely divergent vendor copy — but the paragraph must not cite a measurement taken on a file we had just rewritten. Drop the parenthetical, or replace it with the mechanism ("reported, not fatal, because the device runs the Token_Token program and a vendor-side mismatch in the duplicate cannot be repaired here") without the index-895 numbers.
Everything else in the diff I checked and it is right: the epsilon-marker gate at layers x stride before the norm is written, the two reported flags (final_norm_imported, prefill_tail_seen, final_norm_crosscheck) not being inferable from each other, the "the prefill copy is optional evidence, so a file that cannot be mapped must not abort an import" branch, and the crafted-tail test plus its missing-epsilon control (final norm: crafted tail envelope imported from tail+64, missing-epsilon control refused).
… OGA pack converts whole The importer refused `model.norm.weight` on every OGA NPU pack, on the reading that the 1,856-byte envelope after the layer blocks was `[eps][pad][448 f32]` and therefore not an RMSNorm weight. That reading came from the last 1,856 bytes of the *file*. The envelope actually sits at `layers x stride`, and in the `Llm_Token_Token` program the `lm_head` tile stream follows it — which is why the head's payload was found verbatim at `tail + 1,856`. The position is not assumed: the epsilon has to be at `layers x stride`, and where a pack ships both programs the two copies of the norm have to be byte-identical before either is written. On the 0.5B pack the stored bytes equal the upstream checkpoint's `model.norm.weight` exactly. Before: `1bit npu-pack import Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K` exits 3, "1 of the pack's tensors cannot be represented yet, so nothing was written", `model.norm.weight: the .fconst tail envelope is [eps][pad][448 f32], not an RMSNorm weight`. After: exits 0, "imported 291 tensors, 67088 q4_1 tiles", no refusals, and on the CLI's own `fconst:` line "48/48 RMSNorm anchors found, final norm identical in both .fconst tails". No behaviour change for the other three packs on this box (Llama-3.2-1B, Qwen3-1.7B, Qwen3-4B): they still refuse, now only for the attention payload not tiling, and none of them refuses a norm. Tests: `npu_pack_import` and `npu_pack_import_oga` pass (the pack leg included). A crafted-pack leg covers the envelope where no real pack is supplied -- `ctest` without `-DONEBIT_OGA_PACK` has none of the four packs' prefill programs -- plus the control: with the tail epsilon corrupted the norm is refused, nothing is written without `--allow-incomplete`, and with it the omission is recorded. closes #844
99f5eb5 to
c8c3df2
Compare
Rebased onto
|
| leg | result |
|---|---|
npu_pack_import_test tests/golden (crafted + golden) |
all checks pass; final norm: crafted tail envelope imported from tail+64, missing-epsilon control refused |
| same binary with the real 0.5B pack (the pack leg, which ctest skips) | all npu-pack checks passed (including the pack leg), note: the pack's two norm copies agree |
1bit npu-pack import on Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K, no --allow-incomplete |
whole: 768/768 attention tiles + 2160/2160 MLP groups re-encode, 48/48 norm anchors, final norm identical in both .fconst tails, no refusals |
same on Llama-3.2-1B-Instruct_rai_1.8.0_npu_4K, no --allow-incomplete |
whole: 147 tensors, 150,848 q4_1 tiles, 1280/1280 attention tiles, 2560/2560 MLP groups, 32/32 norm anchors, final norm taken from the tail envelope (the pack ships no prefill copy) — this is the pack that always refused model.norm.weight |
tools/copyright.py --check |
rc 0 |
And the norm the importer wrote is checked against the pack's bytes by a separate reader, not by the importer
(~/ogaload/oga12/final_norm_check.py: .fconst at layers x stride + 64 vs the container's model.norm.weight
payload from the Q4NX header):
| pack | tail offset | pack bytes | container | verdict |
|---|---|---|---|---|
| Qwen2.5-0.5B (24 x 8,708,288) | 208,998,976 | n=896 min −0.0114 max +16.7500 mean 7.1393 | identical stats, 1,792 B | bit-identical |
| Llama-3.2-1B (16 x 33,640,640) | 538,250,304 | n=2048 min +0.0417 max +2.9219 mean 2.3508 | identical stats, 4,096 B | bit-identical |
The 0.5B row also matches FACTS §7.18's recorded min −0.0114 / max 16.75 / mean 7.139 to the digit, from a
different instrument. Nothing merged, tagged or published; head is c8c3df2.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at c8c3df2 — item 1 is done and verified; item 2, the comment, is still open. One paragraph, 60 seconds.
The rebase is right. Own contribution (+/- lines vs the merge base) hashes to 4328f6844287 at this head and at 99f5eb50, so the conflict resolution changed nothing but position — and the call list is what it should be, five calls, including #1030's, which is the one a bad merge would have dropped:
tests/npu_pack_import_test.cpp:1309 test_attn_payload_off();
:1310 test_qwen3_attn_sections();
:1311 test_post_attn_norm_header(...);
:1312 test_final_norm_envelope(...);
:1313 test_text_artifacts(...); <- #1030, merged after this branch was cut
tools/copyright.py --check rc=0. Your pack evidence is consistent with what I measured independently at my own resolution of this conflict earlier tonight (0.5B: 291 tensors, 0 refusals; Llama: 32/32 anchors, "the pack ships no prefill copy"), so the "both packs convert whole" claim is not in dispute.
Still open: npu/pack_import.cpp:1425-1435. The paragraph frames the cross-check as guarding a vendor inconsistency —
// The cross-check compares two copies the VENDOR ships; ... refusing the import over a duplicated copy
// would make an otherwise complete pack unimportable for a vendor-side inconsistency we cannot repair.
// (Measured on this box: the 0.5B pack's prefill .fconst, rewritten 2026-11-03 15:10, differs from its
// Token copy in exactly one bf16 of model.norm.weight -- index 895, 9.375 vs -0.1465 ...)
— and the packs do not have that inconsistency. Run today, all three that ship both .fconst files report the opposite:
Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K: final norm identical in both .fconst tails
Qwen3-1.7B_rai_1.8.0_npu_4K: final norm identical in both .fconst tails
Qwen3-4B_rai_1.8.0_npu_4K: final norm identical in both .fconst tails
The divergence is ours: that prefill file was rewritten at 15:10 on 2026-11-03 by my own review harness during the #842 pass, which is why one bf16 differed. The reason to report rather than refuse survives on its own merits — a duplicated copy we cannot repair must not make an otherwise complete pack unimportable — so keep that sentence and drop the attribution of it to the vendor. Suggested replacement for the parenthesis:
// (Measured on this box: a prefill .fconst that a tool of ours had rewritten differed from its Token
// copy in one bf16 of model.norm.weight; the shipped packs agree byte for byte, so this path is a
// guard against a divergent duplicate rather than a description of the packs.)
I am keeping this as the gate because the only thing left is a fact a future reader will trust, and I have now asked twice — but to be explicit about proportionality: it is a comment, there is no runtime risk in merging as-is, and if the owner's merge window matters more than the wording, take it and I will note the correction on the merged commit.
|
Persistent review updated to latest commit c8c3df2 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
One more data point for the comment fix, from an end-to-end import rather than a reading: I converted the Llama-3.2-1B OGA pack (~/models/oga/Llama-3.2-1B-Instruct_rai_1.8.0_npu_4K) with this PR's npu/pack_import.{h,cpp} applied, and the importer's own line is:
fconst: stride 33640640, second header 5922880 (measured from the eps anchors);
32/32 RMSNorm anchors found, final norm identical in both .fconst tails
That is the string the code prints now — identical in both .fconst tails — while the comment block at npu/pack_import.cpp:1425-1435 still frames the cross-check as guarding a vendor inconsistency and cites index 895, 9.375 vs -0.1465. So on this fourth pack the log and the comment disagree with each other inside the same function, which is the whole of my remaining CHANGES: the mechanism is right, the comment describes a divergence that the code no longer observes.
The rest of that import for the record (it is what makes a Llama-3.2 container convertible at all, which is what my #1052 follow-up needed): 1280/1280 attention .fconst tiles and 2560/2560 MLP groups survive decode→encode byte for byte, 64 + 48 projections imported, 5 of 8 tokenizer/chat files copied, container 1.3 GB, and every visible self-check in its output is a pass.
Replacement text for the comment is in my earlier review; the head is unchanged (c8c3df27), so the verdict still stands as CHANGES-on-the-comment only.
closes #844
Why it was refused
The importer refused
model.norm.weighton every OGA NPU pack, on the recorded reading that the1,856-byte envelope after the layer blocks was
[eps][pad][448 f32]— not an RMSNorm weight — so itsposition was "not established" and it was refused rather than guessed.
That reading came from the last 1,856 bytes of the file. The envelope actually sits at
layers x stride. The 0.5B pack shows why the two look the same and are not: in theLlm_Token_Tokenprogram thelm_headtile stream follows the envelope (which is why the head'spayload was found verbatim at
tail + 1,856), so what is at the end of the file is tile padding, notthe norm.
The position is not assumed, it is required: the epsilon has to be at
layers x stride, and where apack ships both programs the two copies of the norm have to be byte-identical before either is
written. On the 0.5B pack the stored bytes equal the upstream checkpoint's
model.norm.weightexactly.
Measured before / after
1bit npu-pack import Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K -o …Before (
origin/main@eda7542e), exit 3:After, exit 0:
The pack now converts whole: 291 tensors, 67,088 tiles, no refusals.
The other packs
No behaviour change for the other three real packs on this box, and none of them refuses a norm any
more either — they still exit 3, now only for the attention payload not tiling (
Llama-3.2-1B,Qwen3-1.7B,Qwen3-4B), which is the separate open item:model.binflavour)Tests
ctest -R npu_pack_import→ 2/2 passed, includingnpu_pack_import_oga(the real-pack leg,4.97 s). The pack-leg line above is from that leg.
A crafted-pack leg covers the envelope where no real pack is supplied: plain
ctestwithout-DONEBIT_OGA_PACKhas none of the four packs' prefill programs, so the tail-envelope read had nocoverage in the default path. It builds the smallest pack that reaches it and checks the bytes that
land in the container — plus the control: with the tail epsilon corrupted the norm must be refused,
nothing may be written without
--allow-incomplete, and with it the omission must be recorded.That control is the point. Three of the four packs on this box were red behind a green "all npu-pack
checks passed" line before, so the leg that would have caught a bad envelope reads the same
epsilon-anchored position the importer uses and asserts on both outcomes.
tools/copyright.py --check→ rc 0.