Repository navigation
npu: pin the t2 padding convention, and say where the fold lives (P2 follow-up) - #817
Conversation
Consumers asked two questions the format doc did not answer, both of which change what a reader has to do, so they belong in the spec rather than in a comment thread: - A tensor whose shape is not whole tiles: the pinned convention is zero padding (code 0 with scale 0.0 -> a weight of exactly 0, so a padded K column contributes 0 to the reduction for any activation, and a padded N row is never read). `pack()` still refuses such a tensor by default; `pad=True` pads it through the new `pad_to_tile()`, and the GGUF converters (`from_ptq1_0`, `from_tq1_0`, `from_tq2_0`) pass the flag through. All 431 ternary Bonsai-2 images are already whole tiles, so this exists for GGUF tensors and for a projection whose width is not % 32. - The stored weight is the folded (post-rotation) one and t2 carries no rotation: the consumer turns the *activation* into the container's basis, A' = H (A . s) -- signs first, then the normalized Sylvester-Walsh butterfly -- and never touches the weight (npu/forward/src/q4nx_forward.cpp::hadamard_rotate). So the chunk needs no pre/post-rotation flag; the fold stays declared in the container (q4nx_hadamard, q4nx_hadamard_block, HAD1 sign table). The test asserts the convention rather than the size: real block bit-exact, padding exactly 0, a padded column adding nothing to a dot product, and the padded scales themselves zero (a padder that leaves them at 1.0 is caught).
PR Reviewer Guide 🔍(Review updated until commit 41edf86)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 3294efd (tprs-ready applied)
Both halves of the PR are a convention, so I checked that the convention is enforced rather than merely written down.
1. Zero padding is enforced, and the check bites. python3 tests/q4nx_t2_test.py --no-model → PASS q4nx_t2.
Then I mutated the padder in place (np.zeros → np.ones for the padded scale slots) and re-ran:
FAIL the padding carries a zero scale too (0.0, as the format doc says)
FAIL q4nx_t2 (1 checks)
so the assertion is load-bearing, not decoration. The other new checks state the arithmetic and not just the size: the
real block stays bit-exact through pack(..., pad=True)/unpack, the padding decodes to exactly 0, and a padded K
column changes a dot product by exactly 0.0. pack() still refuses a non-aligned tensor without pad.
2. The geometry claim is true of the real images: all 431/431 bonsai2-pb16i-ternary images are whole tiles
(size % 4608 == 0 on every file), which is what makes this a GGUF/odd-width feature rather than a change to the
shipped path — and from_pb16i is not touched by the diff.
3. The fold claim matches the code, line by line. hadamard_rotate (npu/forward/src/q4nx_forward.cpp:126)
multiplies the activation by the signs first, then runs the Sylvester-Walsh butterfly per block, then scales by
1/sqrt(block) — exactly the A' = H (A . s) the new doc text states — and it is called on activations only
(:2598, :2823). The container side it names exists: q4nx_hadamard / q4nx_hadamard_block (:414-415) and the
HAD1 sign table (:2491-2501).
Two nits, neither blocking:
- the new test's comment says "
pad=Truepads 30x300 to the next whole-tile geometry (32x512)" but the fixture is
Kn, Nn = 384, 30, so it is 30x384 → 32x512. The assertion is right; the comment's 300 is stale. inverse_weight_namesis named indocs/npu.mdas living "in the pack", and that identifier appears nowhere in
the public tree (only in this new doc line) — the packer is private. Either name the private file or drop the
identifier; a public doc pointing at a name no public reader can find is the same class as citing a lane state file.
Owed and running, not a blocker: the full 431-image leg (q4nx_t2_test.py --images …, ~6 min). It cannot reach the new
padding code (every image is whole tiles) — it is the unchanged from_pb16i/pack path, and pack's only change for
an aligned tensor is a np.asarray(…, float16) coercion — so I am not holding the verdict on it; I will post the line
when it returns.
|
The 431-image leg returned, green, on the modified converter: So |
|
Persistent review updated to latest commit a8b1963 |
|
Persistent review updated to latest commit 2f76f9b |
|
Persistent review updated to latest commit 41edf86 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 41edf86f1 (head after #816 merged; tprs-ready already on)
Fourth head on this PR, content unchanged and re-checked at it:
- own diff is still
tools/q4nx_t2.py(+49/−8),tests/q4nx_t2_test.py(+29/−1),docs/npu.md(+18), and all three
files are byte-identical to the head I reviewed (3294efd); python3 tests/q4nx_t2_test.py --no-model→PASS q4nx_t2at this head. The 431-image leg is not re-run per head
because the PR's bytes have not changed since the run that produced431/431 bit-exact, 14.44 GB → 6.82 GB— that
result stands for the same tree;- CI on this head:
build,CodeQL, all fourAnalyzejobs andpr_agentsuccess,hw_gateskipped;
mergeable_state=clean.
For the record, the two nits from my first verdict are still open on the head that will merge, both cosmetic:
the test comment says "30x300" where the fixture is 30x384, and docs/npu.md names inverse_weight_names "in the
pack" — an identifier that exists only in that sentence in the public tree.
P2 follow-up to #809 (t2 chunk kind). Two questions were asked on #801 by the consumers, and both
change what a reader has to do, so the answers belong in the spec and not only in a comment thread.
1. A tensor that is not whole tiles is zero-padded (pinned)
K % 256 == 0andN % 32 == 0were stated as geometry and nothing said what to do when a tensor doesnot have it. The pinned convention: pad with code 0 and scale 0.0, i.e. a weight of exactly 0 —
so a padded K column contributes exactly 0 to the reduction whatever the activation is, and a padded N
row is never read. The chunk has no field for the original shape, so the padded shape is the tensor's
shape.
tools/q4nx_t2.py: newpad_to_tile();pack(..., pad=False)still refuses a non-aligned tensor bydefault;
from_ptq1_0/from_tq1_0/from_tq2_0passpadthrough. (from_pb16iis untouched —it is tile-native.)
this exists for GGUF tensors and for a projection whose width is not a multiple of 32.
tests/q4nx_t2_test.pyasserts the convention, not the size: the real block bit-exact after padding,the padding decoding as exactly 0, a padded column adding nothing to a dot product, and the padded
scale slots themselves zero (a padder that leaves them at
1.0is caught — that check was verified bybreaking
pad_to_tilein place).2. The stored weight is the folded (post-rotation) one; t2 carries no rotation
docs/npu.mdnow states it with the code that proves it: a folded container'sW'already has the blockHadamard and the per-width signs baked in, and the consumer rotates the activation into that basis —
A' = H (A . s), signs first, then the normalized Sylvester-Walsh butterfly, then1/sqrt(block)(
npu/forward/src/q4nx_forward.cpp::hadamard_rotate) — never the weight. So the chunk needs nopre/post-rotation flag and no sign vector: the fold stays declared where it belongs, in the container
(
q4nx_hadamard,q4nx_hadamard_block, theHAD1sign table). Reading a t2 tensor in the wrong basis isa consumer bug, not a format one. The inverse direction (the tied embedding,
inverse_weight_names) isS (H y), which round-trips.Only
tools/q4nx_t2.py,tests/q4nx_t2_test.pyanddocs/npu.mdchange; no decoder, no kernel, nocontainer.
Measured gates
The real-model leg is the #809 gate unchanged (431 images, whole tensors, bit-exact against the pb16i
dequantisation); this PR does not touch that path, and it was re-run to confirm the modified converter
still produces it.