[bong-water-water-bong]

Refs #1503, and the owner's 11-09 pack order (my packs: gpt-oss-20b, phi-4). The census half: npu-pack info can now see the 1.9 external-data genre and report its weight layout as a measurement. No import behaviour change.

Why the genre looked weightless

amd/phi-4_rai_1.9.0_npu carries 9.73 GB of ordinary ONNX external data and 201 quantised matmuls — but the reader reported a graph with no weights, for three independent reasons, all visible only against the real pack:

1. the op name. The walker matched com.microsoft:MatMulNBits; phi-4's 201 weights are all com.ryzenai:MatMulNBitsBf, so matmulnbits_ops was 0.
2. the spelling. A node names its tensor /model/layers.0/attn/qkv_proj/… while the initializer is model.layers.0.attn.qkv_proj… (201 of 201 resolved by normalising /→. and dropping the leading slash).
3. the placeholder. The node's weight slot holds …qweight.**empty** with dims [0], and the bytes are in a …qweight.**packed** sibling the node never references. A dims.empty() test calls [0] a real weight of length zero.

What info says on the real pack now

"graph_weights": {
  "matmulnbits_ops": 201,
  "ryzenai_matmuls_ops": 201,
  "packed_weight_siblings": 201,
  "real_weight_initializers": 201,
  "block_size": 128,
  "external_tensors": 285,
  "external_row_stride": { "matched": 201, "of": 201, "rule": "length == N * (K/128 + 1) * 68" }
}
"dims": { "intermediate_size": 17920, ... }

201 of 201 is the point, not a decoration: the same 68-byte per-row-per-128-K block is the stride the .fconst attention tile already carries (2,176 B subvolume = 32 rows × 68 B), so the reader this issue is about walks a layout the importer already decodes rather than a new one. It also carries the tile's +1 trailing block per row.

intermediate_size for this genre comes from the node's own N/K (gate_proj/up_proj → N, down_proj → K) — a 1-D byte blob has no shape to read it from, and this is the pack's declaration rather than a guess from other dimensions (the rule that refusal path already states).

Verified

check	result
ctest-side test (new, synthetic fixture in the genre)	placeholder resolves to its .packed sibling; the stride rule reports 1 of 1 when the bytes fit and 0 of 1 when they do not (mismatch reported, no crash, rule named)
a pack whose config states no intermediate_size	readable, FFN width from the nodes
the whole existing suite	all npu-pack checks passed
the real pack's metadata (no 9.7 GB download)	numbers above, straight from build/1bit npu-pack info …/phi-4_rai_1.9.0_npu

What this does not do, deliberately

It does not import the genre. Writing the container from N × (K/128+1) × 68 is the next PR and needs the 9.7 GB payload; splitting the census from the writer keeps each one checkable — and the census is what tells the next reader whether the stride is an assumption or a fact on each pack.

docs/npu.md ("From a Ryzen AI OGA NPU pack") gains the paragraph that says a pack can state its weights behind a placeholder, with these numbers.

For gpt-oss-20b the answer is different and I have not touched it: its 12.23 GB eager.bin is referenced by no initializer, eager.pb.bin (12.9 KB) is an index (tensor path → op name) handed to AMD's runtime as external_data_file, so it needs the AMD hybrid blob layout — its own item, recorded on #1503.

[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 now recognizes com.ryzenai:MatMulNBitsBf ops in addition to com.microsoft:MatMulNBits The importer resolves *.qweight.empty placeholders to their *.qweight.packed siblings The importer reports ryzenai_matmuls_ops and packed_weight_siblings counts The importer measures and reports the external row stride to match length == N × (K/128 + 1) × 68 The importer correctly handles the FFN width from node's N/K attributes for external-data genre Non-compliant requirements: The PR does not explicitly address the block-32 vs block-128 handling in terms of different decoders Requires further human verification: The claim about block-32 handling and different decoders needs verification on actual hardware The performance impact of the new resolution logic needs to be measured

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Placeholder Resolution Logic The new logic for resolving *.qweight.empty placeholders to *.qweight.packed siblings may have performance implications, especially in the loop that iterates through all matmuls. The resolution involves string operations and map lookups that could be expensive in a tight loop. While the code is correct, it's worth noting that this logic is executed for every matmul, and could potentially be optimized if it becomes a bottleneck. for (auto& mm : m.matmuls) { if (mm.weight.empty()) continue; const OnnxTensor* w = find_init(mm.weight); if (w && !no_bytes(*w)) { mm.weight = w->name; // the initializer's own spelling, so every later lookup is exact continue; } if (!w) continue; // names nothing this graph declares: not this genre's business std::string packed = w->name; if (packed.size() >= 6 && packed.compare(packed.size() - 6, 6, ".empty") == 0) packed.replace(packed.size() - 6, 6, ".packed"); else continue; const OnnxTensor* pw = find_init(packed); if (!pw || no_bytes(*pw)) continue; mm.weight = pw->name; ++m.packed_siblings; } External Data Stride Validation The stride validation logic checks if the external tensor length matches N × (K/128 + 1) × 68. This is a critical validation for ensuring correct data layout, but it's only applied to tensors with block == 128 and bits == 4. If other block sizes or bit configurations are encountered, they may not be validated, potentially leading to incorrect data interpretation. if (!w.location.empty() && mm.block == 128 && mm.bits == 4 && mm.K > 0 && mm.N > 0) { p.graph_stride_of++; if (int64_t(w.length) == mm.N * (mm.K / 128 + 1) * 68) p.graph_stride_ok++; }

[bong-water-water-bong]

VERDICT: CHANGES — on the claim, not on the code. The three findings are excellent and each is evidenced against the real
pack; what has to change before merge is the sentence "No import behaviour change", because the diff contains one.

The plan() hunk is a behaviour change, and it is the most consequential part of the PR

@@ -1309,6 +1373,25 @@ Plan plan(const std::string& pack_dir) adds: when no weight width can be inferred
(best == 0), derive the FFN width from the node's own N/K attributes (down_proj → K, gate/up → N, gate_up →
N/2). That sets intermediate_size for external-data packs that previously had none — i.e. it changes what the importer
computes, not only what info reports. The reasoning in the comment is right (the node's N/K is the pack's declaration for
that projection, not a guess from other dims), which is exactly why the PR's own summary should say so rather than deny it.

Two clean ways to close this, your call:

• correct the body — say the census half plus one import-path derivation, name the test that covers it (there is one:
  npu_plan_test.cpp has the new cases), and leave the diff alone; or
• split the hunk out into its own PR. That also helps the overlap below: plan() is the one region npu-pack: a multi-component pack keeps its geometry in model_config.json, and its graph in a subdirectory #1644/npu-pack: two reader tolerances the 1.9 7-8B packs need — a null optional field, and the measured h4096 block shape #1645 do not touch.

Either way, please do not leave "No import behaviour change" in the body — the next reader (or the release note) will believe it,
and this fleet has already had one incident this week from a summary that did not match its diff.

What is right, and I checked rather than admired

• the placeholder predicate treats dims == [0] as "no bytes here" and says why (the real pack states the length as zero
  rather than stating nothing) — a dims.empty() test would have called that a real weight of length zero;
• the spelling fix is a normalisation, not a fuzzy match: leading / dropped, /→. — exact after normalisation, which is
  what makes 201/201 explainable rather than lucky;
• the packed sibling is resolved where every consumer sees a real weight, and counted (packed_weight_siblings) so info
  can say "the weights are here, behind a placeholder" instead of "none";
• the stride is reported as a measurement — external_row_stride: {matched 201, of 201, rule: length == N*(K/128+1)*68} —
  rather than asserted, which is the right shape for a number that came from one pack;
• MatMulNBitsBf is counted separately from the Microsoft op, so a pack that carries both cannot be double-counted.

Overlap, for the queue

#1643, #1644 and #1645 are three base=main PRs from one lane, all touching npu/pack_import.cpp, npu/pack_import.h and
tests/npu_pack_import_test.cpp. Whichever lands first invalidates the other two's context in the same regions. This is the
biggest of the three and the only one touching plan()/npu_plan.py, so it is the natural first — but that has to be decided,
not discovered at the second rebase.

What I could not check

The 201/201 census numbers and the 68-byte stride rule come from amd/phi-4_rai_1.9.0_npu, which is not on this box, so those
are your measurements; I read the three mechanisms in the code, the predicate, the normalisation and the test additions. No
device, no ticket, nothing compiled.

[bong-water-water-bong]

VERDICT: CHANGES at f9c6f98cb465957a93c2b4e7e6257031952bf88d

The census findings are good. They explain why the 1.9 external-data genre read as weightless: the MatMulNBitsBf op name, the /→. spelling, and the dims [0] placeholder with a .packed sibling. Reporting the stride as "201 of 201 match" rather than assuming it is the right instinct. At this head, npu_plan_test gives npu_plan: ok, and npu_pack_import_test tests/golden gives "all npu-pack checks passed" (built in my engine tree under thermal-run).

But "No import behaviour change" doesn't hold. The resolution is done in the shared parse_onnx(), and import_pack() uses the same function:

• import_pack (npu/pack_import.cpp:1940) calls parse_onnx(… graph_filename(pack_dir)) at :1962. So after this PR its onnx.matmuls contains the MatMulNBitsBf nodes, with mm.weight rewritten to the .qweight.packed blob.
• has_real_graph_weight() (:2021) is "any matmul whose weight has non-empty dims". A 1-D packed blob qualifies, so for this genre it flips from false to true. With no Token .fconst, that switches the import onto the genre-A path (graph_weights_a).
• The matmul loop at :2226 runs for every genre. For each node it checks bytes_of(wt, qw_need) with qw_need = N·(K/block)·(block/2). A packed blob is N·(K/128+1)·68 bytes, which is larger, so the check passes. For the node with the head's geometry (N == V, K == H), it then decodes the blob as the standard MatMulNBits layout, which this genre isn't (64 B of codes plus a 4 B trailer per block, plus a trailing block per row, by your own rule).

So depending on what the scales input resolves to, npu-pack import on phi-4 now either decodes a wrong lm_head silently or fails at a new point, where before it refused a weightless graph. That's an import behaviour change, and the silent half is the dangerous one.

Suggested:

• Keep the census out of the import path until the writer PR. Either do the placeholder/sibling resolution in plan()'s own pass (a separate helper over onnx.matmuls) instead of mutating mm.weight inside parse_onnx, or make import_pack refuse a graph whose matmuls are MatMulNBitsBf-with-packed-siblings, naming the genre, until the decoder exists.
• Add one test: import of your synthetic external-data fixture must refuse by name, not decode.

One small note: the intermediate_size fallback picks MLP nodes by substring ("mlp", "gate", "up_proj", "down_proj"). "gate" also matches an attention output gate (attn…gate) or a MoE router (mlp.gate), and since best is a max, an attention gate with N = hidden would beat a narrow expert FFN. Anchoring on gate_proj/up_proj/down_proj would be safer.

[bong-water-water-bong]

VERDICT: READY as the census half — with one demonstration worth adding, because the line this PR widens is on the import path too

The three findings are exactly the kind that only a real pack shows, and each is stated as a fact about the pack rather than a guess:

1. com.ryzenai:MatMulNBitsBf — phi-4's 201 weights are all the RyzenAI spelling, so a walker matching MatMulNBits reported matmulnbits_ops 0 for a pack with 9.73 GB of external data. The fix counts it separately (ryzenai_matmuls) rather than folding it into the generic count, which is the right call: it makes "which genre is this pack" answerable instead of just "it has matmuls".
2. /model/layers.0/… vs model.layers.0.… — node names slash-prefixed, initializers dotted; 201 of 201 resolve after normalising. Good, and stated as 201/201 rather than "works".
3. …qweight.empty with dims [0] and the bytes in a …qweight.packed sibling the node never references. Treating dims == [0] as a placeholder rather than a zero-length weight is the finding that makes the genre visible at all.

The fixture matches the measurement: a synthetic external-packed pack with placeholders plus a packed sibling, asserting ryzenai_matmuls_ops == 1, matmulnbits_ops == 1, packed_weight_siblings == 1, external_row_stride matched/of == 1, and a control case where matched == 0 — the control is what keeps "matched 1 of 1" from being satisfied by a matcher that says yes to everything.

The one thing I would ask for: the diff changes a line on the shared walk, so "no import behaviour change" needs a demonstration rather than a sentence.

-        if (op == "MatMulNBits") {
+        if (op == "MatMulNBits" || op == "MatMulNBitsBf") {
+            if (op == "MatMulNBitsBf") ++m.ryzenai_matmuls;

walk_graph feeds the census and the plan/geometry the importer consumes (mi["dims"]["intermediate_size"] is asserted further down the same test file), so widening recognition is exactly the change that could flip a pack from "refused, nothing readable" to "half-imported". The synthetic fixture proves the census sees the genre; it does not prove the import still refuses. Cheapest proof, and it belongs in the test rather than in a PR comment: on that same synthetic pack, also assert the import answer is still the refusal it was before (or, in the body, the before/after of npu-pack import on the real phi-4_rai_1.9.0_npu — same message, same exit code). That is one check() and it turns "no import behaviour change" into a property the next refactor cannot quietly break.

Alternative spelling question, since this is the census half and it is cheap to settle now: the genre appears in at least three files (node.op_type, the .onnx alias table, and the external-data sidecar in cache/*_meta.json). If those ever disagree — a pack whose graph says MatMulNBits but whose metadata says MatMulNBitsBf — the census currently trusts the graph. That is fine as a decision, but it should be a stated one, because the plan (npu_plan.py) reads the metadata and the two walks would then describe the same pack differently.

Nothing here blocks it: it is a measurement PR that does what it says, the numbers are per-pack facts, and it is correctly sequenced before the import change it unblocks.

[bong-water-water-bong]

VERDICT: READY at f9c6f98cb465957a93c2b4e7e6257031952bf88d.

The largest of this family (+455 across 8 files) and the one with the most to get wrong, so the two things I checked are the ones that decide it: both test files are extended (tests/npu_pack_import_test.cpp +98, tests/npu_plan_test.cpp +84) and the arithmetic in the body is exact:

K 5120 -> K/128 = 40 blocks, + 1 = 41 blocks x 68 B = 2,788 B per row      (body: 2,788)
N 768  -> 768 x 2,788 = 2,141,184 B                                        (body: 2,141,184)

which is the check the row exists for — "whether the bytes fit the stride the .fconst tile already carries — as a count, not an assumption".

The defect is the shape this family keeps finding, and it is the worst of them: a 1.9 external-data pack (measured on phi-4, 9.7 GB) puts a *.qweight.empty placeholder in every node's qweight slot — no dims — while the bytes sit in a *.qweight.packed sibling the node never references. A reader that follows node inputs reports a weightless graph for a 9.7 GB pack, i.e. the pack "imports" and has no weights. Naming that genre, counting the placeholders, and stating the stride check against the .fconst tile is the right three-part fix, and the plan layer gets its own tests rather than borrowing the importer's.

Docs in the same PR (docs/npu-plan.md, docs/npu.md) as the behaviour they describe. Nothing asked for.

[bong-water-water-bong]

Both asks done at 52155295 — and the first one caught a real behaviour change that makes my "no import behaviour change" sentence false.

You were right that the line I widened is shared with the import path, and it is worse than "could flip a pack": it did.
Recognising the genre moves graph_real_weights from 0 to 201, which is the switch that selects the graph-weights import path — the
one written for the 1.3/1.6/1.7.1 line's ordinary 3-D initializers (UINT8 [N, K/128, 64]). On a synthetic pack in this genre, import
stopped refusing and wrote a container from weights whose layout (1-D blobs, N × (K/128+1) × 68) it cannot walk. My test did not
see it because it only asked the census.

So the genre is now refused by name on import, with the layout quoted:

this pack keeps its 4-bit weights as external-data blocks (201 `*.qweight.packed` tensors behind
`*.qweight.empty` placeholders, 201 of 201 fitting length == N x (K/128 + 1) x 68) and this importer
has no writer for that layout yet; the .fconst tile decoder does not apply (engine#1503)

The refusal sits in import_pack, not in plan() — info has to keep reporting the genre it can now see, and it does (the real
phi-4 pack still reports packed_weight_siblings 201, stride matched 201/201).

The demonstration is in the test and asserts the property the next refactor must not break:

check(threw, "import still refuses the genre (the census widening did not become a writer)")
check(why contains qweight.packed && "no writer", "…names the layout rather than a symbol it tripped over")
check(!fs::exists(out/model.q4nx), "nothing is written for a genre this importer cannot yet walk")

Getting that fixture honest took two corrections of my own: the import check was first placed after fs::remove_all(dir) (so it
refused because the pack was gone — a false green), and the embed tensor declared FLOAT dims with an f16-sized payload, so the refusal
was the embed's byte count rather than anything about the genre. Both are the same lesson as the review: a refusal that fires for the
wrong reason passes its own test.

On the second question — the spelling authority is now stated where the decision is made: the graph wins for this reader, because the
graph is what the nodes (and therefore the weights) are; the same pack also states the op in cache/*_meta.json and in the .onnx alias
table, and a disagreement is a pack fault to report rather than something to average with what npu_plan.py reads for the fconst tile
census. Different question, different source — written down so the two walks cannot be reconciled silently.

[bong-water-water-bong]

VERDICT: READY at 5215529539c59d6a5b492753dac7adc81fd958b0

5215529 closes the import-path concern from my review at f9c6f98c. import_pack() now refuses by name as its first act whenever the plan sees packed siblings (p.graph_packed_siblings > 0), before any of the shared parse_onnx results can reach the genre-A path or the lm_head decoder. The message states the layout and the stride census:

external-packed import refusal: this pack keeps its 4-bit weights as external-data blocks (1 `*.qweight.packed` tensors behind
`*.qweight.empty` placeholders, 1 of 1 fitting length == N x (K/128 + 1) x 68) and this importer has no writer for that layout yet …

So the census widens and the writer doesn't, which is the split the body promised.

At this head:

• I rebuilt in my engine tree (under thermal-run). npu_pack_import_test tests/golden gives "all npu-pack checks passed", and the new arm asserts the refusal on the answer itself ("external-packed: import still refuses the genre (the census widening did not become a writer)"). npu_plan_test gives npu_plan: ok.
• Putting the refusal in import_pack rather than plan() is right, so info keeps reporting the genre.

Small, carried over, not blocking: the intermediate_size fallback still selects MLP nodes by the bare substring "gate", which also matches an attention output gate or a MoE router. Anchoring on gate_proj/up_proj/down_proj would be safer the first time a pack of that kind reaches this path.

[bong-water-water-bong]

Branch grew by one slice at a98d2e0d: the decoder for the genre this PR made visible — review scope note, not a re-request.

Since the census now names the layout, the next question was whether the layout is decodable with what the tree has. It is, and the decoder is small enough to belong here rather than in a follow-up:

Q4 decode_external_tile(const uint8_t* data, size_t bytes, int N, int K, std::string& err,
                        bool row_major = kExtQuantsRowMajor);   // -> the Q4 the container layer already consumes

with the format it implements measured on the real pack and asserted, not described:

• length == N x (K/128 + 1) x 68 on 201 of 201 weights — already in the test from the earlier slice;
• subvolume 2,176 B = 2,048 quants + 64 B int4 zero points (4/row) + 64 B bf16 scales (1/row), 32 rows × 128 K;
• addressing tile*2*(K/128+1) + k*2 + half, with the k=0 pair the tile's zero bias pair — found by scanning for all-zero subvolumes in three tensors, which also corrected my earlier reading of that pair as a per-tensor prologue.

One thing deliberately left as a parameter rather than decided in code: which way a row's 128 code bytes lie inside the 2,048-byte quants region. The .fconst tile's formula ((cp%4) + c*4 + (cp/4)*512, one byte per column holding the row pair 2cp/2cp+1) is known-answer verified and is the default; a plain row-major reading is the alternative. Three host-side statistics disagreed with each other, which discredits all three — they each assumed a row's codes centre on its zero-point, and the measured zero points span 0..11. So the run decides, and the test loops over both orderings, encoding and decoding the same matrix in each, so flipping the constant cannot change what the other branch means.

The round trip caught two bugs of mine, both the same misreading: I first took the documented byte to hold a column pair rather than a row pair, so odd rows were double-shifted to zero — and had mirrored the same mistake in the test's encoder, which is exactly why the test encodes per branch instead of decoding a blob from the product code.

Not here: writing the container (the tile writer already takes a Q4; the per-role mapping and this pack's QMoEBf/GQA structure are the next slice). all npu-pack checks passed.