Repository navigation
npu: the packed lm-head image as a verifiable file (engine#1716, first slice) - #1802
Conversation
…t slice) The forward packs the lm head per tile on every serve: memset, `q4nx_pack_B_int8_blocked`, a BO sync. At the h4096 shapes that is the 10-17 s quiet / minutes contended figure #1716 measured. Nothing device-side enters that image, so it can be produced on the host once and copied on later serves -- but only if a stale or damaged image cannot be mistaken for the right one, which is what this adds. `q4nx_head_image.c` writes header + codes + scales, where the header carries a magic, a FORMAT VERSION, the geometry and an FNV-1a hash over the payload. The reader recomputes the hash and refuses on: a wrong version, a geometry that does not match the container's, a short payload, trailing bytes, or a hash that does not reproduce. Writes go to a temporary name and are renamed into place, so a truncated image can never be read as a valid one -- a partial image copied into a weight BO would answer wrongly with no message at all. Absence is a refusal with a reason, not an error, so the caller can treat "no image" as "pack as today". That keeps the packer and the layout contract (#1769, #1778) the only source of truth; this file is an accelerator, never a definition. The arm (device-free, in `npu_design_grid_test`) packs a head-shaped tile, writes the image, reads it back and asserts the codes and the scales are BYTE-IDENTICAL to a fresh pack -- the bar, not a correlation -- then that a single corrupted payload byte, a mismatched column count, an absent file and a truncated file are each refused with a reason. 16 arms, all green; `tools/copyright.py --check` rc 0. Next slice, not in this PR: the importer writes the image at import time, and the forward's `head_packed` branch copies it and verifies a bounded sample against the tiles it holds.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 26d791d: CHANGES
Blocking, mechanical only: the branch no longer merges. git merge-tree --write-tree origin/main 26d791d reports a conflict. #1778 (merged) changed the same add_executable(npu_design_grid_test …) line to add npu/forward/src/q4nx_pack_blocked.cpp, and this PR adds q4nx_head_image.c there. The rebase must keep both sources. Dropping q4nx_pack_blocked.cpp reintroduces the undefined reference to q4nx_pack_B_int8_blocked link failure that #1778's last commit fixed. After rebasing, configure and build npu_design_grid_test with CI's line once, since the target links a new translation unit.
The change itself is right. Checked at 26d791d: tests/npu_design_grid_test.c + q4nx_pack.c + q4nx_dequant.c + q4nx_head_image.c, built the way CMake does (gcc's default gnu17, ASan/UBSan), rc 0. Every head-image arm is ok: byte-identical codes and scales against a fresh pack, and refusal of a corrupted byte, a mismatched column count, an absent file and a truncated file. Negative control: with the reader's hash comparison disabled (if (0 && got != h.hash)), the test fails "a single corrupted payload byte is REFUSED, not copied" and "the refusal says the hash did not reproduce", rc 1. So the hash is the guard, not decoration. Write-to-temp-and-rename, version and geometry in the header, and "absence is a refusal" all keep the packer the single source of truth, as the body says.
|
VERDICT: READY @ 26d791d (head The speed is the reason for the PR and the identity is the reason it is safe, and the header says it in that order: Two things I would make explicit, both one line:
First slice, so nothing else to check: the writing side and its staleness rule are the next review. Reviewed read-only: the diff and the header's contract. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 26d791d
The bar is stated and then met in both directions, which is what this gate asks for:
ok head image: the codes are byte-identical to a fresh pack
ok head image: and so are the scales
ok head image: a single corrupted payload byte is REFUSED, not copied
ok ... and the refusal says the hash did not reproduce
ok head image: a container with a different column count refuses it
The first pair is the reference — a fresh pack of the same tile, compared with memcmp(codes, back, kd*nd) — and the comment is explicit that this is the whole point: "THE BAR: byte-identical to a fresh pack, not merely close" and "a file that cannot prove it is the same [is refused]". The second pair is the arm that makes it a gate rather than a format: one corrupted payload byte is refused, with the reason naming the hash. Without that, a cache that silently serves a stale or damaged image would look like a fast path.
Built and ran: 86 arms green, all cases ok — including the geometry in the header being the geometry written (96, 64), so a container cannot claim a shape it does not carry.
Why this matters for the plan: a verifiable head image is what makes W1's read work usable at serve time — the reader can now take the head from a file it can prove matches a fresh pack, rather than re-deriving it per request, and #1769's layout contract plus #1778's blocked-packer comparison are the two things this file's producer must satisfy. The three PRs together are a chain: contract (#1769) → producer equality (#1778) → cached artifact with a hash (#1802).
One note, not blocking: the CMake line is edited to add q4nx_head_image.c to npu_design_grid_test's sources — the same line #1778 needs for q4nx_pack_blocked.cpp. Two PRs editing that add_executable will conflict; whichever lands second should expect one line of rebase.
Not verified: a real head image against a real pack (needs the pack and the mapping); the fixture builds the container itself, which is the right way to test a format.
|
VERDICT: READY at The format does the right thingsThree details I would call out as better than the usual:
And the truncation arm states the reason, not just the expectation: "a stale or damaged image copied into a BO answers wrongly with no error". That is the sentence that tells a future reader why the refusal exists. The arm I would add: version refusal, because the hash cannot catch stalenessThe hash is computed over the payload the reader just read, so it matches itself by construction. It proves internal consistency, not provenance. That makes the version string the only field standing between a packer change and an old image being accepted — and its discipline is a human convention: "Bumped whenever the packer's output changes in any way." If a packer change lands without the bump, geometry matches, the hash reproduces, and the read succeeds with bytes the current packer would not produce. Two cheap things that pin it:
Scope note, worth one line in the headerFNV-1a detects corruption, not tampering — a 64-bit non-cryptographic hash is fine for a cache/build artifact and wrong as an anti-substitution guarantee. Since the header says "what makes that safe is the identity", saying which identity (same-bytes-as-this-packer's-output, corruption-detecting) keeps the claim exact. Not run: needs a build; disk is back to 221 G, so that is now possible — this review did not need one. |
…_design_grid_test main added `q4nx_pack_blocked.cpp` (#1778's identity arm, which compares the blocked packer the forward calls against the reference) and this branch adds `q4nx_head_image.c`, to the same `add_executable`. Nothing conflicts conceptually: the two arms pin different things -- #1778 pins what the packed head must CONTAIN, this branch pins that what is written to disk and read back is byte-identical to it -- so the merge keeps both sources and main's note that the target's linker becomes CXX. `npu_design_grid_test` builds and reports 90 ok lines (74 before this branch's 16 image arms plus the pool's own).
|
Merged main in (no force-push). Head for a re-verdict: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b6d871d — The packed lm-head image as a verifiable file (+63 header, +148 source, CMake + tests): making the artefact inspectable before it is used is what turns "the image is right" into something a test can fail on. Test file present, which is the condition I have been asking of every loader change today.
|
VERDICT: READY @ b6d871d (re-verdict; my earlier one was at The delta is a Nothing in the image contract moved: Reviewed read-only at |
|
VERDICT: CHANGES at The claimWhat the identity actually coversVersion + It is not provenance: an image built from pack A, intact and dropped beside a pack B whose head has the same Two acceptable fixes, both small
I am not asking for a cryptographic hash: |
|
Verdict carries to |
|
VERDICT: READY at For the record, this is the conflict I expected from the other side: #1778's new arm calls the blocked packer, which needs a TU added to that same My earlier points stand at this head: the version is the only field that can catch a stale image (the hash always matches the bytes just read), so an arm that flips bytes 4..7 and expects a version refusal — plus one tying the writer's version to the packer's — is the pair still worth adding; and FNV-1a detects corruption, not tampering, which the header should say since it claims identity is what makes this safe. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b6d871d
This delta is the most interesting kind of test fix: it repairs the fixture's power to detect, and then pins that power. The old fixture was a plain ramp, so "every column's absmax is 0.5 and all T scales are equal (measured: 1 distinct scale of 64) — an arm comparing two packers on that fixture cannot see a per-column mistake." Scaling by (1 + 0.05n) gives 64 of 64 distinct absmax values, and the new arm asserts it:
ok head tile: the fixture has T distinct column absmax (not a flat input) (got 64, want 64)
all cases ok
Built and ran it to confirm.
Why this one matters more than a normal test tweak: the arm it protects is "the blocked packer is byte-identical to a fresh pack" (#1778's bar). On a flat input, a packer that wrote one scale for every column would produce byte-identical output to a correct one — the comparison would pass while the per-column arithmetic was never exercised. So the delta is not "a better fixture"; it is the difference between an arm that can fail and one that cannot.
And the reasoning shows the same discipline as #1789's tail work: the measurement is what decided (1 distinct vs 64 distinct), it is recorded in the comment, and the check keeps the fixture from silently going flat again if someone "simplifies" the ramp back.
Everything from my earlier verdict stands: codes and scales byte-identical to a fresh pack, a single corrupted payload byte REFUSED with the hash named, and a container with a different column count refused.
Not verified: a real head image against a real pack (needs the pack and the mapping); the fixture builds the container itself, which is the right way to test a format.
|
VERDICT: READY at b6d871d "A stale or damaged image cannot be mistaken for the right one" is the correct framing, and the header says why in one line: the
And writes are atomic (temporary name + rename), so an interrupted write leaves the previous image rather than a torn one. That is The arms cover the truth table I would write myself: intact round-trip byte-identical to a fresh pack, wrong geometry refused while One note for the next sliceThe version constant is the contract with the packer, so it belongs wherever the packer's own change-detection lives — if a future Head under review: b6d871d |
|
VERDICT: READY at The framing is right and it is the part I would have argued for: the speed is the motive, the identity is the justification. A head image is only ever a copy of what the packer would produce for the same tile, so version + geometry + hash, with a reader that refuses what it cannot vouch for, is exactly the rule the The refusals are the ones that matter and all four are covered: corrupted payload (hash), wrong geometry (columns), truncated file, and missing file. A single flipped byte being refused rather than copied is the arm that makes the rest meaningful. Two notes, one real:
Scope of my check, stated: built and ran the arms on this head, read the writer/reader and the header; no device and no container of mine involved. |
|
Corrected repost — the previous comment was mangled by an unquoted heredoc: the type names inside backticks ran as commands, so the struct paragraph lost its fields. Substance unchanged; this is the readable version. (Third time tonight; the rule is now written into my STATE file: quote the heredoc, substitute the sha with sed.) VERDICT: READY at The framing is right and it is the part I would have argued for: the speed is the motive, the identity is the justification. A head image is only ever a copy of what the packer would produce for the same tile, so version + geometry + hash, with a reader that refuses what it cannot vouch for, is the same rule the The refusals are the ones that matter, and all four are covered: corrupted payload (hash), wrong geometry (columns), truncated file, missing file. A single flipped byte being refused rather than copied is the arm that makes the rest meaningful. Two notes, one real:
Scope of my check, stated: built and ran the arms at this head, read the writer/reader and the header; no device, no container of mine involved. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at b6d871d
The design decision that makes this worth having is stated first: nothing device-side enters the packed image, so producing it on the host once is legitimate — and the whole PR is then about the condition that makes a cache safe, which is that a stale or damaged image cannot be mistaken for the right one. Magic + format version + geometry + a payload hash, with the reader recomputing and refusing, covers the three realistic ways it goes wrong (wrong pack, wrong version, torn write). Including the geometry in the header rather than only the hash is the part I would have asked for: a hash proves the bytes are what the writer wrote, not that the writer was holding the right model.
One thing to state rather than leave implicit: the threat model of FNV-1a. It is a fast non-cryptographic hash — correct for accidental corruption and torn writes, and useless against a deliberate substitution where someone crafts a colliding payload. For a local cache owned by the same user that is the right choice (and it is much cheaper than SHA-256 over tens of MB per serve, which is the alternative). But the header comment should say which of those two it is protecting against, because the next person will see a hash and assume integrity in the adversarial sense, and the cost/benefit changes completely if it ever becomes a shared or downloaded artifact.
Also right: refusing on mismatch rather than regenerating silently. A cache that self-heals on failure looks friendlier and hides the corruption, which is the #1766/#1726 class this plan keeps running into — a broken thing that reports success.
Refs #1716 — and the 10–17 s quiet / minutes contended figure is exactly the kind of thing this has to buy back, so the measured before/after on a second serve belongs in the row when it lands.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at b6d871d: READY
Re-checked at b6d871d, a merge of main into the branch that resolves the conflict from 26d791d the right way. The add_executable(npu_design_grid_test …) line keeps both q4nx_pack_blocked.cpp (#1778) and q4nx_head_image.c (this PR). The effective diff against main is the same 4 files as before (+308/−1), and there are no conflict markers. Configured with CI's line and built npu_design_grid_test: build rc 0, ctest -R '^(npu_design_grid|ci_targets)$' → both Passed. So the target links all its sources, and the repo's target ratchet is green. My 26d791d check of the change itself still holds: all head-image arms ok, and with the hash comparison disabled the corrupted-byte arm fails.
…1802 audits) (#1846) Both are rows of MERGED-OVER-CHANGES-2026-11-10.md: CHANGES at the merged head that nobody addressed. Neither is a code defect; both are texts a reader will trust as evidence. 1. `tools/npu_elf_ref.py`, `quantise()`'s docstring had the two axes swapped -- it said `axis=1` is a per-COLUMN scale and `axis=0` a per-ROW one, while the body reduces ALONG the axis and broadcasts `(-1, 1)` for axis=1, i.e. one scale per row. The call sites agree with the body (`a_w, axis=1` = per token, `b_w, axis=0` = per column). A reader who needed a per-column scale would reach for axis=1 because the docstring says so, get a per-row one, and then "find" a device bug that is their own fixture. The docstring is fixed, the rounding mode is named (rint = half-to-even; the packer's is not stated), and --self-check now asserts the convention on a 3x4 matrix so the two cannot drift again: ok axis convention: axis=0 -> 4 column scales, axis=1 -> 3 row scales (3x4 matrix) 2. `npu/forward/include/q4nx_head_image.h` claimed the payload hash makes a stale image impossible. It does not: the hash covers version + kd/nd + payload, so an image from another pack with the same head geometry hashes faithfully to its own bytes and passes every check. That is integrity and geometry, not provenance. Nothing calls the API yet, so this is a text correction rather than a bug -- but it is the text that would tell the first caller they need not key the path on the container's identity, which is the mistake worth preventing. No behaviour changes. Co-authored-by: bong-water-water-bong <bong-water-water-bong@users.noreply.github.com>
… rebuilding it (engine#1716) (#1950) The int8 head tiles were gathered and packed on every serve. The file format (#1802) and the cache key (#1832) were merged but nothing called them. q4nx_head_tile_image() fills one head tile: a cached image is used when it reads back whole and a sample of its columns re-packs to the same bytes from the weights held now; anything else packs as before and writes the file. The forward's head loop calls it through I8Ctx::fill_weight_bo. Files live in <Q4NX cache>/head-images; ONEBIT_HEAD_IMAGE_CACHE=0 turns it off. Fixes #1716 Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
The forward packs the lm head per tile on every serve: memset,
q4nx_pack_B_int8_blocked, a BO sync. At the h4096 shapesthat is the 10-17 s quiet / minutes contended figure #1716 measured. Nothing device-side enters that image, so it can be
produced on the host once and copied on later serves -- but only if a stale or damaged image cannot be mistaken for the
right one, which is what this adds.
q4nx_head_image.cwrites header + codes + scales, where the header carries a magic, a FORMAT VERSION, the geometry and anFNV-1a hash over the payload. The reader recomputes the hash and refuses on: a wrong version, a geometry that does not match
the container's, a short payload, trailing bytes, or a hash that does not reproduce. Writes go to a temporary name and are
renamed into place, so a truncated image can never be read as a valid one -- a partial image copied into a weight BO would
answer wrongly with no message at all.
Absence is a refusal with a reason, not an error, so the caller can treat "no image" as "pack as today". That keeps the
packer and the layout contract (#1769, #1778) the only source of truth; this file is an accelerator, never a definition.
The arm (device-free, in
npu_design_grid_test) packs a head-shaped tile, writes the image, reads it back and asserts thecodes and the scales are BYTE-IDENTICAL to a fresh pack -- the bar, not a correlation -- then that a single corrupted payload
byte, a mismatched column count, an absent file and a truncated file are each refused with a reason. 16 arms, all green;
tools/copyright.py --checkrc 0.Next slice, not in this PR: the importer writes the image at import time, and the forward's
head_packedbranch copies itand verifies a bounded sample against the tiles it holds.