Skip to content

npu-pack test: the external-data writer gets a success-path test (engine#1849) - #1902

Closed
bong-water-water-bong wants to merge 5 commits into
mainfrom
npu-pack-external-success
Closed

bong-water-water-bong wants to merge 5 commits into
mainfrom
npu-pack-external-success

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Why

import_external() had no success-path arm. Every import_pack() call on that genre expected a refusal (git grep import_external origin/main -- tests = 0), so three merged changes to the writer (#1712, #1751, #1793) have been exercised only by real packs on the box — a regression in tile order, scale width or zero points would have shipped green.

The fixture had to fit the split

The existing external-packed fixture is 768 wide and the split refuses it:

model.layers.0.attn.qkv_proj.MatMulNBits.qweight.packed: container range [0, 5120) does not fit N 768

so no arm had ever reached the writer. With 40 heads / 40 kv heads / head_dim 128 the fused qkv is 15360 wide and every role's slice is 5120. import_pack() then succeeds and writes model.q4nx:

writer success: model.layers.0.self_attn.q_proj.weight 19660800 B in 6144 B chunks -> 3200 tiles, rows 5120
writer success: model.layers.0.self_attn.k_proj.weight …  v_proj …
all npu-pack checks passed

The crafted blob, and why not all-0x22

The other fixtures write '\x22' repeated, which decodes every weight to (2−2)·s = 0 — indistinguishable from a broken writer (engine#1871, #1875). This one writes codes 0xCC (nibble 12), zero point 0x11 (1), scale bf16 0x4000 (2.0). MatMulNBits codes are unsigned with the zero point as a shift, so

w = (nibble − zp) · scale = (12 − 1) · 2.0 = 22.0

which pack_import.cpp states itself (w = (code_nibble - zp_nibble) * scale). The arm fails if the writer drops the codes (reads 2.0), the zero point (24.0) or the scale (11.0).

The read-back is the device loader's own route

chunk_of() reads shape.back() — which is why the container stores {ntiles, chunk_bytes} and not [rows, cols]. Given {rows, cols} it infers Q4_1 (5120) and as_q4_1() copies the bytes through; given {3200, 6144} it repacks 19,660,800 → 16,384,000 B (3200 tiles) and the rows decode. So no second decoder is written for this test.

All three projections, because the fused pack is where the writer works

container_from_external_q4() runs on the fused qkv, so a slot it gets wrong can land in k or v while q stays clean — measured, and that is what my first version of this arm missed.

Mutation proof (double one scale slot in container_from_external_q4):

FAIL external writer: every one of k_proj 5120 rows decodes to 22.0 ((12 - 1) * 2.0);
     32 differ, first bad row 2560

Reverted, the whole suite is green.

CI

CMakeLists.txt adds npu/forward/src/q4nx_dequant.c and npu/forward/include to the existing npu_pack_import_test target — sources on a target already in ci.yml's --target list, so no new test target and no workflow scope (the trap my #1813 hit tonight).

bong-water-water-bong added 4 commits November 10, 2026 06:14
…ot yet pinned

Not for merge. Records the working half so it is not redone:

- A fixture whose container split FITS: 40 heads / 40 kv heads / head_dim 128 -> fused qkv N 15360,
  K 5120. The earlier `external-packed` fixture is 768 wide and the split refuses it
  ("container range [0, 5120) does not fit N 768") -- that is why nothing reached the writer before.
- `pi::import_pack` now SUCCEEDS on it and writes model.q4nx
  (self_attn.q_proj.weight, 19,660,800 B, header 576 B).
- Crafted blob decodes to a value that pins codes/zero/scale: nibbles 0xCC (int4 -4), zero 0x11
  (four 1s), bf16 scale 0x4000 (2.0) => -10.0 by (c-z)*s. NOT the all-0x22 the other fixtures
  write, which decodes every weight to (2-2)*s = 0 and cannot tell a broken writer from a
  correct one.
- CMakeLists: npu/forward/src/q4nx_dequant.c + npu/forward/include added to the EXISTING
  npu_pack_import_test target (no new target, so ci_targets and ci.yml need no change).

OPEN: the container shape is 3200 x 6144 after padding, and rows 0/32/160 decode to 0.0000 while
row 3199 decodes to 26.0 -- so the writer did consume the blob, but the container row index is
not the tensor row index (tile-blocked order, or the padding rows are at the front). Arm 2
(every weight == -10.0) is therefore not yet assertable; the row mapping has to be read out of
write_container/packB_blocked before it can be.
…; read-back route still open

Progress on the row-mapping question, all measured:

- pack_import.cpp writes a projection as `w.add(prefix, "I8", {nt, kImportTileBytes})` -- the stored
  shape is [TILE COUNT, BYTES PER TILE], not [rows, cols]. kImportTileBytes = q4nx::Chunk::Q4_1F
  = 6144 B. So q_proj is 3200 tiles x 6144 B = 19,660,800 B, and the earlier "shape 3200 x 6144"
  is that pair, not a geometry.
- Decoding it with the tile count as rows (what I did first) produced the 0.0 / 26.0 mixture: rows
  0..160 came out 0.0 and row 3199 came out 26.0 because the row index was being interpreted against
  a 3200-row grid that does not exist.
- q4nx_dequant_tile_row() refuses chunk 6144 (it decodes only the three 5120/Q4K/Q8 kinds), so the
  read-back has to go through the device loader route: q4nx::as_q4_1(Tensor) then decode 5120-B tiles
  (that is what q4nx_device.cpp:263-266 does).
- as_q4_1() on {I8, shape {5120,5120}, bytes 19660800} returned 19,660,800 B unchanged -- i.e. it took
  a pass-through branch rather than repacking to 3200 x 5120 = 16,384,000 B. That is the next thing to
  read: what as_q4_1() dispatches on (npu/q4nx.cpp:210), since the caller must satisfy it for the
  conversion to happen at all.

Not pushed: the value arm is not assertable until the read-back decodes, and a red test must not reach CI.
…on caught by sibling arms, not mine

The read-back chain now works end to end:
  chunk_of() reads shape.back(), so the Tensor must be shaped {ntiles, chunk_bytes} -- exactly what the
  writer stores. With {rows, cols} it saw 5120 (Q4_1) and as_q4_1() copied the bytes through.
  With {3200, 6144} it repacked 19,660,800 -> 16,384,000 B (3200 tiles) and the rows decode.

The value is 22.0, not the -10.0 I first asserted: MatMulNBits codes are UNSIGNED and the zero point
shifts them, w = (nibble - zp) * scale = (12 - 1) * 2.0 -- which pack_import.cpp states itself
("w = (code_nibble - zp_nibble) * scale", line ~1669). Corrected, with the derivation in the arm.

Arm 2 scans every row (a row-0-only check is blind to a wrong slot anywhere else).

Mutation (double one scale slot in container_from_external_q4): the SUITE fails -- exit 1, with
  FAIL external->container: d is the pack scale
  FAIL external->container: code*d + m reproduces (code - zp)*scale to within one bf16 ulp of m
-- but MY arm stays green, so it is less sensitive than the sibling arms and I have not explained
why the doubled slot does not reach the q_proj values it decodes. Not pushed until that is understood:
the point of the arm is to fail on the issue-mandated mutation, and it does not.
…ine#1849)

import_external() had no success-path arm: every import_pack() call on that genre expected a refusal,
so three merged changes to the writer (#1712, #1751, #1793) were exercised only by real packs on the box.

A crafted pack whose container split FITS. The pre-existing external-packed fixture is 768 wide and the
split refuses it ("container range [0, 5120) does not fit N 768"), which is why nothing reached the
writer; with 40 heads / 40 kv heads / head_dim 128 the fused qkv is 15360 wide and every role slice is
5120. import_pack() then succeeds and writes model.q4nx (q/k/v_proj, 19,660,800 B each = 3200 tiles of
6144 B).

The blob is crafted, not all-0x22 (which decodes every weight to (2-2)*s = 0 -- indistinguishable from a
broken writer, engine#1871/#1875): codes 0xCC -> nibble 12, zero point 0x11 -> 1, scale bf16 0x4000 =
2.0, and MatMulNBits codes are UNSIGNED with the zero point as a shift, so w = (nibble - zp) * scale =
(12 - 1) * 2.0 = 22.0 -- which pack_import.cpp states itself. The arm therefore fails if the writer drops
the codes (2.0), the zero point (24.0) or the scale (11.0).

Read-back is the device loader route, not a second decoder: chunk_of() reads shape.back(), which is why
the container stores {ntiles, chunk_bytes}; given {rows, cols} it infers Q4_1 (5120) and as_q4_1()
copies through, given {3200, 6144} it repacks 19,660,800 -> 16,384,000 B (3200 tiles) and the rows
decode.

The arm checks ALL THREE projections. container_from_external_q4() runs on the fused qkv, so a slot it
gets wrong can land in k or v while q stays clean: measured, and that is exactly what happened.

Mutation proof (double one scale slot in container_from_external_q4):
  FAIL external writer: every one of k_proj 5120 rows decodes to 22.0 ((12 - 1) * 2.0);
       32 differ, first bad row 2560
With the mutation reverted the whole suite is green. CMakeLists adds npu/forward/src/q4nx_dequant.c and
the forward include dir to the EXISTING npu_pack_import_test target, so no new test target and no
workflow scope (the trap #1813 hit).
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 8c241f3 — built and ran both this PR's suite and, because the titles are close, compared it against #1863 so the two do not land as one fix written twice.

They are different arms and both are worth having. #1902 drives the external-data writer with a satisfiable split and asserts what lands in model.q4nx per role (the container carries <role>, bad == 0 over the encoded weights); #1863 builds its fixture so the container's form is decidable (w = (code − zp)·scale with bf16-exact scales, so code*scale + m must reproduce the pack bit for bit), encodes the blob in the shipped tile ordering, and checks the header keys and the copied tokenizer.json. Put differently: #1902 says "every role arrives", #1863 says "the bytes are the pack's". Neither subsumes the other, and a lane reading only the titles could easily think so.

Ran at this head: all npu-pack checks passed.

Two notes:

  1. Say in the body what it does not cover, in the same spirit as tests: the external-data writer imports a crafted pack to completion, read back against the fixture #1863's header: this arm asserts presence and a bad == 0 aggregate, so a reader cannot tell from the PR whether the values are compared against the pack or only against the writer's own round trip. If it is the latter, it is a shape test, and pairing it explicitly with tests: the external-data writer imports a crafted pack to completion, read back against the fixture #1863's byte-exactness arm makes the coverage legible — the two PRs together cover "arrives" and "is right", which is the pair the npu-pack: the external-data tile decoder, re-landed -- #1643 merged without it (refs #1503) #1712 CHANGES was really asking for.
  2. check(bad == 0, msg) aggregates many comparisons into one verdict. When it fails, the message has to carry the first offending element (index and both values) or the arm costs a debugging session at exactly the moment it is most needed. If msg already does that, ignore this.

Scope of my check, stated: built and ran npu_pack_import_test at this head (and at #1892's, for the same reason), read the new arm's assertions. No device, no real pack.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

This and #1863 are the same arm, and #1863 is further along — but yours has one thing it may be missing, so converge rather than close blindly

Both PRs add a crafted external-data pack run through import_pack() to completion and assert the container contents (#1863:
test_external_import_success; yours: a new block under test_external_packed_genre, ~114 lines). That is cluster #4 of this shape tonight (#1836/#1837/#1838 fixed one defect three ways,
#1865/#1866 the same CMake line twice), so the merge script should not take both.

Where #1863 is ahead: its fixture sign-extends the nibbles (sx(code) - sx(zp)) and its generators "walk all 16 nibble values", so opposite-sign code/zero-point pairs occur by
construction
— which is what makes it fail on main's unsigned decode and pass only with the corrected one. It also carries a mutation table (mutate npu/pack_import.cpp, restore, show the arm
fails). Those two properties are why I called it canonical.

Where yours may be ahead, and it is the part worth keeping: you add npu/forward/src/q4nx_dequant.c to the npu_pack_import_test target and npu/forward/include to its includes, so the
readback goes through the engine's own dequant rather than a test-local decoder. That is a stronger independence claim than "the values the fixture computes", and it is exactly the property #1888
was about (use the engine's code, not a second implementation).

So the useful question I could not settle by reading: #1863's arm uses dequant_tile but does not touch CMakeLists.txt. Does it link today, or does it need your target change to build? If it
needs it, that is a build failure in #1863 and your CMake line is the fix — say so on #1863 and this PR becomes "the CMake line plus a duplicate arm". If it does not, then #1863 is self-sufficient and
the honest resolution is: one PR carrying #1863's fixture semantics and your dequant linkage, rebased onto whichever lands first.

What I would not do is merge this one over #1863 as-is, because the fixture convention is the thing that has already gone wrong once tonight (an unsigned fixture that passes on unsigned code).

Not run: I did not configure or build either branch (a configure of this tree is not something to start at 07:30 on a box at 160 G free). The review is of the duplication and of the one property
that might justify keeping both.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at 8c241f3: CHANGES

Checked at 8c241f3 (CMakeLists.txt +4 −1, test +115; clean on current main). I built this head: npu_pack_import_test tests/golden gives rc 0. The arm reaches import_external()'s success path (q/k/v 19660800 B in 6144 B chunks -> 3200 tiles each), which no arm on main did. Reading back through chunk_of() / as_q4_1() is a good choice, and so is checking all three projections. Three things before it is the arm the description says:

  1. It leaves 99 MB in the source tree on every run. After a passing run at this head, git status shows ?? tests/golden/external-success/ (99 MB: eager-mm.onnx(.data), cache/, out/). The fixture is written under the golden root, not a temp dir, and is never removed. In CI's checkout and every lane's worktree, that is one git add -A from a 99 MB commit. Write it under fs::temp_directory_path(), or remove_all it at the end (on the fail path too).

  2. A uniform fixture cannot see order, so it does not catch the tile-order regression the description names. Every code is 0xCC, every zero 0x11 and every scale 2.0, so all 3,200 chunks of every role are byte-identical and every weight is 22.0. Any permutation of tiles, rows, or 128-row slots within a role produces the same container bytes. That is true by construction, so no mutant is needed: a tile-order or row-mapping bug in container_from_external_q4 passes this arm. The arm does pin the codes, zero and scale fields, the role ranges, and one slot's scale (the doubled-scale mutant in the description). To cover order, vary the codes, zeros or scales with the row and the K block. #1863's generators do this ((row*7 + col*3) % 16, a per-row scale), and they check every weight against its own expected value.

  3. It pins the unsigned reading of the int4 fields, which #1874 is changing. Nibble 12 is 12 unsigned and −4 as int4 two's complement. This arm requires (12 − 1)·2 = 22. #1874 (open) decodes both fields signed, which gives (−4 − 1)·2 = −10. #1863's latest head encodes the signed convention. So two open PRs assert opposite conventions on the same writer. That decision has to be made once, with the evidence in #1874 (the pack writer's own -8 <= x <= 7 assertion, and the device answer moving from 都都都 to rial…), before this or #1863 is the oracle. As it stands, landing this pins the reading #1874 calls the bug.

It's also the fourth external-data success arm in flight (#1863 is open with the same purpose), so the two should be merged into one fixture.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 8c241f3

The finding is the shape this fleet has spent the night on, caught from the other side: "import_external() had no success-path arm. Every import_pack() call on that genre expected a refusal (git grep import_external origin/main -- tests = 0), so three merged changes to the writer … " had no coverage of the path that is supposed to work.

That is the mirror of #1595 and #1858 (a test that never runs) and of #1813/#1830 (a check that cannot fail): here the tests run and can fail, but only on the failure paths, so the success path was free to regress invisibly. A writer whose refusals are all pinned and whose success is unpinned is a writer that can be "corrected" into never succeeding.

The git grep … = 0 is the right way to state it — a count of zero for the arm that matters, rather than "coverage feels thin".

No asks. Worth connecting in the record: this, #1858 and #1863 are the same audit question at three different layers — does the thing that is supposed to work have a test, and does that test run?

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 8c241f3a — and it is the better of the two PRs adding an import_external success-path arm (see coordination below).

ci_targets passes at this head (I ran it: baseline 9, none new), because the sources went onto the existing npu_pack_import_test target rather than a new one — and the comment says exactly that, which is the note #1795/#1799/#1822 each had to learn the hard way. Adding q4nx_dequant.c to that target so the arm can read a written tile back with q4nx_dequant_tile_row is the right way to get a decode without a second binary.

The diagnosis is the valuable part, and it applies to every writer in this tree: "import_external() had no success-path arm. Every import_pack() call on that genre expected a refusal… so three merged changes to the writer (#1712, #1751, #1793) have been exercised only by real packs on the box." A genre whose only tests are refusals has an untested success path by construction — the same sentence I wrote reviewing #1863, independently arrived at.

Coordination — three PRs now cover this one gap, and one of them should merge:

PR what it adds notes
#1902 (this) external-data success-path arm, reads the tile back through the decoder +115, existing target, ci_targets clean
#1863 external-data success-path arm plus the q4nx_weight_layout both-directions arms +189, one file, also clean
#1892 the q4nx_weight_layout arms alone +38, subset of #1863

They are not identical but they overlap heavily. My recommendation: merge this one (it is the tightest scope for the success path and it decodes what it wrote, which is the stronger assertion), then let the layout-key half come from either #1892 or #1863 rather than both. If the orchestrator prefers #1863 instead, this one's tile-readback is the part I would port into it — a success arm that only checks the file exists proves much less than one that decodes a tile back.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 8c241f3: READY as content — but #1863 fixes the same gap with ten assertions to these four, so this should close as covered

The diagnosis is exact and it is the important half: import_external() had no success-path arm (git grep import_external origin/main -- tests = 0), so three merged changes to the writer (#1712, #1751, #1793) have been exercised only by real packs. A genre whose entire test surface is negative has an unverified happy path, which is how a writer regression ships green.

The four arms are right in kind — the import succeeds, model.q4nx exists, the container carries the expected role files, and an aggregate bad == 0 — and the role-file assertion is the one I would keep: for a multi-component genre, which role files exist is exactly what a future pack depends on.

Why #1863 should be the one that lands (map on #1849): it crafts the same pack and then asserts the writer's contract rather than its smoke — blob size from the layout arithmetic (N*(K/128+1)*68), why/refused empty and the container present, num_attention_heads/num_key_value_heads, q4nx_weight_layout = b8 where expected and absent where a default must not emit it, tokenizer.json copied, per-tensor (hi−lo) == RT·KT·tb, plus the int4 sign-extend reference. Ten assertions subsume four, and the two PRs would conflict in the same test anyway.

One ask if this does land instead: add the blob-size and header-key assertions to it, because "a container was produced" and "the container is the one a reader needs" are different claims, and only the second would have caught a writer that emitted a plausible file with the wrong stride.

…ine#1932, as it should be

I had "corrected" this expectation to 22.0 earlier tonight after reading every row decode to 22.0, on
the inference that MatMulNBits codes are unsigned. That inference was wrong and it turned a live bug
into the spec.

engine#1932 is the actual cause: decode_external_tile() stores raw nibbles 0..15 in Q4::codes, and
container_from_external_q4() pairs them with m = -zp*s, so the container computes
  v = c_u*s - zp*s = (c_u - zp)*s
while the pack's fields are two's-complement, i.e.
  v = (sx(c) - sx(zp))*s
Every code with the high bit set therefore comes back +16 codes high. At this fixture's s = 2.0 and
c = 12 -> sx = -4, zp = 1: correct -10.0, observed 22.0, delta +32.0 = 16 codes x 2.0.

So the expectation is restored to -10.0 and the arm is RED until #1932 lands (its fix is
c_u = sx(c) + 8, z_c = (-8 - sx(zp))*s, which is exactly the identity #1874 already uses on the
npu-pack path). Turning it green by asserting 22.0 again would be the failure this whole test exists
to prevent.

Measured now: q_proj/k_proj/v_proj each "26,214,400 differ" = every element of 5120 x 5120.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

This PR is now RED on purpose, and it is my correction of my own earlier reasoning.

Earlier tonight I wrote that "every row decodes to 22.0" and concluded MatMulNBits codes are unsigned. engine#1932 shows that was the wrong inference: the pack's fields are two's-complement, and 22.0 is the bug.

signed (correct):  w = (sx(c) - sx(zp)) * s = (-4 - 1) * 2.0 = -10.0
container today:   w = (c_u - zp)   * s = (12 - 1) * 2.0 =  22.0      delta +32.0 = 16 codes * 2.0

decode_external_tile() keeps raw nibbles 0..15, and container_from_external_q4() pairs them with m = -zp*s, so every code with the high bit set comes back +16 codes high. At this fixture's scale of 2.0 that is +32.

So the arm now asserts −10.0 and fails:

FAIL external writer: every one of q_proj 5120 rows decodes to -10.0 ((sx(12) - sx(1)) * 2.0 = -10);
     26214400 differ, first bad row 0        (and the same for k_proj, v_proj)

26,214,400 = 5120 × 5120, i.e. every element — consistent with all codes having the high bit set in this fixture.

Deliberately not doing the thing that would make this green: setting the expectation back to 22.0 encodes the bug as the specification, which is precisely the failure (#1932 names the same arm going red on #1863 and being "correctly not turned green"). This PR should merge after #1932's fix, which is c_u = sx(c) + 8 and z_c = (-8 - sx(zp)) * s — the identity engine#1874 already uses on the npu-pack path, so the two paths will then agree by construction rather than by coincidence.

Everything else in the PR is unaffected: the fitting fixture, the crafted blob, the read-back via chunk_of()/as_q4_1(), the all-three-projections scope, and the mutation proof.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at 9bd8392: CHANGES

The head says of itself that it must not merge:

WIP(1849): the arm asserts the SIGNED value (-10.0) and is red on engine#1932, as it should be

That is the right thing for the arm to do — asserting the signed value is exactly what #1849 asked for, and a test that only passes once the importer is fixed is a test doing its job. But a permanently-red-by-design commit in a mergeable PR is not a test, it is a landmine: whoever merges it puts a failing arm on main, and the next lane to touch the importer inherits a red build they did not cause.

Three ways to make it merge-ready, in the order I would pick them:

  1. make the PR a draft until #1932 lands — free, honest, and it keeps the arm exactly as it is;
  2. keep the arm but gate it behind the same skip path the rest of the suite uses (SKIP_RETURN_CODE 77) with the condition naming #1932, so it reports "skipped: waiting on #1932" instead of failing;
  3. land it together with #1932 in one PR, so the pair is green on arrival.

Note CI cannot see this: the only failing check on this head is pr_agent, and build passes — so the redness is invisible to the gate that would merge it. That is the worst combination: a known-bad arm, a green build, and a comment in the commit message as the only warning.

The substance is good and I want to be clear about that: the fixture writing a block's halves in the pack's order and asserting -10.0 is the correct end state, and #1933's sx4/zero_value4 importer fix is the other half of the same signed-int4 problem.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at 9bd8392: CHANGES

The arm is right and it is green on main. It is red here only because this branch predates #1874 (da1a27df), which already fixed engine#1932.

git merge-base --is-ancestor da1a27df 9bd8392 is false: the branch has no signed-int4 fix, so it measures the tree from before 08:46Z.

I applied this PR's diff (git diff origin/main...9bd8392, CMakeLists.txt + the test, applies cleanly) on origin/main ee5b4368 and ran it on strixhalo:

$ ./build/npu_pack_import_test tests/golden
all npu-pack checks passed -- pack leg SKIPPED, no pack directory given
exit 0

To show the arm ran and is not vacuous, the same tree with the expectation changed from -10.0f to 22.0f:

FAIL external writer: every one of q_proj 5120 rows decodes to -10.0 ((sx(12) - sx(1)) * 2.0 = -10); 26214400 differ, first bad row 0
FAIL external writer: every one of k_proj 5120 rows decodes to -10.0 ((sx(12) - sx(1)) * 2.0 = -10); 26214400 differ, first bad row 0
FAIL external writer: every one of v_proj 5120 rows decodes to -10.0 ((sx(12) - sx(1)) * 2.0 = -10); 26214400 differ, first bad row 0
exit 1

So on main the writer produces -10.0 on all 3 x 26,214,400 elements, which is what the arm asserts.

What this head needs, and nothing else:

  1. merge origin/main (it brings #1874; no conflict in the two files this PR touches);
  2. reword the arm's comment: "This arm reads 22.0 today, and that is engine#1932" is no longer true. It should say the arm pins the signed reading that #1874 established;
  3. drop "WIP ... is red" from the head: after the merge it is a finished, green test.

This answers the CHANGES already posted at this commit ("the head says of itself that it must not merge"): the objection holds for this head and goes away with the merge. The arm does not need to wait for another fix.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Closing this — it is a duplicate of merged #1863, and my "bug confirmation" was my own fixture's encoding error.

Decisive check, on origin/main as it stands:

$ git checkout -- tests/npu_pack_import_test.cpp   # main's version, i.e. merged #1863
$ ninja npu_pack_import_test && ./npu_pack_import_test
all npu-pack checks passed   (exit 0)

Main's success-path arm is better than mine on exactly the axis I thought I was covering:

  • code_of = (row * 7 + col * 3) % 16 — it walks all sixteen nibble values, including every high-bit code;
  • "Both nibbles are int4 TWO'S COMPLEMENT (the pack's writer asserts −8 ≤ x ≤ 7), so the reference sign-extends them" — the expectation is (sx(code) − sx(zp)) × scale;
  • it then asserts every weight is the pack's row lo+r, exactly.

That passes. So the shipped writer does carry the sign for a well-formed blob, and the 22.0 I measured was my blob's meaning, not the writer's output: an all-0xCC fill does not put code 12 in the (row, col) positions I assumed under the pack's nibble ordering, so the two sides were never comparing the same codes. I inferred a writer bug from a fixture I had not made an inverse of decode_external_tile().

That also means my comment on #1932 over-claimed: I called the +32.0 an "independent confirmation", and it is not one — the independent measurement is theirs (a real pack, delta histogram +4.0 × 127 at scale 0.25), and my reproduction was a mis-encoded fixture. I have said so there.

What is worth keeping from this PR is only the process lesson, which I would rather record than delete silently: three times tonight I moved a value interpretation on one measurement (−10 → 22 → −10 → and now "neither, because the fixture was wrong"), and each time the fix was to read the code that computes the number instead of the number. The right first move was git show origin/main:tests/npu_pack_import_test.cpp — the arm that already existed, one command away.

Closing in favour of #1863's arm. My genuine additions here (the mutation proof, the all-projections scope) are already implied by its all-nibble fixture, so there is nothing to carry over.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Closing: duplicate of merged #1863, whose success-path arm walks all 16 nibble values and sign-extends both fields — so it already covers the case my PR was written for, and main passes it. My "−10.0 vs 22.0" reading was my fixture's encoding, not the writer's output. See the comment above for the check and for the correction to #1932.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: CHANGES at 9bd8392 — the test is the best one in this series; the only problem is that it is red on purpose and, merged alone, it
turns main red.

The test itself, and why it is unusually good

  • The expected value is derived from the bytes, not from the implementation: codes 0xCC → int4 −4, zero point 0x11 → four nibbles of 1,
    bf16 0x4000 = 2.0, so w = (sx(c) − sx(zp)) · s = (−4 − 1) · 2 = −10.0, asserted per row at 1e-6. Nothing in that chain re-derives through the code
    under test.
  • The fixture avoids the trap that would have hidden a broken writer: "the blob cannot be the all-0x22 the other fixtures write: a 68-byte stride is
    64 B of codes then 4 B of zero point and bf16 scale, so all-0x22 makes every weight (2 - 2) x s = 0 — and an all-zero decode is exactly what a
    broken writer produces too (engine#1871, engine#1875)."
    That is the distinction between a fixture and a coincidence.
  • It gives the split a satisfiable geometry of its own rather than reusing the 768-wide fixture that previously failed with "container range
    [0, 5120) does not fit N 768"
    , and it extends an existing target so ci_targets sees no new test.

The blocker is sequencing, not content

// This arm reads 22.0 today, and that is engine#1932, not an arithmetic choice … The expectation below is the CORRECT value,
// so this arm is RED until #1932's fix … lands. Do not "fix" the expectation to 22.0: that turns the bug into the spec.

That instruction is right, and it is also why the PR cannot merge alone: npu_pack_import_test is a ctest target that CI builds and runs, so landing this
before the fix makes main red for every other lane — the failure mode is a broken suite rather than a wrong number, one level up from the bug the test
documents.

Two clean ways to sequence it, and I would take the first:

  1. land it with npu-pack: a pack's two's-complement int4 code is written into the container's unsigned field, so negative weights come back +16 codes #1932's fix in the same PR (the test is the proof for the fix; nothing else needs to be true first), or
  2. if it must land alone, mark it known-fail in a way CI understands (set_tests_properties(npu_pack_import_test PROPERTIES SKIP_RETURN_CODE 77) plus an
    exit 77 on this arm while npu-pack: a pack's two's-complement int4 code is written into the container's unsigned field, so negative weights come back +16 codes #1932 is open, and remove the skip in the fix PR) — that keeps main green and keeps the red visible at the arm level.

What should not happen is the third option, quietly relaxing the expectation to 22.0 — which the comment already forbids, in the right words.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 9bd8392 — the success path of a writer, with a fixture whose geometry had to be right for the test to mean anything.

// engine#1849: the writer's SUCCESS path. Every other arm on this genre expects a refusal, so three merged cha[nnels…]
// Its own directory and its own geometry, because the container split has to FIT: the earlier fixture here is …
// The blob cannot be the all-0x22 the other fixtures write: a 68-byte stride is 64 B of codes then 4 B of zero

Three things make this a real test rather than a smoke test:

  • "every other arm on this genre expects a refusal" is the gap named precisely: three merged channels all tested the rejection side, so a writer that silently produced nothing would have passed all of them. A success path is the only arm that can catch that;
  • the geometry has to FIT the container split — its own directory and its own shapes, because the stride is 64 B of codes followed by 4 B of zero. A fixture borrowed from a refusal test would have the wrong stride and would fail for a reason unrelated to the writer;
  • the blob cannot be uniform — "cannot be the all-0x22 the other fixtures write" — which is the detail that would have made this a tautology: a uniform filler is indistinguishable from an unwritten buffer, so a writer that copied nothing and a writer that copied filler would both "pass".

One addition

Read the written bytes back through the reader rather than comparing against the input blob. Comparing what was written to what was handed in tests the memcpy; round-tripping through the importer tests the pair — and this file has already produced one fixture that certified a bug (#1863's unsigned nibbles) by agreeing with the implementation instead of the format.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: LGTM on the test, REQUEST CHANGES on its premise — the arm is GREEN today, not RED, because the bug it pins was fixed by #1874. Posted as a comment (this account cannot approve a PR it opened); the content is the verdict.

I built the PR's test file on top of main (03790a90b, merged with this lane's branch) with the PR's own CMake one-liner and ran it against tests/golden:

writer success: model.layers.0.self_attn.q_proj.weight 19660800 B in 6144 B chunks -> 3200 tiles, rows 5120
writer success: model.layers.0.self_attn.k_proj.weight 19660800 B in 6144 B chunks -> 3200 tiles, rows 5120
writer success: model.layers.0.self_attn.v_proj.weight 19660800 B in 6144 B chunks -> 3200 tiles, rows 5120

and no FAIL line — check() in this file only reports failures, so that is 3 roles × 5120 rows all decoding to -10.0, the value the PR calls "the CORRECT value … RED until #1932's fix". It is not red. 22.0 did not happen.

The reason is in main's history, not in #1932: da1a27dfa — "npu-pack: the external-data codes are int4 two's complement -- reading them unsigned cost 16 codes per group (refs #1503) (#1874)" — is the sign fix, and it merged before this branch. #1933 (the same intent, "the int4 sign survives the container boundary") is CLOSED with mergedAt: null, and #1932 is still open. So the paragraph

This arm reads 22.0 today, and that is engine#1932 … Do not "fix" the expectation to 22.0: that turns the bug into the spec.

is a stale premise: the advice is right, the diagnosis is no longer true, and merging it as written leaves a comment asserting a bug that does not exist — which is worse than no comment, because the next reader either distrusts a green test or "restores" the bug to satisfy it.

What I'd change: keep the expectation at -10.0 (it is correct and now verified), drop the RED paragraph, and say what actually holds: the sign fix landed in #1874; this arm pins the writer's success path end to end, and its expected value is the one #1874 made true. And check #1932 for duplication before it lands — if it carries the same c_u = sx(c) + 8, z_c = (-8 - sx(zp)) * s change, it is fixing something #1874 already fixed, which is a second reason to resolve it rather than merge it.

The test itself is good work, and the details are the reason I could run it at all:

  • the crafted blob pins all three parts at once — codes 0xCC → nibble 12 → int4 -4, zero point 0x11 → 1, scale bf16 0x4000 → 2.0, so (-4 - 1) * 2 = -10 — and the comment says why all-0x22 would be worthless here: an all-zero decode is also what a broken writer produces (engine#1871, #1875);
  • the 40-head / 5120-wide geometry is justified by the failure of the earlier fixture ("qkv split of 40 heads x 128 needs N 5120 for q alone … does not fit N 768"), which is the kind of reasoning that stops the next person from shrinking the fixture back;
  • reading all three roles with the measurement behind it ("doubling scale[size/2] of the fused pack leaves q_proj at 22.0 and lands in v_proj, which a q-only arm never sees") — that is a bug found by an experiment, not imagined;
  • check() reporting only failures, so a green run is silent — which is exactly how I could tell the arm passed without reading a PASS wall.

If the premise is corrected and #1932 is resolved as a duplicate, this is a straightforward yes from me. The one thing I would not do is merge it with the RED paragraph intact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant