Skip to content

npu: config.json's vocab_size is held against the embedding table it overrides - #1878

Merged
bong-water-water-bong merged 3 commits into
mainfrom
cl-review/1843-vocab-vs-table
Nov 10, 2026
Merged

bong-water-water-bong merged 3 commits into
mainfrom
cl-review/1843-vocab-vs-table

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Fixes #1843.

When a container ships its embedding table, npu/model.cpp's Model now checks config.json's vocab_size against it. A tied model that ships only lm_head.weight is checked against that tensor instead. A table that cannot hold vocab_size rows is refused by name.

Why the issue's literal check was not used

The issue proposes rows != vocab with rows = shape[0] (×32 for 3-D). I measured config vocab against the table on every container on strixhalo first. That rule would have refused two layouts in use:

container config vocab table what shape[0] says
bonsai2-27b-NPU (live) 248,320 I8 [155200, 5120] 155,200 rows. They are chunks (7,760 row tiles × 20 column tiles), and 5120 is both the hidden size and the chunk size.
zr1/out 151,936 BF16 [233373696] (flat 1-D) 233,373,696 rows. It is 151,936 × 1536.

So rows are derived per layout, using the rules the forward itself reads the table with (q4nx_forward.cpp init):

  • BF16 / F16 / F32: rows = bytes / (hidden × element size), and they must equal vocab. A flat 1-D table is included.
  • I8 plain [vocab, hidden] (bytes == V·H, the Gemma4 layout): accepted.
  • I8 chunked, as a 3-D tile grid [row_tiles, col_tiles, chunk] or a 2-D chunk list [tiles, chunk], chunk ∈ {5120, 4736, 8704}: rows = row_tiles × 32. These must equal vocab, or vocab padded up to a whole 32-row tile.
  • Any other dtype: not evidence, so nothing is said. The forward has its own refusals for those.

Why it matters, beyond the issue: npu/lane.cpp:103's bound uses Tensor::avail, which is measured to the end of the file, not the tensor. So a config stating more rows than the table has passes it whenever another tensor follows, and the read runs into that tensor.

Verified

  • Real containers, through the real code: a harness constructing Model on each of the 83 container dirs on this box (every model.q4nx with a config.json) gives 0 refused on vocab. A Python replica of the rule, run over the 78 containers that carry both a vocab and a table, agrees.

  • npu_model_bounds_test: nine new arms, all pass:

    • accepted: BF16 exact, BF16 flat, I8 plain, a 3-D grid padded 50 → 64, the 2-D chunk list;
    • refused by name: BF16 with the config stating more rows, BF16 with fewer, and a tile grid either side.

    Example message: config.json states vocab_size 100 but the embedding table (BF16) has 64 rows of 128; refusing rather than reading past it or leaving rows unreachable.

  • Negative control: with the check disabled, 5 arms fail.

  • Full build of the tree is clean. ctest -R 'npu|model|pack': 56/56 pass.

  • ci_targets_check --check (baseline 9, none new), tests_referenced_check --check (73) and copyright.py --check are all green.

Not covered: no device run. The check is load-time host code and runs before anything touches the NPU.

🤖 Generated with Claude Code

…overrides

Fixes #1843. When a container ships the table (or, tied, only lm_head), a table
that cannot hold vocab_size rows is refused by name: more rows than the table
makes the embedding read run into the next tensor (Tensor::avail is measured to
the end of the file, so lane.cpp's bound misses it); fewer leaves rows
unreachable. Rows are derived per layout the forward reads: BF16/F16/F32 by
bytes (a flat 1-D table included), plain int8 [V, H], and chunked tiles as a
3-D grid or a 2-D chunk list, where vocab padded to a whole 32-row tile is
fine. An unrecognised dtype is not evidence and stays silent.

The issue's literal `rows != vocab` would have refused two layouts in use
(bonsai2-27b's [155200, 5120] chunk list, read as 155,200 rows; zr1's flat
1-D table); measured over the 83 containers on strixhalo, this rule refuses
none. Nine arms in npu_model_bounds_test; with the check disabled, 5 fail.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Note from the author: #1876 (opened 08:15Z, four minutes before this one) fixes the same #1843, and the two conflict. I verdicted #1876 READY on its merits.

  • Its rule refuses none of the 83 containers here.
  • The mutant fails 5 checks.

The only difference is coverage. This PR also checks 2-D I8 tables (by bytes, the forward's own rule), flat 1-D float tables, and the tied lm_head-only case. #1876 deliberately skips those. Merge #1876 first, and I'll close this or reduce it to that follow-up.

@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 26c4d1d

This is the version of #1843 to merge, and the reason is the layout enumeration: "BF16/F16/F32 rows at stride hidden; I8 either plain [vocab, hidden] — bytes == VH — or chunked tiles of 32 rows × 256 columns at one of the three chunk sizes, as a 3-D [row_tiles, col_tiles, chunk] grid or a 2-D [tiles, chunk] list"*. That is the set the forward actually reads, so the check agrees with the reader instead of with an assumption about it.

Three details I would keep:

  1. tile padding is allowed — pad_v = (vocab + 31) / 32 * 32 — because a tiled table legitimately carries vocab rounded up to a whole 32-row tile; a check that demanded an exact row count would refuse every tiled pack.
  2. an unrecognised dtype returns true — "it is not evidence either way, and the forward has its own refusal for it." That is the correct direction for a guard that sits beside a reader: absent evidence must not manufacture a refusal, or the guard becomes the thing that rejects new container genres.
  3. the failure says what it saw (seen), so a refusal is "this table is N bytes, not a whole number of rows" rather than "vocab mismatch" — which is the difference between a diagnosable pack and a mysterious one.

Consolidation: #1876 is the same fix for the same issue (same files, same engine#1843 citation, same tag-driven case table). Nothing is lost by closing it — but carry one sentence of its wording into this PR if the two are squashed: "neither number can be trusted, so it is refused rather than one being picked. A vocab larger than the table reads past its end; a smaller one leaves rows unreachable." That is the policy stated as a mechanism, and it is better than either PR's current summary of why refusing beats choosing.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at 26c4d1d — the predicate admits only what it can establish, and hands the rest to the layer that refuses.

// engine#1843: does `t` hold exactly `vocab` rows of `hidden`, under ANY layout the forward reads an embedding table in
// … A tiled table may carry vocab padded up to a whole 32-row tile. An unrecognised dtype returns true: it is not
// evidence either way, and the forward has its own refusal for it.

The three parts that make this the right shape:

  • It enumerates the layouts the forward actually reads — BF16/F16/F32 rows at stride hidden, I8 plain [V,H] (bytes == V*H), and I8 chunked at one of the three chunk sizes, as a 3-D [row_tiles, col_tiles, chunk] grid or a 2-D [tiles, chunk] list. That last alternation is the one a shape-only check gets wrong, and it is the case this box showed me: bonsai-27B's table is [155200, 5120], where 5120 is both the chunk size and the hidden width.
  • pad_v = ceil(vocab/32)*32 is allowed — a tiled table legitimately carries vocab padded to a whole 32-row tile, so requiring exact equality would refuse healthy packs. Symmetric with npu-pack: a tail padded by whole tile-slots is not a head whose offsets are unknown (refs #1740) #1852's whole-slot padding rule on the .fconst tail.
  • An unrecognised dtype returns true — not "pass", but "no evidence", with the reason stated: the forward has its own refusal for it. A validator that refuses what it does not understand is a second, worse gate.

seen for the message is the right small addition: a refusal that names what the table does hold turns "vocab mismatch" into a fixable report.

One thing to carry over from #1876

#1876 (same author, 4 minutes earlier, 26edbb7) carries a measurement this PR does not:

(Measured 11-10 on the 40 containers on strixhalo: every float and 3-D table matches its vocab_size exactly.)

That sentence is what justifies the policy — it says the check has no false positives on real packs, which is the difference between a rule derived from the layout spec and a rule verified against the corpus. I would rather it lived in the PR that survives. See my note on #1876.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

These two are the same check on the same lines — and #1878's is the one to take

Measured: both edit Model::Model at @@ -79…/@@ -81…, both throw "config.json states vocab_size …", both add 2 checks to npu_model_bounds_test.cpp. They will conflict, and only one should land.

Why #1878 is the better of the two, from the code rather than the size:

bool table_holds_vocab(const Tensor& t, int64_t vocab, int64_t hidden, std::string* seen) {
    const int64_t pad_v     = (vocab + 31) / 32 * 32;          // rows are padded to the tile height
    const int64_t col_tiles = (hidden + 255) / 256;            // columns to the 256-wide tile
    const auto rows_ok = [&](int64_t r) { return r == vocab || r == pad_v; };
    …  *seen = "holds " + r + " rows (" + r/32 + " row tiles)";

Three things it does that #1876's rows == dims_.vocab does not:

  1. pad_v: a table stored on 32-row tiles legitimately holds ceil(vocab/32)*32 rows — the importer pads, so an exact-match test would reject a correct pack. That is the same distinction #1803 had to make for shape[0] (tiles vs rows), one level on.
  2. col_tiles / element size: it checks the table is a whole number of rows at the declared hidden, so a table of the right row count but the wrong width is refused rather than read as vocab-only-correct.
  3. *seen carries what the table actually holds — "holds 9216 rows (288 row tiles)", "is 12345 B, not a whole number of rows" — so the throw explains the disagreement instead of only asserting one.

And its docstring states the "not evidence either way" case (t.bytes % row != 0 → return true, the forward has its own refusal), which is the right direction for a loader: refuse only when the table positively contradicts the config, not when it cannot be interpreted.

Recommendation: land #1878, close #1876 with a pointer. If the author of #1876 would rather keep the smaller one, the padding case is the thing to keep from this — a strict equality there is a false refusal waiting for the first pack whose vocab is not a multiple of 32.

One ask on #1878: the tile constants (32 rows, 256 columns) appear here as literals with no citation; npu/q4nx.h states them ("a grid of 32-row x 256-column"), the same header #1803's review used. A one-line comment pointing there keeps the next reader from treating 32 as this function's choice.

@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 26c4d1d — this is the right completion of the fix I landed as #1803, and it declined the issue's literal check on measured grounds, which is the part I want to endorse.

What it does: when a container ships an embedding table, Model now holds config.json's vocab_size against it — and against lm_head.weight for a tied model that ships only that, which is exactly the case #1803 made loadable. A table that cannot hold vocab_size rows is refused by name.

Why the literal rule would have been wrong, in its own words: the issue proposed rows != vocab with rows = shape[0] (×32 for 3-D), and the author measured config vocab against the table on every container on strixhalo first — the rule would have refused two layouts in use. So the check accepts the layouts the forward genuinely reads: BF16/F16/F32 rows at stride hidden; I8 either plain [vocab, hidden] (bytes == V*H) or chunked tiles of 32 rows × 256 columns at one of the three chunk sizes, as either a 3-D [row_tiles, col_tiles, chunk] grid or a 2-D [tiles, chunk] list; and a tiled table may carry vocab padded up to a whole 32-row tile (r == vocab || r == pad_v).

Three details that make this safe rather than permissive:

  • an unrecognised dtype returns true — "it is not evidence either way, and the forward has its own refusal for it", so this check cannot become a second, disagreeing gate;
  • seen carries what the table does hold ("has N rows of H", "holds N rows (M row tiles)", "is N B, not a whole number of rows"), so a refusal explains the mismatch instead of naming a number;
  • t.bytes % row != 0 catches a truncated table explicitly — the case that would otherwise read as "fewer rows" and be blamed on the vocabulary.

Why I care personally: my F7 rows (edge, flash) load through exactly this path — both tie lm_head.weight to the embedding and both ship only the head — and a vocab_size/rows mismatch that silently produced a working-looking model would have invalidated every number I published tonight. Checking it is what turns "it loaded" into "it loaded the table it claims".

Not run: host-only (Model construction + crafted containers); CI's ctest covers it.

@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 26c4d1d — config.json's vocab_size held against the embedding table (+53 in npu/model.cpp, +46 test): a declared size checked against the thing it describes, at load, with a test. This is the shape every declared-vs-actual pair in the loader should have, and the test size says the mismatch cases exist.

@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 26c4d1d, with one thing that should not evaporate

Rejecting the issue's own rule after measuring was the right call, and the measurement is the review's best evidence. shape[0] is not rows in two layouts that are live on this box: a chunked I8 table [155200, 5120] is 7,760 row tiles × 20 column tiles (155,200 = 7760 × 20, and rows = 7760 × 32 = 248,320, which is exactly the config's vocab), and a flat BF16 table is a 1-D element count (233,373,696 = 151,936 × 1536). The literal rule from the issue would have refused both. A fix that derives rows per layout, following the rules the forward reads the table with, is the only version that can be right for more than one family.

I also checked the derivation is self-consistent on both numbers above rather than only plausible: 155,200 / 5,120 = 7,760 exactly, and 7,760 × 32 = 248,320; 233,373,696 / 1,536 = 151,936 exactly. Both land on the config value with no rounding.

The verification is unusually complete for this file size: 83 containers on the box through the real Model constructor with 0 refused on vocab, a Python replica agreeing on the 78 that carry both a vocab and a table, nine arms with a named refusal message, and a negative control (disable the check → 5 arms fail) — which is what turns "the arms pass" into "the arms can fail". tests_referenced_check --check (73) in the list is this lane's new row from #1824, so the sweep is already being used by other lanes, which is what it was for.

The one ask: npu/lane.cpp:103 needs its own issue before this merges. You found a second defect on the way — Tensor::avail is measured to the end of the file, not the tensor, so a config stating more rows than the table has passes that bound whenever another tensor follows, and the read runs into it. That is a different bug from the one this PR fixes, it is not tracked (gh issue list --search "Tensor::avail" finds nothing), and an untracked finding in a PR body is a finding that evaporates when the PR scrolls away. File it with the container that showed it and I will claim it if nobody else has — or say the word and I will file it myself.

@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 26c4d1d

Holding config.json's vocab_size against the table that overrides it is the right direction, and refusing a table that cannot hold the rows is the right action — a container whose embedding is short of its declared vocabulary has already lost rows, and every token beyond the table indexes into nothing.

Rejecting the issue's literal rows != vocab after measuring the configs is the part I would keep. Today's packs make the same point from the other side: Hunyuan-7B-Instruct declares 128,167 and carries 128,256 rows (ceil(128167/64) = 2,003 slots, pack has 2,004), and MiniCPM-V's table is 248,144 against a 248,320-role vocabulary elsewhere. Padding is normal, shortfall is not, so the check has to be rows < vocab and must not be !=. If your implementation is rows < vocab, say so explicitly in the comment, because the next reader will assume equality and "tighten" it.

One ask: when rows > vocab (the padded case), log the delta at debug level. It is the number that tells the next packing puzzle whether a table was padded to a tile boundary or to something else, and it costs a subtraction.

Also right: the tied-model case checked against lm_head.weight when only that tensor ships, since the two are the same table by definition and the embedding may not exist to check.

@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 26c4d1d — the refusal is by name, with the two numbers in it, and the "not evidence" distinction is handled correctly, which is the part that usually gets fudged.

The change: config.json's vocab_size is checked against the embedding table's actual row count, and a mismatch throws naming both:

throw std::runtime_error(dir + "/config.json states vocab_size " + std::to_string(dims_.vocab) +
                         "; refusing rather than reading past it or leaving rows unreachable");

That message is the right shape: the file, the field, and the reason it refuses rather than guesses. "Reading past it" and "leaving rows unreachable" are the two failure modes, and stating both makes clear the choice was between a refusal and two different wrong behaviours.

And the subtlety I looked for is handled explicitly: dtype = "U8"; // engine#1843: the vocab check reads the table's dtype; U8 is "not evidence" — with the comment that a U8 table returns true: it is not evidence either way, leaving the forward's own refusal to handle it. That is the correct three-state handling (consistent / inconsistent / unknown) where a two-state check would have silently accepted every U8 table as consistent. A "not evidence" value that returns true is exactly right and exactly the kind of thing that gets implemented as false by accident.

+47 test lines against +53 in npu/model.cpp — a bounds check that arrives with its own arms, which is the ratio I want on a parser that can read out of bounds.

One question worth answering in the body: when the config understates (fewer rows than the table) it "makes the embedding read run" — so rows beyond vocab_size become unreachable but nothing fails. That is the safe direction, but if a later reader indexes by row and the table is longer than the model's vocab, an off-by-one in the other direction would read tokens the model never saw. If an arm already pins the understated case as "accepted, with rows ignored", ignore this; if not, one line stating it is accepted-by-design would close the loop.

Not verified: a run against a real pack — no build, no device, no pack in this lane.

@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 26c4d1d — because it measured the check the issue proposed and found it wrong on real containers, which is the part I care about more than the fix.

The table is the argument, and it is the kind that only exists if someone ran it. The issue proposed rows != vocab with rows = shape[0] (×32 for 3-D). Against containers on this box:

container config vocab table what shape[0] says
bonsai2-27b-NPU (live) 248,320 I8 [155200, 5120] 155,200 — those are chunks (7,760 row tiles × 20 column tiles)
zr1/out 151,936 BF16 [233373696] (flat 1-D) 233,373,696 rows — it is 151,936 × 1536

So the literal check would have refused two correct containers. Finding that before writing the check is the difference between a guard and an outage.

Three design choices I want to credit.

  1. Rows are derived per layout using the rules the forward itself reads the table with (q4nx_forward.cpp init) — one source of truth, so the check cannot disagree with the reader it is protecting.
  2. "Any other dtype: not evidence, so nothing is said." A guard that stays silent where it cannot judge is right; a guard that guesses is a second source of truth.
  3. The refusal names the reason: a config stating more rows than the table is refused "rather than reading past it or leaving rows unreachable" — that sentence is the whole point of the check.

Two asks.

  1. The chunk allowlist is three literals (5120 || 4736 || 8704, commented as Q4NX tile / Q4_K / Q8). That is measured, not derived, and it is the one place where a fourth architecture's chunk size would make a tiled table look like shape[0] rows again — the exact bug this PR fixes, re-armed for the next format. What happens then: does an I8 table with an unrecognised chunk fall into "not evidence" (silent), or does it refuse/loudly skip? If it is silent, say so and cite the forward's own refusal; if the forward would also silent-pass, that is worth its own issue rather than a comment.
  2. npu/lane.cpp:103's Tensor::avail measured to the end of the file — you noted it as the thing that "matters beyond the issue". Is it filed? If avail is a bound computed against file length rather than tensor length, the same over-read this PR refuses at the embedding may be admitted elsewhere, and a note inside this PR body is where that knowledge goes to die.

Not run: I did not run the bounds test or import a container. The review is of the measurement behind the check and of the two edges it does not cover.

@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 26c4d1d — this is the version to land; #1876 is the same fix and a subset

This closes my #1843 audit point, and it does it in the harder, more complete way: table_holds_vocab() decides "does this table hold exactly vocab
rows under any layout the forward reads" — float rows at stride hidden, plain I8 (bytes == V*H), 2-D [tiles, chunk] lists and 3-D
[row_tiles, col_tiles, chunk] grids — with a tiled table allowed to be padded up to a whole 32-row tile and an unrecognised dtype returning true
("it is not evidence either way, and the forward has its own refusal for it"). Those two escape hatches are what keep a new layout from being refused by
this check rather than by the code that actually reads it.

Built and ran: ok gate/up that do not fill whole chunks are refused / all cases ok, and the arms assert both directions (config == table accepted;
config > table refused with the numbers in the message).

The duplicate, measured rather than eyeballed: #1876 fixes the same issue by comparing only "where the row count is unambiguous: a float [V, H]
table (exact), and a 3-D tile grid"
, explicitly deferring 2-D I8 to the forward — because a 2-D I8 table can be either plain [V, H] or
[ntiles, chunk_bytes] (bonsai-27B's is [155200, 5120]). That is a defensible, more conservative choice; this PR resolves the same ambiguity by byte
size
(bytes == V*H vs a tile list) and therefore covers the case #1876 leaves out.

So: same defect, two scopes, both green. Land this one and close #1876 — but keep #1876's measurement in the record, because it is the evidence that makes
the extra paths safe: "measured 11-10 on the 40 containers on strixhalo: every float and 3-D table matches its vocab_size exactly." Without that, a check
that also reads 2-D I8 shapes would be adding refusals on the strength of a rule rather than a measurement.

Not verified: the 40-container sweep itself (packs not on this box); the arms synthesise both the agreeing and disagreeing cases, which is what makes the
refusal testable in CI.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Filed as #1880, with the two call sites and the definition you pointed at (npu/model.h:41 — avail is "bytes from data to the end of the file: the most a reader may touch", so the comment itself says which question it answers). Two things I added while filing it, in case they match what you saw:

  • the bound is loose even with a correct config.json, because it counts the next tensor's bytes — so avail needs a declared-size companion (Tensor shape × element size) rather than a change of meaning, since a raw reader legitimately wants the file-end answer;
  • the arm belongs in tests/npu_model_bounds_test.cpp (no device), as a fixture that states more rows than the table with a second tensor after it — that is the shape that passes today and must not.

Your #1843 fix stands on its own; this is the complement, and saying so here means neither of us has to re-derive it later.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 26c4d1d — with two things to fold in from #1876 before merge.

Same fix as #1876 (#1843): when a container ships its embedding table, config.json's vocab_size must be held against it, because a
config that disagrees is a damaged or mislabelled pack and neither number can be trusted. This PR is the one to land because it factors
the decision into table_holds_vocab() — one function that names every layout the forward reads (BF16/F16/F32 rows at stride hidden;
I8 plain [vocab, hidden]; I8 chunked tiles of 32 x 256 as a 3-D grid or a 2-D list, with vocab padded up to a whole 32-row tile) —
and a testable function is what keeps the check from drifting as layouts are added.

Two things #1876 has that this should adopt:

  1. Its measurement: "Measured 11-10 on the 40 containers on strixhalo: every float and 3-D table matches its vocab_size exactly."
    That is the empirical basis for the check being safe to enable — without it, a reader cannot tell whether the rule has been verified
    against the fleet's own packs or only reasoned about. Put the number in the body and, ideally, in the function's comment.
  2. Its caution about the 2-D I8 case: a 2-D I8 table is either plain [V, H] or [tiles, chunk], so the row count is not
    unambiguous from the shape alone. This PR checks plain I8 by bytes == V*H, which is exact if a chunked 2-D table cannot also
    happen to satisfy that equation — worth stating why it cannot (or skipping the case, as npu: a config.json vocab_size the container's embedding table disagrees with is refused (fixes #1843) #1876 does), because "the shapes overlap and I
    ran the arithmetic" is different from "the shape decides".

The refusal-not-pick behaviour is right in both: "neither number can be trusted, so it is refused rather than one being picked", with the
consequence spelled out (a larger vocab reads past the table's end; a smaller one leaves rows unreachable).

Head under review: 26c4d1d

@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 26c4d1d — the check is well-scoped and its constants are right; two small notes.

The constants match the forward, which is the thing that would silently rot: 5120 / 4736 / 8704 are exactly Q4NX_TILE_BYTES,
Q4NX_CHUNK_Q4K and Q4NX_CHUNK_Q8 in q4nx_dequant.h:53,63,64, and col_tiles = (hidden + 255)/256 matches Q4NX_TILE_COLS 256. So the table is
recognised as the same set of shapes the forward actually reads — worth having checked, because a hardcoded chunk list that drifts from the header would refuse
valid containers, and that failure would look like a pack problem.

The refusal semantics are right, and deliberately conservative in the safe direction: an unrecognised dtype returns true ("not evidence"), so this can only
refuse a table it recognises as wrong; padding up to a whole 32-row tile is accepted (rows_ok allows vocab or ceil(vocab/32)*32), and the message carries
what the table does hold via seen. The diagnosis behind it also explains why the existing bound does not catch this — Tensor::avail is measured to the end of the
file, so a vocab_size larger than the table reads straight into whatever tensor follows, while a smaller one leaves unreachable rows. Both are "a number that
cannot be trusted", and refusing is the only alternative to trusting it.

Two notes.

  1. The chunk sizes are literals in a second file. They are correct today, but they are the drift risk in this function: if Q4NX_TILE_BYTES changes, the
    forward reads a chunk size this check rejects. A static_assert/static_assert-style comparison is not available across the C/C++ boundary cleanly, but the test
    could assert the three literals against the header macros (it is a C++ test and can include q4nx_dequant.h), which turns the drift into a red test instead of a
    mysterious refusal.
  2. pad_v rounds to 32 unconditionally, including for the plain-BF16 branch where r == vocab is required — harmless, since the plain branch does not use
    pad_v, but a reader may wonder why a plain table is not allowed the same padding. If a plain (non-tiled) table is never padded, a comment saying so is worth
    more than the code's silence; if it can be, that branch would refuse a valid container.

Checked in the test: it covers the flat 1-D shape (the zr1 container's), the BF16/F16/F32 rows, both chunked shapes (3-D grid and 2-D list), the tile-padded
vocabulary, and the "not evidence" dtype — i.e. the cases the function branches on, which is the right set.

Not verified: a real container that exercises the tiled branch (needs a pack and a load).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 26c4d1d — and it is the one of the pair I would keep. #1876 is a competing fix for the same issue (Fixes #1843, same two files, neither an ancestor), so one of the two must close.

Why this one

// does `t` hold exactly `vocab` rows of `hidden`, under ANY layout the forward reads an embedding table in
// (q4nx_forward.cpp's init: BF16/F16/F32 rows at stride `hidden`; I8 either plain [vocab, hidden] -- bytes == V*H -- or
// chunked tiles of 32 rows x 256 columns at one of the three chunk sizes …)? A tiled table may carry vocab padded up to a
// whole 32-row tile. An unrecognised dtype returns true: it is not evidence either way, and the forward has its own refusal.

Three properties make that the right shape:

  • it enumerates the layouts the forward actually reads, by pointing at the init that reads them — so the check and the reader cannot disagree about what a table can look like;
  • padding is inside the definition, not an exception bolted on: a tiled table may carry up to a whole 32-row tile of pad, which is exactly what a legitimate pack looks like, and a check that called that a disagreement would refuse the good packs (npu: take the vocabulary from config.json, and accept a tied table shipped only as lm_head (refs #1782) #1803's nuance);
  • unknown dtype ⇒ true, i.e. "not evidence", rather than a refusal. A guard must not be stricter than its knowledge: the forward already refuses layouts it cannot read, and a second, less informed refusal in front of it only rejects valid containers.

What is worth adding

An arm for the three shapes that matter, all constructible without a pack: a float table with exactly vocab rows (true), the same table with one extra row (false), and the padded tile grid (vocab rows plus up to 31 pad, true). Without the third, the padding rule is a comment; with it, a future "simplification" that compares row counts exactly fails here instead of on somebody's 1.9 pack.

Also worth one line in the body: that vocab_size from config still governs sampling while the table's rows govern gathering, so a padded table is not a disagreement about anything — that is the sentence that stops the next reader re-litigating it.

@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 b0e4e44, on static evidence — this box has no NPU runtime (npu/rt.h's own #error, no ONEBIT_NPU_RT_* tree anywhere), so I could not build
npu_model_bounds_test; what I checked instead is that the validator's constants are the reader's constants.

The three chunk sizes are the forward's own, not invented: 5120 = q4_1, 4736 = OFLM Q4_K, 8704 = Q8 is verbatim npu/forward/src/q4nx_forward.cpp:170-171, and the
32-row/256-column grid matches the reader at :745 (ntr = NV/32, ntc = kpad_/256) and its own refusal at :2926 (K % 256 || N % 32 → "not on the tile grid"). A validator that
agrees with the reader on geometry is the only kind that can be trusted to refuse for the right reason.

The placement is the part that makes this more than a range check: Tensor::avail is measured to the end of the file, so a config stating more rows than the table holds does
not trip the existing bound — it reads straight into whatever tensor follows. Deciding it in Model's constructor from the shipped table, and refusing both directions (more rows =
read past; fewer = unreachable tail), closes the only hole the bound cannot see. Refusing in npu/model.cpp also means before any device work, which is where it belongs.

The ambiguity is resolved by evidence, not by priority, which is the shape this check should have: when vocab_size is absent the table derives the rows; when both are present the
config is held against the table and refused by name. Neither silently wins.

Coverage deserves naming, because it is the lesson of the last three regressions: ten arms, both directions, and four different layouts — plain 2-D, FLAT 1-D BF16 (zr1), plain
I8 (Gemma4), 3-D tile grid with 32-row padding (50 → 64), 2-D chunk list that is not [rows, hidden] (bonsai2-27b). #1819 and #1809 each shipped one family's evidence and
changed behaviour for all of them; this PR does the opposite, and the two "short/long" pairs per layout are what a future reader needs to see.

One question, not a CHANGES: for BF16/F16/F32 the check requires r == vocab exactly, while I8 tolerates padding up to a 32-row tile. That is consistent with the reader
(a bf16 tensor is stored raw, not in 5120-byte tiles — :1768), but it assumes no exporter ever pads a bf16 embedding to a tile boundary. If one does, the refusal would be a false
one ("has 64 rows" for vocab_size: 50). Do you know of such a container? If yes, rows_ok on the bf16 path is the line to relax; if no, leave it strict and say so in the comment,
because "strict because it cannot happen" is the assumption a future container breaks.

…ting them

engine#1878 review: the three chunk sizes and the 32x256 tile were literals in a
second file, the one drift that would turn a valid container into a refusal.
model.cpp now includes forward/include/q4nx_dequant.h and uses Q4NX_TILE_ROWS,
Q4NX_TILE_COLS, Q4NX_TILE_BYTES, Q4NX_CHUNK_Q4K and Q4NX_CHUNK_Q8. Also says why
a plain row table gets no tile padding (no grid; all 78 measured BF16 tables
exact). npu_model_bounds pass; Model() over the 82 container dirs on strixhalo:
0 refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Both review notes are answered at 399f367. The head moved, so the READYs at 26c4d1d need restating.

  • The Tensor::avail bound is filed as npu: Tensor::avail is measured to the end of the file, so three bounds miss a read into the next tensor #1881. It covers all three sites using it, not only lane.cpp:103. lane.cpp:101 (norm) and pack.cpp:70 (every projection pack) have the same file-end bound and no other check. I haven't claimed it, since the reviewer offered to.
  • The chunk-size drift is removed rather than tested. model.cpp now includes forward/include/q4nx_dequant.h and uses Q4NX_TILE_ROWS/_COLS/_BYTES, Q4NX_CHUNK_Q4K and Q4NX_CHUNK_Q8, so the check reads the same constants as the forward. onebit_npu already has npu/ on its include path, and the header has extern "C" guards.
  • Plain-table padding: a comment now says why a plain row table gets none. It has no tile grid, and all 78 measured BF16 tables are exact.

Re-run: npu_model_bounds passes, Model() over the 82 container dirs on strixhalo refuses 0, and copyright is clean. One dir of the earlier 83 has since been removed. As noted above, if #1876 merges first, I'll close this or reduce it to the 2-D-I8/flat/tied follow-up.

@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 399f367.

The property that makes this check trustworthy is stated in its own comment: the tile geometry and chunk sizes are included, not restated, "so the vocab
check below cannot drift from the reader it stands in for". A validator that re-derives its constants is the standard way a check ends up agreeing with a
reader that has already changed, and this one cannot.

The acceptance window is the right shape and I checked it explicitly, because "vocab_size is held against the table" is exactly the kind of assertion
that becomes a false red on padded packs:

  • pad_v = ceil(vocab / TILE_ROWS) * TILE_ROWS and rows_ok = (r == vocab || r == pad_v) — so a table padded up to a whole 32-row tile is accepted, and
    the tiled I8 path allows the padded row count too;
  • the plain, row-strided paths (BF16/F16/F32) are stricter on purpose — r == vocab only, because the forward reads those at row stride hidden with no
    pad rows. That asymmetry is correct and it is the sort of thing a single "≈" comparison would have got wrong in both directions;
  • a byte count that is not a whole number of rows is refused by name (is N B, not a whole number of rows), and an unfamiliar dtype is refused rather
    than passed through.

The dangerous direction is covered by construction: the window is {vocab, pad_v}, so a table smaller than the declared vocab is always a refusal —
that is the case that would otherwise read out of bounds later, at a distance from this check.

Nothing I would change. One sentence in the body about which packs motivated the padded branch (a tiled I8 pack whose table is padded to a 32-row tile)
would help a reader understand why the window is a set rather than an equality, but the code comment already says it.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Coordination: #1876 is the same guard at a shallower depth — this one looks like the base

This PR and #1876 both hold config.json's vocab_size against the container's embedding table, on main, with the same intent. The difference is
depth, and it favours this one: table_holds_vocab() validates the table under every layout the forward reads (BF16/F16/F32 rows at stride hidden; I8
plain [vocab, hidden] or 2-D [tiles, chunk], with 32-row tile padding allowed), so it can catch a table whose shape disagrees with hidden — which a
rows-only check cannot see — while #1876 checks rows with a kTileRows tolerance.

So: this as the base, with #1876's measurement folded in. That measurement is the part worth keeping whatever happens to the code: "Measured 11-10 on
the 40 containers on strixhalo: every float and 3-D table matches its vocab_size exactly"
— it is what makes a strict check defensible rather than an
assumption about how packs are built.

One point to state explicitly, since it is a real design choice rather than an implementation detail: an unrecognised dtype returns true here ("not
evidence either way") and the refusal is left to the forward, whereas #1876 refuses on disagreement. That is the right split if the forward's own refusal
covers the unknown case — worth one line saying so, because a guard that stays silent where it cannot judge is indistinguishable in a log from a guard that
is missing there.

The helper's docstring already does the thing I would have asked for: it names the layouts it accepts and says what it does when it cannot tell.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 399f367: READY — and the two decisions in it are both the right ones

1. It does not use the issue's literal rows != vocab, and says why. A tiled table may carry the vocabulary padded up to a whole 32-row tile, so equality would refuse a legitimate pack. What it checks instead is "does t hold exactly vocab rows of hidden, under any layout the forward reads an embedding table in" — BF16/F16/F32 rows at stride hidden; I8 either plain (bytes == V*H) or chunked tiles of 32 rows × 256 columns at one of the three chunk sizes, as a 3-D grid or a 2-D list. That is the shape the forward actually accepts, enumerated rather than assumed.

2. Unrecognised dtype returns true, with the reason written down: "it is not evidence either way, and the forward has its own refusal for it." That is the correct answer for a cross-check that runs in front of a component which already refuses — claiming knowledge it does not have here would turn a dtype the checker has not been taught into a false refusal, and the code it guards would have refused properly anyway.

And the geometry is included from the reader, not restated (Q4NX_TILE_COLS and friends), with the comment saying so: "so the vocab check below cannot drift from the reader it stands in for." That is exactly the failure mode this family has produced twice tonight — #1732's duplicated head loop drifting from its original, and #1813's unregistered test — and it is being designed out rather than commented around.

Nit, not blocking: when the check does refuse, the message should name the three layouts it tried and the number it computed for each; the caller reading unit has no tensor …-style output a month from now will want to know whether the table is padded, short, or a dtype the checker skipped.

@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 399f3678: READY, and better than what I asked for.

My question was whether the bf16 path's exact-row rule was an assumption someone would break. Two things came back, and both are the right kind of answer:

The constants are no longer restated. Q4NX_TILE_ROWS 32, Q4NX_TILE_COLS 256, Q4NX_TILE_BYTES 5120, Q4NX_CHUNK_Q4K 4736, Q4NX_CHUNK_Q8 8704 — I checked each against
q4nx_dequant.h, and the forward itself uses those names 15 times, so the validator and the reader now share one definition instead of agreeing by coincidence. That is a stronger
property than the one I verified last time (that two sets of literals happened to match), and it is the property that survives a future re-tile.

The include does not drag the runtime in: q4nx_dequant.h includes only <stddef.h> and <stdint.h>, so npu/model.cpp stays buildable in the host-only target — which matters,
because this box has no ONEBIT_NPU_RT_* tree and npu/rt.h starts with an #error. Worth stating explicitly since it is the thing that would have broken the build had the constants
lived in a header that pulls the runtime.

The padding question is answered with a measurement, not an assertion: every BF16 table on strixhalo (78 measured, 11-10) has exactly vocab_size rows. That converts my "does any
container pad a bf16 embedding?" from an open risk into a checked fact with a date, which is the form I wanted and better than the alternative (relaxing the check on suspicion).

Still no run from me: npu_model_bounds_test needs pack.cpp/lane.cpp, which require the NPU runtime this box does not have, so my evidence remains static — the constants are the
reader's, the geometry matches :745/:2926, and the ten arms cover both directions across four layouts. Nothing here should wait on the CI queue.

@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.

Re-verified at 399f3678f — the delta since my READY is a de-duplication, not a behaviour change, and I checked it value for value rather than by reading the comment:

q4nx_dequant.h:49  #define Q4NX_TILE_ROWS  32     <- was 32
q4nx_dequant.h:50  #define Q4NX_TILE_COLS 256     <- was 256
q4nx_dequant.h:53  #define Q4NX_TILE_BYTES 5120   <- was 5120
q4nx_dequant.h:63  #define Q4NX_CHUNK_Q4K  4736   <- was 4736
q4nx_dequant.h:64  #define Q4NX_CHUNK_Q8   8704   <- was 8704

So pad_v, col_tiles and is_chunk compute the same numbers as before — the vocab rule is unchanged — and the real gain is the one in the comment: the check now includes the geometry the forward reads with instead of restating it, so it cannot drift from the reader it stands in for. That was the weakest joint in the previous head (five literals in a bounds check that has to agree with another file's parser), and it is gone.

Everything from the earlier verdict stands: the two motivating layouts re-derived (155,200 = 7,760 × 20 tiles → 248,320 rows = vocab; 233,373,696 = 151,936 × 1536 elements), 83 containers through the real Model constructor with 0 refused, nine arms, and the negative control that shows the arms can fail. The plain-table comment now also carries its evidence ("every BF16 table on strixhalo … 78 measured, 11-10"), which is the form I want for every rule in that function.

The Tensor::avail bound is filed as #1880; it is the complement to this fix, not a dependency.

@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 399f367 — re-verdict at the new head, whose commit is "the vocab check reads the forward's tile macros instead of restating them". That is the same correction I have been praising elsewhere (#1857/#1819 family): a check that restates the geometry it is checking will agree with a wrong constant forever, while one that reads the macros disagrees the moment the macros move. +60/+46 with the mismatch cases still asserted.

@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 567aa32 — the substance is unchanged from my review at 399f367; I checked that rather than assuming it.

table_holds_vocab() is byte-identical between 399f367 and this head (diff over the function), so the two things my last verdict rested on still hold: the check
#includes forward/include/q4nx_dequant.h and uses Q4NX_TILE_ROWS/COLS/TILE_BYTES/CHUNK_Q4K/CHUNK_Q8 rather than literals — so it cannot drift from the reader
it stands in for — and the plain-table branch is documented by measurement ("every BF16 table on strixhalo (78 measured, 11-10) has exactly vocab_size rows").

The rest of what moved in this head is main arriving under the branch (CMakeLists, app/main.cpp, other npu/ files, tools/), not this PR's content, so there is
nothing new to judge. My earlier notes stand as non-blocking: a real tiled container would be the only thing that exercises the tiled branch end to end.

@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 567aa325 — of the two PRs closing #1843 tonight, this is the one to merge, and the reason is one sentence in its body.

"Why the issue's literal check was not used." The issue proposes rows != vocab with rows = shape[0]; that is wrong for a padded vocabulary, so table_holds_vocab() accepts both rows == vocab and rows == pad_v (ceil(vocab/32)*32). Refusing a legitimately padded table would break every pack that pads, which is most of them — and an issue's proposed check is exactly the kind of thing a lane copies without noticing that it cannot hold.

It derives the row count from bytes, not from shape alone. t.bytes % row != 0 → "is N B, not a whole number of rows" catches a shape/byte disagreement that a shape-only check cannot see, and then the int4 branch reads the row count from the tile grid (shape[0] / col_tiles * 32 for the 2-D form, shape[0] * 32 for the 3-D), naming what it saw when neither form fits. Covering plain-int8 (bytes == vocab*hidden, the Gemma4 case), the three chunk kinds, and both shape spellings is the complete set, and each refusal names its own reason.

Coordination — this is the fifth duplicate cluster tonight: #1876 also closes #1843, and it is the weaker of the two (shape-derived rows, rows == vocab with a < 32 slack rather than a padded-vocab rule). I have told #1876 to stand down if this merges. If this one stalls, #1876 is a working fallback, but it should then take this PR's padded-vocab rule rather than its own slack.

One note: table_holds_vocab() returning true for vocab <= 0 || hidden <= 0 is the right "cannot tell" default, but it means the check silently does nothing when the config omits the geometry. A single debug line when it bails would make "the check ran and passed" distinguishable from "the check could not run" — the same distinction that cost #1851 a round trip.

@bong-water-water-bong
bong-water-water-bong merged commit 1174a75 into main Nov 10, 2026
8 of 9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the cl-review/1843-vocab-vs-table branch November 10, 2026 09:34
This was referenced Nov 10, 2026
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.

npu: config.json's vocab_size is never checked against the embedding table it overrides

1 participant