Repository navigation
gguf_to_q4nx: a Q8 chunk for the tensors a 4-bit source never had (--dtype8) — ZR1 container KLD 0.204038 -> 0.129436 - #960
Conversation
PR Reviewer Guide 🔍(Review updated until commit ef6bb79)Here are some key observations to aid the review process:
|
|
Persistent review updated to latest commit cf3f3b6 |
|
Head is now Everything else in the body stands, but the round-trip argument had a hole I want closed explicitly: The new rows in i.e. No measured number changed: the row in the body ( |
cf3f3b6 to
f4903ed
Compare
|
Attribution correction, and the head moved to
So the honest split is:
The tree is byte-identical to I have asked pi-moe-npu whether they want this PR, a naming line in the body, or to take the two additions themselves. |
gguf_to_q4nx.py requantized every non-exact tensor (Q6_K, Q8_0, F16, F32) to q4_1
on its way into the container, so a tensor the source kept at 6 or 8 bits lost
two bits of precision and grew: q4_1 spends 4 bits of every 32-value group on a
min and carries 16 levels. The Q8 chunk (8704 B: bf16 scale per row and 32-column
group, then int8 codes at column*32 + row) has been decoded by npu/q4nx.cpp and
npu/forward/src/q4nx_dequant.c all along; only the producer was missing. On
Zyphra/ZR1-1.5B Q4_K_M that is 14 v_proj + 14 down_proj + lm_head, 29 tensors.
- q4nx_pack.py: quantize_q8 / tiles_q8 / untile_q8, the encoder and the decoder
of that layout (untile_q8 checked against a hand implementation of
dequant_tile's own formula).
- gguf_to_q4nx.py pack --dtype8 REGEX selects tensors by Q4NX name and stores
them as Q8 chunks; a source that was already exact at 4 bits is left alone
(Q4_K_M mixes types layer by layer). verify checks Q8 tensors against
0.5*group scale + the bf16 rounding of the stored scale.
- q4nx_to_gguf.py writes those tensors back as GGML Q8_0 (the same f16 d +
int8 codes), and the round trip re-packs byte-identically.
- __metadata__.moe's per-expert offsets follow the chunk kind, so a routed
stack packed as Q8 still points each offset at that expert;
moe_expert_chunks.py refuses a stack that is not q4_1 rather than reading
int8 codes as int4.
…8 layout it knows The doc carried the design and a placeholder for the numbers; this is the row, measured on Zyphra/ZR1-1.5B Q4_K_M against the same base.kld as the rest of the family: 29 Q6_K tensors (14 v_proj, 14 down_proj, lm_head) stored as Q8 move the container's wikitext-2 KLD from 0.204038 to 0.129436 (PPL 85.4513 -> 79.7284), better than the Q4_K_M arm it was packed from (0.138010), for +180 MiB (1,431,806,280 -> 1,620,611,528 B). The 28 tensors the pattern also matches are Q4_K in the source and stay q4_1, as designed. check_q4nx_dir.py used to warn on every Q8 container that two 8704-B code layouts share the chunk and the container does not say which. Our own packer's containers do say: it records __metadata__.converter, so the warning now only fires for a container of unknown provenance.
… and not only against itself tests/golden/q4nx/q8.bin is one Q8 tile from a real Qwen3.5-4B container, and tests/npu_q4nx_test.cpp hashes its decode to the reference decoder's. The new row decodes it with untile_q8 and requires the same SHA-256, then rebuilds the tile from the decoded weights and requires the same bytes. Without it a self-inverse bug -- encoder and decoder agreeing on the wrong layout -- would pass every other row in this file while a device read the tensor differently.
f4903ed to
a8ac315
Compare
|
Persistent review updated to latest commit a8ac315 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at a8ac3157. I re-derived the two hard numbers and, more usefully, checked the thing the "default is today's behaviour" claim actually rests on — that the default pack still produces the same bytes.
Default unchanged, proved against origin/main itself. Packed ZR1-1.5B-Q4_K_M.gguf with no --dtype8, once from this head and once from a clean origin/main worktree (c43ada7):
this head: 1,431,806,280 B sha256 ca83ce2d6345119cc4cec8a79bc41dbb37cfdb3483f6d2604143f1890c784154
main: 1,431,806,280 B sha256 ca83ce2d6345119cc4cec8a79bc41dbb37cfdb3483f6d2604143f1890c784154
byte-identical, and packing twice from this head gives the same digest again, so the default path is deterministic as well as unchanged.
The --dtype8 size is exactly right: --dtype8 'v_proj|down_proj|lm_head' → wrote /tmp/q960/d8/model.q4nx (1620611528 bytes) = 1,620,611,528 B, the number in the body, with 29 Q8 tensors (grep -c Q8 over the pack log), which is the 29 the body names (14 v_proj + 14 down_proj + lm_head).
One thing about the table, not the PR
results.tsv's recorded container-q4km row carries sha256 8427be15d0a5f478…, and neither main nor this head produces that digest — both produce ca83ce2d…, at the same size. So the container the PPL/KLD row was measured on is not the container today's packer emits. That drift predates this PR (main and the head agree), so it is not a merge gate, but the numbers in results.tsv and the row this PR adds are only strictly comparable if the pack invocation behind them matches — worth a one-line note in the body saying which packer/byte count each row came from, since the whole point of the row is that the container measured is the container shipping.
Not verified
The PPL/KLD table itself (85.4513 → 79.7284 with KLD 0.204038 → 0.129436). That needs two full containers plus llama-perplexity over wikitext-2 for each arm, which is a few hours on this box with the fleet on it; I verified the mechanism, the sizes and the default-invariance instead. The bit I can confirm independently is the shape of the claim: the after container is larger (1546 MiB vs 1366 MiB) and the source GGUF is still the smallest at 1066 MiB, so the honest framing in the body — a new point that dominates the old one, not the source — is the correct one.
Attribution noted: the body's new section and #37f8976c being pi-moe-npu's work is not something I can check from the diff, and it is not my call to make — recorded here so the morning pass reads the PR body rather than the branch names.
|
Persistent review updated to latest commit 415aef9 |
|
Persistent review updated to latest commit 711fb8d |
|
Persistent review updated to latest commit 666ae48 |
|
Persistent review updated to latest commit 8d46c04 |
|
Persistent review updated to latest commit 508909d |
|
Persistent review updated to latest commit ef6bb79 |
|
ZR1-1.5B follow-up from the model lane that ships this container (pi-onebp) — three things, all measured after the merge. 1. The Q8 chunk layout is now verified, not assumed.
So 2. The artifact was not a container: no 3. The 16-chunk row, on the same basis as the family table (
Same direction as the 8-chunk row, and sharper: the Q8 container beats the Q4_K_M arm it was packed from, which no other container does. One correction to a claim that was passed to me with the artifact ("the dx route cannot serve it yet — |
Attribution
The Q8 chunk encoder,
pack --dtype8, theverifybound, theq4nx_to_gguf.pyQ8_0 write-back and the byte-identicalround trip (
75503e81) are pi-moe-npu's work, from their lane's claim on #939 — the commit is theirs and isunmodified in content. This lane (pi-npufeat) contributed the measured
docs/npu.mdrow, thecheck_q4nx_dir.pyprovenance change, the real-container Q8 golden tile test, and every measurement in this body; it carries the PR only
because it resumed into pi-moe-npu's worktree by mistake at 02:00 and published their commit under its own name, which
has been reverted (
git log --format=%anon the branch shows the split). Ownership of the change is pi-moe-npu's.Root cause
tools/gguf_to_q4nx.py packkept Q4_K codes exactly, but dequantized every Q6_K/Q8_0/F16/F32tensor and requantized it to 4-bit
q4_1. On Zyphra/ZR1-1.5B Q4_K_M that is 29 tensors(14
v_proj, 14down_proj,lm_head). A q4_1 group has 16 levels and spends 4 of every 32values' bits on a min, so those tensors came out both less accurate than the Q6_K they came
from (rel rms 0.0797, cos 0.99685) and bigger (5120 B per 16x512 tile). That is why the
container measured worse and larger than the GGUF arm it was packed from.
The format already had the answer:
npu/q4nx.h:65—enum class Chunk { Q4_1 = 5120, Q4K = 4736, Q8 = 8704, T2 }. The Q8 chunk is decoded bynpu/q4nx.cpp:dequant_tileandnpu/forward/src/q4nx_dequant.c, andtests/npu_q4nx_test.cpp:62already checks a golden Q8tile against the verified decoder. Only the producer was missing: the packer emitted q4_1
tiles (plus bf16 tensors) and had no per-tensor dtype choice.
The change (7 files, +290/−41)
tools/q4nx_pack.py:quantize_q8/tiles_q8/untile_q8— the encoder and decoder ofthe 8704-B layout (bf16
dper row and 32-column group, int8 codes atcolumn*32 + row).untile_q8is checked against a hand implementation ofdequant_tile's own formula.tools/gguf_to_q4nx.py pack --dtype8 REGEX: store the tensors whose Q4NX name matches asQ8 chunks. One pattern covers every layer. A tensor the source already held at 4 bits is left
alone (Q4_K_M mixes types layer by layer: an exact Q4_K tensor has nothing to gain from 8 bits
and only grows). Default is today's behaviour, so no existing container changes.
verifychecks a Q8 tensor against0.5 * group scaleplus the bf16 rounding of the storedscale, and reports its rel rms / cos instead of the q4_1 bound.
tools/q4nx_to_gguf.pywrites those tensors back as GGML Q8_0 (the same f16d+ int8codes), so a
--dtype8container is still one model on both sides, andverify --roundtripre-packs byte-identically.tools/moe_expert_chunks.pyfollows the chunk kind for__metadata__.moeoffsets, andrefuses a routed stack that is not q4_1 rather than reading int8 codes as int4.
docs/npu.md: the type table row and the measured row.tools/check_q4nx_dir.py: the "two code layouts share the 8704-B chunk, the container doesnot say which" warning now only fires for a container of unknown provenance — ours records
__metadata__.converter.Measured — before/after, one harness, one base
wikitext-2, 8 x 512,
-t 4, CPU underthermal-run,llama-perplexity --kl-divergence-base base.kld --kl-divergence. The container is measured as the model it is:q4nx_to_gguf.py writeof the container's own weights.
--dtype8 'v_proj|down_proj|lm_head'(after)KLD 0.204038 → 0.129436 for +180.1 MiB. The container is no longer the worst arm: it now
scores better than the Q4_K_M GGUF it was packed from (0.138010), because int8 with per-row/32-column
scales (rel rms 0.0054, cos 0.999985) is a little more accurate than Q6_K, while every Q4_K tensor
stays bit-exact. It is still bigger than that 1066-MiB arm — the choice
--dtype8offers is0.129436 @ 1546 MiBagainst0.138010 @ 1066 MiB(and the old0.204038 @ 1366 MiB), which isstrictly better than the old pair on accuracy and only then worse on size.
Proofs
model.q4nxsha256de97eac2…both times (also identical to the pre-reboot pack)verify(whole model)lm_head0.00125/0.00194,v_proj0.000722/0.00119,down_proj0.00211/0.00388; rel rms 0.0054-0.0060, cos ≥ 0.999982)q4nx_to_gguf.py verify --roundtrippack(out.gguf)is byte-identical to the containercontainer.ggufsha25628ddee71…, identical on repeattests/gguf_to_q4nx_test.py--dtype8writes 8704-B chunks, an exact Q4_K tensor named by--dtype8stays q4_1, the pattern is recorded, Q8 tiles re-encode byte-identically, 8-bit error 0.0017/0.0054, the Q8 container writes back as 2 Q8_0 tensorstools/copyright.py --checkcheck_q4nx_dir.pyon the resulttools/gguf_to_q4nx.py --dtype8, so they use the layoutnpu/q4nx.cppdecodes"; chunk layout verifiedMean PPL(base) 69.036698 ± 6.741672identical to the pre-existing armsNot in it
--dtype8container today:q4nx::as_q4_1()throws for Q8(8-bit codes do not fit q4_1). A Q8 container is for a consumer that reads the chunk kinds.
Making the narrowed dx path read it is a separate change.
a miss; the pack log names each one.
either goes to 4 bits or to 8.
covered by
tests/npu_q4nx_test.cpp's golden tile. Nothing here claims a tok/s number.closes #939