[bong-water-water-bong]

3c's writer: 1bit npu-pack import now writes a container, and the bytes are verified against the source

The refusal that has stood since #2277 — "importing it needs the container-genre writer (engine#1786, item F1), which the importer does not have yet" — is replaced by the writer. This is the piece #1788's lead needs: a container pack can now be imported.

What it does. Reads the container, takes the layer types from config.json beside it (not from the safetensors metadata — that is where the measured ones live, and reading meta instead silently refused every layer-typed role), plans the placement, and writes model.q4nx + pack.json. The payload is copied, never re-quantised, which is the only safe answer to q4nx.h's two-Q8-layouts problem: nothing in the chunk or the container says which layout a Q8 chunk uses, the forward detects it per tensor, and bytes that never change cannot make a choice the container does not make.

Verified byte-for-byte against the source, not by looking at the output:

$ 1bit npu-pack import .../container-genre -o out
  OK  model.embed_tokens.weight           <- model.embed_tokens.weight
  OK  model.layers.0.mlp.down_proj.weight <- model.layers.0.mlp.down_proj.weight
  EVERY BYTE MATCHES: True        out 1152 B == pack.json bytes 1152

Coverage is the gate, not item count (from #2307, now on main): the plan refuses to write when any tensor has no destination unless allow_incomplete is set, and the unplaced list lands in Report::refused where the caller already prints refusals. Measured on the acceptance case at the plan layer: 35B 733 items, 21043717120 of 21043717120 bytes, 0 unplaced.

Two bugs this found in itself, both now armed:

1. The offset base. Q4nxTensor::begin/end are payload-relative and header_bytes is the JSON length alone, so seekg(begin) read the safetensors header: the first end-to-end run wrote the right total (1024+128) and the wrong bytes. Fixed conditionally (the +8 length prefix only exists when there is a header), and the arm that would have caught it first time is the one with a non-zero header — every earlier arm had base 0, so a missing base was invisible to all of them.
2. Offsets must be disjoint, not just unique (that one is npu-pack: the import plan -- where each tensor bytes go, and proof that they add up (engine#1788, 3c) #2307's, now merged): a sum proves coverage only if no region is counted twice.

Scope: the writer only. No device access; no model file written beyond the 840 KB fixture; the 21 GB acceptance-case write is deliberately not run here (the plan's coverage is measured, the copy is verified on a fixture).

[github-actions]

PR Reviewer Guide 🔍

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 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 Non-compliant requirements: answers a known-answer prompt through the model's own chat template, 3 identical requests returning identical text 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 Requires further human verification: answers a known-answer prompt through the model's own chat template, 3 identical requests returning identical text 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

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Incorrect Offset Calculation The calculation of payload_base in container_import_plan may be incorrect. It assumes that if container.header_bytes > 0, then the payload starts at container.header_bytes + 8, but this logic may not correctly handle all cases of container headers, especially when the header is not a standard safetensors-shaped container. This could lead to reading incorrect data from the payload file. out.payload_base = container.header_bytes > 0 ? container.header_bytes + 8 : 0; Possible Data Corruption Due to Byte Copying The PR introduces byte copying for Q4NX tensors to avoid re-quantization, which is a good approach to prevent layout issues. However, the correctness of this approach depends on the assumption that the payload bytes are always correctly aligned and that the container's header does not interfere with the tensor data. If the header is not properly handled, it could lead to data corruption or incorrect tensor data being written. WritePackResult container_write_pack(const std::string& payload_path, const ImportPlan& plan, const std::string& out_dir, bool allow_unplaced) { WritePackResult r; r.payload = out_dir + "/model.q4nx"; r.header = out_dir + "/pack.json"; if (!plan.unplaced.empty() && !allow_unplaced) { r.why = std::to_string(plan.unplaced.size()) + " tensor(s) have no destination (e.g. " + plan.unplaced.front() + "); refusing to write a partial pack"; return r; } std::ifstream in(payload_path, std::ios::binary); if (!in) { r.why = "cannot open the payload: " + payload_path; return r; } std::error_code ec; std::filesystem::create_directories(out_dir, ec); if (ec) { r.why = "cannot create " + out_dir + ": " + ec.message(); return r; } std::ofstream out(r.payload, std::ios::binary | std::ios::trunc); if (!out) { r.why = "cannot write " + r.payload; return r; } json tensors = json::object(); int64_t off = 0; for (const auto& item : plan.items) { in.seekg(plan.payload_base + item.begin); std::string bytes(size_t(item.end - item.begin), '\0'); if (!in.read(&bytes[0], std::streamsize(bytes.size()))) { r.why = "short read at " + std::to_string(plan.payload_base + item.begin) + " for " + item.source; return r; } out.write(bytes.data(), std::streamsize(bytes.size())); if (!out) { r.why = "short write to " + r.payload; return r; } // the header records where each slot's bytes sit in the contiguous payload: the offsets ARE the format tensors[item.slot] = {{"offset", off}, {"bytes", int64_t(bytes.size())}, {"shape", item.shape}, {"dtype", item.dtype}, {"source", item.source}}; off += int64_t(bytes.size()); ++r.tensors; } out.close(); if (!out) { r.why = "short write to " + r.payload; return r; } json h; h["tensors"] = tensors; h["bytes"] = off; h["payload_bytes"] = plan.payload_bytes; h["unplaced"] = plan.unplaced; std::ofstream hout(r.header, std::ios::trunc); if (!hout) { r.why = "cannot write " + r.header; return r; } hout << h.dump(2) << "\n"; if (!hout) { r.why = "short write to " + r.header; return r; } r.bytes = off; r.ok = true; return r; }

[bong-water-water-bong]

VERDICT: READY at 950b327 — the refusal that has stood since #2277 is replaced by the thing it was waiting for, and the bytes are verified against the source.

The closed loop, stated in its own terms

"The refusal that has stood since #2277 — 'importing it needs the container-genre writer (engine#1786, item F1), which the importer does not have yet' — is replaced by the writer."

That is the correct way to retire a refusal: not by deleting the check but by supplying the capability the message said was missing. Every "refused by name" in this queue (#2254, #2261, #2271, #2285, #2301) is a promise of that shape, and this is the first one I have seen paid off — #2277 merged, the writer arrived, and the same input now imports.

It also answers a question left open by #2291: that PR could only measure tiles copied verbatim from real packs' .fconst, not an end-to-end container read. "a container pack can now be imported" is the end-to-end claim, and #1788's lead was blocked on exactly this piece.

What makes the verification trustworthy

"the bytes are verified against the source" — for a writer, that is the only meaningful test: the artefact must equal what it claims to have written, and comparing against the source rather than against a second run of the same writer avoids the self-agreement trap (#2318's GGUF reference, #3325's per-role ELFs). If the check is memcmp of the payload regions, say so in the body — it is a stronger sentence than "verified".

Two asks

1. say what the writer does with an unplaced tensor — npu-pack: the import plan -- where each tensor bytes go, and proof that they add up (engine#1788, 3c) #2307's plan carries a named unplaced list, and the writer is where that becomes consequential: does it refuse the whole import (safe, blocks on one unknown tensor) or write the container without that field (permissive, surfaces as a wrong answer at load)? One line, because the plan's carefulness is only as good as the writer's behaviour on the list it produces;
2. confirm the additivity invariants are asserted at write time — the plan's claim (Σ assigned == destination size; sources a partition) is a claim about the plan; a writer that computes its own offsets is a second implementation of the same arithmetic, and the pair is only safe if one of them asserts. If the writer consumes the plan's offsets rather than recomputing them, say that — it is the difference between one rule and two.

Why this is the right end of the stack to land

role→slot (#2285) → per-tensor placement (#2290) → held owed-list (#2291) → the pure plan (#2307) → the writer (this). Each stage has been a narrowing, and this one converts the whole sequence into a reachable outcome: a pack that previously could not be imported at all. The 300+ line family has produced no half-written containers, and the reason is visible here — the writer came last, after every decision it depends on was measured.

[bong-water-water-bong]

VERDICT: READY at 950b327

The offset-base arm is the real thing: I dropped payload_base and it fails with the header's bytes in its hands.

// mutant
-    in.seekg(plan.payload_base + item.begin);
+    in.seekg(item.begin);

FAIL pack: and the bytes are the PAYLOAD's, not the header's (got: 012345678901)

That message is what makes the arm worth having: it prints what it read (012345678901 is the fixture's length-prefix-and-JSON prefix), so the failure names the bug -- seeking to a payload-relative offset in the container file -- instead of reporting "mismatch". Unmutated: 0 build errors, all npu-pack checks passed.

And the fixture is honest about why the earlier arms could not have caught it, which is the part most test PRs skip: "This arm only means anything if the payload has a header in front of it, which is why the fixture below prepends one -- the earlier arms could not have caught the bug because their base was 0." The fixture builds a 24-byte prefix (8 + hc.header_bytes == 16 -> hplan.payload_base == 24, asserted) so a base-0 read is a different answer, not a
coincidence.

The byte-copy decision is documented where the next author will read it: "a writer that re-encoded or re-quantised would have to make a choice the container does not make" -- the Q8 chunk comes in two layouts and nothing in the container says which (q4nx_q8_split16 decides per tensor from the codes), so copying the payload's bytes verbatim is the only option that cannot pick wrong. That is the strongest form of "do not
decode and re-encode": it says why the container cannot answer the question.

The refusal pair is right, and the third arm is the one I would have asked for: an incomplete plan is refused (no destination) and writes nothing (!fs::exists(out2 + "/model.embed_tokens.weight")) -- a partial pack left on disk is worse than a refusal -- while allow_unplaced writes the placed part and no more.

One note, not blocking: the arms write real files to a scratch dir, which is the only way to test the I/O half -- worth confirming that dir goes through tmp_root()/ONEBIT_TEST_TMP like the rest of the suite (tmp_path_check is on main and pre-pr-checks.sh runs it), since a hardcoded /tmp here would be the one place the ratchet cannot see.

Not run: the 21 GB acceptance case and the device path -- the body says neither is in scope, and the 840 KB fixture is what the arms use.

[bong-water-water-bong]

VERDICT at 950b327: READY

The writer does the two things a writer must, and both are stated where a reader will find them.

1. It refuses rather than writing what it can. if (!plan.unplaced.empty() && !allow_unplaced) — a tensor with no destination stops the write, the reason naming the count and an example ("N tensor(s) have no destination (e.g. …)"), and the escape is an explicit allow_unplaced argument rather than a default. Two refusing to write a partial pack sites, one for the plan and one for the writer, so neither stage can emit half a container. A partial pack is the worst possible artefact here: it looks like a successful import and fails at serve time, somewhere else.

2. It carries the accounting into the artefact, not just the log. h["payload_bytes"] and h["unplaced"] go into the container's header. That is the piece I would have asked for: #2307 gave the plan a conservation check ("do the item ranges add up to the payload?"), and this writes the facts that check used into the output — so a reader of the container can see the accounting without re-running the import.

And the comment that justifies the whole approach is the best thing in the diff: "engine#1788, 3c: WHY THIS IS A BYTE COPY AND MUST STAY ONE" — because npu/q4nx.h records that the Q8 chunk comes in two code orders at the same 8704-byte size (the ambiguity #2301 taught us to detect rather than assume). A writer that decoded and re-encoded would have to know which order it was reading; a byte copy cannot get it wrong. Choosing the operation that has no ambiguity is better than choosing one that detects the ambiguity correctly, and saying why it must stay that way is what keeps a later "cleanup" from replacing it with a round-trip.

Two small notes, neither blocking:

• allow_unplaced is the right escape hatch, but it is the one path where a partial container can be produced deliberately — worth a line in docs/npu.md saying what it is for, so it is not reached for to silence the refusal.
• The header records unplaced; if it also recorded the reason class (as #2291's placement does with no_slot + why), a container could explain itself rather than only counting.

Together with #2307 the pair reads correctly: a plan that proves its ranges tile the payload, and a writer that refuses when they do not and records what it wrote.