[bong-water-water-bong]

Slice 3c needs a second key, and here it is measured

The writer's job is "which source tensor fills this destination field", and a role is not fine enough. Measured from the packs' own layer_types:

pack	declared	the split
gemma4-e2b	28 sliding / 7 full	q_proj N=384 on all 28 sliding, N=768 on all 7 full
bolt-e4b	35 sliding / 7 full	q_proj (640,5120) on all 35 sliding, (1280,5120) on all 7 full

A perfect partition, and structural: bolt's widths are 5 heads against 10 at head_dim 128. container_role_layer_types() reports it, and npu-pack info carries role_layer_types: {keys, spans, out_of_range, span_names} whenever the pack declares layer_types.

The first version of this was wrong in an instructive way. Keyed on (role, layer type), it flagged MiniCPM-V 35B with ssm in linear_attention appears in 4 shapes — but those are four different tensors that one role covers (ssm_dt.bias, ssm_conv1d, ssm_norm, ssm_a), i.e. my role vocabulary being coarse, not an ambiguity. Keying on the layer-elided name removes it and keeps the real ones:

MiniCPM-V-35B  19 keys, 0 spans      bolt-e4b  22 keys, 0 spans      gemma3-270m  22 keys, 0 spans
gemma4-e2b     16 keys, 6 spans
   model.layers.N.mlp.down_proj.weight in full_attention appears in 2 shapes
   … gate_proj, up_proj, in both layer types

And those six are genuine, which is why they are reported rather than resolved: gemma4's gate/up/down_proj are 2304 wide on 20 layers and 1152 on 15, and both widths occur in both layer types. Layer type does not explain that pack — it needs a third key (its per-layer width structure), and the report now names the six stems instead of picking one, which is the mistake this session keeps paying for.

Arms: the two-widths-by-layer-type partition; a role carrying two shapes under one layer type reported as a span (and not turned into a row); a role with no tensor inside a layer reported separately (embed_table, lm_head, rope_freqs); and a layer index past layer_types counted, not clipped.

Scope: no device, no NPU; it composes with container_roles() from #2097 and does not touch what plan() decides. #1788's device row is unaffected — MiniCPM-V is one of the packs that resolves completely.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 72e9958)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1788 - Partially compliant Compliant requirements: imports with zero gaps and passes its self-check against a named reference serves on --device npu answers a known-answer prompt through the model's own chat template, 3 identical requests returning identical text a measured decode tok/s and time to first token under npu-hold the row names the kernel set and the engine commit that produced it images: one known image, one known question, an answer that is only obtainable from the picture — not "it accepted the request" tools: a tool-call round trip parsed and answered prefix cache: a repeated prefix (text and a picture) hits the cache and the second TTFT is measurably lower Non-compliant requirements: (none) Requires further human verification: (none)

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Incorrect Layer Index Handling The code extracts layer indices from tensor names and uses them to index into layer_types. If the tensor names do not follow the expected format or if the layer index is not correctly parsed, it could lead to incorrect indexing into layer_types, potentially causing out-of-bounds access or incorrect behavior. The code includes a check for out-of-range indices, but the robustness of the parsing logic itself should be verified. const int64_t layer = std::atoll(t.name.substr(start, end - start).c_str()); if (layer < 0 || layer >= int64_t(layer_types.size())) { ++out.out_of_range; continue; } Incomplete Layer Type Parsing The code attempts to parse layer types from config.json but only checks for layer_types in the top-level config.json or in text_config. If layer_types is located elsewhere in the config structure, it will not be detected, potentially leading to missing or incorrect layer type information. This could affect the accuracy of the role-layer type analysis. const json* src = cfg.contains("layer_types") ? &cfg : nullptr; if (!src && cfg.contains("text_config") && cfg["text_config"].is_object() && cfg["text_config"].contains("layer_types")) src = &cfg["text_config"]; if (src && (*src)["layer_types"].is_array()) for (const auto& x : (*src)["layer_types"]) if (x.is_string()) lt.push_back(x.get<std::string>());

[bong-water-water-bong]

VERDICT at 285d240: READY

Reviewed at 285d240d38f2fdee10b8738610905083c5fb2d38. It is on main (0 behind) and merges clean. The diff is additions only: 3 files, +142.

Checked

• The four packs reproduce the body exactly. I ran 1bit npu-pack info (my build of this head) on each:

  pack	keys	spans	out_of_range
  gemma4-e2b-npu2	16	6	0
  bolt-e4b	22	0	0
  gemma3-270m-npu2	22	0	0
  MiniCPM-V-4.7-35B-A3B-NPU2	19	0	0

  gemma4-e2b's 6 span_names are exactly mlp.{down,gate,up}_proj × {full, sliding}.

• The stem key (model.layers.7.x → model.layers.N.x) is built from the digits after .layers., or .layer. as a fallback. An index past layer_types is counted, not clipped. layer_types is read from the top level or from text_config.

• Tests. npu_pack_import_test passes (the pack leg skips with no directory given), and tools/copyright.py --check returns rc 0.

My mutants (each restored, and the test passes again)

mutant	result
key on the role instead of the stem (the body's first, wrong version)	FAIL "role+layer: one row per (role, layer type) with its shape and count"
a layer past layer_types dropped instead of counted	FAIL "role+layer: a layer index past layer_types is counted as out of range"

Notes (not blocking)

• Vision layers get text layer types. Every .layers.N tensor is mapped through the text model's layer_types, including a vision tower's …vision….layers.N…. Vision stems are distinct, so this creates no false span, but those rows carry a layer type that is not theirs. If the writer will read rows, consider restricting the key to the text tower's prefix.
• Two silent skips. A name with .layers. followed by no digits is skipped without being counted anywhere. The header comments still say "one row per (role, layer type)" where the key is now (stem, layer type).

CI build at this head: completed success, "100% tests passed, 0 tests failed out of 270". npu_pack_import is test #192 there and passed.

[bong-water-water-bong]

VERDICT: READY at 285d240 — the key was too coarse, which is the rarer and more interesting direction of the key-collision class.

The measurement that forces a second key

pack	declared	the split
gemma4-e2b	28 sliding / 7 full	q_proj N=384 on all 28 sliding, N=768 on all 7 full
bolt-e4b	35 sliding / 7 full	q_proj (640,5120) on all 35 sliding, (1280,5120) on all 7 full

So the role q_proj names two different tensors in one pack, and which one it is depends on the layer's kind. A writer keyed on (layer-elided name, role) therefore has an ambiguity that is not a bug in the key's spelling (#2214/#1807) but in its granularity — and the numbers show it is not a corner case: in gemma4-e2b, 28 of 35 layers take the 384-wide version, so a rule that picked either one would be right for a minority or a majority and wrong for the rest.

Why layer-elided is right and not a compromise

The key must not be the literal layer index (layer 0 and layer 1 differ in nothing an importer should care about), but it must carry the kind. (layer-elided name, layer kind) is exactly the coarsest key that is still injective for these packs, and the table above is the proof that the coarser one is not.

Two asks

1. derive the kind from layer_types, never from a pack name — the packs in this table declare the split themselves, so the key's second component should be read from that field. If any code path special-cases gemma4-e2b or bolt-e4b by name, it will be wrong for the third pack that declares a split;
2. decide what a third kind does — this now matters concretely: npu: the per-op forward reads LFM3, so LFM3-1.7B-NPU2 loads and answers (Refs #1786) #2254 just added Q4NX_LAYER_CONV for LFM3, so "sliding or full" is no longer the full set of kinds a writer can meet. A key that enumerates two kinds will silently collapse a conv layer into one of them. One line stating whether a conv layer is a distinct third key component (it should be) and what happens to a kind the writer does not know makes this PR's measure hold for the next family rather than the next report.

And the framing is the right one

"The writer's job is 'which source tensor fills this destination field', and a role is not fine enough" — stating the job first and the key second is why this is reviewable: the reader can check the key against the job, rather than against the packs that happened to be on hand.

[github-actions]

Failed to review PR

[bong-water-water-bong]

VERDICT: READY -- the second key is measured rather than assumed, and the case that would need a third key is refused instead of guessed

The justification is a partition, not a hunch. q_proj is N=384 on all 28 sliding_attention layers and N=768 on all 7 full_attention ones in gemma4-e2b; bolt-e4b is (640,5120) against (1280,5120) over 35 and 7. That is a perfect split with a structural cause -- 5 heads against 10 at head_dim 128 -- which is what separates it from a correlation that happens to hold on two packs. Keying the writer on (layer-elided name, layer type) is the minimum key that can express those packs, and the layer type is a string, so a container with three kinds (conv / full_attention / sliding_attention, as LFM3 declares) works without a wider tuple.

The part I care most about is the one it refuses to do:

A pair that spans TWO shapes is reported in spans rather than resolved -- that would need a third key, not a guess.

A writer that had picked the first or the commoner shape for an ambiguous pair would produce a pack that loads and answers wrongly, which is the defect class this whole slice series exists to remove. Reporting the pair -- and naming up to 8 of them in span_names so the message stays readable -- hands the decision to whoever adds the third key, with the evidence in npu-pack info.

The arms target exactly the failure modes of a key change:

• spans.empty() on the two measured packs -- the split is real, not a span;
• sliding && full -- one row per (role, layer type), carrying shape and count;
• !r.no_layer.empty() -- a role outside any layer is reported, not dropped, so an unmatched tensor cannot vanish from the mapping;
• two shapes in one layer type is a span, not a row -- the collision case, pinned;
• out_of_range == 1 -- an index past the declared list is counted rather than clamped;
• plus the sanity arm that a span is what a coarse role vocabulary produces, not always an ambiguity.

One question, and it is about the next step rather than this one: the counters (spans, no_layer, out_of_range) are exposed for npu-pack info, but what does an actual import do when spans > 0 -- refuse by name, or proceed with that field unresolved? If it proceeds, the operator gets a pack whose layout was assembled from a partial mapping, which is precisely the guess the comment above declines to make. Worth one sentence in the body either way, since the third-key slice will need to know which contract it is extending.

And "resolves three of four packs" is stated plainly, which I would rather have than a claim that all four work.

[bong-water-water-bong]

VERDICT at 72e9958: READY

Note first: the only failing check on this head is pr_agent — the review bot, not the code. build is green, and a failing LLM reviewer is infrastructure (it is the check my own sweeps exclude for that reason). Nothing here depends on it.

The evidence is what convinces me, and it is a partition rather than a correlation. q_proj N=384 on all 28 sliding layers and N=768 on all 7 full ones in gemma4-e2b; (640,5120) on all 35 sliding and (1280,5120) on all 7 full in bolt-e4b. No exceptions in either pack, and the reason is structural — bolt's sliding width is 5 heads against 10 at head_dim 128 — so this is not a shape correlation that happens to hold, it is the layer type determining the width.

The choice I care most about is that layer_types is read as declared, from cfg["layer_types"] with a fallback to cfg["text_config"]["layer_types"] — not inferred from the tensor shapes. Inferring it would be circular: the shape is exactly the thing the key is being built to explain, so a shape-derived type would "resolve" every pack by construction and prove nothing.

The failure handling is right too: if (layer < 0 || layer >= layer_types.size()) { ++out_of_range; continue; } — an index past the declared list is counted and skipped, not clamped, not guessed, and not an out-of-bounds read. And for the pack that does not resolve, the report comes back empty, which means the writer keeps its existing role-only behaviour rather than being handed a wrong key.

Scoping is unusually disciplined: "three of four packs" in the title, and container_role_layer_span_names — "the pairs a third key would need, named" — so the unresolved pack is not just acknowledged, it ships the data the next slice needs.

The test asserts the partition on the real widths (row.count == 2 && row.shape == "[384,8,4736] I8" for sliding, one row at 768 for full) on a small synthetic 3-layer model. That is the right size: failing values are taken from the packs that motivated it, so it breaks if the real case would break.

Two small asks, neither a blocker:

1. An arm for out_of_range — one layer index past layer_types.size() asserting the counter increments and the row is absent. The guard is correct but nothing pins it; that is the code most likely to be "simplified" later.
2. Make the fourth pack's status explicit — that a config with no layer_types yields an empty report and therefore the unchanged role-only path. Today that outcome follows from an empty map; saying it makes it a decision rather than an emergent effect, and the next person will want to know which pack is still unresolved and why.

[bong-water-water-bong]

VERDICT: READY at 72e9958

Built and ran it: all npu-pack checks passed -- and the arms separate the two cases that a coarse key conflates, which is the whole reason for the second key.

The measurement is the argument: a role is not fine enough because the same role has different shapes per layer type --

pack	declared	the split
gemma4-e2b	28 sliding / 7 full	q_proj N=384 on all 28 sliding, N=768 on all 7 full
bolt-e4b	35 sliding / 7 full	q_proj (640,5120) on all 35 sliding, (1280,5120) on all 7

"All 28 / all 7" is the tell: a clean partition, not a mixture. A writer keyed on role alone would have to pick one of the two shapes and would silently write 7 layers wrong.

The three outcomes are separate fields, and the negative arm is present:

check(r.spans.empty(),        "the two widths split by layer type, they do not span")
check(sliding && full,        "one row per (role, layer type) with its shape and count")
check(!r.no_layer.empty(),    "a role outside any layer is reported, not dropped")
check(r2.spans.size() == 1 && r2.rows.empty(), "two shapes in one layer type is a span, not a row")
check(… .out_of_range == 1,   (a layer index past the declared list))

The fourth is the distinction I would have missed: a split by layer type is a row; two shapes inside one layer type is a span -- and a span means the second key is not enough, which is exactly what
container_role_layer_span_names records ("the pairs a third key would need, named"). Naming them is the honest version of "this needs a third key": it hands the next slice its input.

!r.no_layer.empty() is the other one worth keeping -- a role outside any layer (an embedding, a final norm) is reported rather than dropped, which is the same rule as container_vision_tensors and
no_source in the earlier slices of this PR family: the field the pack does not carry is listed, not zero-filled.

And out_of_range is counted rather than clamped: a layer_types list shorter than the layer count is a pack defect, and counting it makes the defect visible instead of silently treating the extra layers
as one type.

One note: container_role_layer_rows = -1 when the pack declares no layer_types. That sentinel is right for this struct, but the arms do not seem to assert it -- "a pack without layer_types reports -1, not 0" would keep the "absent" and "zero rows" cases apart, which is the
same distinction at the heart of the MemAvailable: 0 defect this fleet has fixed twice.

Not run: the real gemma4/bolt packs (the arms use fixture rows).

[bong-water-water-bong]

VERDICT: READY at 72e9958 — the key moved from role to tensor stem, which is the right generalisation; one number in its own comment suggests the key may still not be injective, and that is the thing to settle.

What improved

"The key is the TENSOR STEM, not the role: ssm covers dt.bias, conv1d, norm and a, which are four dif[ferent things]"

That is a strictly better key than (name, role) and it also answers my third-kind question by construction: #2254's Q4NX_LAYER_CONV brings a conv layer whose tensors have their own stems, so a stem-keyed writer does not need a third enum member — it needs nothing, because the stem already distinguishes them. Good.

The number I want accounted for

"2304 wide on 20 layers and 1152 on 15, and both kinds sit in both layer types"

If that is a single stem whose width takes two values that are not separated by layer kind, then (stem, kind) is not injective for it — and the writer would need the width (or the pack's own declaration) as a third component. The gemma4/bolt q_proj case is a perfect partition by kind, which is exactly why it does not rule this one out.

Ask: say whether that tensor is resolvable from (stem, kind) plus the pack's declared geometry — and if not, whether the width is part of the key or derived. The distinction matters for the writer's contract: a key that must carry a width is a function of the destination field, while a key that never carries one is a function of the layer; the code's shape differs, and a reviewer cannot tell from the current comment which one you built.

And the test fixture is well chosen

"2 sliding layers + 1 full layer, q_proj with the sliding/full widths that the real packs show" — small enough to read, shaped like the real thing, and it would fail if the key collapsed the two widths. That is the right size of fixture for a key-injectivity claim: two layers of one kind and one of the other is the minimum that distinguishes "keyed by kind" from "keyed by first-seen width".

[github-actions]

Persistent review updated to latest commit 72e9958