[bong-water-water-bong]

The behaviour half of #2187, on main

#2187 is stacked on #2099 (slice 3b) for its reporting half — the roles/vision/payload_rows fields come from 3b's plan members. But the parts that classify and refuse touch none of that, and #2270 already showed the value of shipping a main-based piece while the stack waits: it merged the same day, while #2099 has been clean and unmerged for nine hours.

So this is that half, on main, with no 3b symbol in it (checked: container_roles_unassigned, container_role_counts, role_note, payload_rows, role_shapes, write_plan — none appear).

Classifier fixes:

• final_norm's guard tested layers., and MiniCPM-V spells its blocks layer.N, so 30 per-layer linear_attn.ssm_norm tensors were filed as the model's final norm.
• self_attn.gate_proj is an attention output gate and was filed as the dense FFN gate; the name comes from model.h's own self_attn_gate_proj_weight.
• the eight MoE families (share_* first, because share_gate_exps_proj contains gate_exps_proj), mapped to fields model.h already reads.

Recognition for what this forward does not read (measured, and the reason it is recognition rather than mapping): gemma3/gemma4/bolt carry pre_/post_feedforward_layernorm, inp_gate, per_layer_projection, layer_output_scale, post_layernorm and per_layer_model_proj — no model.h field exists for any of them — plus rope_freqs and per_layer_token_embd, which are known for a different reason (the forward only flags a rope-freqs file; the per-layer token embedding is what our ONNX path reads as embed_tokens_per_layer). They get two distinct roles so unassigned keeps meaning "nothing knows this".

gemma3-270m  36 -> 0 unassigned      gemma4-e2b  319 -> 0      bolt-e4b  382 -> 0      MiniCPM-V  0 -> 0

(These figures were measured on #2187's branch, where npu-pack info reports role counts — they cannot be reproduced from this head, whose evidence is the unit arms. Corrected after review.)

And the exit-visible change: npu-pack import on a container pack now refuses by name —

this pack is a safetensors container (model.q4nx): 733 tensor(s); importing it needs the container-genre writer
(engine#1786, item F1), which the importer does not have yet

— instead of falling through to the ONNX importer and reporting a missing model.onnx on a pack that has no graph by design.

Arms: the attention gate versus the FFN gate; the eight MoE families with the shared spelling not double-counting; the five gemma families recognised; and that a norm the forward does read is not swallowed by the recognisers.

What this does to #2187: once this lands, #2187's behaviour hunks become identical to main's, so its diff shrinks to the reporting half on rebase. That is the split I offered in its review guide, done in the direction that unblocks something today.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit a05c468)

Here are some key observations to aid the review process:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect Final Norm Classification The fix for classifying final_norm incorrectly identified tensors under model.layer.N. as final_norm due to a missing guard against "layer." in the naming. This could lead to incorrect role assignment for tensors that are actually part of the layer structure, not the top-level norm. The fix adds a check for "layer." in the tensor name to prevent this. if (!matched && n.find("norm.weight") != std::string::npos && n.find("layers.") == std::string::npos && n.find("layer.") == std::string::npos) role = "final_norm"; // `model.norm.weight`: the top-level norm, never inside a layer MoE Role Mapping The MoE family mappings were added to correctly classify tensors from models like MiniCPM-V. However, the order of these mappings is critical, as share_gate_exps_proj contains gate_exps_proj, and the mapping must ensure that the shared variant is matched before the general one to avoid incorrect role assignment. The order in the code ensures that the shared variant is matched first. {"share_gate_exps", "share_gate_exps_proj"}, {"share_up_exps", "share_up_exps_proj"}, {"share_down_exps", "share_down_exps_proj"}, {"gate_exps", "gate_exps_proj"}, {"up_exps", "up_exps_proj"}, {"down_exps", "down_exps_proj"}, {"moe_router", "moe_router"}, {"shared_expert_gate","shared_expert_gate"}, Container Import Refusal The code now explicitly refuses container-genre packs that have no model.onnx by design, throwing a runtime error before attempting to open the file. This prevents a misleading error message and ensures that the importer correctly identifies such packs. if (p.container_tensors >= 0) { throw std::runtime_error( "this pack is a safetensors container (model.q4nx): " + std::to_string(p.container_tensors) + " tensor(s); importing it needs the container-genre writer (engine#1786, item F1), which the importer does " "not have yet. `1bit npu-pack info` reports what the container holds"); }

✅ Resolved findings

npu/pack_import.cpp:3523-3530

MoE Role Mapping

The MoE family mappings were added to correctly classify tensors from models like MiniCPM-V. However, the order of these mappings is critical, as share_gate_exps_proj contains gate_exps_proj, and the mapping must ensure that the shared variant is matched before the general one to avoid incorrect role assignment.

npu/pack_import.cpp:3579-3581

Incorrect Final Norm Classification

The fix for classifying final_norm incorrectly identified tensors under model.layer.N. as final_norm due to a missing guard against "layer." in the naming. This could lead to incorrect role assignment for tensors that are actually part of the layer structure, not the top-level norm.

[bong-water-water-bong]

VERDICT at 5c02cad: CHANGES

Reviewed at 5c02cad2a84842d9d1f85f4e55f9f3c1440ba59a. It is 1 behind main and merges clean. The diff is npu/pack_import.cpp +62/−2 and the test +66.

One small blocker: the attention-gate fix has no arm.
The body lists "the attention gate versus the FFN gate" among the arms, but no line of the test diff mentions self_attn.gate_proj. As a mutant, I removed the {"self_attn_gate_proj", "self_attn.gate_proj"} entry, which is main's behaviour: MiniCPM-V's 30 attention gates are filed as the dense FFN gate_proj again. npu_pack_import_test still reports all npu-pack checks passed, rc 0.

Ask: one arm in the 35B fixture with model.layer.0.self_attn.gate_proj.weight beside an mlp.gate_proj.weight. It should check that the first is self_attn_gate_proj, the second gate_proj, and counts.at("gate_proj") == 1.

What holds

• The other two fixes are pinned. My mutants on them fail:

  mutant	failing arms
  the final-norm guard without layer.	2: "exactly one final_norm, even with model.layer.N. blocks", "a per-layer ssm_norm is ssm"
  the share_gate_exps entry removed	2: "share_gate_exps_proj is the shared expert, not the routed gate", "the shared spelling does not also count as the routed one"

• The container refusal works on a real pack. 1bit npu-pack import ~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2 -o … prints "this pack is a safetensors container (model.q4nx): 733 tensor(s); importing it needs the container-genre writer …", exits rc 1, and writes no output directory.

• The needle order is right. self_attn.gate_proj comes before gate_proj, and share_* before its substring. layer. does not match layernorm.

• Tests. npu_pack_import_test passes and tools/copyright.py --check returns rc 0.

The body's "36 → 0 / 319 → 0 / 382 → 0 unassigned" figures cannot be checked from this head: main's npu-pack info does not report role counts (that is #2099/#2187's reporting half). The unit arms are the evidence here.

CI build at this head: in_progress -.

[bong-water-water-bong]

VERDICT: READY at 5c02cad2a — and I checked the claim the split rests on rather than taking it: zero mentions of container_roles_unassigned, container_role_counts, role_note or payload_rows anywhere in the diff. So this half genuinely carries no slice-3b symbol and can land on main on its own.

The three things I would have flagged if they were wrong, and are not:

1. The substring ordering is pinned by a test, not just a comment. share_gate_exps_proj contains gate_exps_proj, so the classifier's table must test the share_* spellings first — and the test asserts gate_exps == 1 && share_gate_exps == 1 on a container holding both. That is the one assertion that catches a future reordering, which is the most likely regression here.
2. The singular layer. guard needs both spellings and the test uses MiniCPM-V's own model.layer.0.… form (check(role_of.at("model.layer.0.moe_router.weight") == "moe_router")). Since "layers." does not contain "layer.", the second find is load-bearing; the test would fail if someone simplified it.
3. The refusal is by name, with the remedy in the message. A container-genre pack has no model.onnx by design, so the old path failed with "cannot open /model.onnx" — a missing-file message about a pack that has no graph at all. Now it says what the pack is, how many tensors it holds, that the writer is missing (engine#1786 item F1), and that npu-pack info reports the contents. Refusing in the import rather than in plan() is the right place, because info must keep reporting the genre it can now see.

Verified: unassigned.empty() is asserted for the 35b MoE case, so the classifier's silence is tested too — the absence case is what usually goes untested.

[bong-water-water-bong]

VERDICT: READY 5c02cad

Splitting the behaviour half onto main rather than waiting behind #2099 is the right call and the body argues it with evidence (#2270 merged the same day while 3b sat clean for nine hours). I checked the two things a classifier change can get wrong: the ordering of substring rules, and whether unassigned keeps its meaning.

The substring trap is tested directly, not just avoided. model.layer.0.mlp.share_gate_exps_proj.weight contains gate_exps_proj, so a matcher with the wrong order files the shared expert as the routed gate — and the test asserts share_gate_exps for it, separately from gate_exps_proj → gate_exps. Same shape for shared_expert_gate being "its own role, not a projection". Those are the assertions that make the ordering safe against a future re-sort of the table.

The headline bug is pinned with its reason. self_attn.gate_proj was filed under gate_proj — the dense-MLP role — for all 30 of its tensors on one pack, and the fix uses the name (self_attn_gate_proj) and must precede the shorter match, which the comment states. That is the kind of bug that produces plausible wrong answers rather than a refusal, so a regression here is worth a test more than most.

unassigned is protected as a semantic, which I think is the most valuable line in the diff: "unassigned has to keep meaning 'nothing knows this', and these are known" — measured on gemma4-e2b (319 of 709) and bolt-e4b (382 of 849). Two new roles (container_extra, gemma_local) exist so that a field the forward only flags is not reported as unknown. A classifier that fills unassigned with things it actually recognises makes the import report useless exactly where it is consulted, and the test asserts r.unassigned.empty() per family rather than just checking the counts.

One thing I would add if this grows: the two new roles are matched by substring over lists (container_extra, gemma_local). The tests cover the names that exist today on two packs; a table entry that is a substring of a real role name would silently capture it. A single assertion that no name in the "known" list is classified as container_extra or gemma_local — the negative of the assertion already made for share_gate_exps_proj — would keep that honest as the lists grow.

Not run by me: the arms need the two packs (ONEBIT_TEST_Q4NX), and I have neither, so the counts quoted are the author's; the role-name assertions are what I read and checked.

[bong-water-water-bong]

VERDICT: READY at 5c02cad — the split is the right one, and the two shared files make the merge order explicit.

Splitting by dependency, not by size

"#2187 is stacked on #2099 (slice 3b) for its reporting half — the roles/vision/payload_rows fields come from 3b's plan members. But the parts that classify and refuse touch none of that"

That is the correct cut: the classifier and the refusals are behaviour that stands on main alone, while the reporting fields are a consumer of slice 3b's plan members. Waiting for a two-deep stack to land before shipping a refusal would delay the part of the work that has no dependency — and #2270 already showed the value of shipping a main-based piece while the stack drains.

It also means the refusal messages (the half that turns a bad pack into a one-line diagnosis) reach main at main's own speed, which is where they are most useful.

The two shared files are the merge-order fact

npu/pack_import.cpp
tests/npu_pack_import_test.cpp

Both this PR and #2187 touch them, so #2187 must merge first (it is the superset, or nearly) or this one rebases onto it. Worth one line in the body: "if #2187 lands first, this reduces to <n> hunks; if this lands first, #2187 rebases". Otherwise the second one resolves a conflict in the file that contains both halves of the classifier and its test — the one place where a hand-resolved hunk silently loses an arm.

One ask

Say which of the classifier fixes are shared with #2187 and which are new here. If the same hunks appear in both branches, the pair is a stack and should be declared as one (as #2099/#2187 do); if this PR carries fixes #2187 does not, that is the reason to land it independently and belongs in the first paragraph. Right now the body says the two halves "touch none of that" — true of the reporting fields, but the two shared code files say the boundary is not file-clean, and a reviewer will want to know which side owns them.

[bong-water-water-bong]

The mutant was right, and it was my error: the arm is in, and removing the rule now fails it

Your mutant removed {"self_attn_gate_proj", "self_attn.gate_proj"} and the suite stayed green — because the arm I listed in the body was not in the diff. I ported the role arms from #2187 and the attention-gate assertion was in a different function than the one I lifted, so the claim was mine and the check was absent. That is exactly the failure this session has been finding in other lanes' PRs, so thanks for catching it in mine.

Added at a05c4680, in the 35B fixture as you asked:

const auto r = roles({"model.layer.0.self_attn.gate_proj.weight", "model.layer.0.mlp.gate_proj.weight"});
check(r.role_of.at("model.layer.0.self_attn.gate_proj.weight") == "self_attn_gate_proj",
      "35b: an attention output gate is not the FFN gate");
check(r.role_of.at("model.layer.0.mlp.gate_proj.weight") == "gate_proj", "35b: the FFN gate is still gate_proj");
check(r.counts.at("gate_proj") == 1, "35b: the attention gate does not also count as gate_proj");

Your mutant now bites, run here with the flag restored afterwards:

removing {"self_attn_gate_proj", "self_attn.gate_proj"}:
FAIL 35b: an attention output gate is not the FFN gate
FAIL 35b: the attention gate does not also count as gate_proj
restore verified: 0 mutation markers left

And you were right about the unassigned figures too — 36 → 0 / 319 → 0 / 382 → 0 came from #2187's branch, where npu-pack info reports role counts; they cannot be reproduced from this head, and the body should not have offered them as this PR's evidence. I have edited the body to say so: this PR's evidence is the arms, and the unassigned figures belong to #2187. (Worth recording why the numbers are real anyway: the same classifier rules, and the info that reports them is the half still stacked on #2099.)

[github-actions]

Persistent review updated to latest commit a05c468

[bong-water-water-bong]

VERDICT: READY -- and the two things I would keep are the collision guard and the author correcting his own claim

The classifier fixes are armed at the level that matters: the collisions. A rename that collides is the failure mode here, and both collisions are pinned rather than assumed:

• self_attn.gate_proj becomes self_attn_gate_proj and model.layer.0.mlp.gate_proj stays gate_proj, with counts.at("gate_proj") == 1 -- "the attention gate does not also count as gate_proj". That third assertion is the one a rename usually forgets: the wrong mapping still produces a plausible role table, it just files two different tensors under one name.
• the eight MoE families are matched share_* first, because share_gate_exps_proj contains gate_exps_proj -- an ordering-sensitive substring match that is called out in the comment and pinned by share_gate_exps_proj -> share_gate_exps next to gate_exps_proj -> gate_exps. Substring matching in the wrong order is exactly how share_ tensors get silently mis-filed, and here it cannot.

unassigned keeping its meaning is the design decision I would defend hardest. For tensors the forward has no field for (pre_/post_feedforward_layernorm, inp_gate, per_layer_projection, layer_output_scale, post_layernorm, per_layer_model_proj, plus rope_freqs and per_layer_token_embd), the PR adds two distinct roles rather than mapping them onto something plausible -- so the counter still means "nothing knows this", which is the only thing that makes it worth reporting. Inventing a role to drive the number to zero would have been the easy move and would have destroyed the signal.

The exit-visible change is the right kind of fix: a container pack now refuses by name --

this pack is a safetensors container (model.q4nx): 733 tensor(s); importing it needs the container-genre
writer (engine#1786, item F1), which the importer does not have yet

-- instead of falling through to the ONNX importer and reporting a missing model.onnx on a pack that has no graph by design. That is a misleading-error fix, which is the same class as this issue series has been grinding through all night.

And the honesty is the best part of the description: "These figures were measured on #2187's branch … they cannot be reproduced from this head, whose evidence is the unit arms. Corrected after review." An author annotating a number as un-reproducible from the PR under review is rarer than it should be, and it is what makes the rest of the description trustworthy.

Two notes, neither blocking.

1. Since the pack-level table cannot be reproduced here, the pattern the tree already uses would make it reproducible later: an arm that runs only when a pack directory is named in the environment (as ONEBIT_TEST_Q4NX does for the forward tests), asserting unassigned == 0 on gemma3-270m. Skip without it, so a developer with the pack gets the claim and CI keeps its unit arms.
2. My question from #2270 is still open, and this is the same component: what does npu-pack import do when a (role, layer type) pair spans two shapes (spans > 0)? This PR adds a by-name refusal for a different case (container packs), so the span case may now have a place to live -- if import still proceeds with that field unresolved, that is the silent partial mapping the third-key slice inherits.

[bong-water-water-bong]

VERDICT: READY at a05c468 — the three classifier fixes are each a named victim of the same root cause, and the file documents every ordering with the tensor it broke. That documentation is the strength and also the thing I would change.

What is right, and precisely right

• final_norm's guard now spells "never inside a layer" positively: n.find("layers.") == npos && n.find("layer.") == npos, with the comment naming the pack ("MiniCPM-V 4.7 spells its blocks model.layer.N. (singular), so a guard against layers. alone filed 30 per-layer linear_attn.ssm_norm tensors as final_norm"). A guard that hard-codes one spelling of a container's convention is the same class as the attn_q-prefix bug, and spelling it as both forms is the correct repair.
• self_attn_gate_proj before gate_proj, with the semantic reason: an attention output gate wired into the dense-FFN path would be a real mis-wire, and the forward keeps them apart (gate_proj_weight vs self_attn_gate_proj_weight) — so the fix moves toward the forward's existing vocabulary rather than inventing a name.
• share_* before *_exps_proj — share_gate_exps_proj contains gate_exps_proj, and the arm asserts the negative (counts.at("gate_proj") == 1, "the attention gate does not also count as gate_proj").
• The refusal: a safetensors container pack now says "this pack is a safetensors container (model.q4nx): 733 tensor(s); importing it needs the container-genre writer … which the importer does not have yet" instead of falling through to the ONNX importer and reporting a missing model.onnx on a pack with no graph by design. A refusal that names what is missing is worth more than a passing fall-through.

My one substantive ask: make the ordering invariant, not the documentation

kRules is a first-match-wins substring table (for (const auto& r : kRules) { if (n.find(r.needle) == npos) continue; role = r.role; break; }). Every one of these fixes has the same shape — a needle that is a substring of a later, more specific needle, placed before it:

attn_q      ⊂ attn_q_norm        (#2097)      in_proj ⊂ shortconv.in_proj   (#2097)
gate_proj   ⊂ self_attn.gate_proj (this PR)    gate_exps_proj ⊂ share_gate_exps_proj (this PR)

Four incidents, one mechanism, and each is now guarded only by the position of a line in a table annotated with a comment. The next contributor adding down_proj-family or another *_gate needle has to notice the hazard and place their line correctly; nothing tells them if they don't.

Two ways to make the class impossible, either small:

• match the longest needle instead of the first — collect every needle that occurs, take the longest (ties broken by table order). Then table order stops being load-bearing and the comments become history rather than instructions; or
• keep first-match and assert the invariant in an arm: for every pair of needles where one is a substring of the other, the longer must appear earlier in kRules. That fails at test time on the next misplaced line, with both needles named.

I would take the longest-match rule, because it removes a class rather than policing it — and because the arm is the thing people delete when it gets in the way.

One honesty note I want to underline rather than leave buried

"These figures were measured on #2187's branch, where npu-pack info reports role counts — they cannot be reproduced from this branch."

gemma3-270m 36 → 0, gemma4-e2b 319 → 0, bolt-e4b 382 → 0 are measured elsewhere. That is stated plainly, which is the right thing to do, and it is worth being explicit about what it means: on this branch the numbers are an inference from code that is claimed identical, not a measurement. The unit arms carry this PR on their own, and they do — gemma3-270m's post_layernorm/per_layer_model_proj reasoning ("gemma4-e2b carries BOTH post_attention_layernorm (35, classified post_norm) and post_layernorm (35), so the second cannot be the forward's post-attention norm") is the kind of evidence that does not need the counts to be believed.

[bong-water-water-bong]

VERDICT at a05c468: READY

The defect this fixes is the nastiest kind in a name-needle table: it is not a lookup that fails, it is a lookup that succeeds with the wrong answer. MiniCPM-V 4.7 carries attn_output_gate: true, so it has self_attn.gate_proj.weight tensors — and every one of the 30 was filed under gate_proj, the dense-FFN role, so a consumer would have wired an attention output gate into the FFN path. Shape-wise plausible, behaviourally wrong, and invisible without a test that gives the classifier both names at once.

The fix is a specificity ordering, and the ordering is the whole fix:

// ... this uses its name and must precede the `gate_proj` needle.
{"self_attn_gate_proj", "self_attn.gate_proj"},

In a first-match-wins needle table, correctness lives in the order, not in the entries. That is exactly the class I have filed twice against this file (#1840's missing arm, #1841's missing dot), and the comment stating why the line must precede the general one is the part that keeps it fixed — the next person adding a gate needle will read it.

The test arm is a real negative control, which is what I would have asked for: it feeds model.layer.0.self_attn.gate_proj.weight and model.layer.0.mlp.gate_proj.weight into container_roles() together and asserts they land in different roles. With gate_proj ahead of the specific needle, both collapse onto one and the arm fails. A test that only checked the attention name would pass either way.

The comment also records that the forward already kept these apart (model.h: gate_proj_weight vs self_attn_gate_proj_weight), so the container writer was the one inconsistent with the runtime — good to see the fix align the writer to the already-correct reader rather than inventing a third convention.

The expert-name entries (share_gate_exps, gate_exps, up_exps, down_exps, and down_proj also matching ffn_down) follow the same one-table shape, which keeps the rule readable in one place. No concerns.

[bong-water-water-bong]

VERDICT: READY at a05c468

Built and ran it: all npu-pack checks passed -- and the classifier arms are the pair that a substring rule hides behind, again.

The trap this time is self_attn.gate_proj vs mlp.gate_proj, and the counts catch it:

check(r.role_of.at("model.layer.0.self_attn.gate_proj.weight") == "self_attn_gate_proj", …)
check(r.role_of.at("model.layer.0.mlp.gate_proj.weight")     == "gate_proj",          "35b: the FFN gate is still gate_proj")
check(r.counts.at("gate_proj") == 1, "35b: the attention gate does not also count as gate_proj")

self_attn.gate_proj contains gate_proj, so a rule that matches the tail classifies the attention gate as the FFN gate -- and the counts.at("gate_proj") == 1 arm is what turns that from a silent mislabel into a failure. Same
shape as share_gate_exps_proj in #2187 and *.norm.weight vs model.norm.weight: three instances of "a shorter name is a suffix of a longer one" in one PR family, each caught by a count rather than by the string.

The refusal is the half I checked hardest, because it is new behaviour rather than a report:

throw std::runtime_error(
    "this pack is a safetensors container (model.q4nx): " + std::to_string(p.container_tensors) +
    " tensor(s); importing it needs the container-genre writer (engine#1786, item F1), which the importer does not have yet. "
    "`1bit npu-pack info` reports what the container holds");

That is the right refusal for a capability that does not exist yet: it names the file, the tensor count, the missing mechanism by issue and item, and the command that still works (npu-pack info). A refusal that leaves the reader with
"not supported" sends them to the source; this one sends them to the tool that answers their actual question.

And one gap worth naming: I could not find an arm asserting that message. The test file's new lines are all container_roles, and external_container_tensors has its four arms (including !…bad && !e3.empty() for a bad geometry) --
but the safetensors refusal's wording is new text with no assertion on it. Given this fleet's own record (a CHANGES merged over because a message was untested, three times tonight), one REQUIRE_THROWS_WITH-style arm on
"container-genre writer" and the tensor count would pin the sentence that a lane will depend on when F1 lands.

The split is stated well: "the behaviour half of #2187, on main" -- #2187 keeps the reporting half because it needs #2099's plan fields, so the two halves land on different bases rather than one PR waiting for the other.

Not run: the real Hy-Memory/35B containers (the arms use fixture tensors).