Repository navigation
npu-pack test: the external-data writer gets a success-path test (engine#1849) - #1902
bong-water-water-bong wants to merge 5 commits into
Conversation
…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).
|
VERDICT: READY at They are different arms and both are worth having. #1902 drives the external-data writer with a satisfiable split and asserts what lands in Ran at this head: Two notes:
Scope of my check, stated: built and ran |
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 blindlyBoth PRs add a crafted external-data pack run through Where #1863 is ahead: its fixture sign-extends the nibbles ( Where yours may be ahead, and it is the part worth keeping: you add So the useful question I could not settle by reading: #1863's arm uses 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 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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:
-
It leaves 99 MB in the source tree on every run. After a passing run at this head,
git statusshows?? 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 onegit add -Afrom a 99 MB commit. Write it underfs::temp_directory_path(), orremove_allit at the end (on the fail path too). -
A uniform fixture cannot see order, so it does not catch the tile-order regression the description names. Every code is
0xCC, every zero0x11and every scale2.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 incontainer_from_external_q4passes 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. -
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 <= 7assertion, and the device answer moving from都都都torial…), 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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
VERDICT at
|
…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.
|
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.
So the arm now asserts −10.0 and fails: 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 Everything else in the PR is unaffected: the fitting fixture, the crafted blob, the read-back via |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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:
- make the PR a draft until #1932 lands — free, honest, and it keeps the arm exactly as it is;
- 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; - 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
left a comment
There was a problem hiding this comment.
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:
- merge
origin/main(it brings #1874; no conflict in the two files this PR touches); - 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;
- 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.
|
Closing this — it is a duplicate of merged #1863, and my "bug confirmation" was my own fixture's encoding error. Decisive check, on Main's success-path arm is better than mine on exactly the axis I thought I was covering:
That passes. So the shipped writer does carry the sign for a well-formed blob, and the That also means my comment on #1932 over-claimed: I called the 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 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. |
|
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. |
|
VERDICT: CHANGES at The test itself, and why it is unusually good
The blocker is sequencing, not contentThat instruction is right, and it is also why the PR cannot merge alone: Two clean ways to sequence it, and I would take the first:
What should not happen is the third option, quietly relaxing the expectation to 22.0 — which the comment already forbids, in the right words. |
|
VERDICT: READY at Three things make this a real test rather than a smoke test:
One additionRead 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
left a comment
There was a problem hiding this comment.
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 point0x11→1, scale bf160x4000→2.0, so(-4 - 1) * 2 = -10— and the comment says why all-0x22would 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.
Why
import_external()had no success-path arm. Everyimport_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-packedfixture is 768 wide and the split refuses it: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 writesmodel.q4nx:The crafted blob, and why not all-
0x22The 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 codes0xCC(nibble 12), zero point0x11(1), scale bf160x4000(2.0). MatMulNBits codes are unsigned with the zero point as a shift, sowhich
pack_import.cppstates 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()readsshape.back()— which is why the container stores{ntiles, chunk_bytes}and not[rows, cols]. Given{rows, cols}it infersQ4_1(5120) andas_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):Reverted, the whole suite is green.
CI
CMakeLists.txtaddsnpu/forward/src/q4nx_dequant.candnpu/forward/includeto the existingnpu_pack_import_testtarget — sources on a target already inci.yml's--targetlist, so no new test target and no workflow scope (the trap my #1813 hit tonight).