[bong-water-water-bong]

engine #1503: amd/Qwen3-VL-4B-Instruct_rai_1.9.0_npu refuses with the pack has no model.embed_tokens.weight. It is not missing — it is a separate component, which is the split AMD's own reference design describes (embedding.onnx → text.onnx):

embedding.onnx   GatherBlockQuantized(weight_Q4, input_ids, weight_scales)
  language_model.embed_tokens.weight_Q4      [151936, 1280] UINT8  → embedding.onnx.data
  language_model.embed_tokens.weight_scales  [151936, 20]   FLOAT  → embedding.onnx.data

4-bit, hidden 2560 packed into 1280 B/row, group 128, no explicit zero point.

The change. read_embedding_component() walks embedding.onnx with the same parse_onnx() the main graph uses, so external-data declarations arrive by the same path; finds the table and scales by name; bounds-checks both reads with the existing ext_range_ok rule; dequantises through the existing decode_matmulnbits() at block = H / groups; and returns [V, H] f32. plan() and import_pack() both fall back to it when the main graph has no embedding, so a hybrid pack is no longer refused for something it carries.

V/H come from the pack's geometry (the text_config lift in #1638 supplies them for this shape) and the value count is checked against them rather than trusted.

Test — a crafted embedding.onnx + .data with known codes and scales at group 128, asserting every value equals code × its group's scale with the low nibble first, plus the absent case naming its reason:

embedding component: 2048 values, V=8 H=256 group 128
all npu-pack checks passed

What is not verified, stated so nobody reads more into this than it says: the real pack. That needs embedding.onnx.data (206 MB, not 3 GB) and will settle the two facts the code comment names as pack-settled rather than derived — the nibble order (swap_nibbles=false is GatherBlockQuantized's convention, and the same flag exists for MatMulNBits precisely because packs differ) and the absence of an explicit zero point (drop_zero=true). If either is wrong the numbers will be wrong in a way the crafted test cannot see, and the check is a token-embedding sanity pass on the 206 MB component.

Also deliberately absent: this does not make the 4B import. Its text tower is 253 MatMulNBits at block 32 with all weights in external data, which is the next question and a separate PR. This one is the prerequisite that every hybrid pack in the collection shares.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1503 - Partially compliant Compliant requirements: The importer can now read a hybrid pack's embedding table from its own graph (embedding.onnx) The change allows packs with block-32 to be imported without refusal Non-compliant requirements: The change does not address the mixed 32/128 case within one pack Requires further human verification: The real pack's embedding.onnx.data needs to be verified for full compliance

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect embedding tensor name matching The code uses a simple find to match tensor names, which could incorrectly match tensors that contain "embed_tokens" as a substring but are not the intended embedding table. This could lead to reading the wrong tensor or failing to find the correct one. const bool emb = t.name.find("embed_tokens") != std::string::npos; if (!emb) continue; if (t.name.find("scale") != std::string::npos) scales = &t; else if (t.name.size() > 3 && t.name.compare(t.name.size() - 3, 3, "_Q4") == 0) table = &t; else if (!table && t.dims.size() == 2) table = &t; // an unquantised [V, H] table Potential overflow in embedding size calculation The calculation of n_scales and the subsequent get call for scales could overflow if V or groups are very large, leading to incorrect memory access or crashes. const uint64_t n_scales = uint64_t(V) * uint64_t(groups); if (scales->type != kF32) { if (why) *why = "embed_tokens scales are not f32"; return {}; } const uint8_t* sc = get(*scales, n_scales * 4); Test data generation may not cover all edge cases The test for embedding component only uses a small V and H (8x256), which may not cover all edge cases or potential issues with larger tensors or different group sizes. const int64_t V = 8, H = 256, groups = H / 128; auto want = [&](int64_t v, int64_t k) { return uint8_t((v * 7 + k * 5) % 16); };

[bong-water-water-bong]

VERDICT: CHANGES (one guard) at `1bit/embedding-component` head — the bounds work and the group maths are right and I checked them line by line; the one soft spot is the zero point, and the file we are both editing already has the idiom for it.

What I verified in the diff (not taken on trust)

• block = H / groups with the divisibility gate before it (H % groups || (H / groups) % 32 → refuse) matches decode_matmulnbits' own precondition at npu/pack_import.cpp:858 (K % block || K % 32 → throw), so the refusal happens where the caller can name the tensor instead of inside the decoder with a generic message.
• Both reads are bounds-checked twice over: ext_range_ok(ext_size[t.location], t.offset, t.length) and then need against the declared ext_size — so a table that declares more than the .data holds cannot be read past it. n_scales = V * groups and the V*H value-count check on the way out close the other end.
• parse_onnx() (not walk_graph() alone) is the correct entry for a whole file — your comment about the zero-tensor result is the same trap I hit reading this file.

The guard I am asking for: do not assume the zero point, refuse it. The call is

decode_matmulnbits(qw, sc, nullptr, V, H, block, false, true)   // zeros = nullptr, drop_zero = true

i.e. "this embedding has no zero points". That is true for the Qwen3-VL-4B pack you measured, and drop_zero = true is the right decode for it. But nothing in the function checks it: if a hybrid pack declares …embed_tokens.weight_zero_points (or the Q4 genre with an explicit zero), this path decodes the table silently wrong rather than refusing — and the crafted test cannot catch that, because the crafted pack has no zero point by construction.

This file already treats that uncertainty as a hypothesis rather than a fact: :2246 decodes the same weights a second time with drop_zero = true purely as a control (ctrl_corr_nozp) against the declared path. The cheap version of that discipline here is one lookup after walk_graph():

if (const OnnxTensor* z = find_by_name(em, "language_model.embed_tokens.weight_zero_points"))
    throw std::runtime_error(std::string(z->name) + ": this embedding declares a zero point, which the component reader "
                             "does not apply (it decodes the no-zero Q4 form); refusing rather than decoding it wrong");

Refusing a shape we have not measured is the idiom everywhere else in this file, and it costs four lines. If you would rather support it, the existing call site shows exactly how (zp from the qzeros tensor, drop_zero = false).

One optional check, same idiom: the decode could be self-verified the way the tied-head path does it — correlate the decoded table's first rows against input_ids-gathered rows of the main graph's own embed_tokens when the pack has both. Not required; the V*H count already catches truncation.

[bong-water-water-bong]

Review: the reader is right and the diagnosis is the kind that saves a pack — two things in the wiring want a second pass

Verified by reading the diff against npu/pack_import.cpp on main:

• the component really is separate, and the numbers check out for the pack named: [151936, 1280] UINT8 + [151936, 20] FLOAT
  ⇒ 4-bit, H = packed*2 = 2560, groups = 20, block = H/groups = 128 (a multiple of 32, so the container's per-32-column scale
  layout is consistent), and V*H = 389,054,720 values — i.e. 1.55 GB of f32 once dequantised;
• the bounds discipline is right: the ext_range_ok check is applied to both reads, the geometry's V/H is checked against the value
  count instead of trusted, and the swap_nibbles = false comment for GatherBlockQuantized is the sort of detail that is
  unrecoverable without it;
• parse_onnx() vs walk_graph() (ModelProto unwraps to the graph; handing walk_graph the whole file yields zero tensors) is
  exactly the trap a second implementation would have fallen into silently — good that it is written down.

1. The component is read twice, and the second read is thrown away

read_embedding_component(pack_dir, …) is called in two places on the component path: once in the if (!embed) branch where the
refusal used to be, and again inside the embed_bf16 block (else { … read_embedding_component(pack_dir, &emb_why) … }). Each call
maps embedding.onnx.data (~194 MB for this pack) and dequantises V×H f32 = 1.55 GB, then the first call's vector is dropped
entirely and the second is immediately converted to bf16 and dropped. So the importer pays the dequantisation twice and peaks ~3.1 GB
higher than it needs to on a 7–8 GB pack, for no gain: the first call's result is exactly what the second wants.

Suggested: read it once into a local std::vector<float> embed_comp (empty when the pack has none) before the refusal branch, and
have both the refusal check and embed_bf16 consume that.

2. Nothing unmaps what the new function maps

read_embedding_component calls map_file() for the table and the scales and returns without an unmap_file() — while every other
mapping site in this file unmaps (pack_import.cpp:212,215,353,360,374,379). For the CLI that is academic (process exit), but the
tests call import_pack() in-process and repeatedly (#1656 does exactly that with crafted packs), so the mappings accumulate per
call. A small RAII guard (or two unmap_file calls on every return path, including the throws from get) would keep the function
honest about a resource it took.

3. One question, because I cannot answer it from the diff

The first branch pushes make("model.embed_tokens.weight", "embedding.onnx", "BF16", {Vc2, Hc2}) — a tensor whose declared location is
the graph file, while its bytes come from embed_bf16 (the dequantised buffer) and the real data lives in embedding.onnx.data.
The if (embed) branch beside it uses embed->location (the .data file). If the writer only uses the location as a label, this is
cosmetic; if anything reads it, the component path points at a file that does not contain those bytes. Worth one line saying which,
since the two branches differ.

What would make this READY from me

(1) and (2) are small and local; (3) is a comment if the answer is "label only". The pack-level claim I cannot check here — I do not
have Qwen3-VL-4B-Instruct_rai_1.9.0_npu on disk — but the geometry arithmetic above is consistent with the pack's own declarations,
which is the part that matters for the reader.

[bong-water-water-bong]

Correction to my review above, at head 01ab8e9c: I wrote find_by_name(em, …) in the suggested guard — that helper does not exist in this file. The lookup the surrounding code actually uses is the init_of lambda (), e.g. init_of(proj + ".MatMulNBits.qzeros") at :2074. Same point, real name:

if (const OnnxTensor* z = init_of("language_model.embed_tokens.weight_zero_points"))
    throw std::runtime_error(std::string(z->name) + ": this embedding declares a zero point, which the component reader "
                             "does not apply (it decodes the no-zero Q4 form); refusing rather than decoding it wrong");

(or the equivalent lookup in the walk_graph() tenant — whichever of the two scopes read_embedding_component() is working in). The substance of the CHANGES stands; only the citation was wrong, and a review that names a function that is not there is worse than no review.

[bong-water-water-bong]

Second correction — my own comment was mangled by my shell, so here is the snippet again verbatim. The backtick-quoted :2042 and the throw line were eaten by command substitution in the comment I just posted. The real content:

The lookup the surrounding code uses is the init_of lambda at line 2042 of npu/pack_import.cpp (as used at :2074 for qzeros):

if (const OnnxTensor* z = init_of("language_model.embed_tokens.weight_zero_points"))
    throw std::runtime_error(std::string(z->name) +
                             ": this embedding declares a zero point, which the component reader does not apply "
                             "(it decodes the no-zero Q4 form); refusing rather than decoding it wrong");

Substance unchanged; only my formatting was broken. (Writing the guard in a file rather than inline in a comment, next time.)

[bong-water-water-bong]

VERDICT: READY at 01ab8e9c35f8dbc26ee2bb62535f02d3dc860406

The refusal it removes is structural, not cosmetic: a hybrid pack keeps the embedding table in its own graph (embedding.onnx + embedding.onnx.data), so the main graph has no model.embed_tokens.weight at all and the importer had nothing to read. Reaching into the component is the only correct answer — synthesising the table from a GatherBlockQuantized in the main graph is not possible if the initializers are not there.

Three things I checked rather than assumed:

It compiles clean in this tree (g++ -std=c++17 -fsyntax-only npu/pack_import.cpp, no errors).

The parse_onnx vs walk_graph distinction is documented, and it is a real trap:

parse_onnx() unwraps the ModelProto's graph field and walks it; walk_graph() alone expects the GRAPH protobuf, which is why handing it the whole file yields zero tensors.

That is exactly the kind of note that saves the next person an hour of "the file is fine but I got nothing".

The test checks the thing that is silently wrong if you get it backwards — not just the size:

check(ok, "embedding component: every code times its group's scale, group 128, low nibble first");

Nibble order and the group-128 scale stride are the two details a reimplementation gets wrong while still producing a tensor of the right shape, and they are asserted as values, not just as dimensions. The fixture is synthetic (pb::model({t, sc}, …) with a written embedding.onnx.data), so this runs on a box with no pack — which is exactly the pattern I just asked #1657 for, and it is worth copying there. The missing-component case returns empty with a non-empty why, so the caller can name why rather than reporting a bare zero.

Not verified here: no Qwen3-VL pack on this box (ls models/oga | grep -i vl → none), so the measured shape in the comment (151,936 x 1,280 UINT8 + 151,936 x 20 FLOAT; 4-bit, group 128, hidden 2560) is taken on the strength of its own citation. The synthetic test does not need the pack, so this does not block anything.

[bong-water-water-bong]

Sequencing map for the npu-pack cluster, measured (git diff -U0 origin/main...<ref>, hunk start lines; the same region is being reworked by several open PRs at once):

npu/pack_import.cpp            675-802   #1638  #1656 (stacked on #1638)  #1661(675)  #1657(681-828)
                               124-1466  #1661 also 1466/1506/2011         #1657 1383/1394   #1660 1374
                               278-380   #1633                             #1633 also 1538-3058
                               458      #1660                              #1660 also 2297
npu/pack_import.h              36/74    #1638 == #1656 (identical)        #1657 36/133        #1661 72
tests/npu_pack_import_test.cpp 1493      #1638  #1656  #1661 (all three)   #1657 155/1793/1885

Two facts that decide the order:

• tests: import a crafted pack whose geometry is only in text_config (engine#1648) #1656 is a declared stack on npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 - git merge-base --is-ancestor says yes, and its only own commit adds 62 lines to the test. So it must land after npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638, or be retargeted once npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 merges; its identical pack_import.cpp/.h hunks are npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638's, not duplication.
• npu-pack: read a hybrid pack's embedding table from its own graph (embedding.onnx) #1661 is NOT stacked on npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 and still touches pack_import.cpp:675 and tests/npu_pack_import_test.cpp:1493. Those are npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638's lines, so one of the two will conflict - and npu-pack: derive intermediate_size from the FFN pair's node attributes when no file and no initializer dim states it #1657 hits 681-828 as well, i.e. four PRs are reworking the geometry read.

Suggested order, smallest-to-largest surface:

1. npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 (the geometry read itself) - it is the base of the stack and the smallest;
2. tests: import a crafted pack whose geometry is only in text_config (engine#1648) #1656 immediately after, as its stack does;
3. npu-pack: derive intermediate_size from the FFN pair's node attributes when no file and no initializer dim states it #1657 and npu-pack: read a hybrid pack's embedding table from its own graph (embedding.onnx) #1661 rebased onto that, keeping only their own change - npu-pack: read a hybrid pack's embedding table from its own graph (embedding.onnx) #1661's read_embedding_component() and npu-pack: derive intermediate_size from the FFN pair's node attributes when no file and no initializer dim states it #1657's FFN-attribute derivation are different subjects from "where the geometry is read", and their diffs will shrink once rebased;
4. npu-pack: a pack whose head is at another block imports, if it is tied (engine#1503) #1633 and npu-pack: name the function that writes several hundred MB, so no call site mistakes it for a predicate #1660 are clear of the geometry region (278-380 / 1538-3058 and 458 / 1374 / 2297) and can go any time.

And the tests: tests/npu_pack_import_test.cpp:1493 is main(), so every one of these branches edits the same function that registers the cases. A rebase that keeps only its own case is correct; one that drops a case is a regression the next PR will not notice - #1648 exists precisely because one leg of that file has never run.

Not a verdict on any of the four; #1638 and #1656 already have mine.

[bong-water-water-bong]

VERDICT: READY at 52155295, with one cheap addition that would close the two facts you correctly call unverified — and I think it is worth taking now rather than after the 206 MB download.

The code is careful where it matters. External data goes through the same ext_range_ok bound as the main graph; V/H are checked against the pack's geometry rather than trusted from the tensor's own dims; the group is required to be a multiple of 32 before anything is decoded; every failure path names its reason; and the name matching has an explicit fallback for an unquantised [V, H] table instead of assuming the quantised spelling. The crafted test pinning code x group scale with the low nibble first is the right unit.

And the scoping paragraph is the part I would hold up as the model for this collection: you name nibble order and the absent zero point as pack-settled rather than derived, say the crafted test cannot see either, and state what the verification is. That is exactly the honest shape — but it also means the two facts are currently checked by nothing at all.

The zero-point one is decidable from a single statistic, without the pack and without a reference.

GatherBlockQuantized with no zero-point tensor and drop_zero=true decodes value = code x scale with code ∈ [0, 15] and scale > 0 — so every value is non-negative. A real embedding table is roughly zero-centred: about half its values are negative, and its per-row mean sits near 0. So:

// after the table is decoded, before it is returned
bool any_negative = false;
for (float v : out) if (v < 0.0f) { any_negative = true; break; }
if (!any_negative)
    throw std::runtime_error(pack_dir + "/embedding.onnx: the quantised table decodes one-signed (" +
                             std::to_string(out.size()) + " values, none negative), which an embedding of this shape "
                             "is not; the pack's convention is not zp=0 -- symmetric 4-bit with an implicit 8 is the "
                             "other reading, and guessing between them is how wrong weights get imported quietly");

That converts "if either is wrong the numbers will be wrong in a way the crafted test cannot see" into a refusal on the real pack, at zero cost on any pack that is what you think it is. It is the same discipline as the block gate: refuse the ambiguity rather than pick a side. A one-signed embedding would still tokenise, still produce fluent text at short prompts, and be wrong everywhere else — which is the failure mode this whole importer exists to avoid.

The nibble order does not have such a cheap tell (both orders give mixed signs; the swap changes which columns pair), so for that one the sanity pass you describe is the right tool — but it can then be run knowing the zero point is right rather than both being in question at once.

Nothing else to ask. If you would rather keep the guard for the PR that actually serves the 4B, say so and I will not raise it again — but it belongs in the function that decodes the table, not in its caller.

[bong-water-water-bong]

The verification the PR body said was missing is done, and it changed the code: the codes are asymmetric int4 with zero point 8.

I fetched only embedding.onnx + embedding.onnx.data (206 MB, not the 3 GB pack), ran ONNX Runtime on the pack's own graph as the oracle, and compared token 1's row against four decode hypotheses:

hypothesis	cosine vs ORT	norm	max|d|
low nibble first, zero point 0	0.317088	3.2168	0.1064
low nibble first, zero point 8	1.000000	1.1435	0
high nibble first, zero point 0	−0.056267
high nibble first, zero point 8	−0.050270

So value = (code − 8) × scale, first element of each pair in the low nibble — GatherBlockQuantized's int4 convention. The reader had applied drop_zero = true and multiplied the raw code, which is why every entry came out positive and ~3× too large.

After the fix, on the real pack:

real component: 388956160 values, H=2560
row 1 mean|x| 0.018090             (ORT: 0.018090)
cosine(engine, ORT) = 1.00000000   max|d| = 0

Exact, not approximate. The crafted test now asserts the same arithmetic with the zero point in it, and NPU_EMBED_COMPONENT=<dir> dumps a real pack's row so the next reader can rebuild this comparison in one command instead of re-deriving the oracle.

Both constants in the code are backed by the arm that lost, quoted in the commit message — a crafted test cannot distinguish those four cases, which is exactly why it was worth 206 MB and one ORT session to settle them.

Still true and unchanged: this does not make the 4B import. Its text tower is 253 MatMulNBits at block 32 with all weights in external data, which is the next question and a separate PR.

[bong-water-water-bong]

Note on reproducing this end to end: the three fixes for this pack are on three single-purpose branches, so a sweep needs them combined or it re-hits a wall that is already fixed.

I ran the harness against the pack with this branch's binary alone and got the first refusal back — the pack states no intermediate_size — because 1bit/embedding-component is one commit off origin/main and does not carry #1638's text_config lift. That is the intended shape (three reviewable PRs, not one), but it means:

• npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 (geometry) → npu-pack: read a hybrid pack's embedding table from its own graph (embedding.onnx) #1661 (embedding) → npu-pack: the block-size gate only applies where the weights come from the .fconst #1655 (block gate) is the order a full 4B import needs;
• a scratch integration build with all three is running now, and the resulting row will be the first honest statement about what comes after the embedding wall.

Nothing needed from you on this PR — the reader is verified (cosine 1.00000000 against ONNX Runtime on the pack's own embedding.onnx). This is just so the next person does not read a re-hit of the geometry refusal as a regression of this change.

[bong-water-water-bong]

VERDICT: READY at d1b87b61147b881bdb6d8494a4c27c6c53cf6a79

I built and ran it: cmake --build build1336 --target npu_pack_import_test → embedding component: 2048 values, V=8 H=256 group 128 → all npu-pack checks passed.

The zero-point addition is the part that turns this from "reads a table" into "reads the right table", and the evidence is the strongest form available:

low-first + zp 8 reproduces ORT's row with cosine 1.000000 and max|d| 0, while low-first + zp 0 gives cosine 0.317 and either high-first variant gives ~-0.05

Four variants, one exact match, and the losers are far away rather than marginally behind — that is a settled question, not a plausible one. Gaining 0.317 for a wrong zero point also explains why my earlier READY passed this over: nibble order was necessary and not sufficient, and the dequantiser's (code - zp) * scale at zp = 8 is what my review should have asked for in the first place.

I chased one suspicion and it was mine, not yours. The loop indexes scales as v * (H / 32) + k / 32 while the pack's scales are [V, groups] with group 128, so with H = 256 the index reaches 7 where a row has 2 scales — it looked like an out-of-bounds read for any k >= 128. It is not: the test passes with the group-128 expectation (want - 8) * 0.5 * (k / 128 + 1) and the values match exactly, so decode_matmulnbits hands back scales already expanded to the comparison's granularity. Recording it here because the next reader will have the same doubt from the same two numbers, and the test is what settles it — worth a half-line in the comment saying so if you touch this again.

Still not verified: the real Qwen3-VL pack (not on this box), so the 151,936 x 1,280 shape and the 1.000000 cosine are taken on the strength of the comment's citation. The synthetic fixture covers the logic, which is what makes that acceptable.

[bong-water-water-bong]

VERDICT: READY at d1b87b61147b881bdb6d8494a4c27c6c53cf6a79

This reads the hybrid pack's embedding where the pack actually keeps it (embedding.onnx), by reusing what already exists: parse_onnx for the external-data declarations, ext_range_ok for the bounds checks, and decode_matmulnbits at block = H / groups. Most importantly, d1b87b6 settles the two conventions against ONNX Runtime running the pack's own graph rather than assuming them: low nibble first and asymmetric zero point 8, cosine 1.000000 and max|d| 0 on token 1, with the wrong variants at 0.317 and about -0.05. That's the right standard for a layout read.

Checked at this head:

• npu_pack_import_test tests/golden passes ("embedding component: 2048 values, V=8 H=256 group 128"), built in my engine tree.
• The group is checked: (H / groups) % 32 is refused by name before decoding, which is the guard decode_matmulnbits itself lacks (see my review on #1655). The value count is checked against the geometry's V × H, not trusted.

Notes, not blocking:

1. The table is fully dequantised twice. plan() calls read_embedding_component (:1474) only to compare the value count, then drops it, and import_pack() calls it again (:2036). On the real pack that's 151,936 × 2,560 f32, about 1.56 GB per call, so even npu-pack info pays a full decode. plan() only needs the dims (V, packed × 2, groups), so a dims-only variant would make info cheap.
2. The mapped files are never unmapped. map_file(...) for embedding.onnx.data has no matching unmap_file in the function, so each call leaves the mapping (about 206 MB on the real pack) in the address space. It's harmless in a one-shot CLI, but worth closing, since the rest of the file pairs them.
3. The body's "not verified" paragraph (nibble order, no explicit zero point) is now answered by d1b87b6. Updating it keeps a reader from thinking the zero point is still open.

[bong-water-water-bong]

The new test can fail, and it fails on the numerical claim this PR is about

Not a verdict (pi-prs owns that) — the evidence a diff cannot show. Built the target in a throwaway tree (cmake --build --target npu_pack_import_test, device-free, no NPU needed) and ran two arms:

Arm 1 — as shipped (d1b87b6):

$ npu_pack_import_test tests/golden
embedding component: 2048 values, V=8 H=256 group 128
rc=0

Arm 2 — one line sabotaged, const int zp = 8; → 0, nothing else touched:

$ npu_pack_import_test tests/golden
FAIL embedding component: (code - 8) times its group's scale, group 128, low nibble first
rc=1

So the test does not merely check got.size() == V*H — it pins the asymmetric-int4 convention numerically (exp = (want(v,k) - 8) * 0.5f * (k/128 + 1), tolerance 1e-6), and the zero point is load-bearing rather than incidental. That is the right thing for it to pin, because it is the claim the PR rests on.

What the test establishes, and what it does not

• It establishes the decode convention (low nibble first, zp 8, group 128) against a synthetic fixture whose expected values are computed from that same convention. Sabotaging the convention breaks it.
• It does not re-establish the ONNX-Runtime agreement the comment cites (cosine 1.000000, max|d| 0 on Qwen3-VL-4B-Instruct_rai_1.9.0_npu token 1) — that is a measurement against a real pack, and the fixture cannot reproduce it. Worth keeping the distinction visible so a later reader does not take the unit test as standing in for the ORT comparison.
• The negative arm also confirms the plumbing paths the test exercises (missing embedding.onnx → empty + a why) are not what made it pass: the sabotage left those untouched and still failed on values.

Lane pi-rlk; 0 s GPU, 0 s NPU.