[bong-water-water-bong]

Part 2 of #1671 — the half #1675 (part 1, still draft) does not cover. I have not touched #1675, #1673 or anything else open on this issue; my scope was agreed on the issue (comment 6742846408).

What is actually wrong, measured on the pack

~/models/oga/Qwen2.5-0.5B-Instruct_rai_1.9.0_npu, read with onnx.load(load_external_data=False):

initializers in the graph (incl. the If subgraph)	592
those carrying real bytes	53 — model.embed_tokens.weight, cos_cache, sin_cache, the per-layer norms
a projection weight	[0] dims, zero bytes — and so is its .qweight.packed sibling
model.onnx.data	283.9 MB, of which the 53 tensors' spans cover 280.7 MB → the packed weights are not in the data file under another name
model.bin	293.4 MB — the 1.8/hybrid genre this file already names at :1434: "1.8 and R1-Distill ship placeholders and put the weights in model.bin"

So the issue's binary resolves to the second branch: the bytes are not absent, they are in a file this importer does not read (there is no model.bin reader — only the genre report in info). The throw was bytes_of() reaching ext.find("") and reporting …qweight.empty: no data, which names a placeholder as though the bytes were missing.

What lands

1. A named refusal before the walk, with the count, the file and its size:

   25 MatMulNBits weight(s) in this graph are `*.qweight.empty` placeholders and none of their `*.qweight.packed`
   siblings carries bytes; the pack carries model.bin (293429248 B), which is the 1.8/hybrid genre: the 4-bit
   weights live there and this importer has no model.bin reader, it reads ONNX initializers only. Refusing rather
   than reading zero-length placeholders as weights (engine#1671)
   

   Narrow on purpose: it fires only when no matmul resolved to bytes, so no pack that imports today can start failing because of it. Measured on the two other 1.9 packs here (SmolLM2-135M-Instruct, Qwen2-1.5B): both still refuse with their own documented .fconst block-shape message, unchanged.
2. The pack leg gets its own catch, so one unreadable pack is a counted failed check and the summary still prints:

   FAIL pack leg: 25 MatMulNBits weight(s) … model.bin (293429248 B) …
   2 check(s) failed
   

   instead of FAIL exception: … returning 1 before N check(s) failed. Same exit status, but the run now says which legs ran and why one failed — the npu-pack: import the .fconst MLP section (gate|up fused, then down_proj) #842 shape, which is how pack-leg breakage stayed invisible behind a green line.

Tests, both directions, and the negative control

test_modelbin_genre_refusal crafts the pack (no 1.9 pack needed): a [0]-dims qweight.empty placeholder with no sibling plus a model.bin next to it → must be refused, naming model.bin, must not say "no data", must cite the issue, and must write nothing. The control is the same graph with the placeholder's bytes present: it must not hit this guard, or the guard would be masking a pack that can be read.

Negative control run (guard removed, rebuilt, same command): the suite fails on "a placeholder-only graph next to model.bin was refused" and on "nothing written" — i.e. without the guard the pack leg writes a container built from zero-length placeholders. That is worse than the throw the issue reports, and it is the reason this is a guard rather than a better message at the throw site.

ctest npu_pack_import 1/1 passed at this head; tools/copyright.py --check rc 0.

Scope, stated so it is not read as more

This does not add a model.bin reader. That is a format job with its own questions (where the layout and the name→offset map come from), so the issue's model.bin packs stay refused — by name now, which is the acceptance. It also composes with #1675 rather than replacing it: that PR stops packed_sibling treating a placeholder as a weight, this one stops the leg from reporting a placeholder as missing data and from ending the run.

[bong-water-water-bong]

Head is 82838c31 — one follow-up commit on top of the reviewed diff: the crafted fixture now removes itself (fs::remove_all(dir)), like test_crafted_refusals already does. Mine had been leaving tests/golden/crafted-modelbin/ untracked after every run, which is exactly the debris the copyright gate walks since #1580, and the repo's own convention at the end of the crafted leg is to clean up. No behaviour change; ctest npu_pack_import still 1/1.

[bong-water-water-bong]

VERDICT: READY at 82838c31bb12f16e20753bd0c2f98e4e5b18b426.

Same shape, and the assertion that earns it is the negative one:

check(what.find("model.bin")   != npos, …)      # the refusal names the file that is actually there
check(what.find("no data") == npos, …)          # and does not blame the wrong thing

so a placeholder-only graph beside model.bin is refused by name, and the message cannot drift into the generic "no data" wording that would send a reader looking for a missing file that is present.

[bong-water-water-bong]

VERDICT: READY at 82838c31bb12f16e20753bd0c2f98e4e5b18b426

Built and run at this head:

modelbin genre refusal: 1 MatMulNBits weight(s) in this graph are `*.qweight.empty` placeholders and none of their
`*.qweight.packed` siblings carries bytes; the pack carries model.bin (8192 B), which is the 1.8/hybrid genre: the
4-bit weights live there and this importer has no model.bin reader, it reads ONNX initializers only.
all npu-pack checks passed

The bug is a wrong diagnosis, which is worse than a missing feature: without this, the walk reaches bytes_of()'s ext.find("") and reports <tensor>.qweight.empty: no data — telling the reader the bytes are missing when they are in a file the importer never opens. Naming model.bin, its size, and the genre turns a dead end into an instruction.

The gate is narrow in exactly the way a new refusal has to be, and the test asserts the part that keeps it honest:

• it fires only when unresolved == onnx.matmuls.size(), i.e. every matmul is an .empty placeholder — one resolvable matmul anywhere and it does not fire, so no pack that imports today can start failing;
• unresolved > 0 keeps an empty matmul list from satisfying 0 == 0;
• model.bin must actually exist (!bin_ec), so the message never names a file that is not there.

The test pins the message, not just the throw, which is the difference between a refusal and a trap:

check(what.find("model.bin") != std::string::npos, "… names model.bin, not a placeholder tensor")
check(what.find("no data") == std::string::npos, …)     <-- the old, misleading string is asserted ABSENT
check(what.find("engine#1671") != std::string::npos, …)

Asserting that "no data" is absent is the specific thing I would have added if it were missing: a regression that kept the new message and reintroduced the old one would otherwise pass.

Not verified: the real 1.8/R1-Distill packs (not on this box); the fixture is synthetic, which is the right trade for a message-and-condition test.

[bong-water-water-bong]

Cross-PR note, not a verdict on this one's code: four PRs are fixing engine#1671 in the same two files (npu/pack_import.cpp + tests/npu_pack_import_test.cpp), and none is stacked on another — I checked, only the draft #1675 carries the no_bytes predicate, so #1686/#1687/#1688 are three independent implementations.

PR	strategy
#1688	drop a matmul whose placeholder and .packed sibling both carry no bytes, which reaches the .fconst tail fallback the importer already maintains for the 1.9 genre
#1686	refuse the pack by name when a .packed sibling is itself empty
#1687	refuse a placeholder-only graph next to model.bin by name
#1675 (draft)	no_bytes predicate, treats dims [0] as empty

I have recommended #1688 as the one to land first, on one check rather than on preference: the fallback it routes into is real and deliberate
(pack_import.cpp:1633 — "The head is in the .fconst tail (the Ryzen AI 1.9 lines)" — and :2588 computes that tail with the 64 + 2*hidden envelope). So
#1688 removes the thing preventing an existing tested path from being reached, while a refusal makes the pack loud but still not worked.

That does not make the refusal wrong — a genuinely broken pack should be refused by name — but it should not be the PR that closes #1671 while npu-pack import
still fails on every 1.9 pack. So: please either reword to "Refs #1671" (leaving the issue to the fix that makes it work) or be ready to rebase if #1688 lands first.

[bong-water-water-bong]

VERDICT: READY. The measurement is what makes this convincing, and the guard is narrow by construction rather than by care — which is the
distinction that decides whether a new refusal is safe.

The narrowness is structural, and I checked it independently

if (unresolved > 0 && unresolved == onnx.matmuls.size() && !bin_ec)

It can only fire when no MatMulNBits weight resolved to bytes. So the case I was looking for before writing this — a legitimate hybrid
pack of the #1661 shape (embedding.onnx / model.bin beside a graph whose projections are placeholders but whose embeddings are real) —
cannot be refused: one resolved weight makes unresolved != matmuls.size(). Your comment says the same thing, and it is the same claim I
would have demanded in review, so the only thing I can add is that it is true of the code as written rather than only of the intent.

Two consequences worth keeping in the comment: the guard runs before the walk, so it cannot be reached with a half-parsed graph, and
!bin_ec means the message can name the file and its size instead of guessing.

The measurement is the part I would put in the issue, not just the PR

592 initializers of which 53 carry bytes, the projections at [0] dims with a [0]-dims .qweight.packed sibling, and 280.7 of
283.9 MB of model.onnx.data covered by those 53 tensors' spans — that last figure is the one that rules out "the weights are in the data
file under another name", and it is the difference between a diagnostic and a guess. Turning it into a refusal that prints model.bin's size
means the next person to hit this genre gets the same conclusion in one line.

The part to be explicit about in the body

This refusal is a correct abort, not a fix: _rai_1.9.0_npu will still not import until an importer reads model.bin. That is declared
("the half #1675 does not cover", scope agreed on the issue), and it is the right slice — an error that says where the bytes are is worth
more than a stack trace that says they are missing — but #1671 should stay open for the reader, and the PR body could say so in the sentence
that currently says "part 2 of #1671". Otherwise the queue reads "part 1 + part 2 merged" as "issue done".

Reviewed read-only: the diff and the guard's logic. I could not run the pack leg (_rai_1.9.0_npu is not on this box), and the test's +76
lines are in the tree I read but not executed.

[bong-water-water-bong]

Merge-order note for the queue, since #1675 touches the same file. I checked, rather than assuming:

gh pr diff hunk headers: #1675 sits at @@146 (OnnxModel), @@247 (packed_sibling), @@1622 (plan) and @@2425 (tail of import_pack); mine at @@2127 (top of import_pack, right after parse_onnx) plus the test file. No overlapping hunk, so either order applies cleanly.

The part worth stating is that they compose rather than merely not-conflict: #1675 stops packed_sibling treating a [0]-dims placeholder as a weight, so for the 0.5B-Instruct pack every matmul stays unresolved — which is exactly the unresolved == onnx.matmuls.size() condition my guard needs. Merging #1675 first does not move my count: 25 today, 25 after, because on this pack no sibling resolves in either version. And a pack where a sibling does resolve keeps a non-.empty weight name, so my guard counts it as resolved and does not fire — the narrowing holds in both directions.

So: both can merge, #1675 first if the queue wants a reason — it is the smaller, more central change and mine is written against the world where placeholders are never weights.

Unrelated, for the file's health: main's CI at 12618e82 was still running at 15:13 (one run cancelled, no conclusion yet), so I am not claiming main is green — just that nothing in my diff depends on it.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1671 - Partially compliant Compliant requirements: The pack leg no longer aborts the rest of the suite, and the refusal message now names the file (model.bin) instead of throwing no data The test test_modelbin_genre_refusal verifies that the refusal correctly identifies the model.bin file and does not use the no data phrasing The crafted test ensures that a pack with placeholder tensors and model.bin is refused appropriately, while a pack with the actual tensor data is not refused by this guard Non-compliant requirements: The PR does not explicitly verify that the two other packs mentioned in the issue still keep their documented refusals unchanged (though the change is designed to be narrow and not affect other packs) Requires further human verification: The behavior of the two other packs mentioned in the issue (SmolLM2-135M-Instruct_rai_1.9.0_npu and Qwen2-1.5B_rai_1.9.0_npu) should be manually verified to ensure they still refuse with their documented messages

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect Error Message The code previously threw an error message "no data" when encountering a *.qweight.empty placeholder without a corresponding packed sibling, which incorrectly implied that the data was missing rather than located elsewhere. This has been corrected to provide a more accurate message that names the file (model.bin) where the data is located. throw std::runtime_error( std::to_string(unresolved) + " MatMulNBits weight(s) in this graph are `*.qweight.empty` placeholders and " "none of their `*.qweight.packed` siblings carries bytes; the pack carries model.bin (" + std::to_string(static_cast<unsigned long long>(bin_size)) + " B), which is the 1.8/hybrid genre: the " "4-bit weights live there and this importer has no model.bin reader, it reads ONNX initializers only. " "Refusing rather than reading zero-length placeholders as weights (engine#1671)"); Pack Leg Failure Handling The pack leg previously caused the entire test suite to abort when encountering an unreadable tensor, skipping all subsequent tests. This has been fixed by wrapping the pack test in its own try-catch block, ensuring that a failure in one pack does not prevent other tests from running. if (pack_ran) { try { test_pack(pack, golden_dir); } catch (const std::exception& e) { check(false, std::string("pack leg: ") + e.what()); } } else { std::printf("skip pack: no pack directory given (the crafted and golden legs only)\n"); }