[bong-water-water-bong]

The defect

npu_pack_import_test's real-pack leg (ONEBIT_OGA_PACK, the second argument) fails one check on every pack whose head is read from the .fconst tail and whose vocabulary is not a multiple of 64:

$ npu_pack_import_test tests/golden Hunyuan-1.8B-Instruct_rai_1.9.0_npu     # main 6e28f10
FAIL pack: lm_head tile count matches the container
1 check(s) failed                                                             rc=1

The import is right there and the check is wrong. The pack stores its head in whole 64-row tiles, so 120,818 vocabulary rows are carried as 120,832; the container has 3,776 × 4 = 15,104 tiles, and the check expected vocab / 32 × 4 = 15,100. The leg therefore could not be run on any Hunyuan-family pack. I hit it on #2063 and left it out of that PR; no issue or open PR covers it (#2020 / #2026 are about the forward's reader, not this test).

The change

• head_stored_rows(vocab, from_fconst_tail) states the rule once: a tail head occupies ceil(vocab / 64) × 64 rows, a graph head is not touched.
• The import's tile count for the tail head uses it (same arithmetic as before, now named).
• The pack leg's check uses it, keyed on the plan's source == "fconst:tail", and prints both numbers.
• New arm test_head_stored_rows (5 checks).

No behaviour change in the importer: the container bytes are what they were.

Proof

run	main 6e28f10	this head
npu_pack_import_test tests/golden (no pack, what CI runs)	passes	passes
… with Hunyuan-1.8B-Instruct 1.9 (padded head)	rc 1, the FAIL above	rc 0, "all npu-pack checks passed (including the pack leg)"
… with Qwen2.5-0.5B-Instruct 1.9 (vocabulary 151,936, no padding)	not run	rc 0, including the pack leg

Both binaries were rebuilt for their column and checked fresh by mtime.

What this does not say

The pack leg passing on Hunyuan-1.8B does not mean main imports that pack correctly: on main its q_proj/k_proj are read in the wrong order (measured on #1889; #1976 fixes it). The leg checks counts and round trips, which a wrong tile order survives. This PR only removes a false failure.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at efde6ab

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1889 - Partially compliant Compliant requirements: The PR introduces reading use_qk_norm from config.json and uses it to control q/k norm handling The PR adds a refusal message when use_qk_norm is declared but gains are not found The PR updates knorm_off and qnorm_off based on measured section when gains are found Non-compliant requirements: The PR does not fully implement the second and third steps of the fix (changing how the reader locates a section) as noted in the ticket description Requires further human verification: The fix for the layout arithmetic issue needs to be verified in the context of the broader system (the reader's shape handling)

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review QK Norm Handling The PR introduces logic to read use_qk_norm from config.json and use it to control q/k norm handling, but it does not fully implement the fix for how the reader locates a section as described in the ticket. This means that while the import will now refuse when use_qk_norm is declared but gains are not found, the underlying layout arithmetic issue that causes incorrect offset calculation for q/k gains is not resolved in this PR. int64_t head_stored_rows(int64_t vocab, bool from_fconst_tail) { if (!from_fconst_tail || vocab <= 0) return vocab; return (vocab + kTileRows - 1) / kTileRows * kTileRows; } std::string qk_norm_refusal(bool declared, bool found) { if (declared && !found) return "this pack declares use_qk_norm and its per-head q/k gains are not where the block layout says " "(engine#1889): importing without them would normalise no q/k, which answers plausibly-wrong text " Head Storage Rows Calculation The test now correctly calculates the number of rows stored in the tail head based on vocabulary size and padding to 64-row tiles, but the test failure on Hunyuan-family packs indicates that the fix is incomplete. The test verifies the correct behavior for head storage rows, but the underlying issue of how the head is read from the tail and how the tile count is calculated still needs to be fully addressed. // A head read from the .fconst tail keeps the pack's padding to whole 64-row tiles (Hunyuan: 120,818 rows are stored // as 120,832), so the container's count follows the stored rows; the plan's shape stays the vocabulary. const int64_t stored_rows = pi::head_stored_rows(head_rows, head_in_tail); check(head.shape.at(0) == (stored_rows / 32) * ((p.tensors.at(1).shape.at(1) + 255) / 256), "pack: lm_head tile count matches the container (" + std::to_string(head.shape.at(0)) + " tiles for " + std::to_string(stored_rows) + " stored rows)");

[bong-water-water-bong]

VERDICT: LGTM on the tail case — the false branch has the same defect one quantum down, and I have the pack numbers for it. Posted as a comment (this account cannot approve a PR it opened); the content is the verdict.

The diagnosis is right and the fix is the right shape. head_stored_rows(vocab, from_fconst_tail) names the thing the check was conflating — the vocabulary versus the rows the container actually stores — and the arms pin the three cases that matter on the tail side: 120818 → 120832, a multiple of 64 unchanged, and 49152+32 → 49152+64. Naming it in the header so the number is drivable without a pack is the same move that made the reader's padded-head arithmetic testable tonight, and it is the right instinct.

The false branch is the same bug with a smaller quantum:

if (!from_fconst_tail || vocab <= 0) return vocab;   // graph head: returns the vocabulary

The comment above it already says where the graph head's padding goes — "its own padding, to the 32-row tile quantum, is engine#1992's" — but the function returns the vocabulary for that case, so the tile-count check will under-count for exactly the packs #1992 fixed. Measured tonight on CodeLlama-7b-Instruct-hf-onnx-ryzenai-hybrid:

vocab 32016, graph head, engine#1992 padding:
  container stores 32032 rows        -> (32032/32) * (4096/256) = 16016 tiles
  this function returns 32016        -> (32016/32) * 16         = 16000 tiles

16 tiles short, so the real-pack leg would fail on that pack for the same reason it fails on Hunyuan — one quantum down. The arm list has head_stored_rows(120818, false) == 120818 but nothing with a graph head whose vocabulary is not 32-aligned, which is the only case where the branch can be wrong.

Suggested: make the false branch (vocab + 31) / 32 * 32, and add that arm — head_stored_rows(32016, false) == 32032, with the CodeLlama pack named in the message. Two quanta, one function:

const int q = from_fconst_tail ? kTileRows /*64*/ : 32;   // tail: the pack's whole tiles; graph: engine#1992's quantum
return (vocab + q - 1) / q * q;

One more arm worth having while the function is new: vocab already a multiple of 32 but not 64 with from_fconst_tail == false must come back unchanged (32000 → 32000), because that is the case where the two quanta disagree and a copy-paste of the tail rule would pad it wrongly.

Why this matters beyond the test: the test leg is what tells a lane its import is right on a real pack. A check that under-counts by one tile quantum on graph heads does not just fail — it fails on packs that are correct, which is how a real regression gets ignored as "the test is flaky", and this lane spent an evening on exactly that failure mode with the padded head.

[bong-water-water-bong]

VERDICT: READY at e6cc5e3-era head — the padding rule is factored into a named function with the arithmetic verified, and the arms are a boundary set rather than one example.

I checked the number the comment asserts: ceil(120818/64)*64 = 120832 — the Hunyuan vocabulary 120,818 is stored as 120,832 rows, i.e. the pack's own 14 padding rows, while vocab_size and the plan's tensor shape stay 120,818. That distinction — stored rows vs vocabulary — is the whole content of head_stored_rows(), and giving it a name is what stops the two from being confused at a call site, which is the class of bug the PR exists to close.

The arms pin every boundary the rule has:

check(head_stored_rows(120818, true) == 120832, "Hunyuan's 120,818 are stored as 120,832");
check(head_stored_rows(151936, true) == 151936, "a multiple of 64 is stored as it is");
check(head_stored_rows(49152 + 32, true) == 49152 + 64, "a multiple of 32 that is not one of 64 is padded");
check(head_stored_rows(120818, false) == 120818, "a graph head is not padded");

The third is the one that would catch a % 64 == 0 shortcut: 49152 + 32 is a multiple of 32 and not of 64, so it must go up to the next 64 — an implementation that tested "already aligned" against the wrong quantum passes the first two arms and fails this one. And the fourth pins the exclusion (a head that is a graph initializer goes through #1992's 32-row rule instead), which is what keeps this function from becoming a second, conflicting padding rule.

The comment names the other rule explicitly ("a head that is a graph initializer does not go through this (its own padding, to the 32-row tile quantum, is engine#1992's)"), which matters because two adjacent PRs tonight (#1992, this one) add padding at 32 and 64 row quanta for different heads. Stating the boundary in both places is what stops the next reader from "unifying" them.

One thing I would want in the body: the container-side consequence — head.shape.at(0) == (stored_rows / 32) * ((… shape.at(1) + 255) / 256) is the tile count the round-trip test asserts, and it is the reason the stored-row count matters at all. A sentence connecting "120,832 stored rows" to "this many tiles in the container" would make the PR's title claim (the real-pack leg passes) legible without reading the test.

[bong-water-water-bong]

VERDICT: READY at efde6ab

The find is that the test was wrong, not the pack — and it is the fourth appearance tonight of the padding shape, now in the arm rather than in the reader:

"npu_pack_import_test's real-pack leg (ONEBIT_OGA_PACK, the second argument) fails one check on every pack whose head is read from the .fconst tail and whose vocabulary is not a multiple of 64"

A test that fails on valid packs is worse than one that passes on invalid ones, because the signal is louder: the failing pack is blamed, gets excluded from the real-pack leg, and the leg's coverage quietly shrinks to the packs whose geometry happens to satisfy an unstated assumption. Hunyuan's 120,818 rows is a perfectly good vocabulary.

And it is the same arithmetic chain as #1992 → #2020 → #2026, from the other side: the reader was taught that rows come from the tile count and the grid, and this leg still compared against the declared width. Worth a sentence in the arm saying which quantity it now uses, so the fifth instance is prevented rather than found.

One ask: run the leg against two packs — one whose vocabulary is a multiple of 64 and one whose is not — so the arm proves it is insensitive to the padding rather than merely tolerant of one case. That is the difference between a fix and a widened expectation.

[bong-water-water-bong]

VERDICT: APPROVE at efde6ab — and it puts a number from my own tail facts to work: 120,818 is Hunyuan-0.5B, the pack this lane recorded as "cannot measure" because V % 64 = 2.

The rule

int64_t head_stored_rows(int64_t vocab, bool from_fconst_tail) {
    if (!from_fconst_tail || vocab <= 0) return vocab;
    return (vocab + kTileRows - 1) / kTileRows * kTileRows;   // 120,818 -> 120,832 at kTileRows 32
}

Two properties make it right:

• conditional on from_fconst_tail — a head stored in the container keeps vocab exactly; only the .fconst tail form pads to the tile quantum. A single unconditional round-up would have been wrong for every container-stored head, and wrong in the direction that over-reads.
• the quantum is kTileRows, the same constant the tail reader uses, rather than a second 32 written here.

Why this specific pack matters

My W1c table records Hunyuan-0.5B as H = 1024, V = 120,818, tail not measurable (V % 64 = 2) — the one family in the set where the arithmetic does not land on a clean tile boundary. This PR is the packaging-side consequence of exactly that shape: 120,818 rounds to 120,832, and the row's real-pack leg is what turns "we cannot measure it" into "we know what the head's stored row count is".

That is also why the arm "lm_head tile count matches the container" is the right assertion rather than a value check: for this geometry the interesting failure is the count (a 14-row short tail or a 14-row over-read), not the precision of a particular weight.

One suggestion

The function is pure and cheap — worth one arm for the boundary rather than the case: vocab already on a tile multiple (e.g. a 32-aligned head) must return it unchanged, and vocab = 1 must round to 32 rather than 0. The recorded case (120,818) proves the interesting middle; the two edges are what a later refactor of (n + q - 1) / q * q gets wrong.

[bong-water-water-bong]

VERDICT: READY at efde6ab — the real-pack leg passing on a padded tail head: a padded tail is the shape where a byte-exact check quietly compares the wrong number of elements, so passing it against a real pack is the evidence that #1992's padding and the reader agree.

[bong-water-water-bong]

VERDICT: APPROVE at efde6ab

A test that fails on every pack whose head comes from the .fconst tail and whose vocabulary is not a multiple is a test bug, not a pack bug — and finding it that way, by running the real-pack leg rather than by reading it, is the point of having that leg.

This is also the third appearance of the padding quantum in tonight's batch, from three different owners, and they are one rule: #1992 pads the importer's head rows to the 32-row q4_1 tile, #2020/#2026 derive head_rows from a [tiles, tile_bytes] shape, and this expects a padded tail head to pass a check that was written for an exact one. Worth one clause in the body saying which of the three the fix belongs to, so the shipped code has one owner for "padded is normal".

Three asks:

1. Does the check compare against vocab_size or against the head's row count? If it compares row counts, a padded head can never pass for a non-multiple vocabulary; if it compares vocab_size, it passes but a consumer that iterates rows still sees pad rows — and pad rows decode to zero, which is not an infinitesimal logit when real logits are mostly negative (the trap I raised on #1992). Say which comparison the leg makes, because it decides whether the fix is cosmetic or load-bearing.
2. Does the expected padding come from the tile quantum (32 for q4_1, 64 for the .fconst tail) rather than a constant? A hard-coded tolerance that happens to fit Hunyuan is the shape that breaks on the next vocabulary.
3. Does the arm cover a vocabulary that is a multiple as well, so the padding path and the exact path are both exercised? One case proves the tolerance; two prove the rule.

The ONEBIT_OGA_PACK gating is right — this leg cannot run in CI without a pack, which makes the stated reason it was failing more valuable than the fix.

[bong-water-water-bong]

VERDICT at efde6ab: READY

The padded-row arithmetic is a named function with the boundary visible: head_stored_rows(vocab, from_fconst_tail) returns vocab unless the head comes from the .fconst tail, in which case (vocab + kTileRows - 1) / kTileRows * kTileRows.

That is the right split. The paddings are not a property of the vocabulary — they are a property of where the head is stored, and a reader can see the from_fconst_tail guard rather than inferring it from a conditional inside a reader. vocab <= 0 returning early keeps a degenerate call from producing a tile-rounded nonsense row count.

The arm is the real-pack leg on a padded tail head, which is the combination that has no other coverage: an unpadded pack exercises none of this, and a synthetic one would not prove the real tail layout.

[bong-water-water-bong]

VERDICT: READY at efde6ab — "the import is right there and the check is wrong" is the correct verdict on a failing arm, and the fix names the rule instead of patching the assertion.

The defect is a test bug with a real cost: the real-pack leg fails on every pack whose head is stored in whole 64-row tiles — Hunyuan-1.8B 120,818 vocabulary rows → the tail head occupies ceil(120818/64) × 64 rows, which is not 120,818. So the arm compared a tile count against a row count, and the failure message (lm_head tile count matches the container) reads like an import bug — which is how an hour gets spent debugging correct code.

head_stored_rows(vocab, from_fconst_tail) is the right shape: one named function stating the rule once — "a tail head occupies ceil(vocab / 64) × 64 rows, a gra[phed head …]" — and the import's tile count now uses it rather than repeating the arithmetic inline. That is the same anti-drift move as #2101's single predicate: two spellings of "how many rows does the head occupy" is how the test and the importer came to disagree in the first place.

Why this matters beyond the arm: the padded-tail head is exactly what #1992/#2020/#2070 spent today on (32,016 → 32,032 for a q4_1 quantum, and the tile-count-not-row-count bug). Having the rule in one function that the test and the importer share means the next head geometry change updates one place — and the arm then fails only when the behaviour changes, not when the arithmetic is re-derived differently.

[bong-water-water-bong]

On the graph-head branch (the LGTM comment's suggestion to return (vocab + 31) / 32 * 32 for from_fconst_tail == false): not taken, and here is why. ready for review at 96e49c0

The helper has to state what the importer on main does, because the test compares it with the container main writes. On main a graph head is not padded: tile_q4_1 refuses it instead (npu/pack_import.cpp:2080, "q4_1 tiles need N % 32 == 0"), and the PR that padded it, #1992, was closed unmerged. So for a 32,016-row graph head there is no container on main to count tiles in, and a 32-row quantum in this function would describe a behaviour that is not in the tree. The 16,016-tile CodeLlama container you measured comes from a branch that carries that padding.

What I changed at 96e49c0: the header comment no longer reads as if #1992's padding existed. It now says a graph head is stored as it is, that an unaligned one is refused by the tile writer, and that if graph-head padding lands its quantum belongs in this function. Whoever lands that padding should add the false-branch quantum and your two arms (32,016 → 32,032 and 32,000 → 32,000) in the same PR, where the import and the expectation change together.

Comment-only change; npu_pack_import_test rebuilt and passes. The pr-agent guide above describes a different PR's content (use_qk_norm); nothing in this diff touches that.