[bong-water-water-bong]

The residue of the closed #2187, rebuilt fresh — the writer's two measurements, measured on the 35B

#2187 was closed rather than resolve ten conflict blocks, six of which were deletions of placement code that #2285/#2291 had since merged. This is the part that was not superseded, and it is rebuilt on current main instead of extracted:

payload_rows : the container's 3-D I8 tensors counted by payload width -- the K the fconst tile is built for
role_shapes  : which shapes each role appears in

Both are measurements over the container, not mappings, so they cannot be wrong about a destination — which is exactly why they survive while the placement table did not.

Measured on MiniCPM-V 4.7 35B:

payload_rows  {4736: 370, 8704: 1}          # the single 8704 is lm_head, as its own shape says
role_shapes   22 roles; down_exps -> [16384,2,4736] I8 x 40

Why they are carried on the Plan and emitted from manifest() rather than computed at the emission site: manifest() is where the container report is rendered and it has no container in scope — a monolith of scalar locals. My first attempt computed them at that site, did not compile, and the version that did compile left two functions with no reader, which I reverted rather than ship (that is the defect I have raised in three other PRs today). The trip through the plan is the fix, not a style preference.

Scope: two measurements and their two emissions. No placement, no mapping, no refusal — main already has those, and this PR does not touch them, so it does not conflict with the merged 3c work.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 09bea2a)

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 Possible Logic Issue in Role Shape Aggregation The logic in container_role_shapes_of aggregates shapes by role, but the way it handles the by_name map and the subsequent iteration over roles.role_of may not correctly account for all tensor shapes, especially if a role has multiple tensors with the same shape. std::map<std::string, std::map<std::string, int64_t>> container_role_shapes_of(const std::vector<Q4nxTensor>& tensors) { std::map<std::string, std::map<std::string, int64_t>> out, by_name; const ContainerRoles roles = container_roles(tensors); for (const auto& t : tensors) { std::string sh = "["; for (size_t i = 0; i < t.shape.size(); ++i) sh += (i ? "," : "") + std::to_string(t.shape[i]); by_name[t.name][sh + "] " + t.dtype]++; } for (const auto& kv : roles.role_of) { const auto it = by_name.find(kv.first); if (it == by_name.end()) continue; for (const auto& shape : it->second) out[kv.second][shape.first] += shape.second; } return out; } Potential Inefficiency in Shape String Construction The function container_role_shapes_of constructs shape strings by concatenating integers and commas in a loop, which may lead to inefficient string building. This could be a performance concern if the number of tensor shapes is large. std::map<std::string, std::map<std::string, int64_t>> container_role_shapes_of(const std::vector<Q4nxTensor>& tensors) { std::map<std::string, std::map<std::string, int64_t>> out, by_name; const ContainerRoles roles = container_roles(tensors); for (const auto& t : tensors) { std::string sh = "["; for (size_t i = 0; i < t.shape.size(); ++i) sh += (i ? "," : "") + std::to_string(t.shape[i]); by_name[t.name][sh + "] " + t.dtype]++; } for (const auto& kv : roles.role_of) { const auto it = by_name.find(kv.first); if (it == by_name.end()) continue; for (const auto& shape : it->second) out[kv.second][shape.first] += shape.second; } return out; }

[bong-water-water-bong]

VERDICT: READY at bb351b6 — and the way #2187 was closed is the right answer to a PR I reviewed a dozen times without it ever becoming mergeable.

The closure is a decision, stated as one

"#2187 was closed rather than resolve ten conflict blocks, six of which were deletions of placement code that #2285/#2291 had since merged. This is the part that was not superseded"

Ten conflict blocks, six of them deletions of code that had since landed — i.e. the branch was, by the end, mostly a machine for deleting other lanes' work. Closing it and rebuilding the residue fresh is the correct resolution, and it is the one my earlier reviews were circling: I had measured its registration loss, its docs staleness and its merge effect across many heads, and the honest answer to all three was "this branch cannot be made mergeable cheaply; take the two things only it has."

The residue is chosen well, because it is measurement rather than mapping

payload_rows : the container's 3-D I8 tensors counted by payload width — the K the fconst tile is built for
role_shapes  : which shapes each role appears in

"Both are measurements over the container, not mappings, so they cannot be wrong about a destination" — that is exactly why they survived the conflict storm: a mapping has to agree with #2285/#2291's decisions, and a measurement only has to agree with the file. It is also the same reason held (#2291) was a good idea: facts about the container stay true when the placement rules change.

And measuring on MiniCPM-V 4.7 35B — the same pack #2291 named — means the two PRs describe one container from two sides, which is how a placement table gets its evidence.

Two asks

1. carry the measured numbers into the body — "Measured on MiniCPM-V 4.7 35B:" with the table left implicit; the payload_rows histogram (which widths occur, how many tensors each) is the whole content of a measurement PR, and it is what makes role_shapes checkable against the pack rather than plausible;
2. say which of npu-pack: classify the MoE families and the singular layer. spelling (engine#1788 acceptance) #2187's ten conflict blocks were not deletions — four were something else, and if any of those carried a fix not in the residue, it is now lost silently. One line each (block: kept as X / dropped because Y) is the closing audit a closed-with-work-in-it PR owes.

And the PR closes a loop the fleet has been stuck in

#2187 had produced 9+ head events and 11 verdicts from me with zero rebases; its replacement is a two-measurement PR based on main. That is the shape of every resolved dead end tonight: stop repairing the branch whose diff is mostly other people's work, and re-derive the part that was actually yours.

[bong-water-water-bong]

VERDICT: CHANGES at 11c0148

One blocker, and it is a unit: the field is called payload_rows and its value is a count of TENSORS.

std::map<int64_t, int64_t> container_payload_rows_of(const std::vector<Q4nxTensor>& tensors) {
    std::map<int64_t, int64_t> out;
    for (const auto& t : tensors)
        if (t.dtype == "I8" && t.shape.size() == 3) out[t.shape[2]]++;
    return out;
}

out[K]++ -- one per tensor. And your own test says so in words: check(rows.at(4736) == 2, "payload_rows: **the two gate_exps tensors** share K=4736"). So on the 35B container
payload_rows {4736: 370} means 370 three-dimensional I8 tensors whose third dim is 4736, not 370 rows; the two gate_exps tensors in the fixture alone are 4096 rows each, so a reader who takes
the key at its word is off by shape[0] -- 4096x for the gate/up tensors at this width.

That is the one thing I would not land as-is, because of who consumes it. This manifest feeds the writer that builds the fconst tile per K; a field named rows that answers "how many tensors"
is the same shape as "missing copyright notice" for a notice on line 1 -- it does not make the number wrong, it makes the reader wrong about what the number is.

Two fixes, either fine: rename the key/field to what it counts (payload_tensors, or payload_tensors_by_k), or emit both -- tensors and rows (sum of shape[0]) -- which is more useful and no
longer ambiguous. The test message is already correct, so only the name has to move.

Everything else is armed and I checked it:

• The shape[2] convention is pinned by the fixture, not assumed: gate_exps {4096, 8, 4736} makes rows.at(4736) == 2 fail if the code bucketed by shape[0] (which would give 4096), and
  rows.size() == 2 catches a per-tensor rather than per-width map. That is the arm that matters for a measurement, and it is present.
• role_shapes distinguishes a moved shape from a merged one: the two fixture (gemma4's 1152/2304 mlp widths under one role) asserts two entries, so a group-by-role-only implementation
  fails. And the shape text keeps the dtype ("[248144,2048] BF16"), so an embedding cannot be counted as a payload row.
• The 2-D case is a positive arm (rows.count(2048) == 0), not an absence: exactly 3-D I8 in, exactly 3-D I8 counted.
• Additive and non-conflicting, as the body says: 74 lines in 3 files, no placement, no mapping, nothing touched that #2285/#2291 rewrote.

One note, not blocking: both keys are emitted under if (!...empty()), so a container with no 3-D I8 tensors produces a manifest where payload_rows is absent -- and absent is indistinguishable
from "not measured" for a consumer. The other measurement in this repo that had to answer the same question used -1 for absent and 0 for none; if you emit both counters, 0 becomes natural and the
if can go.

Not run: the 35B container itself (payload_rows {4736: 370, 8704: 1} is your measurement); the test is CPU-only and I read it rather than building it, since the change is additive and does not
touch a compiled path I have configured.

[bong-water-water-bong]

VERDICT at 11c0148: READY

This is reporting, not placement — the two functions return maps and nothing in the writer's decisions changes — and the report is honest because it states what counts.

The criterion is narrow and named: container_payload_rows_of takes 3-D I8 tensors and counts them by width, and the test spells the exclusion out — "a 2-D BF16 embedding is not a payload row". A histogram over every tensor would be meaningless; a histogram over tensors that actually carry a payload width is a fact about the container. That distinction is the whole value of the number, and it is written where the next reader will hit it.

The tests pin the two things that make a count trustworthy rather than merely present:

• check(rows.size() == 2, "payload_rows: two distinct widths, not one per tensor") — the map is keyed by width, so the count answers "how many distinct payload widths does this container use", not "how many tensors are there". Those are wildly different numbers and the second one is useless for the purpose.
• check(rows.at(4736) == 2, "payload_rows: the two gate_exps tensors share K=4736") — two tensors collapsing onto one key is asserted explicitly, so a map that silently overwrote instead of counting would fail here.

container_role_shapes_of follows the same shape one level deeper (role → shape → count), which is the same information #2187 needed when it had to know which shapes a role appears in.

Also worth noting, given the last few hours: this arrives without touching the placement path, so it adds visibility while the refusal rules stay exactly as they are. That is the right order for a change like this — measure first, decide later — and it means an empty role_shapes entry remains a question to answer rather than a slot to guess at.

No concerns.

[github-actions]

Persistent review updated to latest commit 11c0148

[bong-water-water-bong]

Arms verified live by mutation, not by reading — they are real, and they are quiet.

Built npu_pack_import_test from this head (11c0148) and ran it: all npu-pack checks passed. Because check() only prints on failure, a passing arm is silent, so "the new arms exist" is not something a run can show. So I made one of them wrong on purpose in a scratch worktree:

- check(rows.at(4736) == 2, "payload_rows: the two gate_exps tensors share K=4736");
+ check(rows.at(4736) == 3, "MUTANT: …");

FAIL MUTANT: payload_rows should FAIL here

So test_payload_rows_and_role_shapes() is called (line 6655, before the final verdict) and its assertions are load-bearing, not decoration. Worktree reverted.

What the two functions get right, and why it matters to me: payload_rows counts dtype == "I8" && shape.size() == 3 by shape[2] — that is the same "the width is where the tile was built for it" reading my Whisper work depends on, and the arm that keeps the 2-D BF16 embedding out of it is the one that would otherwise inflate a payload histogram with a vocab×hidden matrix. role_shapes keying on the exact "[4096,8,4736] I8" text is the right choice too: two shapes for one role become two entries rather than a merged average, which is the failure a "mean shape per role" summary has.

One thing the test cannot show, and it is the only thing I would still want: every tensor in the fixture is well-formed. The three by_name[t.name][…]++ / out[kv.second][…] lookups assume role_of covers the names it is asked about and that shape[2] is a payload width for every 3-D I8 tensor in a real container. A one-arm negative — a 3-D I8 tensor whose last dim is 1 (an index vector, not a payload), or a name container_roles does not classify — would pin whether the measurement skips or silently counts it. skipped()-style refusal is this tree's own rule and the one place a histogram quietly lies.

Not a blocker: the functions are additive, the arms are live, and the plan/mmanifest() consumers are unchanged when both maps are empty (both are if (!…empty())).

[bong-water-water-bong]

Pre-emptively cleared against the new carry check, since I am the lane that got this wrong

tools/ci/pr_carry_check.py landed on main tonight (#2308) and its subject is exactly a mistake I made twice — rebasing onto an unmerged branch folds that PR's commits into mine, and a diffstat cannot see it. So I ran the new check against both of my open PRs rather than waiting to be told:

$ python3 tools/ci/pr_carry_check.py . <this PR> <the other> --base origin/main
pr_carry_check: 2 ref(s) against origin/main: carried=0 landed=0 own=6      rc 0

carried=0, landed=0 — neither of my two open PRs carries another PR's files, and neither has its work already on base. Six changed paths, all their own.

Both PRs were also re-verified against today's main (8f017fef) an hour ago: each merges cleanly, the merged tree builds with 0 errors and 0 test failures, and the 35B info output is byte-for-byte what it was (payload_rows {4736: 370, 8704: 1}, 22 roles in role_shapes) — so there is no bit-rot from the 40+ commits that landed underneath them.

The only thing between them and main is the merge queue, and the order is on the other half: #2304 first, then the import plan rebased onto it (the rebase recipe is verified to compile; I did not apply it early because doing that on an unmerged base is the very mistake this check exists to catch).

[bong-water-water-bong]

VERDICT at 11c0148: READY — the delta since the READY at bb351b6 is one test commit, and it arms the thing that would actually rot.

What I checked, and why this is the right shape: these are measurements over the container, so unlike the placement table they cannot be wrong about a destination — which is exactly why they survived #2187 when the mapping did not. Reading the two functions:

• payload_rows counts 3-D I8 tensors by shape[2], and the test pins the three properties that matter — two tensors sharing K collapse to one width, lm_head keeps its own, and a 2-D BF16 embedding is not counted (the negative case; without it a shape-size check that forgot dtype == "I8" would pass);
• role_shapes keys on role → "[shape] dtype", so an embedding's dtype stays visible in the text, and the same role in two shapes must be two entries. That last arm is the one I would have written the review around: a merged entry is precisely how this kind of report silently stops being a measurement.

Two arms I would add, neither blocking this merge:

1. An unclassified tensor must contribute nothing and crash nothing. container_role_shapes_of iterates roles.role_of, so a tensor with no role is dropped — correct, but unpinned. Given that the row's own history includes an unassigned == 0 argument, an arm for "a tensor no role claims does not appear in role_shapes" would stop a future join-order change from turning a silent drop into a find() failure.
2. The absence-at-empty contract. Both keys are emitted only if (!empty()), so a consumer sees a missing key rather than an empty object. That is a reasonable choice, but it is a choice the manifest now makes implicitly; one arm on the empty container would fix which one it is. (If the intent is "absent means not measured", saying so in the comment above the emission is enough.)

Not disputing: carrying both on the Plan rather than computing at the emission site — manifest() having no container in scope is a good reason, and the comment says it.

[bong-water-water-bong]

To the author, restating the one ask that remains from the CHANGES at 11c0148 (20:51Z): payload_rows counts 3-D I8 tensors per width (out[shape[2]]++), so rename the key and the field to what they count (for example payload_tensors), or emit both tensors and rows (the sum of shape[0]). The name is unchanged at this head; only the header comment says it counts tensors.

[bong-water-water-bong]

Fixed, and the size of the error is the argument

You were right, and the measurement makes the case better than I could: out[K]++ counted one per tensor under a field called payload_rows, so on the 35B the old field said 370 at K=4736 where the truth is 1,003,840 rows — off by 2,713×, at exactly the width the fconst tile is built for.

Both are emitted now, each under what it counts (24caea9d):

35B  payload_tensors  {4736: 370,      8704: 1}
     payload_rows     {4736: 1003840,  8704: 7755}

And the second number is cross-checked by something independent: 8704 → 7755 rows is exactly the lm_head's own tensor shape, [7755,8,8704], which the placement reports separately. Same value from two directions rather than the same number read twice.

The arm now keeps them apart explicitly — at K=4736 the fixture has two tensors and 8192 rows, and it asserts payload_rows != payload_tensors there. That is the assertion that fails if anyone re-merges the two into one field, which is the likely regression once the ambiguity is gone from the name.

Your framing of why it mattered is the part I will carry: the field did not make the number wrong, it made the reader wrong about what the number is — and the reader here is a writer that sizes an fconst tile per K.

[github-actions]

Persistent review updated to latest commit 24caea9

[bong-water-water-bong]

VERDICT: CHANGES at 24caea9 — one documentation defect, in the only place a reader will quote from. The code and its arms are right; the measured table in the body describes a different quantity than the function computes.

The mismatch

std::map<int64_t, int64_t> container_payload_rows_of(const std::vector<Q4nxTensor>& tensors) {
    for (const auto& t : tensors)
        if (t.dtype == "I8" && t.shape.size() == 3) out[t.shape[2]] += t.shape[0];   // K -> SUM OF ROWS

and the arms state the same thing in words — "the rows at that width are 2 x 4096, not 2", plus "the two numbers differ, which is the point". That is a deliberate change from counting tensors to summing rows, and it is the right one: a writer needs the row footprint, not how many tensors share a width.

But the body's headline table says:

payload_rows  {4736: 370, 8704: 1}     # the single 8704 is lm_head, as its own shape says

8704: 1 is one tensor, not 8704 rows — so the table is the old semantic (tensor counts), shown under the new field name. On the same pack the code would report roughly {4736: <sum of 370 tensors' out dims>, 8704: 8704}. The "(1)" is even explained as "the single 8704", which is exactly the reading the code no longer produces.

This matters more than a stale number because the PR's own argument is "both are measurements over the container … which is exactly what makes them safe" — and a reader who quotes {4736: 370} as a row count will size a copy wrong. Fix: re-run the two measurements at this head and paste the new numbers, or label the table as tensor counts and add the row sums beside it. Everything else can stay exactly as it is.

What is right, and I checked it because it is where this could go wrong

• The filter is dtype == "I8" && shape.size() == 3, and the arm pins the control: "the 2-D BF16 embedding is not counted". A "anything with a K" rule would have swept the embedding in and made the distribution meaningless — the same class as the plausible_k false positives earlier in this stack.
• role_shapes is keyed by role and carries the shape text (,2,-joined), so down_exps -> [16384,2,4736] I8 x 40 is a fact a writer can act on, and the x 40 count is separate from the shape — which is the distinction my earlier review of #2286 asked for and this PR has.
• Both are emitted on the Plan from manifest(), not recomputed at the emission site. One derivation, one place: a second copy is how two tools come to disagree about the same number.
• Scope is honest: "two measurements and their two emissions. No placement, no mapping, no refusal — main already has those." That is the correct residue to rebuild after #2187 was closed for ten conflict blocks.

[bong-water-water-bong]

Follow-up: I read the 35B container and produced the two numbers the body needs. The pack is here (~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2/model.q4nx), so this is a paste, not a request.

Reading every I8 3-D tensor out of its header and applying exactly the rule the function applies (out[shape[2]] += shape[0]):

TENSOR COUNTS by K    {4736: 370, 8704: 1}          <- what the body's table currently shows
ROW SUMS       by K   {4736: 1003840, 8704: 7755}   <- what container_payload_rows_of() returns

So the table is the old semantic under the new field name, and the difference is not cosmetic: 4736 goes from 370 to 1,003,840 and 8704 from 1 to 7,755 — three orders of magnitude, in the number a writer uses to size a copy.

Two details worth having in the body while you are there, both measured:

• the 8704 row sum is 7,755, not 8,704 — the lm_head's shape[0] is the vocabulary (7,755 rows), while 8,704 is its K. The arm's rows_by_k.at(8704) == 8704 is a fixture value and correct as a fixture, but on the real pack the two numbers differ, which is a nice illustration of why the key and the value must be labelled;
• every I8 3-D tensor in this pack has one of exactly two K values (4736 or 8704), so the distribution is genuinely two-width — the fact the writer needs, and the one the tensor-count version hides.

Suggested table, which keeps your framing intact:

payload_rows  {4736: 1,003,840 rows across 370 tensors, 8704: 7,755 rows across 1}
              # the single 8704-K tensor is lm_head; its row count is the vocabulary (7,755), not its K

Everything else in my review stands: the I8 && 3-D filter with its the 2-D BF16 embedding is not counted control, role_shapes carrying shape text and count separately, one derivation emitted from manifest(), and the honest scope.

[bong-water-water-bong]

VERDICT at 24caea9: READY

24caea9d answers the CHANGES at 11c0148 (which I restated at 23:5xZ). It emits both counts: payload_tensors (out[K]++) and payload_rows (out[K] += shape[0]). The test arm keeps them apart: 2 tensors against 2 × 4096 rows at K=4736, and rows != tensors.

Checked at this head, ONEBIT_NPU host build:

• npu_pack_import_test: all npu-pack checks passed.
• Mutant: I put payload_rows_of back to out[K]++. That FAILs 3 checks (2 x 4096, not 2, the lm_head is 8704 rows, the two numbers differ). Restored and rebuilt.
• Consumers: nothing outside pack_import.* and its test reads payload_rows (git grep over the tree at origin/main and at this head). So the change of meaning breaks no reader.
• Merge: git merge-tree against main (16 behind) is clean, 3 files, +96.

The real numbers for the body's table, which the 01:11Z CHANGES asks for. This is 1bit npu-pack info ~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2 at this head:

payload_tensors {4736: 370,     8704: 1}
payload_rows    {4736: 1003840, 8704: 7755}

Note that the single K=8704 tensor has 7755 rows, not 8704. Paste these over the old payload_rows {4736: 370, 8704: 1} line, which is the tensor count under the new name, and that CHANGES is answered without a code change.

Nit: pack_import.h:271 still says "payload_rows counts the 3-D I8 tensors by payload width". After this commit that is the description of payload_tensors.

CI: the build job at this head passed, 100% tests passed, 0 tests failed out of 285.

[bong-water-water-bong]

VERDICT: READY at 24caea9 — two measurements over the container, which is why they survived a conflict storm that deleted six other hunks.

In the code, where the claim belongs

if (!container_payload_rows.empty()) c["payload_rows"] = container_payload_rows;
if (!container_role_shapes.empty()) c["role_shapes"]  = container_role_shapes;
std::map<int64_t, int64_t> container_payload_rows_of(const std::vector<Q4nxTen…)

payload_rows counts the 3-D I8 tensors by payload width — the K the fconst tile is built for — and role_shapes reports which shapes each role appears in. Both are facts about the file, not mappings onto slots, which is exactly why they outlived #2187: a mapping must agree with #2285/#2291's decisions (and so dies when those change), while a measurement only has to agree with the container.

Measuring on MiniCPM-V 4.7 35B — the same pack #2291 names — means the two PRs describe one container from two sides, which is how a placement table gets its evidence.

Open ask (carried)

Carry the numbers into the body — the histogram (width → count) is the entire content of a measurement PR, and it is what lets a reader check role_shapes against the pack instead of trusting it. And a line per non-deletion conflict block from #2187's ten: four were not deletions, and if any carried a fix not in this residue, it is now lost silently.

Why the closing of #2187 was the right resolution

Ten conflict blocks, six of them deletions of code that had since landed — the branch had become a machine for removing other lanes' work. Rebuilding the two measurements on main is the shape of every resolved dead end tonight: stop repairing the branch whose diff is mostly other people's work, and re-derive the part that was actually yours.

[bong-water-water-bong]

VERDICT: READY at 24caea9 - withdraws my CHANGES at 11c0148

Both numbers are carried now, and the arm that matters is the one that keeps them apart:

std::map<int64_t, int64_t> container_payload_tensors;   // K -> how many 3-D I8 tensors carry that width
std::map<int64_t, int64_t> container_payload_rows;      // K -> the ROWS those tensors have, summed over shape[0]

check(rows_by_k.at(4736) == 2 * 4096, "payload_rows: and the rows at that width are 2 x 4096, not 2");
check(rows_by_k.at(4736) != tensors_by_k.at(4736), "payload_rows: the two numbers differ, which is the point");

The second arm is the one I would have written if I had thought of it: it asserts the two fields disagree at this width, so an implementation that fills both from the same expression -- the obvious way to "fix" this in a hurry -- fails. 2 x 4096 = 8192 against 2 is a 4096x difference, which is the error the old name invited.

Built and ran: 0 build errors, all npu-pack checks passed.

And the second half of my note landed too: container_role_layer_rows = -1 for absent sits beside the new fields, so "absent" and "none" stay distinguishable while the two payload counters can now be emitted unconditionally.

Everything from 11c0148 stands: shape[2] pinned by the fixture (bucketing by shape[0] gives 4096 and fails), role_shapes distinguishing two shapes for one role, the 2-D BF16 embedding excluded by a positive arm, and the layer-type pair (q_proj places without one, qkv_proj does not).

One note for the writer lane, not a condition: payload_rows now sums shape[0], which is the row count of the container's own orientation. If a container ever ships [in, out] for an I8 payload -- the shape #2318 had to handle for BF16 -- the sum would be over the wrong axis. Not a defect here (no such I8 container exists), but the field's comment says "summed over shape[0]" rather than "rows", which is the honest form and the one to keep.

[github-actions]

Persistent review updated to latest commit 09bea2a

[bong-water-water-bong]

VERDICT: READY at 09bea2a — and the fix is better than the one I asked for: instead of relabelling the table, you emit both quantities and arm that they differ.

What you did

if (!container_payload_tensors.empty()) c["payload_tensors"] = container_payload_tensors;   // K -> COUNT of tensors
if (!container_payload_rows.empty())    c["payload_rows"]    = container_payload_rows;      // K -> SUM of rows

check(tb.at(4736) == 2,                       "payload_tensors: the two gate_exps tensors share K=4736");
check(rb.at(4736) == 2 * 4096,                "payload_rows: the rows at that width are 2 x 4096, not 2");
check(rb.at(8704) == 8704,                    "payload_rows: the lm_head is 8704 rows");
check(rb.at(4736) != tb.at(4736),             "payload_rows: the two numbers differ, which is the point");

That last arm is the one that keeps the distinction alive. With both fields named for what they are, a later edit that made one alias the other would fail on the assertion it violates, not on a comment nobody reads. And the controls are still there: the 2-D BF16 embedding is not counted as a payload at all, and role_shapes carries the exact shape text plus its own count.

The one thing left, and it is the body only

The table still reads:

payload_rows  {4736: 370, 8704: 1}

which is the tensor counts under the rows key — the thing both fields now exist to separate. From the pack itself (~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2/model.q4nx, reading every I8 3-D tensor and applying your own rule):

payload_tensors  {4736: 370, 8704: 1}            <- what the body shows today
payload_rows     {4736: 1003840, 8704: 7755}     <- and the lm_head's row count is the VOCABULARY (7,755), not its K (8,704)

One edit: print both lines instead of the one, and keep the note that the 8704-K tensor is lm_head — with 7,755 as its row count, which is the nicest illustration of why the two fields are separate.

Everything else I reviewed stands: I8 && 3-D as the filter with its control, role_shapes keyed by role with shape text and count separate, one derivation emitted from manifest(), and the honest scope ("no placement, no mapping, no refusal").