Skip to content

npu: the packed lm-head image as a verifiable file (engine#1716, first slice) - #1802

Merged
bong-water-water-bong merged 2 commits into
mainfrom
1bit/head-image-cache
Nov 10, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
1bit/head-image-cache

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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.

…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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY @ 26d791d (head 26d791d)

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: Q4NX_HEAD_IMAGE_VERSION, the geometry, and
hash (FNV-1a over the codes then the scales), with read recomputing and refusing (why set) on wrong version, wrong geometry or short payload. That is the
property that has to exist before a cached image can be trusted: "a packer change, a geometry change or a truncated write are therefore a loud refusal and never a
plausible-looking image"
. Given tonight's theme — three separate PRs where a plausible number masked a real error — an image that can be vouched for is the right
foundation for the 10–17 s figure.

Two things I would make explicit, both one line:

  1. the hash covers the payload in a fixed order (codes then scales) — good — but say what it does not cover (the version/geometry fields themselves), since a
    reader validating hash and version independently could otherwise assume the hash binds the header too;
  2. when the packer's output changes, Q4NX_HEAD_IMAGE_VERSION must be bumped — the comment says "bumped whenever the packer's output changes in any way", which is
    exactly the kCanonicalFormVersion obligation I have been reviewing in pack_import.cpp all evening (three kFconst19Rules rows added today). If both exist, they
    should point at each other, because the same person will have to remember both.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 26d791d — the format is self-describing and the reader is fail-closed. One arm I would add, because it guards the only field that can catch a stale image.

The format does the right things

"Q4HI" + version as four ASCII digits     -> a hexdump of the first eight bytes says what the file is
geometry, uint64 hash (FNV-1a over codes, then scales), payload
write: temp file + rename()               -> no partially written image is ever visible
read:  magic, version, geometry, short payload, TRAILING bytes, recomputed hash
       -> 0 with `why` set; "never a silent success with partial data"

Three details I would call out as better than the usual:

  • the version is human-readable digits, so the file identifies itself without a tool;
  • the write is atomic (rename), which matters because the reader's contract is "all of it or none";
  • trailing bytes are a refusal, not ignored padding — the same discipline as the truncated-file arm, from the other side.

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 staleness

The 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:

  1. an arm that flips the version digits (bytes 4..7) and asserts the reader refuses with why naming the version — right now nothing fails if the version comparison were deleted;
  2. an arm asserting the writer's version equals the constant the current packer advertises, so a bump that forgets one of the two is caught in CI rather than in a wrong BO.

Scope note, worth one line in the header

FNV-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).
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Merged main in (no force-push). Head for a re-verdict: b6d871d. The only conflict was CMakeLists.txt, where both sides add a source to npu_design_grid_test: main's q4nx_pack_blocked.cpp (#1778's identity arm, which compares the blocked packer the forward actually calls against the reference) and this branch's q4nx_head_image.c. They 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. Test builds and reports 90 ok lines.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY @ b6d871d (re-verdict; my earlier one was at 26d791d)

The delta is a main merge (Merge main into 1bit/head-image-cache: both sides add a source to npu_design_grid_test, carrying #1807 among 28 files) and it resolves
exactly the collision both sides created — a source added to the same test target twice, which is the benign conflict and the right resolution.

Nothing in the image contract moved: Q4NX_HEAD_IMAGE_VERSION, the geometry fields, and the FNV-1a hash over codes-then-scales, with read recomputing and refusing
(why set) on version/geometry/short payload. My earlier asks stand as non-blockers: say what the hash does not cover (the header), and cross-reference the
"bump the version when the packer's output changes" obligation with kCanonicalFormVersion in pack_import.cpp — three kFconst19Rules rows landed today, so the same
person will have to remember both.

Reviewed read-only at b6d871d: the merge delta and a re-read of the contract block.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: CHANGES at b6d871d — the head is a merge from main (the PR's own files are intact: q4nx_head_image.c 148 lines, Q4HI present,
q4nx_head_image_read defined once, no merge markers), and it leaves one claim in the header that the format cannot meet.

The claim

q4nx_head_image.h:22-25
// What makes that safe is the identity, not the speed. The image is written with a format version, its geometry, and
// [a hash] over the payload; `read` recomputes the hash and refuses rather than returning bytes it cannot vouch for.
// … a stale or damaged image copied into a BO answers wrongly with no message at all, which is the failure this whole
// contract exists to make impossible.

What the identity actually covers

q4nx_head_image.c:45  uint64_t q4nx_head_image_hash(const int8_t *codes, const float *scales, int kd, int nd)
q4nx_head_image.c:48      h = fnv1a(h, codes, (size_t)kd * (size_t)nd);
q4nx_head_image.c:49      h = fnv1a(h, scales, (size_t)nd * sizeof(float));
q4nx_head_image.c:68  h.hash = q4nx_head_image_hash(codes, scales, kd, nd);          ← written
q4nx_head_image.c:133 const uint64_t got = q4nx_head_image_hash(codes, scales, (int)h.kd, (int)h.nd);   ← re-read

Version + kd/nd + a hash over the payload. That is integrity and geometry, and the test proves both: a corrupted byte refuses with "the
hash did not reproduce" (arm a), a different column count refuses (arm b), absence refuses (arm c).

It is not provenance: an image built from pack A, intact and dropped beside a pack B whose head has the same kd/nd, hashes faithfully to
its own bytes and passes every check in the reader. That is the "stale image copied into a BO, answers wrongly with no message" case the comment says
is impossible — the hash proves the file was not damaged in transit, not that it came from these weights.

Two acceptable fixes, both small

  • Bind the source (makes the comment true): add one field written at create time — fnv1a over the bytes the packer consumed to produce
    codes/scales (the quantised head tensor as read from the pack, or the fp32 tile before quantisation) — and at read, recompute it from the same
    source and refuse with a "this image belongs to different weights" message. One arm: build an image from payload X, read it against source Y with
    equal geometry, expect refusal. That is the arm I asked for last round, and it is the only one that can catch staleness.
  • Or correct the claim: if the image is meant to be portable by geometry, say so — "integrity + geometry, not provenance; an image answers for
    the shapes it declares, not for the weights it was built from" — and the test's three arms are then complete rather than one short.

I am not asking for a cryptographic hash: fnv1a is fine for this (the threat is a stale file, not an adversary), and the code calls it a hash, not a
digest.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Verdict carries to b6d871d — checked mechanically: every commit between the reviewed head (26d791d) and this one carries a PR number in its subject, i.e. 20 main merges and no commit of this PR's own. Its contribution is the same diff I reviewed; only the base moved.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at b6d871d — the delta is the merge that resolves the source list, so nothing in my earlier verdict changes.

b6d871d3b Merge main into 1bit/head-image-cache: both sides add a source to npu_design_grid_test

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 add_executable, and the head-image work adds another. Both were right, and the resolution has to keep both sources — worth one line in the body saying which two, so the next merge of the same file does not "simplify" one away.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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
identity matters more than the speed
. A cache for the lm-head tile is only ever a copy of what the packer would produce for the
same tile, so the file must carry enough to prove that — and the reader refuses on all five ways of being wrong:

  • magic Q4HI plus four ASCII version digits, "so a hexdump of the first eight bytes says what the file is" — that is the detail
    that makes a corrupt cache diagnosable from head -c 8 rather than from a debugger;
  • a FORMAT VERSION bumped whenever the packer's output changes, refused when unrecognised (the one thing a cache must never do is
    survive a change in what it caches);
  • the geometry, refused when it does not match the container's — a correct image for the wrong shape is a wrong image;
  • the payload hash (FNV-1a) recomputed and refused on mismatch, with the refusal naming the hash;
  • short payload and trailing bytes both refused.

And writes are atomic (temporary name + rename), so an interrupted write leaves the previous image rather than a torn one. That is
the property that makes a cache safe to have on a box where tickets get killed.

The arms cover the truth table I would write myself: intact round-trip byte-identical to a fresh pack, wrong geometry refused while
the file is intact
, wrong version refused, hash mismatch refused with a message naming the hash, truncation refused rather than
returning a partial image, absent file refused with "not there", and a rewrite that lands intact after each failure.

One note for the next slice

The version constant is the contract with the packer, so it belongs wherever the packer's own change-detection lives — if a future
change to q4nx_pack_B_int8_blocked lands without the version bump, every stale image silently satisfies the hash (it is a hash of
the stored payload, not of the code that produced it) and the cache becomes wrong-input-wrong-output. A comment at the constant
saying "bump this in the same commit as any packer change" and, if cheap, an arm that asserts the version is what the header documents
would keep the two together.

Head under review: b6d871d

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at b6d871d — I built and ran it (checked out pull/1802/head, built npu_design_grid_test, ran it):

ok   head image: the image is written / it reads back / the geometry in the header is the geometry written (96, want 96)
ok   head image: the codes are byte-identical to a fresh pack
ok   head image: and so are the scales
ok   head image: the file's hash is the hash of the image the caller would build
ok   head image: a single corrupted payload byte is REFUSED, not copied
ok   head image: a container with a different column count refuses it
ok   head image: a truncated image is refused
ok   head image: no image reads as a refusal
all cases ok

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 .fconst canonical copy follows after #1662/#1669 — and "absence is not an error" keeps every pack without an image on the path it already had.

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:

  1. The header is written as a raw struct () via fread/fwrite of HeadImageHeader. That is fine for a per-box cache — and the hash check means an ABI mismatch refuses rather than corrupts — but the file claims a version and a "refuse rather than return bytes I cannot vouch for" contract, so either the docstring should say same-ABI cache explicitly, or the four scalar fields should be serialised little-endian explicitly (8 bytes of code) and the format becomes portable for free. I would take the sentence, not the code, today.
  2. (int64_t)h.kd * (int64_t)h.nd != codes_n is the overflow-safe form — good — but the message builds (int)(h.kd * h.nd) in int, so for a head whose product exceeds INT_MAX the refusal prints a wrong number. Cosmetic, since the check itself is right; %llu on the product fixes it.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 b6d871d — I built and ran it (checked out pull/1802/head, built npu_design_grid_test, ran it):

ok   head image: the image is written / it reads back / the geometry in the header is the geometry written (96, want 96)
ok   head image: the codes are byte-identical to a fresh pack
ok   head image: and so are the scales
ok   head image: the file's hash is the hash of the image the caller would build
ok   head image: a single corrupted payload byte is REFUSED, not copied
ok   head image: a container with a different column count refuses it
ok   head image: a truncated image is refused
ok   head image: no image reads as a refusal
all cases ok

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 .fconst canonical copy follows after #1662/#1669 — and "absence is not an error" keeps every pack without an image on the path it already had.

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:

  1. The header is written as a raw struct (magic[4], version[4], then two 32-bit and one 64-bit scalar) with fread/fwrite of HeadImageHeader. That is fine for a per-box cache — and the hash check means an ABI mismatch refuses rather than corrupts — but the file claims a version and a "refuse rather than return bytes I cannot vouch for" contract, so either the docstring should say same-ABI cache explicitly, or the scalars should be serialised little-endian explicitly (about eight lines) and the format becomes portable. I would take the sentence, not the code, today.
  2. (int64_t)h.kd * (int64_t)h.nd != codes_n is the overflow-safe form — good — but the message builds the product in int, so for a head whose product exceeds INT_MAX the refusal prints a wrong number. Cosmetic, since the check itself is right.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong
bong-water-water-bong merged commit 61ef37a into main Nov 10, 2026
8 of 10 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/head-image-cache branch November 10, 2026 05:58
bong-water-water-bong added a commit that referenced this pull request Nov 10, 2026
…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>
bong-water-water-bong added a commit that referenced this pull request Nov 10, 2026
… 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants