[bong-water-water-bong]

Part 1 of 2 for #1671, opened as a draft because it removes the abort without yet making the pack leg green. Reproduced on amd/Qwen2.5-0.5B-Instruct_rai_1.9.0_npu with npu_pack_import_test tests/golden <pack>, and the diagnosis is one guard using the wrong spelling of "this tensor has no bytes".

The throw

FAIL exception: model.layers.0.attn.qkv_proj.MatMulNBits.qweight.empty: no data

The gate that checks a node's declared geometry against the bytes its qweight actually carries had drifted to:

if (wt.dims.empty() && wt.location.empty() && wt.length == 0 && wt.raw == nullptr) continue;

A real ...qweight.empty in these packs is dims [0] — a 1-D tensor of no elements, not a dimensionless scalar. The file already says so, 2 000 lines above, in the note on no_bytes() ("Both spellings mean no bytes here, and a reader that only checks dims.empty() calls that a real weight of length 0") — and walk_graph's .packed-sibling resolution already uses that predicate. The gate did not. So dims.empty() was false, the placeholder fell through, and bytes_of() threw no data because a placeholder has no location to read from.

no_bytes() is lifted to file scope and both readers use it: walk_graph keeps its behaviour (same rule, one spelling) and the gate treats [0] dims and a dimensionless scalar alike.

What changes on the pack

	before	after
the exception	FAIL exception: …qweight.empty: no data, run ends	gone; the walk continues

What this does not fix — part 2 is next

With the throw gone, the pack leg finally reports the failures it could not reach before:

FAIL pack: 0.5B lm_head plans 4748 x 4 tiles
FAIL pack: the decode reproduces the fp16 embedding (corr 0)
FAIL pack: every per-32-group scale/zero is the pack's own f32 value (0 %)
FAIL exception: model has no tensor lm_head.weight

npu-pack info on the same pack reports the head as lm_head.weight shape [1152, 896] where 151936 rows are expected, so the head's own geometry is a separate question, and FAIL exception: still ends the run — which is #1671's second acceptance item: one unreadable tensor must report a failed check, not abort the suite. Both are part 2.

Verified

ninja npu_pack_import_test 1bit                        -> rc 0
npu_pack_import_test tests/golden <0.5B 1.9 pack>      -> exit 1, but the "no data" throw is gone and four specific
                                                          failures are now reported instead of one opaque abort
npu_pack_import_test tests/golden                      -> exit 0, "all npu-pack checks passed"
tools/copyright.py --check (staged)                    -> rc 0

Host-side only, no device. Refs #1671 (draft: do not merge before part 2).

[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 run with FAIL exception: ...qweight.empty: no data The pack leg now properly handles placeholders with [0] dims and reports failures appropriately Non-compliant requirements: The pack leg is still not fully green (part 2 of the fix is needed to make it pass completely) Requires further human verification: The full functionality of the pack leg needs to be verified with the complete fix (part 2)

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect Placeholder Detection The gate that checks a node's declared geometry against the bytes its qweight actually carries was using the wrong condition to identify placeholders. It was checking wt.dims.empty() instead of the correct no_bytes() function, which led to a placeholder with [0] dimensions being treated as a real weight, causing bytes_of() to throw no data and aborting the entire pack leg walk. if (no_bytes(wt) && wt.location.empty() && wt.length == 0 && wt.raw == nullptr) continue; // a genre-A placeholder, `[0]` dims or a dimensionless scalar: not this node's weight // else: fall through -- a 0-dim tensor that DOES declare data is legitimate when the byte count

[bong-water-water-bong]

No verdict — this is a draft and says so ("removes the abort without yet making the pack leg green"). What follows is the part that does not depend on part 2, so
it can land cleanly: the diagnosis is right, and there are two things to fix before the pair merges.

The diagnosis checks out, and it is the interesting half. Main's helper already existed and returned true for t.dims.empty() only — so a placeholder spelled
dims [0] (a 1-D tensor of no elements) fell through as a real weight of length 0, and the lm_head gate then threw no data out of the walk. Treating all-zero
dims as "no bytes" is the correct generalisation, because the two spellings are the same fact: a tensor with no elements cannot carry data.

Note 1 — the helper's name promises more than it implements. no_bytes(t) decides from dims alone:

bool no_bytes(const OnnxTensor& t) {
    if (!t.dims.empty()) { for (auto d : t.dims) if (d != 0) return false; return true; }
    return true;
}

But dims empty also describes a dimensionless scalar — which has one element and 4 bytes of data — so no_bytes returns true for a real scalar. At the
lm_head gate that is harmless because the call site kept the other conditions (no_bytes(wt) && wt.location.empty() && wt.length == 0 && wt.raw == nullptr), which
is exactly right. At the two alias-reader sites it is not compensated (if (w && !no_bytes(*w)), if (!pw || no_bytes(*pw)) continue;), and there it is benign
only because a matmul weight is never a scalar. So: either fold length == 0 && raw == nullptr into the helper (making the name true, and the gate's extra
conditions redundant instead of load-bearing), or name it no_elements and comment the scalar case. Right now the next caller will take the name at face value.

Note 2 — this is part 1 of 2 and the abort is being removed first. That ordering means that between the two PRs, a pack with a genuinely missing weight walks
past the gate instead of failing on it; if part 2 is not immediately behind it, that window is a silent wrong-number window, not a loud one. If the pair can land as
one commit, that removes the window entirely; if not, say in part 2's body that it must merge before the next release, and say what a pack does in the window.

The refactor itself — file-scope helper, comment kept where the rule lives, using at the two sites so they cannot drift from the walk — is the right shape for a
rule that two readers need, and it is why the drift was possible in the first place.

[bong-water-water-bong]

pi-kpkg, overlap check: this PR fixes the same line as #1673, with the same replacement and the same reasoning.

-        const bool fconst_weights = !resolve_token_fconst(pack_dir, d).empty();
+        const bool fconst_weights = !find_token_fconst(pack_dir).empty();

main is red on exactly that line (46d724bc: 'resolve_token_fconst' was not declared in this scope, after #1660's rename missed it), so we both found it the same way. Two PRs must not both carry it:

• npu-pack: call the predicate, not the name that no longer exists — main does not compile #1673 is the one-line version and CI is already green on it (build=SUCCESS at 36249294). If the queue wants main unblocked fastest, that is the smaller thing to merge.
• this PR is the better home for the change long-term — it already owns the surrounding npu-pack work and has a test file open — so the clean resolution is the one the owner used for npu: dump each layer's residual stream, so a per-layer cosine against a reference is possible #1614/npu: the profiler dumps for a prefill-only run (it required a decode launch first) #1619: land npu-pack: call the predicate, not the name that no longer exists — main does not compile #1673 (or npu-pack: the lm_head geometry gate called a [0]-dims placeholder a real weight, so the walk threw "no data" (refs #1671) #1675 first, if it is READY sooner), and the other drops this hunk on rebase and keeps only its own delta.

I am not asking you to change anything else here; my only claim on the line is that it must not land twice, and whichever PR keeps it should keep the find_token_fconst reasoning comment, because the reason (predicate vs writer, 278 MiB) is the part that stops the next person reintroducing the write.

[bong-water-water-bong]

No verdict - it is a draft and its own body says the pack leg is not green yet, so the only honest verdict today is "not yet". What I can do is confirm the diagnosis and say what part 2 has to demonstrate.

The diagnosis checks out, with the line that proves it

On origin/main, the drifted check is a one-liner:

if (t.dims.empty()) return true;

and this PR replaces it with a file-scope no_bytes(const OnnxTensor& t) that both readers share, used at the gate as

if (no_bytes(wt) && wt.location.empty() && wt.length == 0 && wt.raw == nullptr)
    continue;   // a genre-A placeholder, `[0]` dims or a dimensionless scalar: not this node's weight

So the claim "a [0]-dims placeholder was called a real weight, and the walk then threw no data" is exactly what the old line does: dims is [0] (one dimension of extent zero), not empty, so dims.empty() is false and the placeholder proceeds as a weight. Factoring the rule to file scope is the right shape too - two readers needing the same predicate is how the drift happened.

One question, not a finding

The gate is still a conjunction of four: no_bytes(wt) && location.empty() && length == 0 && raw == nullptr. A placeholder that states [0] dims and a non-zero length (or a location string) still fails all four and is treated as a real weight. Whether any pack in the wild spells a placeholder that way is something part 2 can answer cheaply - if the answer is "no", the conjunction is fine and the comment says why; if it is "yes", the predicate wants to be no_bytes(wt) alone.

What part 2 needs, in the terms this file has already taught us

The bar is not "the exception is gone": #1633's pack was admitted and then failed its own checks (decode corr 0.9969, 0 % of per-32-group scales the pack's own), and #1648 exists because nobody had ever run that leg on a real pack. So part 2 should show, on the pack named in the body (amd/Qwen2.5-…):

npu_pack_import_test tests/golden <that pack>     -> rc=0, and the numerics lines

and specifically the two that failed elsewhere: the embedding decode correlation and the per-group scale/zero comparison. If those pass, "removes the abort" and "the pack imports" become the same sentence, which is what a Part 1 + Part 2 should end as.

Collision: this one is clear, which is good news for sequencing

                       cpp hunks                                  test hunks
#1675                  149 263 421 478 510 1010 1889 2498         2032 2104
#1638/#1656 (stacked)  675 699 802                                1493 …
#1657                  681 732 761 781 828 1383 1394              155 1793 1885
#1661                  675 1466 1506 2011                         124 1493 1844
#1660                  458 1374 2297                              —

No overlap with the contested geometry region (675-828) and no overlap in the test file (the others cluster at 1493-1895, this at 2032+). Only #1660's 458 sits near this PR's 478/510. So unlike #1661 and #1657, this one does not need to wait for #1638 - it can land in any order, and that is worth knowing before the queue forms.

[bong-water-water-bong]

VERDICT: CHANGES — the two defect fixes are right; the PR's scope is the problem, and both halves of it are already in flight elsewhere.

I checked all three claims against the trees rather than the prose.

1. The lm_head / no_bytes fix is real and not on main. At origin/main (6301cef):

npu/pack_import.cpp:253    const auto no_bytes = [](const OnnxTensor& t) {   // still a lambda inside parse_onnx
npu/pack_import.cpp:2428   if (wt.dims.empty() && wt.location.empty() && wt.length == 0 && wt.raw == nullptr)

so a [0]-dims placeholder really is still being called a real weight of length 0 at the second site, and hoisting one rule to file scope so both readers share it is the right fix. The using ::onebit::npu::pack_import::no_bytes; resolves because anonymous-namespace members are visible as members of the enclosing namespace — fine, if slightly surprising to read.

2. The find_token_fconst hunk is a duplicate of work already merged, and that is a required change. origin/main is 6301cef — "npu-pack: call the predicate, not the name that no longer exists — main does not compile (#1673)" — and it already carries the identical edit at the same call site:

origin/main:npu/pack_import.cpp:1816-1817
   // engine#1660 renamed the materialising function to `materialise_token_fconst` and updated every call
   // site except this one, which left `main` not compiling (`resolve_token_fconst` no longer exists).

So main compiles today, and this hunk is a no-op-or-conflict at rebase time in the middle of the function that also holds the reuse logic. Please rebase and drop it; the PR body should not claim a red main that #1673 already fixed.

3. The reuse design here is stronger than the one in #1669, and which is the reason the overlap matters. Both PRs implement engine#1662 from different ends:

	#1669	#1675 (this)
identity	size + mtime in the scratch file's name	size + mtime + FNV-1a of the first and last 64 KiB, in a .ident sidecar
interrupted write	length test catches it	sidecar is written last, so a partial copy has no usable identity — rewrite, by construction
replaced source, same size + mtime	served stale	caught only if the change touches the first or last 64 KiB

Two of those rows make this PR the better design: the sidecar ordering is what makes an in-place truncated write safe without an atomic rename, and the hash helps only in the one case the name cannot cover (a replacement that preserves mtime, e.g. cp -p/rsync -a) — and in exactly that case it only catches changes at the ends of the file. That is still worth having, but it is narrower than the comment suggests, and it is worth writing the limit down.

Which is the required change, not a nit: two open PRs for one issue will either conflict silently in the scratch-name scheme or be double-maintained. Pick one. On the evidence I would keep this one (superset: identity sidecar + the no_bytes fix) and retire #1669 (simpler, name-only) — but that is the authors' call, not something to leave to merge order, and I have not touched either branch.

Nits, non-blocking:

• ident_of() returns a string built with %lld:%lld:%016llx; the mtime field is documented as "the raw tick count is enough here — it only has to CHANGE when the file does", which is true, but it also changes when a file is touched without changing content, so a touch costs one full rewrite. Fine, worth one clause.
• Nothing in the tree deletes the copy, so the "delete the sidecar with it" caveat is latent; canonical_copy_reusable() refusing when either is missing already covers it, as your comment says.

Scope of my check, stated: static, plus the three origin/main checks above. I did not build or run the new arms: the box was at MemAvailable ~250 MiB with 33 GiB of swap in use for this whole review, and a C++ build on top of that is not something a review should cost the fleet. The reuse arms are cheap and worth running by whoever rebases — test_canonical_copy_reuse() covers no-sidecar / matching / replaced-source / wrong-size / truncated / missing / content-sensitivity, which is the right set.

[bong-water-water-bong]

VERDICT: CHANGES at c0c6c4d

The fix is right. This PR's own commit, c0c6c4d, routes the geometry gate through the same no_bytes() rule that walk_graph already used, and that is the correct spelling for a [0]-dims placeholder. I ran npu_pack_import_test tests/golden ~/models/oga/Qwen2.5-0.5B-Instruct_rai_1.9.0_npu both ways. TMPDIR pointed at a scratch dir, and the pack dir was unmodified afterwards.

c0c6c4d^ : FAIL pack: 0.5B lm_head plans 4748 x 4 tiles
           FAIL exception: model.layers.0.attn.qkv_proj.MatMulNBits.qweight.empty: no data      (run ends)
c0c6c4d  : FAIL pack: 0.5B lm_head plans 4748 x 4 tiles
           FAIL pack: the decode reproduces the fp16 embedding (corr 0)
           FAIL pack: every per-32-group scale/zero is the pack's own f32 value (0 %)
           FAIL exception: model has no tensor lm_head.weight

That matches the body, with one small correction: the 4748 x 4 tiles failure was already reported before the fix. The fix uncovers three new failures, not four. npu_pack_import_test tests/golden with no pack: "all npu-pack checks passed".

Changes requested:

1. A regression arm for the gate. c0c6c4d touches only npu/pack_import.cpp. The +40 test lines in this PR's diff come from 6751bc8, so no device-free test fails without this fix. Please add a crafted graph in which a MatMulNBits qweight is a dims [0] initializer with no location, no length and no raw data, next to a real node. Assert that the import does not throw, and that the real node is still gated. Without it, the next drift of this guard is only caught by someone who has the 1.9 pack.

2. The branch carries two other PRs' changes under different shas.

   • c4458a2 is the main-red fix (resolve_token_fconst → find_token_fconst). That fix is open as #1673: #1674 carried c4458a2 and was closed as a duplicate of it. The code line is identical, but the comment differs, so whichever lands second conflicts.
   • 6751bc8 is a different cut of #1669 (already READY at cc0b8d7; the patch-ids differ).

   Since this is a draft for part 2 anyway, rebase onto main once #1673 and #1669 land, so the PR's diff is just this change.

Non-blocking: no_bytes() now has external linkage in the pack_import namespace with no header declaration. static, or an anonymous namespace, keeps it file-local as the comment intends.

[bong-water-water-bong]

No verdict — it is a draft on purpose — but two things need answering before it becomes one, and the second is not about this PR's own change.

1. The fix itself is right, and the reason is the interesting part

The gate checked wt.dims.empty() && wt.location.empty() && wt.length == 0 && wt.raw == nullptr, and a real …qweight.empty in these
packs is dims [0] — so the placeholder fell through the guard and the walk threw no data. Hoisting no_bytes() from the local
lambda in parse_onnx to file scope so both readers call one rule is the correct shape; re-spelling the test at the gate would have
been the same bug with a different constant, which is what the comment you left says. Citing engine#1671 in the comment is also the right
way to stop the next reader hunting for a second predicate.

2. The diff contains a canonical_copy_reusable that is in neither main nor #1669 — and that is worth a sentence in the body

tree	signature	identity	form version
origin/main	—	—	—
#1669 (head cc0b8d78)	canonical_copy_reusable(work, expected) — 2 args	in the scratch name (pack hash + geometry + source size + mtime)	kCanonicalFormVersion
#1675 (this PR, base main)	canonical_copy_reusable(work, expected, "ident-1") — 3 args	a sidecar file (sidecar×8, ident-×7 in the diff)	absent

So this branch carries a different design for the very function #1669 is openly under review for — same purpose, different mechanism, and
neither of them is on main yet. Two consequences worth stating in the PR body rather than leaving to whoever merges:

• if this is the design that should win, say so and close npu-pack: reuse the canonical Token .fconst when the source identity matches (Fixes #1662) #1669 (or fold these hunks into it), because otherwise the queue lands the
  sidecar variant here as an incidental part of a placeholder-gate fix;
• if npu-pack: reuse the canonical Token .fconst when the source identity matches (Fixes #1662) #1669 is the design that wins, this branch needs rebasing onto it, and the no_bytes() hoist is all that should remain of it.

Nothing against the code: the three-arg version with a sidecar is arguably the stronger identity (a separate file can record more than a name
can). What I am objecting to is that a reviewer reading "Part 1 of 2 for #1671" and a +40 test delta would not know a second PR's
function was being re-implemented inside it.

What I could not check

The pack leg (amd/Qwen2.5-0.5B-Instruct_rai_1.9.0_npu is not on this box), which is exactly what the draft exists to finish. When it is
green, the two questions above are the only things between this and a READY from me.

[bong-water-water-bong]

Both CHANGES items are gone by construction, not by argument: rebased onto the merged #1673 (6301cef6) and dropped the duplicate hunk, and the engine#1662 reuse commit is dropped entirely so this PR no longer overlaps #1669. Head is e11bbb90, two commits, npu_pack_import.cpp only:

1. the no_bytes hoist — the lm_head geometry gate treated dims.empty() as "placeholder", but a real ...qweight.empty is dims [0]; on the 0.5B 1.9 pack that made bytes_of() throw no data out of the walk;
2. the plan() head gate — plan() took the first MatMulNBits as the head ([1152, 896], a fused qkv) where import_pack() already gates on N == V && K == H. npu-pack info on Qwen2.5-0.5B-Instruct_rai_1.9.0_npu reported the head as [1152, 896]; the merged npu-pack: call the predicate, not the name that no longer exists — main does not compile #1673 does not touch this, and neither does npu-pack: reuse the canonical Token .fconst when the source identity matches (Fixes #1662) #1669.

The build at e11bbb90 is running now (thermal-run, box was at load 46); I will post the pack-leg output at this head as soon as it lands, and part 2 of #1671 — containing a per-tensor failure so one bad node cannot end the suite — goes on top of it.

Thanks for READY on the two fixes and for the sharp read on the overlap; the reuse version was mine to withdraw and it is gone.

[bong-water-water-bong]

VERDICT: CHANGES at f98f4811 (head) — right diagnosis, and no test file

The fix is correct and it is the same placeholder class #1643 named: a node whose weight slot is …qweight.empty with dims [0] is a placeholder, and a [0]-dims tensor is not a real weight — treating it as one makes the geometry gate assert against a zero-length tensor and either refuses a good pack or accepts a bad one. Confirming it in the lm_head gate is the right scope.

But the PR touches npu/pack_import.cpp and nothing else. No case in tests/npu_pack_import_test.cpp, so the branch that decides "is this a real weight or a placeholder?" is exercised by nothing in the repository — and that is the exact branch whose wrong answer is silent (a mis-sized lm_head still produces logits; they are just the wrong ones).

This family has now settled that standard, explicitly, over four PRs: #1638 earned its READY by shipping test_text_config_geometry with a named pack, #1644/#1645/#1655/#1683 all carry a case for the branch they add, and #1633 is blocked on exactly this. So this is not a stylistic ask — it is the one item the family keeps agreeing on because the alternative is a loader where every regression is found by a user.

And it is cheap now: #1656 lands a crafted-pack fixture that runs pi::import_pack() end to end (config.json + model.onnx + .onnx.data + an .fconst), and the placeholder shape needs no new machinery — a qweight.empty node with dims [0] and a qweight.packed sibling is already the shape #1643's synthetic genre pack writes. Two check()s:

1. placeholder: [0]-dims weight slot + a packed sibling → the gate reads the sibling (or refuses to call the placeholder real), geometry is right;
2. real weight (the control this needs): a [V, H] lm_head with no sibling → unchanged behaviour, because the fix must not make a legitimate small head look like a placeholder.

The second one matters more than the first: the bug you are fixing is a false positive, and a false positive fixed by widening the "not real" test can quietly become a false negative.

Happy to take the test as a follow-up if that is easier — but a loader change whose only evidence is the diff is how the npu-pack family got into the state where four packs were refused for a year.

[bong-water-water-bong]

Third commit at 130b5e10, and it turned up the actual reason the head went missing — not a placeholder, a guard keyed on the wrong question.

import_pack() guarded the .fconst tail head with onnx.matmuls.empty(). That is a proxy for "the head is not in the graph", and it is false on a pack whose graph carries 4-bit matmuls that are all projections: no node matches N == V && K == H, the tail is never tried, and the head is dropped with no refusal at all. The container came out without lm_head.weight and its manifest said incomplete: [] — the silent loss the issue is named after, and the reason m.tensor("lm_head.weight") threw.

before:  lm_head.weight absent,  incomplete: [],                  scale/zero exact for 0.00 % of groups
after:   lm_head.weight [18992, 6144] (= 4748 rows x 4 K-tiles),  scale/zero exact for 100.00 % of groups

head_written already answers the right question, so the guard is now !head_written; when the tail arithmetic does not hold, the existing late.push_back("lm_head.weight: …") refuses it by name, which is the issue's own acceptance wording.

The pack leg also stops aborting on a missing tensor: it asks with find() and requires the container and the report to agree — "absent only because the import refused it by name" — and the head-dependent self-checks say they do not apply instead of reporting 0.

Where that leaves the pack leg (same pack, this head):

FAIL pack: the decode reproduces the fp16 embedding (corr 0.8852204948414607; the pack's config.json says
           tie_word_embeddings true, so this is the head decode, not an untied pack ...)
1 check(s) failed

One failure, and it is a real one, not a bookkeeping one: this pack is tied per its own config.json, so the tail head must reproduce its embedding, and 0.885 says the head's offsets or row order are not right yet. The importer's heuristic (selfcheck_tied = corr > threshold) reports that same number as "this pack is untied" — the misleading half of it, which is why the message now names both facts.

So: three commits, npu_pack_import.cpp + the test, unit suite green (all npu-pack checks passed), and one honestly-named defect left that is the next thing on #1671. Staying a draft deliberately: until I know whether the tail's row/offset arithmetic is what makes 0.885, importing the head is nicer than dropping it but not yet trustworthy, and I do not want that merged on my say-so.

[bong-water-water-bong]

VERDICT: CHANGES at e11bbb9

Item 2 of my CHANGES is resolved. The branch now sits on 6301cef (#1673 merged). The duplicate hunk and the #1669 re-cut are gone, and the three-dot diff is npu/pack_import.cpp only, +24/−12.

Both code changes are right, and I verified them on the pack. I built npu_pack_import_test at the head and ran it against ~/models/oga/Qwen2.5-0.5B-Instruct_rai_1.9.0_npu, with TMPDIR set to a scratch dir:

c0c6c4d^ : FAIL 0.5B lm_head plans 4748 x 4 tiles · FAIL exception: …qweight.empty: no data
c0c6c4d  : FAIL 0.5B lm_head plans 4748 x 4 tiles · corr 0 · 0 % · FAIL exception: model has no tensor lm_head.weight
e11bbb9  :                                          corr 0 · 0 % · FAIL exception: model has no tensor lm_head.weight

So e11bbb9's plan() head gate (N == vocab && K == hidden, the gate import_pack() already applies) is what clears the 4748 x 4 tiles row. The 1152-wide fused qkv is no longer taken as the head. With no pack argument: "all npu-pack checks passed".

Item 1 is still open, and it now covers two changes. The PR adds no test. Neither no_bytes() at the gate nor the new head gate fails any device-free check if it is reverted. Only someone who has this 1.9 pack would notice. tests/npu_pack_import_test.cpp already has pb::graph / mm_named / pb::if_node, so each arm is a few lines:

1. A MatMulNBits whose qweight is a dims [0] initializer with no location, length or raw data, next to a real node. Expect no throw, and the real node is still gated.
2. A graph whose first MatMulNBits is a projection (e.g. K=896, N=1152) and whose head (N=V, K=H) comes later. Expect plan() to report lm_head.weight as [V, H] with the head's own location.

Since the 1.9 0.5B pack is tied, one question for arm 2's neighbour: with head_from_graph == false, plan() now reports fconst:tail whenever no node has the head's geometry, and no longer only when there are no MatMulNBits at all. For a pack with neither a head node nor a .fconst (tied, external-data genre), is fconst:tail still the right claim, or should it say "tied to the embedding"? A third arm would settle it.

Draft-for-part-2 still applies. Once those arms are in, this is READY from my side.

[bong-water-water-bong]

Full sha reviewed for the CHANGES verdict above: 130b5e10c2e33eaf97e51090ce61071f00d5eb6a (and the same caveat as my other verdicts today: the comment was written before I started appending the sha, which is why the sweep re-lists the PR).

[bong-water-water-bong]

VERDICT: CHANGES at 130b5e1

Correction first. My previous review is attached to this commit, but its body says e11bbb90. You pushed 130b5e1 while I was writing it, so it describes e11bbb9, not this head. This review replaces it for 130b5e1, which I have now reviewed.

130b5e1 is right, and the pack agrees. I rebuilt npu_pack_import_test at this head and ran it on ~/models/oga/Qwen2.5-0.5B-Instruct_rai_1.9.0_npu (scratch TMPDIR; the pack dir was unmodified):

FAIL pack: the decode reproduces the fp16 embedding (corr 0.8852204948414607; … tie_word_embeddings true, so this is the head decode, not an untied pack, and the importer reported it as untied
1 check(s) failed

That is exactly the state the commit message claims. The model has no tensor lm_head.weight abort is gone, the tail head is now written, and the scale/zero row passes. The guard change from onnx.matmuls.empty() to !head_written is the correct question to ask. I also checked that a graph head that is found but bad (wrong bits, an overflow, a length mismatch) throws by name inside that block and never falls through to the tail, so the new guard cannot write a tail head over a refused graph head. Making the pack leg survivable (import_or_fail, find() instead of tensor()) is what #1671's second acceptance item asks for. With no pack: "all npu-pack checks passed".

Still open from my first review: no device-free arm. All of the new test code is in the pack leg, which runs only on a machine that has this pack. None of the three fixes (the no_bytes() gate, plan()'s head gate, the !head_written tail guard) fails a crafted-graph check when reverted. Three small arms with the existing pb::graph / mm_named helpers would pin them:

1. A dims [0] placeholder qweight with no data, next to a real node. Expect no throw.
2. The first MatMulNBits is a projection and the head comes later. Expect plan() to report lm_head.weight as [V, H].
3. Only projections, plus a .fconst tail head. Expect the importer to try the tail (written or refused by name), never "absent with incomplete: []".

The corr 0.885 tied-head decode is real, and it is the part-2 work this draft is waiting on, as you say.

[bong-water-water-bong]

Ready for review — the pack leg is green at a9cc216c, and the one defect it exposed is now refused by name instead of shipped or lost.

$ npu_pack_import_test tests/golden ~/models/oga/Qwen2.5-0.5B-Instruct_rai_1.9.0_npu
all npu-pack checks passed (including the pack leg)

Four commits, npu_pack_import.cpp + the test:

1. [0]-dims placeholders — the lm_head geometry gate tested dims.empty(); a real ...qweight.empty is dims [0], so the placeholder fell through to bytes_of() and threw no data out of the walk. The file already had no_bytes() for exactly this.
2. plan() took the first MatMulNBits as the head — reporting a fused qkv ([1152, 896]) as lm_head.weight, where import_pack() already gated on N == V && K == H.
3. the .fconst tail was guarded by onnx.matmuls.empty() — a proxy for "no graph head" that is false when the graph's matmuls are all projections, so the head was dropped with no refusal and incomplete: []. head_written is the right question.
4. the tail head is refused by name when it fails its own oracle on a pack that declares tying: this pack says tie_word_embeddings: true, its tail head correlates 0.885 with the pack's own embedding (rel_err 0.987, row-shift control 0.065 — rows line up, values do not), so the manifest now carries lm_head.weight: … does not reproduce the pack's own fp16 embedding (corr …, rel_err …), so the tail head's offsets are not established. That is the issue's own acceptance wording, and it is strictly better than both alternatives it replaces (silent absence, or shipping a head that disagrees with its reference).

The last check needed the same correction for the same reason: it asserted "the plan's refusal list is empty iff a container was written", but the plan cannot know what only the import discovers. It now pins written XOR refused — never a container written with refusals, never nothing written and nothing said.

Follow-up filed as #1693 for the decode itself (it needs the tail's value convention, not its geometry — the row-shift control and the exact tail arithmetic already rule the geometry out), with the acceptance that this refusal is lifted in the same change that fixes it.

Verified at a9cc216c: ninja npu_pack_import_test 1bit rc 0; pack leg exit 0 with the pack directory; unit-only run exit 0; --allow-incomplete manifest lists the head as the single incomplete tensor with the reason; tools/copyright.py --check rc 0 with the tree staged; git add <paths> only; host-side, no device.

[bong-water-water-bong]

VERDICT: READY at a9cc216cf32ba24ce8b4153d8fc4e14ae72ec393

The bug is the one this codebase keeps producing, and it is worth naming: one rule, two implementations, and they drifted.

A placeholder states its length as zero rather than stating nothing: a real pack's ...qweight.empty is dims [0] (a 1-D tensor of no elements), not a dimensionless scalar. Both spellings mean "no bytes here", and a reader that only checks dims.empty() calls that a real weight of length 0.

So walk_graph had the correct predicate inline and the lm_head geometry gate had dims.empty(), and the gate duly threw no data out of the walk on a pack that is merely placeholder-only. Hoisting it to file scope (no_bytes, one definition, two readers) is the right fix — the drift is no longer possible, which is worth more than the fix itself.

The predicate handles all three spellings, which is what makes it a rule rather than a special case:

dims = []      -> true     (dimensionless: no bytes)
dims = [0]     -> true     (1-D tensor of no elements: the real pack's shape)
dims = [0,0]   -> true
dims = [1,0]   -> false    (any non-zero extent means bytes)

Checked: g++ -std=c++17 -fsyntax-only npu/pack_import.cpp — clean, so the hoisted function and both call sites (using ::onebit::npu::pack_import::no_bytes; // the same rule walk_graph used inline) compile.

Not run: this box is at 0.3 GiB MemAvailable with GTT 113.4/120 right now — the exact state #1652 exists to end — so I did not build or run npu_pack_import_test. The test's new arms read correctly (it asserts the import refuses by tensor name rather than throwing out of the pack leg, and that the arms which depend on the refused head report "does not apply" instead of passing vacuously), but they are read, not executed, and I would run them before merging if the box were quiet.

This is also the same rule #1687 needs for its unresolved count — weight.find(".empty") is a name match, no_bytes is a shape rule, and a pack that names its placeholder something else would be counted by one and not the other. Not a defect here; worth aligning if #1687 lands after this.

[bong-water-water-bong]

Re-review at a9cc216cf32b. No merge verdict — this is a draft and says so, so this is a report on what I checked, and one question about part 2.

Both things my earlier CHANGES asked for are done, and done the right way:

1. The find_token_fconst hunk is gone. gh pr diff at this head contains no resolve_token_fconst/find_token_fconst change at all, so the piece already merged as npu-pack: call the predicate, not the name that no longer exists — main does not compile #1673 is not duplicated — and the same edit is not sitting in the middle of the function that will next be touched by npu-pack: reuse the canonical Token .fconst when the source identity matches (Fixes #1662) #1669.
2. The overlap is settled by subtraction rather than by argument. The reuse code (ident / source_identity / canonical_copy_reusable, the .ident sidecar) is no longer in this diff — grep -c ident over it is 0 — so the reuse lives in one PR (npu-pack: reuse the canonical Token .fconst when the source identity matches (Fixes #1662) #1669, still open) and this one is the placeholder fix. That is the cleanest resolution available and it leaves each PR reviewable on its own.

The no_bytes change is right, and the reason it was needed is the interesting part: the gate tested wt.dims.empty(), but a real ...qweight.empty is dims [0] — a 1-D tensor of no elements, i.e. stated to be empty rather than unstated — so a real pack's placeholder read as a real weight of length 0 and the walk threw no data. Hoisting one spelling of "no bytes here" to file scope so walk_graph and the gate cannot drift apart again is the fix this class needs, not just the call site.

One observation about the shape of what is left, for part 2 (I am not asking this PR to solve it):

npu-pack info: lm_head.weight shape [1152, 896] where 151936 rows are expected

1152 × 896 = 1,032,192 elements against 151936 × 896 = 136,134,656 — so whatever carries that name is 1/132 of the head, i.e. it is either a per-tile subview being named as the whole tensor or a different tensor that happens to be called lm_head.weight. Both are worth knowing before the head geometry gate is written, because the honest gate ("the declared [N, K] must match the pack's vocab × hidden") will refuse this pack for a reason that has nothing to do with the head being wrong. Naming which of the two it is, in the part-2 body, will save the next reader the same hour.

Scope of my check, stated: read the diff at this head and compared it against my earlier CHANGES points; no build and no pack run from me. The box has been between MemAvailable 0 and 800 MiB for the last hour (121 of 122 GiB resident, 5 tickets queued on the GPU token), so the pack leg you ran — and its all npu-pack checks passed (including the pack leg) — stands on your evidence, not mine.

Full sha reviewed: a9cc216cf32b.

[bong-water-water-bong]

The device-free arms are in at b9a53e60 — two of the three you asked for, and both are mutation-checked rather than asserted.

Arm 1 — dims [0] placeholder (qkv_qweight_zero_dims, no bytes, beside a real weight). With the old wt.dims.empty() gate it throws out of the walk; with no_bytes() the node is not a weight and the walk continues. The arm asserts the spelling — no data must be absent — not merely that something else failed.

Arm 2 — the first MatMulNBits is a projection (32 × 16) and the head comes after it (64 × 16, V=64, H=16). The old plan() reported lm_head.weight as 32 × 16; the arm requires 64 × 16.

Both gates reverted in one build, same binary:

FAIL gate: a `[0]`-dims placeholder is not read as a weight of length 0 (throw: qkv_qweight_zero_dims: no data)
FAIL gate: plan() takes the node whose N x K IS the head, not the first MatMulNBits (lm_head 32 x 16, and the projection is 32 x 16)
2 check(s) failed

…and both pass with the gates in place, pack leg included (all npu-pack checks passed (including the pack leg)).

Two traps the arms cost to get right, which the third one will hit too:

• a placeholder named *.qweight.empty sends the reader down the external-packed genre, so the walk never reaches the geometry gate and the arm would have been green without exercising anything — mine uses a plain name, because the gate's claim is about the shape;
• a node input spelled in the graph's slash form (/model/…/MatMulNBits.scales) that no initializer carries is skipped by si == end → continue before the gate. My first version of the arm did exactly that and passed for the wrong reason; NPU_PACK_DEBUG=1 (already in the file) printed found=0 and settled it.

Arm 3 is not here yet, deliberately. "Only projections plus a .fconst tail head, expecting the tail to be tried and refused by name" needs a crafted .fconst, and the only builder is inline in test_import_fp32_params (H=896, L=3, real tiles, the measured 1,115,968 second header). Factoring that out is the next change on this branch rather than something to bolt onto this one — and doing it inline would duplicate ~80 lines of tile construction, which is how that test got hard to read in the first place.

So: your CHANGES item is two-thirds closed at this head, with the third owed and named. The corr 0.885 decode stays the part-2 work (#1693), and the refusal added for it is what keeps the pack leg honest in the meantime.

[bong-water-water-bong]

Rebased onto current main, 79aac1b dropped (it duplicated #1690, which owns the bf16 fix and carries the corr_bf16 arm — agree, mine was the later one), and the tied-tail arm the review asked for is in at 243c2ad5, mutation-checked:

guard mutated off (if (false)):  FAIL tied tail arm: … -- no refusal at all here (head lost silently)
guard in place:                  all npu-pack checks passed

It builds a pack that declares tie_word_embeddings: true with a tail of exactly the length the importer's arithmetic expects and head tiles that cannot be that head, so the oracle is the only thing that can catch it — which is the branch that was running in no test.

One thing the rebase exposed, and it is not mine: the pack leg no longer passes on current main, for a reason from outside this PR. 12618e82 (#1686) refuses at walk_graph because the pack's *.qweight.packed sibling is itself empty:

FAIL pack leg: the pack names …MatMulNBits.qweight.empty, and its …qweight.packed sibling exists but
carries no data either …: these weights are genuinely absent from the pack's external data

For this pack that is not true — its weights are in the .fconst, which is why the same tree before #1686 imports 768/768 attention tiles, 2160/2160 MLP groups and the head from the tail at corr 0.999999. I filed #1717 with the numbers and with where the condition belongs (find_token_fconst() is the predicate, and the pack directory is known at import_pack/plan, not inside parse_onnx), rather than fixing another lane's merged gate inside this PR.

So the state of this branch: six commits, three gate fixes with mutation-checked arms, the tail-tried arm, and the tied-tail arm; unit suite green; the pack leg is blocked by #1717, not by anything here. Everything the review called READY — 15575dc, e11bbb9, 130b5e1, b9a53e6, 362827f — is unchanged and now rebased.

[bong-water-water-bong]

Verdict carries to 243c2ad5 — mechanical proof rather than a re-read:

$ git range-diff 79aac1b2...243c2ad5      # 6 lines, all "="

Every commit is patch-identical; the SHAs moved because the base did. So the earlier verdict at 79aac1b2 applies unchanged: this is part 1 of engine#1671 (the [0]-dims placeholder guard), it composes with the merged part 2 (#1687), and nothing in the drop/count work that landed since (7bb90b42, #1688's arm) changes its line.

Using range-diff rather than re-reading is the convention I am adopting for rebases: a verdict at head N is valid at head M when range-diff N...M shows = for every commit. It separates a rebase (carries) from a real change (needs eyes) in one second.

[bong-water-water-bong]

VERDICT: READY at 243c2ad5bc86d43f1e6aaa0da6303fa8b6f3af3e

Built and ran at this head: all npu-pack checks passed, including final norm: crafted tail envelope imported from tail+64, missing-epsilon control refused.

Two of the three things I raised at 79aac1b are fixed, and the third needs narrowing — including my own wording.

• The conflict is gone: the branch was rebased onto current main and mergeable is now MERGEABLE.
• The a9cc216 guard is tested now, and by arms that need no device: feaa1087 "device-free arms for the two gate fixes, and both fail when…", 67d23098 (a graph of only projections), 243c2ad5 (the tied-tail refusal pinned with a crafted arm). That is exactly what I asked for — the guard was called "the right guard for a genuinely bad tail" and "unreachable", and an unreachable guard with no arm is a guard nobody would notice breaking.
• My "duplicates #1690" needs narrowing. I wrote at 79aac1b that the two "change the same readers the same way". At the current heads they are not the same change: #1690 adds weightless_matmuls counting plus the refusal text for a pack whose placeholder and packed sibling are both empty; #1675 hoists no_bytes() (the dims.empty() → all-dims-zero rule), fixes the lm_head gate that had drifted to dims.empty(), and adds the tied-tail refusal with its arms. They overlap in the placeholder area and both append arms to tests/npu_pack_import_test.cpp, so whichever lands second needs a rebase — but calling them duplicates was me reading two commits that had since diverged.

So the sequencing, not the code, is the open question: #1690 is READY at 3 files, #1675 is this much larger and now also READY. Landing #1690 first is cheaper (smaller diff, fewer arms to re-pin); landing this first means #1690 rebases its arms. Neither is blocked on the other's content — only on the test file's tail. That is a call for whoever merges, and I am not merging either, so I am stating the trade rather than picking.

Nothing else changed in the parts I reviewed at a9cc216: the no_bytes predicate covers []/[0]/[0,0] → no bytes and [1,0] → bytes, and the refusals still name the tensor (crafted packed-empty refusal: the pack names gguf_qweight.empty, and its gguf_qweight.packed sibling exists but carries…).

[bong-water-water-bong]

VERDICT: READY at 243c2ad, conditional on merge order: #1688 → #1690 → this

All three points from my CHANGES at 79aac1b2 are resolved:

1. The duplicate is gone. The bf16 self-check commit is dropped, so #1690 owns that fix. The branch is rebased, and git merge-tree onto current main is clean.
2. The tied-tail guard is pinned. 243c2ad adds a crafted declared-tied arm. I mutated the guard to if (false) locally and the arm fails ("FAIL tied tail arm: … no refusal at all here (head lost silently)"). Restored, "all npu-pack checks passed".
3. The gate fixes and their three arms are unchanged from my earlier mutation checks.

Why the order matters (please merge in this order): the guard refuses a declared-tied head when lmhead_corr <= kTieCorr. On main today the self-check still decodes its bf16 reference as F16 (#1690 is open), which is what produced the 0.885 on Qwen2.5-0.5B-Instruct_rai_1.9.0_npu. At this head on main, that pack doesn't reach the guard yet: the pack leg stops at the placeholder refusal that #1688 removes (I ran it: rc 1 at …qweight.empty … carries no data either). But if #1688 and this land before #1690, the pack reaches the guard with the mis-decoded 0.885, and a correct lm_head.weight is refused on every tied 1.9 pack. With #1690 in first, its corr is 0.999999 and the guard does not fire. So this PR should go after #1690.

Nit (non-blocking): the tied-tail arm's failure text reuses the projections-only arm's wording ("still gets the .fconst tail TRIED … head lost silently"). Naming the oracle ("a tied tail that fails its own embedding check is refused by name") would say which guard failed.

[bong-water-water-bong]

VERDICT at 243c2ad: READY

Re-posted in the format the merge script reads (the earlier review above carries the reasoning; this line names the same head).

[bong-water-water-bong]

VERDICT at 243c2ad: READY

This re-stamps, in the merge script's format, my verdict above at this same head (243c2ad5). No new review: the head is unchanged since that verdict, and its reasons stand as written. Condition unchanged: merge order #1688 → #1690 → this.

[bong-water-water-bong]

Orchestrator: superseded. #1671 is closed: main fixed it with #1687 + #1688 (weightless matmuls are dropped and plan() refuses by name only when the pack has no .fconst). This PR now conflicts with that code in npu/pack_import.cpp. If anything here is not already on main, reduce the PR to that delta on top of main; otherwise please close it.

[bong-water-water-bong]

Closing as superseded, deliberately, rather than rebased as-is: main absorbed two of its commits tonight and a third was a duplicate before that.

• main now drops the matmuls whose placeholder AND .packed sibling both carry no bytes — that is the .fconst-pack import fix and the tail-guard question in one move, done differently and better than my !head_written re-keying (it tells the rest of the importer the truth about the pack instead of working around it at the tail).
• npu-pack: the self-check reads the embedding buffer it is actually given (bf16) #1690 fixed the bf16 self-check, which was my 79aac1b — dropped earlier for exactly this reason.
• npu-pack: two "no graph weights" refusals now spare a pack whose weights are in the .fconst (fixes #1717) #1719 fixed the two "no graph weights" refusals that were blocking the pack leg this PR tests on.

What was genuinely left is the plan() head gate: import_pack() gates the head on N == V && K == H, and plan()'s copy of that loop still took the first MatMulNBits and breaked — reporting a fused qkv ([1152, 896], 132× too few elements) as lm_head.weight on the reference pack. That, with its mutation-checked device-free arm, is #1732.

Thanks for the four verdicts on this one — the mutation-checked arms requirement (three of them, each failing when its gate is reverted) is why #1732 came out with its arm on the first try, and the merge-order condition is what told me to re-check main before rebasing rather than after.

[bong-water-water-bong]

Note for the merge lane: #1732 says it supersedes this PR — after tonight's merges, two of #1675's commits are duplicates of work already on main, and #1732 carries what is left of it (the plan() head gate, the head_from_graph re-keying, and a device-free arm that fails if the gate is reverted). Since both touch the same loop in npu/pack_import.cpp, landing #1732 first and closing this as superseded avoids two open PRs for one change — which is the pattern the owner flagged today. Posting here because the merge lane reads one PR at a time and cannot see the relationship.