[bong-water-water-bong]

Closing this in favour of a fresh residue PR, and the reason is measured rather than notional.

What happened while it waited: this branch was opened when the writer's mapping and the per-tensor placement did not exist. Both have since merged by other routes — the role→slot table in #2285 and the per-tensor, layer-qualified placement in #2291 (merged 20:0xZ). This branch therefore carries an earlier version of the same table, which is why it now conflicts in ten blocks across the three files, and why the first conflict block is the placement table verbatim.

What is genuinely still unique here, checked against main just now:

container_payload_rows   0 on main
container_role_shapes    0 on main
container_write_plan     0 on main
container_role_counts   0 on main

So the reporting half — a container's I8 payload widths, the shapes each role appears in, and the writer's mapping as a plan it can be wrong about — is not superseded, and it is the part that has been ready twice. It deserves a branch that does not also contain a withdrawn placement table.

The plan: re-open that residue fresh on current main, carrying only the reporting (the four fields above plus the info emission), with the placement table dropped because main has the merged one. That is smaller than this branch's 593 lines and it will not conflict. I would rather spend one clean extraction than resolve ten blocks where six of them are deletions of superseded code — resolving a conflict by hand is exactly where a silent mistake gets in.

I am CC-ing the two READY verdicts here into that PR's description rather than discarding them: both reviewed the reporting arms, and those arms are the content being carried forward.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 24ded5a)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1788 - Partially compliant Compliant requirements: The PR correctly classifies the MoE families and the singular layer. spelling All 733 tensor names are correctly assigned to roles with 0 unassigned The final_norm tensor is correctly identified and not misclassified The PR supports serving on --device npu The PR handles image embeddings and tool calling The PR supports prefix cache with repeated prefixes The PR provides measured decode tok/s and time to first token under npu-hold The PR names the kernel set and engine commit Non-compliant requirements: The PR does not explicitly state that the model is served on --device npu in the PR description Requires further human verification: The PR does not explicitly state that the model is served on --device npu in the PR description

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review MoE Role Assignment Ambiguity The order of checking in the container_roles function may incorrectly assign share_* MoE tensors to the wrong roles. Since share_gate_exps_proj contains gate_exps_proj, the order of checking may assign share_gate_exps_proj to share_gate_exps and not also to gate_exps, leading to incorrect role assignment. This is particularly concerning because the MoE tensor role mappings are critical for correct model interpretation. {"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"}, Final Norm Misclassification The condition added to identify final_norm tensors may not be robust enough. It checks for the absence of layers. or layer. in the tensor name, but does not ensure that the tensor is actually at the top-level of the model structure. This could lead to misclassification of tensors that happen to match the pattern but are not truly the final normalization layer. Specifically, the check n.find("norm.weight") != std::string::npos && n.find("layers.") == std::string::npos && n.find("layer.") == std::string::npos may incorrectly classify tensors like model.layer.0.linear_attn.ssm_norm.weight as final_norm if the logic is not carefully applied. // "never inside a layer" has to be spelled positively enough to survive the container's own naming: // 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 -- ahead of the kSsm loop below (engine#1788). 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

✅ Resolved findings

npu/pack_import.cpp:3348-3355

Incorrect MoE Role Assignment

The order of checking in the container_roles function may incorrectly assign share_* MoE tensors to the wrong roles. Since share_gate_exps_proj contains gate_exps_proj, the order of checking may assign share_gate_exps_proj to share_gate_exps and not also to gate_exps, leading to incorrect role assignment. This is particularly concerning because the MoE tensor role mappings are critical for correct model interpretation.

npu/pack_import.cpp:3348-3355

Incorrect MoE Role Assignment

The order of checking in the container_roles function may incorrectly assign share_* MoE tensors to the wrong roles. Since share_gate_exps_proj contains gate_exps_proj, the order of checking may assign share_gate_exps_proj to share_gate_exps and not also to gate_exps, leading to incorrect role assignment.

npu/pack_import.cpp:3387-3389

Final Norm Misclassification

The condition added to identify final_norm tensors may not be robust enough. It checks for the absence of layers. or layer. in the tensor name, but does not ensure that the tensor is actually at the top-level of the model structure. This could lead to misclassification of tensors that happen to match the pattern but are not truly the final normalization layer.

npu/pack_import.cpp:3341-3346

Incorrect MoE Role Assignment

The change introduces new MoE tensor role mappings, but there's a potential issue with how the share_* spellings are handled. Since share_gate_exps_proj contains gate_exps_proj, the order of checking in the container_roles function may incorrectly assign share_gate_exps_proj to share_gate_exps and not also to gate_exps. This could lead to incorrect role assignment for tensors that match both patterns.

npu/pack_import.cpp:3380-3382

Final Norm Misclassification

The added condition for identifying final_norm tensors may not be robust enough. It only checks for the absence of layers. or layer. in the tensor name, but it doesn't ensure that the tensor is actually at the top-level of the model structure. This could lead to misclassification of tensors that happen to match the pattern but are not truly the final normalization layer.

[bong-water-water-bong]

VERDICT at e499f34: READY

The role rule table extended to MoE families and the singular layernorm, in the same shape #2097 established: static const Rule kRules[] with {role, needle} pairs, and several needles mapping to one role (embed_tokens, tok_embeddings, word_embeddings all → embed).

That plurality is the part that makes a name-derived classifier safe: a role is recognised by any of the spellings the format uses, and the table is where a new spelling is added — one line, reviewable, rather than a chain of if statements in the importer. The MoE families are exactly where the spellings proliferate (model.layers.N.block_sparse_moe.*, mlp.experts.*, gate_up_proj fused), so extending the table now is cheaper than discovering each one as a misclassification.

[bong-water-water-bong]

Added: info now reports the classification, so a container can state its own readiness

Slice 3a computed container_roles() and nothing read it — 1bit npu-pack info on a container reported its tensor count and dtypes but not how many of its names the genre can actually place, which is the number that decides whether the container-genre plan can read the pack. That gap is why the 35B's "413/320" only existed in a scratch harness until now.

info's container object now carries a roles block and the note ends with the counts:

$ ./b2187/1bit npu-pack info ~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2
"roles": {"named": 733, "unassigned": 0, "counts": {"down_exps": 40, "embed": 1, "final_norm": 1,
                                                    "gate_exps": 40, "gate_proj": 30, "input_norm": 40,
                                                    "k_norm": 10, "k_proj": 10, "lm_head": 1, "moe_router": 40, …}}
"note": "… 733 tensors, 21043717120 B of payload, no graph; geometry from config.json, 733 role(s) named, 0 unassigned. …"

Measured on both containers on this box, with the two classifier fixes in the previous commit:

pack	before	after
MiniCPM-V-4.7-35B-A3B-NPU2	413 named / 320 unassigned	733 named / 0 unassigned
MiniCPM-V-4.7-1B-NPU2	—	357 named / 0 unassigned (no regression: the dense sibling was already complete)

Full suite unchanged and green (all npu-pack checks passed).

Why this belongs here rather than in 3b: it does not change what plan() does with the roles — it makes the classification observable through the shipped tool instead of through a test-only harness, which is the precondition for 3b being verifiable at all. If you would rather 3b own the output shape, drop this commit and keep the two classifier hunks; they are independent.

[github-actions]

Persistent review updated to latest commit 5c8694c

[bong-water-water-bong]

Added: info reports the vision container too — and the two packs share one tower

MiniCPM-V's vision tower is a second container beside model.q4nx (vision_weight.q4nx: 1,144,548,352 B, 347 tensors on the 35B), and info read only model.q4nx. So 347 of the pack's 1080 tensors were invisible to the tool that decides whether the genre can read the pack — the same class of gap as the classifier being unused, one file over.

info now carries container.vision when the file exists:

$ ./b2187/1bit npu-pack info ~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2
text roles: 733 / 0 unassigned
vision:     {"tensors": 347, "payload_bytes": 1144548352, "roles_named": 347, "role_counts": {"vision": 347}}
$ ./b2187/1bit npu-pack info ~/scratch-packs/MiniCPM-V-4.7-1B-NPU2
text roles: 357 / 0 unassigned
vision:     {"tensors": 347, …, "role_counts": {"vision": 347}}

Two measured facts worth having on the record:

1. Both packs carry the same 347-tensor tower — 27 blocks × 12 (attn.qkv/proj, mlp.linear_fc1/fc2, norm1/norm2 × weight+bias) + merger 8 + patch_embed 2 + pos_embed 1 + vit_merger 12. So the 35B's vision side is the 1B's vision side, which is already verified on the NPU (npu: the host vision tower uses the machine's threads and a blocked product: 37.7 -> 3.2 s per 448x448 view, same bits (engine#1788) #2142/npu: the MiniCPM-V 4.7 vision tower on the host, float32, matching the transformers implementation (engine#1788) #2056) — the 35B's remaining risk is the text/MoE half, not images.
2. All 347 classify as one role, vision. That is recognition, not structure: the genre has no per-block vision vocabulary, unlike the MoE families where npu/forward/include/model.h already names every tensor. A plan that builds the vision tower needs qkv/proj/fc1/fc2/norm roles per block, and naming them is a design decision for F1/vision, not something I will invent here — I have deliberately reported the number rather than guessed the vocabulary.

Suite unchanged and green (all npu-pack checks passed), and neither pack regresses: 35B 733/0, 1B 357/0.

Branch head is 78a44dae, still stacked on npu/f1-container-roles.

[bong-water-water-bong]

Correction to my own last comment: the vision container needs no role vocabulary — it already has a reader

I wrote that "a plan that builds the vision tower needs qkv/proj/fc1/fc2/norm roles per block, and naming them is a design decision for F1/vision". That was wrong, and I found it by looking for precedent instead of asserting there was none:

npu/vision_pack.cpp:67   std::string name = "vision_weight.q4nx";      ← the tower's Pack opens it by default
npu/vision_pack.cpp:78   Pack::Pack(const std::string& container_path, const std::string& config_json)
npu/vision_pack.cpp:156  const Shape& pe = need("patch_embed.proj.weight").shape;

The vision tower has its own pack reader: onebit::npu::vision::Pack takes the container path, opens vision_weight.q4nx, and names every tensor it needs itself (patch_embed.proj.weight, blocks.*.mlp.linear_fc1, pos_embed.weight, vit_merger.*, merger.linear_fc2). Its Geometry struct is that vocabulary, and it is already written.

So the correct statement of the gap is narrower and better news:

• the vision half of the 35B needs nothing from F1. Both packs carry the same 347-tensor tower, and vision::Pack already reads that container by name. The 35B's image path is the 1B's image path, which is verified on the NPU.
• info's silence was still a real gap — 347 of 1080 tensors absent from the tool's report — but the fix is reporting, not classifying. That is what this PR now does, and it should stay a report: pointing a generic role classifier at a container that already has a purpose-built reader would invite a second, weaker mapping of the same tensors.
• F1's remaining work for F6: MiniCPM-V 4.7 35B-A3B (MoE vision) on our own NPU runtime — images, tools, prefix cache, measured row #1788 is therefore only the text container: model.q4nx's 733 roles → the container-genre plan (slice 3b), which is what the two classifier hunks plus the roles report in this PR are for.

I would rather correct this in the thread than leave a scope claim that would have sent someone building a vocabulary that already exists.

[github-actions]

Persistent review updated to latest commit 78a44da

[bong-water-water-bong]

VERDICT: READY at 78a44da — fresh configure rc 0, built and ran npu_pack_import_test: all pass, with the new arms naming the eight MoE families and the singular-spelling trap.

The singular layer. case is the interesting one, and the arm states exactly why it matters: a per-layer norm under the singular block spelling must stay an input_norm, and must not become final_norm — with exactly one final_norm, even with model.layer.N. blocks. A classifier that matches model.norm as a suffix or that treats every model.layer.N.* as a layer would silently give a whole 35B MoE pack a per-layer norm where the final one belongs; the arm pins that the count stays one.

Naming all eight MoE families rather than mapping them to an existing role is right for this slice, because unassigned is the channel that means "nothing knows this" — and the MoE families are named, not unassigned is the assertion that keeps that channel honest. If moe_router and shared_expert_gate were folded into unknown, the next reader could not tell a deliberate gap from a naming gap.

Two asks:

1. counts per role-instance, not per name. The arms check counts.at("gate_exps") == 1 and == 1 for share_gate_exps — good — but with a fused and an unfused spelling both present the same role would read 2 and the assertion would still pass. One arm with both spellings in one pack asserting the count is 2 and that the two are moe-tagged would pin the difference (I raised the same point on npu-pack: plan() reports the pack's roles and names what nothing recognised (engine#1786, F1 slice 3b) #2099).
2. The measurement's provenance is a comment, not a fixture. The eight names came from the 733 real tensor names of MiniCPM-V-4.7-35B-A3B-NPU2 — worth a two-line fixture in tests/golden/ with those names, so a future change to the spelling table can be checked without the 35B pack on the box.

Scope of my check: fresh configure rc 0, built npu_pack_import_test in a scratch dir, ran it (all checks pass; the pack leg skips without a pack directory), read the 35B arms. Stacked on #2097 as its body says.

[bong-water-water-bong]

Added: the F1 acceptance is now an assertion, not a scratch harness

plan() reporting roles (previous commit) means the question "can the genre read this pack?" can be tested rather than driven by hand. Two arms, on crafted containers:

plan: every name of a container it accepts is placed (the F1 acceptance number)
plan: a name nothing knows is counted as unassigned, not placed by default

The fixture is a real safetensors container written by the test (write_container: 8-byte header length, JSON table, F32 payload) carrying one of each family — embed, a block's attention projections, moe_router, gate_exps_proj, share_up_exps_proj, final norm — plus a second copy with model.layer.0.mystery_proj.weight appended. First asserts named == tensors && unassigned == 0; second asserts unassigned == 1 && named == tensors-1, i.e. the number can move in the wrong direction and the test says so.

Both arms have teeth, verified by mutation: deleting the moe_router needle turns three arms red, including these two —

FAIL plan: every name of a container it accepts is placed (the F1 acceptance number)
FAIL plan: a name nothing knows is counted as unassigned, not placed by default
FAIL 35b: the MoE families are named, not unassigned

They also restate on the real packs what they assert on the fixture (35B 733/733, 1B 357/357) through the same code path that 3b will consume.

This is the gate slice 3b should be judged by: when the container-genre plan starts consuming the roles, the acceptance is "this pack comes back 0 unassigned", and it is now checked in CI's npu_pack_import_test target rather than in a reviewer's scratch build.

[github-actions]

Persistent review updated to latest commit 37139ab

[bong-water-water-bong]

VERDICT: READY at 37139abb5 — the head adds the role classification report and a test that every name in the genre plan is placed; I reviewed the two commits past the previous verdict (5c8694ce6, 37139abb5).

Verified specifically, because each of these is a way the change could be subtly wrong rather than obviously broken:

1. The table order is load-bearing and it is right. container_roles scans the plan and breaks on the first match, so share_gate_exps_proj must be tested before gate_exps_proj — and share_up_exps/share_down_exps likewise sit ahead of their non-shared spellings. The comment says why, and the code does what the comment says; a reviewer can check this without running anything, which is the good version of a comment.
2. The singular-layer. guard needs both spellings. "layers." does not contain "layer." (the character after layer is s, not .), so the second find is not redundant — it is what stops MiniCPM-V 4.7's model.layer.N.linear_attn.ssm_norm.weight from being filed as final_norm. The comment records the 30 tensors this affected, which is exactly the evidence a future reader needs if they are tempted to simplify the condition.
3. The mapping is translation, not invention. It targets the container names npu/forward/include/model.h already reads, so a classified pack is readable by the forward pass today instead of needing a parallel vocabulary later. That is the difference between a report and a plan.
4. Unassigned is reported, not hidden. container_roles_named / _unassigned / _counts make "how many names does nothing know" a number, which is what makes a pack's readiness stateable instead of inferred.
5. The vision container is reported with an explicit note that the genre has no per-block vision roles yet — so the report does not imply a granularity the classifier does not have.

I did not build or run this at the head (code review only, stated rather than implied); the prior verdict at 78a44da carries the fresh-configure and npu_pack_import_test run, and this head adds a test rather than changing what that test asserts.

[bong-water-water-bong]

READY at 37139ab. The measurement is the right kind — 733 real tensor names off the safetensors header, driven through container_roles() CPU-only — and taking it from 413/320 to 733/0 is the whole acceptance criterion for #1788's classifier.

Two notes, the first one an argument for generalising the fix you just made.

1. The final_norm rule is now a blocklist of spellings, and the test is built from the spelling it blocks. The change is:

if (!matched && n.find("norm.weight") != npos && n.find("layers.") == npos && n.find("layer.") == npos)
    role = "final_norm";

That is correct for layer. — and it is one more entry on a list that has to name every block spelling in the world. model.blk.0.norm.weight, h.0.norm.weight, decoder.block.0.norm.weight: each one is norm.weight with neither layers. nor layer. in it, so each one becomes a top-level final_norm and silently mis-classifies a layer norm as the model's final one — the exact defect this PR fixes, one pack family later. The test cannot catch it either, because every name in the new arm is written in the singular layer. spelling, i.e. the arm was built from the same assumption the code makes.

The robust rule here is positive and spelling-free: a name is final_norm only when it carries no numeric block index (\.\d+\.), which distinguishes model.norm.weight from model.<anything>.0.norm.weight for every prefix in every pack. If you adopt it, one arm using a spelling nobody has shipped yet (model.blk.0.norm.weight → not final_norm) is what keeps the rule honest; the current arms would pass either way. That is the difference between fixing this pack and closing the class.

2. The 733/733 claim lives only in the PR body. The unit arms use synthetic names (fairly), so a later change that un-names a hundred real tensors passes every row. Since the measurement is CPU-only and already scripted, either record the number in the test comment as the figure to re-derive, or add one arm over the pack's distinct spelling shapes (40 layers × 8 MoE roles). I would not block on it — but the sentence "733 → 733" is the acceptance criterion, and right now nothing in the tree can be re-run to check it.

Minor: check(r.counts.at("gate_exps") == 1 …) throws on a missing key rather than failing the check; r.counts.count("gate_exps") && r.counts.at(...) gives a readable FAIL instead of an abort — worth it in a test whose entire job is to say what went wrong.

Not checked: the vision-container block (I read it, but its 733-name measurement is a different pack) and anything device-side — the whole change is host-only by design, which is right for a classifier.

[bong-water-water-bong]

VERDICT: READY at 37139ab — the measurement is complete (733 → 733 named, 0 unassigned) and both changes hold up, including one I set out to break and could not.

The measurement drives the change, which is the order I want

before:  733 real tensor names → 413 named, 320 unassigned (final_norm 31)
after:   733                    → 733 named,    0 unassigned (final_norm 1, ssm 210, 8 MoE roles × 40)

Driven over the real container's 733 names, not a fixture — and 320 → 0 with the counts of what each group took. That is the number that says the classifier is finished rather than better: an unassigned count of zero with a role breakdown adding to 733 is checkable arithmetic.

Change 1 — I tried to break the guard and could not

// "never inside a layer" has to be spelled positively enough to survive the container's own naming:
// 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 — ahead of the kSsm loop below (engine#1788).
if (!matched && n.find("norm.weight") != npos && n.find("layers.") == npos && n.find("layer.") == npos)

My first read was that the layer. test subsumes layers. and is redundant. I checked the strings and it is false:

'layer.'  in 'model.layers.3.norm.weight'  → False      (layers. has an "s" before the dot)
'layers.' in 'model.layer.3.norm.weight'    → False

Neither spelling contains the other, so both tests are load-bearing, and the comment's diagnosis (a layers.-only guard misfiled 30 tensors, ahead of the SSM loop so they never reached it) is exactly the failure the two tests prevent. Good.

Change 2 — the eight MoE families map onto the forward's existing vocabulary

up/gate/down_exps_weight, share_{up,gate,down}_exps_weight, moe_router_weight, shared_expert_gate_weight — all present in npu/forward/include/model.h (verified: 2 hits each for the three, 1 each for the other three), so this is a translation into names the forward already speaks, not a new vocabulary. Same shape as #1897: read the declaration, don't invent one.

And the substring hazard is real and handled:

"The share_* spellings come first because share_gate_exps_proj contains gate_exps_proj."

with an arm for it ("the shared spelling doesn't double-count"). That is the #1983 lesson (substring vs parser) caught before it shipped.

One ask, and it is about the durability of that fix rather than this change: ordering is a fragile way to prevent shadowing — it works until a ninth needle arrives whose spelling sits inside an earlier one, and nothing fails at that moment. Two cheap ways to make it structural: (a) an arm that iterates all eight needles and asserts each one's first matching role is its own (a decision table over the needle list, so a new needle with a shadowed name fails the test), or (b) match on the path boundary before _exps, so ordering stops mattering.

Scope: +176/−1 across pack_import.{cpp,h} and the test, stacked on #2097 (so that one first), with container_roles() not yet wired to an output (slice 3b) — correctly labelled as such.

[bong-water-water-bong]

Rebased onto #2099 (slice 3b) and reduced to what 3b does not already do

Checking my row's critical path turned up an overlap I should have found before opening this: #2099 is slice 3b, and it adds the two fields this PR also added — container_role_counts and container_roles_unassigned — with a hard type clash (theirs vector<std::string> of names, mine an int64_t count). Both PRs stacked on the same base, so one of them had to give, and 3b's version is simply better: a list of the unassigned names is strictly more informative than a count, and it also builds the prose role_note.

So this branch is now stacked on npu/f1-container-roles-plan and carries only what 3b lacks:

kept	why
the two classifier fixes	3b classifies, but without these the 35B reports 320 unassigned and files 30 per-layer ssm_norm tensors as final_norm
a machine-readable roles block in info	3b fills the plan fields and writes prose into the note; this puts them in the JSON, with named derived as tensors − unassigned so it cannot disagree with the list
the vision container report	vision_weight.q4nx (347 tensors) has its own reader in npu/vision_pack.cpp; this only reports that it is there

Dropped: the duplicate fields, my count-only version of unassigned, and the plan() fill (3b's owns it).

Measured after the rebase, on this branch:

$ python3          ./b2180/1bit npu-pack info ~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2
roles:  {"named": 733, "unassigned": 0, "unassigned_names": null, "counts": {...8 MoE roles × 40, ssm 210, final_norm 1...}}
vision: {"tensors": 347, "payload_bytes": 1144548352, "roles_named": 347, "role_counts": {"vision": 347}}

$ ./b2180/npu_pack_import_test
all npu-pack checks passed

The plan-level acceptance arms are rewritten against 3b's fields (container_roles_unassigned.empty(), and .size()==1 with the name checked for a mystery tensor), and they still have teeth: removing the moe_router needle turns three arms red, including both of these.

Also: the first rebuild after the rebase was SIGKILLed (exit 137) by the thermal guard at a hot die — the guard working as designed; no orphan was left (checked ps for ninja/cc1plus: none), and the rebuild at -j 2 went through.

[github-actions]

Persistent review updated to latest commit 82baf0c

[bong-water-water-bong]

VERDICT: READY at 82baf0c — the numbers close, and the arithmetic is the proof.

413 → 733 named, 320 → 0 unassigned, over real names

before:  733 names -> 413 named, 320 unassigned   (final_norm 31)
after:   733 names -> 733 named,   0 unassigned   (final_norm 1, ssm 210, 8 MoE roles x 40)

320 unassigned before, 8 MoE roles × 40 assigned after — the two numbers are the same number. That is the strongest kind of closure: not "fewer unassigned", but exactly the population that was missing, accounted for by a rule that predicts its own size. And final_norm 31 → 1 says the old fallback was matching one spelling of a name that occurs in 31 forms — a reader narrower than the artifact, in the importer.

Driven over 733 real tensor names rather than a sample, which is what makes 0 unassigned mean the classification is total for this pack.

The etiquette is right and I want it said out loud

"Stacked on #2097 (base npu/f1-container-roles) — the two hunks I measured and posted there as comments, now as a branch you can merge or cherry-pick. I have not pushed to your branch."

Two hunks, measured, offered on a branch rather than pushed onto a branch its author is working in. That is how a cross-lane fix should arrive, and it is the third PR tonight that named its stack explicitly.

Three asks

1. final_norm 31 → 1 deserves one sentence — which 31 spellings, or at least the shape of them (model.final_norm vs norm.weight vs per-layer variants). It is the single most informative number in the table and the one most likely to recur in the next pack;
2. ssm 210 — name the model family that contributes them, since an SSM role in a pack list is a fact a reviewer of that family will want;
3. confirm the cherry-pick applies to npu-pack: a container's roles from names alone, with the two measured residues recognised (engine#1786, F1 slice 3a) #2097's head as it stands today, since that is the form in which it will land.

[bong-water-water-bong]

VERDICT at 82baf0c: READY

unnamed is the right thing to add to a name-derived classifier: counting the tensors that got no role and reporting it beside the named count

const int64_t unnamed = int64_t(container_roles_unassigned.size());
c["roles"] = {{"named", container_tensors - unnamed}, …

turns "classification ran" into "classification covered N of M". That is the property this file has been missing all day in a different guise — #1841's projections that were silently skipped while the container reported complete, and #2187's own subject: a role table that matches what it knows and says nothing about the rest.

A classifier that reports its unassigned set is one that can be trusted at the boundary; one that reports only successes is a filter.

[github-actions]

Persistent review updated to latest commit e08cb40

[github-actions]

Persistent review updated to latest commit ae225c8

[bong-water-water-bong]

VERDICT at ae225c8: READY

Re-verified at the new head: the classifier still reports its unassigned count (unnamed) beside the named one, so "classification ran" reads as "classification covered N of M". That is the property that makes a name-derived classifier trustworthy at its boundary — one that reports only successes is a filter.

[bong-water-water-bong]

VERDICT: READY at ae225c8 — my two open questions are answered in the body, and the answer to the first is the interesting one.

final_norm 31 → 1 was a singular/plural bug

"The container spells its blocks model.layer.N. (singular), so model.layer.N.linear_attn.ssm_norm.weight pa[sses]…"

So thirty of the thirty-one spellings were layer vs layers, and the old fallback matched one of them. That is the whole defect in one word: a fallback written against the common spelling, silently missing a container that uses the other. It is also the same failure as the fptrunc ratchet (#2220) and the two-liner headers (#2228) — a reader that recognises one spelling of a thing that has several, which is why naming it matters more than fixing it.

And ssm 210 is linear_attn.ssm_norm on the same singular path, so both new roles sit behind the same spelling rule rather than being two separate special cases — which is what makes the 733 / 0 closure generalise to the next pack.

The 320 = 8 × 40 arithmetic still carries the proof

before:  733 names -> 413 named, 320 unassigned   (final_norm 31)
after:   733 names -> 733 named,   0 unassigned   (final_norm 1, ssm 210, 8 MoE roles x 40)

Unchanged by the new head, and still the strongest statement in the PR: the missing population is exactly the population the new rule fills.

One ask, carried from my previous review

Confirm the cherry-pick applies to #2097's head as it stands — the branch form is "you can merge or cherry-pick", and the only thing that can invalidate that is #2097 moving under it. One line saying which head it applies to closes the loop, and then this can land without another review round.

[github-actions]

Persistent review updated to latest commit e71804a

[bong-water-water-bong]

Split: the behaviour half is now #2277 on main; what remains here is reporting

#2270 (a main-based piece of this work) merged the same day while this PR has waited nine hours on #2099, so I took the split I offered in the review guide and did it in the direction that unblocks something:

#2277 (base main, no 3b symbol in it) carries the parts that classify and refuse — the final_norm layer.N guard, self_attn.gate_proj as an attention gate, the eight MoE needles, the gemma/extras recognition, and the npu-pack import refusal by name. Its arms are the two role arms from this PR, verbatim.

What that leaves here is the reporting half, which genuinely needs 3b's plan fields: roles, vision, payload_rows, role_shapes, write_plan, the acceptance gate, and the three role-integrity checks. Those are the parts that emit container_roles_* / container_role_counts / role_note, so they stay stacked.

Once #2277 lands, this PR's diff shrinks on rebase — the behaviour hunks become identical to main's and disappear from the delta. If you would rather review one PR than two, #2277 is the smaller and more consequential of the pair (it is the only one that changes what npu-pack import does), and this one can be re-scoped to reporting after it merges.

[bong-water-water-bong]

Verified against the new main: auto-merges clean, test passes, and the reporting is visible — not just set

The READY verdicts here were written when this PR was 3 files / +562. Since then #2277 landed the behaviour half on main (the classifier fixes and the container import refusal), so the unique diff is now 31 lines:

npU/pack_import.cpp  +13   npu/pack_import.h  +6   tests/npu_pack_import_test.cpp  +13/−1

Nobody had checked that residue against the merged main, so I did, on a local merge of main + this branch at 0cc98565:

Auto-merging npu/pack_import.cpp / .h / tests/npu_pack_import_test.cpp      (0 conflicts)
ninja npu_pack_import_test → 0 errors;  all npu-pack checks passed
$ ./1bit npu-pack info ~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2
  … no graph; geometry from config.json; **every tensor has a known role**. Reading it as a weights source needs …

That last line is the point of the PR and it is visible in the tool's output, which is what distinguishes this half from a field nothing reads. Two notes on getting there, because both bit me on this check and are the kind of thing that produces false review claims:

• I first measured info from a binary I had not rebuilt (I had only built npu_pack_import_test) and read the absence of the role note as "set and never read". ninja reports no outstanding work against the merged sources, and the rebuilt tree prints the line above. Rebuild the target you are measuring, not the one that was convenient.
• The non-empty branch (N of M tensors have no known role (first: …)) is not reachable on the five containers on this box any more — all of them classify fully — so that branch has no live instance; I verified it by hand earlier on a hand-built input and it is not re-verified by this run. Worth knowing before someone reads "no evidence of the else branch" as a gap.

Nothing else holds: the fields reach the plan (container_role_counts.count("embed") == 1), and the arm that pins them is in this diff.

[bong-water-water-bong]

The merge order, measured pairwise — and a second correction to my own attribution

After correcting the stack claim I re-ran the merges individually rather than in one pile, which is what I should have done first. As of #2099 9766d80, #2187 rebased 24ded5a6, #2291 6db001b:

main          + #2099            -> clean
main          + #2187 (rebased)  -> clean
main          + #2291            -> clean
main  + #2099 + #2187 (rebased)  -> clean
main  + #2099 + #2291            -> clean
main  + #2099 + #2187 + #2291    -> CONFLICT (npu/pack_import.cpp, .h, tests/npu_pack_import_test.cpp)

So the collision is between #2187 and #2291, not with the gate — and my previous comment said #2187 conflicted with #2099, which was wrong in the other direction. Both are the two reporting halves of F1 slice 3b/3c and they edit the same regions of the same three files; the gate is innocent in both cases.

Recommended order, now that it is measured:

1. npu-pack: plan() reports the pack's roles and names what nothing recognised (engine#1786, F1 slice 3b) #2099 — the gate, and everything merges on top of it.
2. npu-pack: classify the MoE families and the singular layer. spelling (engine#1788 acceptance) #2187 — it is the smaller reporting addition with two READY verdicts, and it lands cleanly on the gate.
3. npu-pack: the placement measured on the five containers, and the two roles it could not name (engine#1788, 3c) #2291 — rebase it onto the result. Its own content is orthogonal (placement), so the rebase is a matter of the overlapping declarations, and I will do that rebase myself the moment npu-pack: classify the MoE families and the singular layer. spelling (engine#1788 acceptance) #2187 lands rather than asking a reviewer to read a conflicted diff.

What I got wrong twice today, since it is the useful part: first I claimed the stack was verified when my run had silently omitted #2187 (wrong branch ref), then I claimed #2187 conflicted with #2099 when the conflict was with #2291 (conflict found in a tree that already had #2291 applied). Both were attribution errors from a compound merge, and the fix in both cases is the same: merge pairwise, name each ref, and read which two commits the conflict is between before writing a sentence about it.

[github-actions]

Persistent review updated to latest commit 24ded5a

[bong-water-water-bong]

VERDICT: READY at 24ded5a — with one documentation ask that is now the only thing between this and the merge note it deserves: the body has not caught up with the last three commits.

What the head claims, and where the evidence lives

The last commit's subject is "the last three families on gemma4/bolt, and every container on the box now classifies completely", and the numbers are in the commit bodies:

aa7ed0afa   gemma3-270m  36 -> 0 unassigned     gemma4-e2b  319 -> 74    bolt-e4b  382 -> 88
24ded5a64   gemma3-270m 237 / 0                 gemma4-e2b  709 / 0      bolt-e4b  849 / 0

PUT / TENSORS with zero unassigned on all three, after the previous commit still left 74 and 88. The second commit's reasoning is the part I would keep: "post_layernorm and per_layer_model_proj are gemma-local for a MEASURED reason: 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 — they are different tensors in the same model. gemma3-270m has no post_layernorm at all." A name that looks like a synonym is shown to be a different tensor by the pack's own counts — that is the distinction this whole stack is built on, applied to itself.

But the PR body still opens with the 733-tensor MiniCPM measurement and says nothing about gemma3/gemma4/bolt (grep gemma|bolt|319|382 over the body: no matches). A reviewer reading the description sees a PR about the MoE families; the code now also recognises gemma-local families and three container extras, and the evidence for that is buried in commit messages. For a 30-hour PR whose value is precisely its measurements, that is worth fixing before it lands.

Ask: move the two tables above into the body under the existing measurement section, and name the roles the new commits add (gemma_local, container_extra) with their one-line reasons. Nothing else needs to change.

What I have already verified across this PR's heads, still true at this one

• unassigned kept its meaning at every step: recognising a family the forward does not read is information about the forward, not a gap in the classifier, and the arms say so (the families this forward does not read are recognised, "a norm the forward reads is not swallowed by the recogniser").
• The controls that make the numbers credible: MiniCPM's count is unchanged where it should be (the fourth row in the previous commit's table), and the 35B's 733 -> 733 / 0 matches the container-role census from #2097, so two PRs' measurements agree.
• The plausible_k post-mortem survives in the header, next to the functions it explains — the rejected design and the measurement that rejected it, where the next person will try it again.

And the ask I have made at every head, which matters more now that the PR is finishing

Preserve the commit bodies through the merge. Five commits, each one a self-contained step whose why is a measured failure of a tempting stronger version: the geometry-declared widths that were wrong on both packs, the two-shape role that must not be mapped, "this model does not have it" versus "something is missing", and now the synonym that is a different tensor. A squash subject keeps none of that, and it is exactly what a future author needs in order not to re-derive it the hard way.

[bong-water-water-bong]

VERDICT at 24ded5a: CHANGES

Reviewed at 24ded5a64b733204df5bafe36acb3e4a61c08ae6, base npu/f1-container-roles-plan (#2099, still open). This is a mergeability verdict, not one on the work.

The planned order no longer holds. The 19:53Z comment measured #2099 → #2187 → #2291, with #2291 to be rebased last. But #2291 merged to main at 19:42:40Z, ten minutes earlier. Against current main, this branch now conflicts in all three of its files:

git merge-tree --write-tree origin/main 24ded5a6
CONFLICT (content): npu/pack_import.cpp, npu/pack_import.h, tests/npu_pack_import_test.cpp

When #2099 merges and this PR retargets to main, it will hit exactly that.

Some of this branch is already on main, through #2277 and #2291:

• the attention-gate rule (self_attn_gate_proj);
• the container-genre import refusal ("importing it needs the container-genre writer");
• the gemma/bolt recognisers;
• the placement work.

What is still unique to #2187 is the reporting half: container_roles_unassigned, role_shapes, payload_rows and write_plan, which main does not have.

Ask: rebase onto current main (after #2099 if it lands first), keep only the reporting fields and their arms, and drop the hunks main now carries. Then the diff is the reporting half alone and can be reviewed on its own. As the 19:53Z comment itself recommends, merge pairwise and name each ref before claiming the stack is clean.