Repository navigation
tools: check a Q8 tile's code order against the source GGUF instead of assuming it - #1037
Conversation
…e order is checked not assumed `check_q4nx_dir.py` says of itself: "it cannot tell the two Q8 code layouts apart - the container does not record which one it holds - so it reports the Q8 tensors and says so, rather than guessing." Two 8704-byte chunks share a shape, and a container in the wrong order decodes to plausible-looking floats: it serves noise, not an error. `--against <source.gguf>` closes that the only way it can be closed - from the weights. For each Q8 tensor (first N, default 3) it reads the first tile, decodes it in BOTH orders (npu/q4nx.cpp's `column*32 + row`, and the Qwen3.6-35B order), and correlates each against the tensor in the GGUF the container was packed from, matched through the packer's own `hf_name()` mapping. It passes only when the order `npu/q4nx.cpp` decodes wins with corr >= 0.99 and rel-rms <= 0.10, and otherwise fails with the tensor's name and both scores. The default path stays metadata-only and dependency free; numpy and gguf are imported inside that one mode. Measured, ZR1-1.5B (the shipped 1bit-MONSTER/ZR1-1.5B-GGUF container, 29 Q8 tensors, source Q4_K_M GGUF): lm_head.weight vs output.weight A corr +1.0000 rel-rms 0.0053 | B corr +0.0324 model.layers.0.self_attn.v_proj vs blk.0.attn_v.weight A corr +1.0000 rel-rms 0.0054 | B corr +0.0009 model.layers.0.mlp.down_proj vs blk.0.ffn_down.weight A corr +1.0000 rel-rms 0.0055 | B corr +0.0250 The 0.005 residual is Q6_K -> Q8 requantization, not layout error. The same container with one tile's codes moved to the other order (scales untouched) fails: "A corr -0.0044 rel-rms 1.4519, B corr +1.0000". Tests: three new rows in tests/gguf_to_q4nx_test.py against the synthetic GGUF it already builds - `--against` accepts the packed container (rc 0), refuses the same container with one tile permuted into the other order (rc 1), and names the tensor - so the new mode runs in the existing CI step. Plus a self-test row in the tool that builds a tile in one order and proves the two decoders disagree about it, so the verdict cannot come from a decoder agreeing with itself.
PR Reviewer Guide 🔍(Review updated until commit 2fad106)Here are some key observations to aid the review process:
|
…thing Two ways the new mode could have passed without verifying anything: * `hf_name()` raises `SystemExit` for a GGUF entry with no Q4NX name and `ValueError` for a name that is not `blk.N.part.suffix`, so pointing --against at a different model's GGUF crashed with a traceback instead of a verdict. Such entries are now skipped. * if no container tensor matched a tensor in the GGUF, the loop finished with no rows and the tool printed "chunk layout verified". It now fails with "no Q8 tensor … matched a tensor in <gguf>: nothing was verified" — the silent success this mode exists to remove. Measured: ZR1's container against ZR1's own GGUF → rc 0; against Qwen3.8-27B's GGUF (names match, weights are another model's) → rc 1 with "A corr -0.0216 rel-rms 1.0582, B corr +0.0034"; `tests/gguf_to_q4nx_test.py` still PASS, `--self-test` PASS, copyright rc 0.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b1d31692
The claim is that the tool now decides a Q8 tile's code order against the model rather than assuming it, and that is exactly what it does. I ran it on a real --dtype8 container against its real source GGUF rather than only on the test's fixtures:
$ python3 tools/check_q4nx_dir.py ~/models/npu/ZR1-1.5B-npu-q4km-q8 \
--against ~/models/zyphra-zr1/ZR1-1.5B-bf16.gguf
lm_head.weight vs output.weight layout A: corr +0.9998 rel-rms 0.0187 | B: corr +0.0321 rel-rms 1.4363
blk.0.self_attn.v_proj.weight vs blk.0.attn_v.weight layout A: corr +0.9998 rel-rms 0.0188 | B: corr +0.0011 rel-rms 1.4447
blk.0.mlp.down_proj.weight vs blk.0.ffn_down.weight layout A: corr +0.9998 rel-rms 0.0193 | B: corr +0.0247 rel-rms 1.4302
-> chunk layout verified, and the Q8 tiles hold the code order npu/q4nx.cpp decodes (exit 0)
29 Q8 tensors in that container, so this is the case the PR is about, and the separation is not marginal: the correct layout correlates at 0.9998 while the other lands at 0.001-0.03 — i.e. the "serves noise with no error anywhere" failure is now a two-number comparison. The acceptance gate is stated in the code and is the right shape (a[0] >= 0.99 and a[1] <= 0.10 and a[0] > b[0], so a container that is right must also be better than the alternative, which a both-wrong decode cannot fake).
Also checked:
python3 tests/gguf_to_q4nx_test.py→ PASS, rc=0, including--against refuses the other code order (rc 1)andthe refusal names the tensor— so the negative is in the suite, not only in the prose;- a container with no Q8 tensors is handled honestly rather than vacuously:
~/models/npu/Qwen3-0.6Breportsno Q8 tensors, nothing to check against the source weightsand still exits 0 with its chunk table verified (197 q4_1 tensors), which is the right behaviour for the many packs that have none; tools/copyright.py --check→ rc=0;- the write-back case in the test (
the Q8 container writes back as 2 Q8_0 tensors (got 2)) covers the other direction, which is the one the packer could get wrong while the decoder stays right.
One nit: the thresholds are in the code but not in the help text, and a user who sees corr +0.90 needs to know what the tool will call it. One line in --help (passes when correlation >= 0.99, rel-rms <= 0.10, and the declared order beats the other) makes the printed table self-explanatory, and it is the number a future packer change should be judged against.
Not re-run: nothing device-side — this tool is pure host code over the container and the GGUF, which is why the check above is the whole claim.
|
Persistent review updated to latest commit d468141 |
|
Persistent review updated to latest commit 2fad106 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d4681414
This push closes the hole I flagged and it does so on the right case. Every mode, run just now:
Q8 container + the WRONG gguf -> rc=1, "chunk layout NOT verified"
FAIL ... v_proj.weight: not the layout npu/q4nx.cpp decodes (A corr -0.0140 rel-rms 1.6072, B corr -0.0196)
FAIL ... down_proj.weight: not the layout npu/q4nx.cpp decodes (A corr +0.0198 rel-rms 1.7021, B corr -0.0186)
Q8 container + its own gguf -> rc=0, "chunk layout verified, and the Q8 tiles hold the code order npu/q4nx.cpp decodes"
(A corr +0.9998 rel-rms 0.0193 for down_proj, B +0.0247 rel-rms 1.4302)
container with no Q8 tensors -> rc=0, "no Q8 tensors, nothing to check against the source weights"
The middle row is the one that matters: a wrong GGUF now lands at rc=1 naming the tensor, where before it would have compared nothing and printed success. rows non-empty is the right guard, and except (SystemExit, KeyError, ValueError): continue for a GGUF entry with no Q4NX name is correct — that entry is not ours to check, so skipping it cannot turn into a false negative, whereas a blanket "skip anything that fails to map" could.
Two small things:
chunk layout verified, and the Q8 tiles hold the code order npu/q4nx.cpp decodesis printed in the no-Q8 case too, where no order was established. It is true (the chunk layout is verified, and there are no tiles to be wrong), but the sentence reads as though a code order was checked. Something likechunk layout verified; no Q8 tensors to checkthere would be exact.- The acceptance thresholds are still not in
--help(corr >= 0.99,rel-rms <= 0.10, and the declared order must beat the other). A user readingA corr -0.0140in the FAIL line above knows it failed, not what it had to reach.
The device-free claim is unchanged and still the whole story here — this tool never touches the GPU.
Root cause
tools/check_q4nx_dir.pystates its own limit: two 8704-byte Q8 chunks share a shape, "thecontainer does not record which one it holds - so it reports the Q8 tensors and says so, rather
than guessing". That gap is silent by construction: a container whose codes are in the other
order decodes to plausible floats, so it serves noise with no error anywhere.
--dtype8(#960)made Q8 chunks a container feature, and the only way anyone could confirm the order was a
hand-written script that is not in the tree.
The check cannot come from the packer's round trip: encoder and decoder can agree with each other
and both disagree with the weights. It has to come from the model.
Fix
check_q4nx_dir.py --against <source.gguf>: for each Q8 tensor (first N, default 3) read its firsttile, decode it in both code orders, and correlate each against the same tensor in the GGUF the
container was packed from — matched through the packer's own
hf_name()mapping, so there is onename mapping in the tree, not two. Pass = the order
npu/q4nx.cppdecodes wins at corr ≥ 0.99 andrel-rms ≤ 0.10; otherwise exit 1 with the tensor's name and both scores. The default path is
unchanged: metadata only, no numpy, no gguf, nothing dequantized. numpy/gguf are imported inside
the new mode only.
Measured — ZR1-1.5B, the shipped
1bit-MONSTER/ZR1-1.5B-GGUFcontainer (29 Q8 tensors)Source:
ZR1-1.5B-Q4_K_M.gguf(9457f6ba…), tile (0,0) of each tensor. Layout A iscolumn*32 + row(whatnpu/q4nx.cppdecodes), B is the Qwen3.6-35B order.lm_head.weightoutput.weightmodel.layers.0.self_attn.v_proj.weightblk.0.attn_v.weight(Q6_K)model.layers.0.mlp.down_proj.weightblk.0.ffn_down.weightThe 0.005 is Q6_K → Q8 requantization, not a layout error. Before/after on the same container with
one tile's codes moved to B (scales untouched, i.e. the silent failure):
Tests
Three rows in
tests/gguf_to_q4nx_test.py, against the synthetic GGUF that test already builds(so this runs in the existing CI step, no new dependency, no model on disk):
--againstaccepts the container the packer just wrote — rc 0;--againstrefuses the same container with one Q8 tile permuted into the other order — rc 1;Plus a
--self-testrow that builds a tile in one order and asserts the two decoders disagree aboutit, so the verdict cannot come from a decoder agreeing with itself.
--self-testandtests/gguf_to_q4nx_test.pyboth pass;tools/copyright.py --checkclean.Not in this PR: making
--self-testrun in CI (it needs aworkflow-scoped token to edit.github/workflows/ci.yml); the rows above already run throughtests/gguf_to_q4nx_test.py.Related: #960 (adds the
--dtype8chunks this verifies), #799 (plan/kernel-layer Q8 notes).No issue is closed by this — it is the missing half of #960's own verification story.