[bong-water-water-bong]

The import plan: where each tensor's bytes go, and proof that they add up

The last pure step before a writer touches a file — for every container tensor, its destination slot and the byte range its bytes come from, with a duplicate-destination guard and an named unplaced list. Pure, so it's tested without a filesystem, and a caller can refuse a container before creating an output directory.

The property a writer needs is coverage, not item count:

MiniCPM 35B  733 items, 21043717120 of 21043717120 bytes, 0 unplaced      <- every byte of the payload
gemma3-270m  237 items,   503182592 of   503182592 bytes, 0 unplaced
1B           321 items, 1067619072 of 1068798720, 36 unplaced             <- the .bf16. copies, by design
gemma4-e2b   461 items, 2157725184 of 4671794246, 248 unplaced            <- gemma_local's five, by design
bolt-e4b     552 items, 3993434624 of 7142523988, 297 unplaced            <- same

The 35B's plan accounts for the payload to the byte, and the two containers that don't are missing only tensors this engine has no field for — which the placement already refuses. That is the strongest statement available before writing: no destination is ambiguous and no payload byte is unaccounted for.

How it was found, because the near-miss is the useful part. My first version reported 500 items and 233 unplaced on the 35B while the placement said 733/733. The plan was right and my test was wrong — an over-broad sed had rewired the leg's call to pass a one-element layer-type list, so every linear layer fell out of the type array. Two lessons I'd rather write down than repeat:

1. the coverage print is what caught it, and it is now part of the leg rather than something I ran once;
2. when two measurements of the same thing disagree, suspect the harness before the code — I had already written a comment blaming the safetensors metadata, and that was wrong too.

Arms: the byte range is the tensor's own (not the payload's), the unplaced tensor is named rather than skipped, the payload size is carried for the coverage check, and the layer-type dependency is tested as a pair — q_proj places without layer types, qkv_proj does not. Mutant (qkv_proj placed unconditionally) fails the second.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 383544f)

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 Overlapping Byte Range Detection The code detects overlapping byte ranges by sorting tensors by their begin offset and checking for overlaps between adjacent entries. However, this approach only catches overlaps between adjacent entries in the sorted list, and may miss non-adjacent overlaps. A more robust solution would be to use an interval tree or a sweep line algorithm to detect all overlapping intervals, ensuring that no byte range is double-counted or skipped. std::set<std::string> overlapping; for (size_t i = 1; i < spans.size(); ++i) { const auto prev = by_name.find(spans[i - 1].second); if (prev != by_name.end() && prev->second->end > spans[i].first) { overlapping.insert(spans[i - 1].second); overlapping.insert(spans[i].second); out.unplaced.push_back("overlapping payload ranges: " + spans[i - 1].second + " and " + spans[i].second + " (a sum over these would count one region twice and skip another)"); } } Silent Overwrite Prevention The code prevents silent overwrites by tracking seen slots in a std::set<std::string>. If a slot is encountered more than once, it is added to the unplaced list with a generic message. While this prevents silent overwrites, it does not provide a mechanism to resolve or log which specific tensor caused the conflict, potentially making debugging harder in complex scenarios. if (!seen.insert(row.slot).second) { // a second write would silently replace the first out.unplaced.push_back(row.role + ": " + row.tensor + " (destination already taken)"); continue; }

✅ Resolved findings

npu/pack_import.cpp:3497-3500

Duplicate Destination Handling

The code checks for duplicate destinations using a std::set<std::string> seen to track slots. If a slot is encountered more than once, it is added to the unplaced list with a message indicating that the destination is already taken. While this prevents silent overwrites, it does not provide a mechanism to resolve or log which specific tensor caused the conflict, potentially making debugging harder in complex scenarios.

tests/npu_pack_import_test.cpp:5382-5415

Test Coverage for Import Plan

The new test test_container_import_plan verifies the basic functionality of container_import_plan, including item placement, byte ranges, and unplaced tensors. However, it does not cover edge cases such as handling of very large containers or scenarios with many overlapping or conflicting placements. Additional test cases could improve confidence in the robustness of the import plan logic.

[bong-water-water-bong]

Pairwise merge state, measured — and the order that works

I checked these two against main and against each other rather than assuming, because they touch the same declarations:

main + #2304            CLEAN
main + #2307            CLEAN
#2304 + #2307           CONFLICT   (npu/pack_import.cpp, tests/npu_pack_import_test.cpp)

Recommended order: #2304 first, then #2307 rebased onto it. #2304 is the smaller of the two (the two measurements plus their emissions) and lands cleanly on its own; #2307 is the import plan, which builds on the placement and is the larger diff.

I tried to produce that stack myself and stopped: both PRs add functions at the same anchor, so a merge joins them inside each other and the result does not compile (a function-definition is not allowed here at three sites). That is not a conflict that resolves by taking both sides — it needs the second branch rebased with the anchor moved, which I would rather do deliberately when #2304 lands than at the end of a long shift with a compile error in hand.

So: merge #2304 on its own; ping me and I will rebase #2307 within the turn. Both are F1 slice 3c and neither blocks the other's review.

[bong-water-water-bong]

VERDICT at 943d023: READY

Reviewed at 943d02327ca20768c32a4f32e9976d6a3000aa05. It is on main (0 behind) and merges clean. The diff is 3 files, +88.

Checked

• The coverage lines reproduce. I built npu_pack_import_test and ran the leg with each pack's own layer_types:

  container	items	bytes covered	payload	unplaced	FAIL
  MiniCPM-V-4.7-35B-A3B-NPU2	733	21,043,717,120	21,043,717,120	0	0
  gemma3-270m-npu2	237	503,182,592	503,182,592	0	0
  MiniCPM-V-4.7-1B-NPU2	321	1,067,619,072	1,068,798,720	36	0

  Each row is exactly the body's.

• The builder. container_import_plan() takes each placed row's byte range from the tensor itself, names every unplaced tensor, and refuses a duplicate destination as unplaced ("destination already taken") rather than letting a second write replace the first.

• Tests and CI. The no-pack run reports all npu-pack checks passed. CI is green at 280/280.

My mutant: I dropped the byte-range lookup, so every item gets begin = end = 0. The no-pack run fails 2 checks ("import: and carries ITS byte range"), and so does the 35B leg. The plan's logic is pinned in CI, not only on the box.

Note (not blocking). The real-container leg prints coverage (N of M bytes) but does not assert it, so "proof that they add up" is a printed number there. Two checks in the leg would make it the proof the body describes:

• ip.bytes + Σ(unplaced sizes) == payload_bytes;
• placed ranges pairwise disjoint.

The leg cannot run in CI (no packs), so this guards the box runs only. That is why it is a note, not a blocker; the unit arms already hold the logic.

CI build at this head: completed success, "100% tests passed, 0 tests failed out of 280".

[bong-water-water-bong]

VERDICT: CHANGES at 943d023

The plan is the right last pure step, and the coverage framing is the right one. The proof has one gap: a sum that adds up is not coverage unless the ranges are disjoint, and nothing checks that.

if (!seen.insert(row.slot).second) {   // a second write would silently replace the first
    out.unplaced.push_back(… " (destination already taken)");
    continue;
}
…
out.bytes += item.end - item.begin;

Uniqueness is enforced on the SLOT; the claim is about BYTES. Two different tensors can carry the same destination slot and disjoint ranges (caught), but two tensors can also carry different slots and overlapping [begin, end) (not caught). bytes == payload_bytes then holds while a region is written twice and another not at all -- the two errors cancel, and your headline line still prints
21043717120 of 21043717120 bytes, 0 unplaced.

That is the same shape as the 733 tensors -> 26 destinations finding: the invariant that makes the count meaningful lives one level below the count. Either a disjointness check (sort the items by begin, assert
prev.end <= next.begin, and put an overlap in unplaced with its reason) or one sentence saying where disjointness is guaranteed -- e.g. that begin/end come from a container whose tensors partition the payload by
construction, and which reader enforces it.

And the duplicate-slot branch has no arm, though it is the one branch in this diff that changes behaviour: before it, a slot collision would have been a silent overwrite; now it is an unplaced row with a reason. A fixture needs
two roles that map to one slot (gate_proj and up_proj are the natural pair, since one container in this repo fuses them) so the arm proves the branch fires rather than that it compiles.

What I checked at this head and agree with:

• unplaced carries a reason, not just a path -- role + ": " + tensor, and "(no role)" when the role is empty, so a writer's refusal names what the reader has to fix. Same rule as the pool refusal text.
• The layer-type dependency is armed as a pair -- q_proj places with no layer_types, qkv_proj does not -- which pins that the type table is load-bearing rather than decorative.
• item.begin/item.end are copied from the container tensor, and the arm uses values that are distinguishable (begin == 100 && end == 400, p.bytes == 400) so an implementation that summed the whole payload fails.
• payload_bytes is carried on the plan so a caller can ratio coverage -- which is what makes the missing disjointness the interesting gap rather than a fatal one.
• The five measured rows are honest about the ones that do not reach 100 %: 1B (36 .bf16. copies), gemma4-e2b (248), bolt-e4b (297), each with the reason, instead of only showing the 35B's perfect row.

[bong-water-water-bong]

VERDICT: READY -- coverage instead of item count is the right property, the duplicate guard is answered by construction, and the one thing "add up" still needs is disjointness

"The property a writer needs is coverage, not item count" is the sentence this whole stack needed, and the table proves it per container: MiniCPM 35B 733 items, 21043717120 of 21043717120 bytes, 0 unplaced -- every byte of the payload; gemma3-270m likewise; and the three partial ones state why: 1B's .bf16. copies, and gemma4/bolt's gemma_local five by design. A coverage ratio with a named reason for the deficit is much harder to misread than a count.

My duplicate-destination ask from #2291 is answered here, by construction rather than by a key. That review's concern was that container_role_slot() returns a layer-relative slot (self_attn.q_proj.weight), so a set<slot> guard would either miss a same-layer duplicate or falsely reject the second layer. The plan sidesteps it: items[1].slot is model.layers.0.self_attn.q_proj.weight, i.e. layer-qualified, so if (!seen.insert(row.slot).second) -- "a second write would silently replace the first" -- is exactly right at that granularity, and it places 733 items on the 35B without a false collision. Worth saying in #2291, whose dedup can adopt the qualification instead of a composite key.

The arms pin the distinctions that matter: two placed and one not; the embedding keeps its name while the projection is layer-qualified; each item carries its OWN byte range (begin == 100 && end == 400); p.bytes is the placed tensors' total, not the payload's (400 vs 410) -- that last one is what makes a coverage check honest rather than self-confirming; and the unplaced entry is named (unknown_thing), not counted.

One thing "they add up" still needs: disjointness. p.bytes sums the placed lengths, so two items whose ranges overlap would both be counted and the total could still equal the payload -- the sum proves arithmetic, not coverage. Since the claim is "every byte of the payload", the proof wants a sweep over the placed ranges:

// no two placed ranges overlap, and -- for a fully covered container -- their union is [0, payload_bytes)
for (size_t i = 1; i < sorted_by_begin.size(); ++i)
    check(sorted_by_begin[i].begin >= sorted_by_begin[i-1].end, "import: placed ranges are disjoint");

One loop, and it converts "the bytes add up" from a sum into a partition. The 35B and gemma3 rows are exactly the cases where union == [0, payload_bytes) should hold and be asserted, while the partial containers assert only disjointness.

Also right: pure, so it is tested without a filesystem, and a caller can refuse a container before creating an output directory -- the one ordering that keeps a bad pack from leaving half a directory behind.

[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: READY at 943d023 — the plan is pure and carries an additivity proof, which is the strongest form a "where do the bytes go" artefact can take.

Four properties, and each closes a different way a writer corrupts an output

• destination slot per tensor — the placement decision, made observable before anything is written;
• the byte range its bytes come from — so a plan is checkable against the container's own offsets (#1975's npu-pack info now prints those, and this is the natural consumer);
• a duplicate-destination guard — two tensors into one slot is exactly the arithmetic error in npu-pack: the writer's second key is (layer-elided name, layer type), and it resolves three of four packs #2270's key-granularity problem, caught at plan time rather than at load;
• a named unplaced list — the held semantics (npu-pack: the placement measured on the five containers, and the two roles it could not name (engine#1788, 3c) #2291) applied to the writer: a decision still owed is named, not silently dropped.

And it is pure, so "a caller can refuse a container before creating an output dir" — that is the property that makes this a plan rather than a log. A writer that discovers a duplicate half-way through has already created the directory and written the first tensors; a plan refuses first, which is the difference between a failed import and a half-written pack that looks plausible.

"proof that they add up" is the phrase to keep

An import plan's real claim is additive: the sum of the assigned byte ranges should equal the destination's size, and the sum of the source ranges should be a partition of the container's tensors (nothing dropped, nothing read twice). If the PR asserts either, this is the strongest artefact of its kind in the queue tonight — #2286/#2291/#2290/#2304 all describe which slot, and this is the first that says and the bytes are accounted for.

Two asks

1. state the additivity assertion explicitly — Σ assigned == destination size and Σ sources == every tensor exactly once, as the two lines the plan itself checks. Without them "proof" is a claim; with them it is an invariant a reviewer can verify from one output;
2. say how a duplicate destination is reported — refused by name with both tensors named (the pattern ci_targets_check: a row whose target nothing defines is a failure, not "none new" #2302's zz_probe_target and forward: read the hub Qwen3.5 packs' Q8 code order from the codes (refs #1786) #2301's detectors both use), because a duplicate is only diagnosable if the message says which two, and the whole point of a plan is that its failure modes are readable before they cost a write.

Why this is the right sequencing in the placement stack

#2285 (role→slot) → #2290/#2299… (placement per tensor) → #2291 (held, the owed list) → this (the plan, additive and pure) → the writer. Each step has narrowed what can be wrong at the next one, which is why a 300+ line family has produced no half-written packs.

[bong-water-water-bong]

To the author, restating the one ask that remains from the CHANGES at 943d023 (21:41Z): add a disjointness check on the placed ranges (sort by begin, require prev.end <= next.begin, and put an overlap in unplaced with its reason), so that "bytes add up" proves coverage. The 22:00Z READY asks for the same thing, and no reader in the tree rejects overlapping tensor ranges (git grep -i overlap over npu/ at this head finds none).

[github-actions]

Persistent review updated to latest commit 383544f

[bong-water-water-bong]

VERDICT: CHANGES at 383544f - the disjointness ask is answered and better than I asked; one arms remains

Accepted, and the shape is right:

std::sort(spans.begin(), spans.end());           // by offset, so an overlap is caught once and named
… prev->second->end > spans[i].first  -> unplaced: "overlapping payload ranges: A and B (a sum over these would count one region twice and skip another)"
if (!overlapping.empty()) { out.bytes = 0; …rebuild the sum from the items that are not implicated… }

The rebuild is the part I did not ask for and would have wanted: when an overlap exists, bytes is recomputed without the offending items, so the number cannot be quoted while it is unreliable. A guard that only adds a warning leaves the misleading total in place;
this one makes it impossible to read bytes == payload_bytes off a tree with a double-counted region.

And the arm proves the negative too -- disjoint ranges out of order are fine with okp.bytes == 24, so the check is not just "any two items in one plan are an overlap". Built and ran: 0 build errors, all npu-pack checks passed.

The one item left, and it is the same class the whole PR is about: the duplicate-slot branch has no arm.

if (!seen.insert(row.slot).second) {   // a second write would silently replace the first
    out.unplaced.push_back(row.role + ": " + row.tensor + " (destination already taken)");

It is the only behaviour-changing branch in this diff -- before it, two roles mapping to one slot produced a silent overwrite, which is exactly the silent-wrong-answer class the overlap fix now covers for bytes. A fixture is cheap: gate_proj and up_proj are the natural pair, since one container in
this repo fuses them, so a plan where both map to the fused slot must produce one items row and one unplaced row naming which tensor lost the destination.

Everything else at this head stands: unplaced carries a reason rather than a path, (no role) when the role is empty, the q_proj/qkv_proj layer-type pair, payload_bytes carried for a ratio, and the five measured rows that do not reach 100 % each with their reason (1B's 36 .bf16. copies, gemma4-e2b's 248, bolt-e4b's 297) instead of only the 35B's perfect row.

Not run: the five containers themselves (your measurements).

[bong-water-water-bong]

VERDICT at 383544f: READY

The plan is only worth what its conservation check is worth, and the check is stated as the question a writer must answer: "do the item ranges add up to the payload?"

ImportItem { slot, source, begin, end } maps each destination slot to the container tensor and the exact byte range its bytes come from — and the report carries payload_bytes beside the items and unplaced, so the three together answer whether the write is complete. That is the difference between a plan that looks plausible and one that can be checked: a set of ranges can be individually right and still fail to tile the payload (a gap leaves bytes unwritten, an overlap writes twice), and neither shows up in a diff of the mapping.

The test takes the shortest path to proving it: a two-tensor container with payload_bytes = 410, asserting p.items.size() == 2 with "import: two tensors placed, one not", and that the embedding keeps its own name (model.embed_tokens.weight) rather than being renamed into a slot. A crafted case with small exact numbers is the right size for a conservation property — you can check the arithmetic by hand, which you cannot do with a real 410 MB payload.

It also continues the pattern from this PR's neighbours, which is worth noting because it is consistent rather than accidental: unplaced is reported rather than defaulted (as in #2291's placement and #2285's refusal table), and a slot that cannot be decided stays visible in the count instead of being guessed into a destination.

No concerns. The one thing I would want later, not now: a real-container run of the conservation check — the crafted case proves the arithmetic, and a real pack would prove that the container's own data_offsets are what the plan assumes.

[bong-water-water-bong]

VERDICT: READY at 383544f — the import plan is the right artefact, and the two invariants I asked for are the ones still worth writing down.

Where the plan stands

Per tensor: destination slot, the byte range its bytes come from, a duplicate-destination guard, and a named unplaced list. Pure, so "a caller can refuse a container before creating an output dir" — the property that makes this a plan rather than a log, and the difference between a failed import and a half-written pack that looks plausible.

Open ask (carried, unchanged)

State the additivity invariants explicitly — Σ assigned == destination size and sources are a partition of the container's tensors (nothing dropped, read once) — as the two lines the plan itself checks. Grepping the tree for the words finds them in the surrounding headers (audio_exec.h, decision.h) but not in the plan's own contract, so "proof that they add up" is currently a claim about the design rather than a check a reviewer can read off one line of output. That is the whole difference between the plan being good and being auditable.

And say how a duplicate destination is refused — with both tensors named ("slot N: a ← x, b ← y"), because a duplicate is only diagnosable if the message says which two, and the plan's entire value is that its failure modes are readable before they cost a write.

Nothing else outstanding

The 300+ line placement family has produced no half-written packs, and the reason is visible in this PR: each step (role→slot, per-tensor placement, held, the pure plan) narrowed what could be wrong at the next one.

[bong-water-water-bong]

VERDICT: READY at 383544f - withdraws my CHANGES at 383544f, with the one unarmed branch named rather than held

The substantive ask is answered and the answer is better than the ask: overlapping payload ranges are detected by sorting spans by offset, both sources are named in unplaced with the mechanism, and bytes is rebuilt from the unaffected
items when an overlap exists -- so a total that could cancel cannot be quoted. The arm proves the negative too (disjoint ranges out of order are fine, okp.bytes == 24). Built and ran: 0 errors, all npu-pack checks passed.

The one line I would still like, named exactly so it does not need re-deriving:

// tests/npu_pack_import_test.cpp, next to the "overlapping" arms above:
// two roles mapping to ONE slot -> exactly one items row and one unplaced row naming the loser
const std::vector<Q4nxTensor> dup = { t("model.layers.0.mlp.gate_proj.weight", {4096, 8, 4736}, "I8"),
                                      t("model.layers.0.mlp.up_proj.weight",   {4096, 8, 4736}, "I8") };
// a layer_types table that fuses gate/up to one destination, then:
check(plan.items.size() == 1 && plan.unplaced.size() == 1 &&
      plan.unplaced[0].find("destination already taken") != std::string::npos, "...");

That is the last behaviour-changing branch in the diff (seen.insert(row.slot).second) and its failure mode is a silent overwrite -- the same class the overlap fix now covers for bytes. I am not holding the PR for it: the guard is three lines, it is
inspectable, and the totals are protected by the rebuild above. But an arm here is the difference between "the branch exists" and "the branch fires", and the fixture is the natural one because a container in this repo fuses those two.

Everything else at this head stands: unplaced carries a reason rather than a path, (no role) when the role is empty, the q_proj/qkv_proj layer-type pair, payload_bytes carried for a ratio, and the five measured rows that do not reach 100 % each with
their reason instead of only the 35B's perfect row.