From 1f1d7265b7a8bcf652cf9bbb24e4b2db5ffea35e Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 12:37:40 -0400 Subject: [PATCH 01/13] npu-pack: the placement measured on the five containers, and the two roles it could not name (engine#1788, 3c) The placement had no real-container evidence at all: the pack leg gates on model.onnx and a container ships model.q4nx. This adds the leg that reads a container with the shipped reader and reports put/held/unnamed plus the reasons and the unknown roles, then closes what it found on the five containers: ssm -> refused with a reason: one role over dt.bias, conv1d, norm and a, four destinations gemma_local -> refused with a reason: a group of seven stems, three of which DO have engine slots container_extra-> refused with a reason: recognised, and deliberately not written (rope_freqs is a flag, not a weight) Before: the 35B reported 210 tensors under "a role this table does not know: it needs a decision", which is the table saying nothing. After, on every container on this box, that bucket is empty: gemma3-270m put 165, held 72, unnamed 0, UNKNOWN none (237 tensors) gemma4-e2b put 320, held 389, unnamed 0, UNKNOWN none (709) bolt-e4b put 383, held 466, unnamed 0, UNKNOWN none (849) MiniCPM 1B put 183, held 174, unnamed 0, UNKNOWN none (357) MiniCPM 35B put 303, held 430, unnamed 0, UNKNOWN none (733) Also fixed: the refusal table carried the literal string "same" as the reason for three shared-expert roles and it reached the output (35B: "held: 120 x same"). A placeholder is not a reason; each of the four now says what is unverified. held is not a failure count: it is the list of decisions the writer still owes, and every one of them now has a stated reason instead of a shrug. --- npu/pack_import.cpp | 19 +++++++++++++++---- tests/npu_pack_import_test.cpp | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index 622e4a33d..c84a588d9 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -3555,10 +3555,21 @@ static const struct { const char* role; const char* why; } kRoleNoSlot[] = { {"k_norm", "same: the engine's `k_norm` string is a config field, not a verified weight slot"}, {"gate_up_proj", "fused in some containers, two slots in the engine; needs a measured split"}, {"moe_router", "the engine reads `mlp.router.bias`; the weight slot is not verified"}, - {"share_gate_exps", "the shared expert's slots differ per architecture and none is verified"}, - {"share_up_exps", "same"}, - {"share_down_exps", "same"}, - {"shared_expert_gate", "same"}, + {"share_gate_exps", "the shared expert's gate slot differs per architecture; none is verified in npu/forward/src"}, + {"share_up_exps", "the shared expert's up slot; same open question as the gate"}, + {"share_down_exps", "the shared expert's down slot; same open question as the gate"}, + {"shared_expert_gate", "the shared-expert router gate; no slot with this name exists in the engine"}, + // Measured on the real containers (35B: 210 tensors): `ssm` is not one destination but FOUR -- dt.bias, conv1d, norm + // and a, with four different shapes. It needs a fourth key (the tensor's own suffix), not a slot, so it is refused + // here on purpose rather than mapped to whichever of the four looked most plausible. + {"ssm", "one role covering dt.bias, conv1d, norm and a -- four destinations with four shapes, so it needs the tensor's own suffix"}, + // Measured on gemma3/gemma4/bolt. `gemma_local` is likewise a GROUP: pre/post_feedforward_layernorm (both of which DO + // have engine slots), inp_gate, layer_output_scale, per_layer_projection, post_layernorm and per_layer_model_proj. + // Refusing the group is honest; the three with slots need the suffix key to be told apart from the four without. + {"gemma_local", "a group: pre/post_feedforward_layernorm have engine slots, inp_gate / layer_output_scale / per_layer_projection / post_layernorm / per_layer_model_proj do not -- separating them needs the suffix key"}, + // Measured on gemma4/bolt. These ARE recognised and deliberately have no destination: `rope_freqs.weight` is a file the + // forward only FLAGS (model_config.h: has_rope_freqs_file) and the per-layer embeddings have no slot of their own. + {"container_extra", "recognised so it is not a gap, and deliberately not written: a table the forward only flags (rope_freqs) or a per-layer embedding with no slot"}, }; // The layer types that are linear attention, as an EXACT set rather than a substring: the defect family this stack diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index dffe981ef..30d00b5cb 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5179,6 +5179,31 @@ void test_container_role_layer_types() { // engine#1788, 3c: the writer iterates TENSORS, not roles, so the placement has to answer per tensor and use that // tensor's own layer type. The arm that matters is the pair: the same role lands in two different places on two layers. +// engine#1788, 3c: the same decision driven by a REAL container. The pack leg gates on model.onnx, so a container +// (model.q4nx) could never reach the placement above; this reads the container with the shipped reader and reports the +// three counts, so the numbers in a PR body are measured on the five containers rather than on the synthetic fixture. +static void test_container_placement_real(const std::string& dir, const std::vector& layer_types) { + using namespace onebit::npu::pack_import; + const Q4nxContainer c = read_q4nx_container(dir + "/model.q4nx"); + const PlacementReport r = container_placement(c.tensors, layer_types); + std::printf("placement: %s -> put %lld, held %lld, unnamed %lld (of %zu tensors)\n", dir.c_str(), + (long long)r.placed, (long long)r.unplaced, (long long)r.no_role, c.tensors.size()); + std::map held_by_reason; + for (const auto& row : r.rows) if (row.slot.empty()) held_by_reason[row.why]++; + for (const auto& kv : held_by_reason) std::printf(" held: %lld x %s\n", (long long)kv.second, kv.first.c_str()); + std::map roles_seen; + for (const auto& row : r.rows) if (!row.role.empty()) roles_seen[row.role]++; + std::printf(" roles:"); for (const auto& kv : roles_seen) std::printf(" %s=%lld", kv.first.c_str(), (long long)kv.second); + std::printf("\n"); + std::printf(" UNKNOWN:"); + for (const auto& kv : held_by_reason) if (kv.first.find("does not know") != std::string::npos) { + for (const auto& row : r.rows) if (row.slot.empty() && row.why == kv.first && !row.role.empty()) { + std::printf(" %s", row.role.c_str()); break; } + } + std::printf("\n"); + check(r.placed + r.unplaced + r.no_role == int64_t(c.tensors.size()), std::string("placement: accounts for every tensor in ") + dir); +} + void test_container_placement() { using pi::Q4nxTensor; const auto t = [](const std::string& n) { Q4nxTensor x; x.name = n; x.dtype = "I8"; return x; }; @@ -6391,6 +6416,14 @@ int main(int argc, char** argv) { test_container_roles_gemma_local(); test_container_roles_moe_and_singular_layer(); test_container_placement(); + if (argc > 4) { + std::vector lt; + std::string all = argv[4]; + std::string cur; + for (char ch : all) { if (ch == ',') { lt.push_back(cur); cur.clear(); } else cur += ch; } + if (!cur.empty()) lt.push_back(cur); + test_container_placement_real(argv[3], lt); + } test_container_role_slots(); test_container_role_layer_types(); test_container_plan(golden_dir.empty() ? "." : golden_dir); From ea738d228454262ec1e763d0dafece3517afcdce Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 12:42:12 -0400 Subject: [PATCH 02/13] npu-pack: a group role's destination is in the tensor suffix, where the engine has one (engine#1788, 3c) ssm and gemma_local were refused as groups, which was right, but two of gemma_local's seven stems DO have slots in npu/forward/src -- pre_feedforward_layernorm and post_feedforward_layernorm -- and for those the fourth key is not a mystery: the suffix IS the destination. Placed now, with the other five members still refused. Measured on the containers (put / held, before -> after): gemma3-270m 165/72 -> 201/36 (+36, 18 layers x 2) gemma4-e2b 320/389 -> 390/319 (+70, 35 x 2) bolt-e4b 383/466 -> 467/382 (+84, 42 x 2) MiniCPM 35B 303/430 -> 303/430 (unchanged: no gemma_local in a MiniCPM) held falls by exactly what put gains, which is the accounting this PR asserts. The five members with no engine slot (inp_gate, layer_output_scale, per_layer_projection, post_layernorm, per_layer_model_proj) stay refused with their reason. --- npu/pack_import.cpp | 22 ++++++++++++++++++++++ tests/npu_pack_import_test.cpp | 14 ++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index c84a588d9..7b89f810d 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -3435,6 +3435,25 @@ std::string header_witness_refusal(int vote_hdrs, bool declared_qk) { // classifies as a bias of q_proj rather than as an unknown tensor, and a name that matches nothing lands in `unassigned` // with its own name rather than being guessed from its shape. // engine#1788, slice 3c step 1: (role, layer type), the key the writer needs. See the header for the measurement. +// The two GROUP roles (ssm, gemma_local) carry several destinations each, and the destination is in the tensor's own +// suffix. Two of gemma_local's seven stems have a slot in npu/forward/src -- the einsum-free `pre/post_feedforward_ +// layernorm` pair -- and the rest have none, so this table places exactly those two and leaves the others refused. +static const struct { const char* role; const char* suffix; const char* slot; } kGroupSuffixSlots[] = { + {"gemma_local", "pre_feedforward_layernorm.weight", "pre_feedforward_layernorm.weight"}, + {"gemma_local", "post_feedforward_layernorm.weight", "post_feedforward_layernorm.weight"}, +}; + +static std::string group_suffix_slot(const std::string& role, const std::string& tensor) { + for (const auto& kv : kGroupSuffixSlots) { + if (role != kv.role) continue; + const std::string suffix = kv.suffix; + if (tensor.size() >= suffix.size() && + tensor.compare(tensor.size() - suffix.size(), suffix.size(), suffix) == 0) + return kv.slot; + } + return std::string(); +} + PlacementReport container_placement(const std::vector& tensors, const std::vector& layer_types) { PlacementReport out; @@ -3460,6 +3479,9 @@ PlacementReport container_placement(const std::vector& tensors, } } row.slot = container_role_slot(row.role, row.layer_type); + // a group role carries its destination in the tensor's own suffix -- the fourth key, applied where the engine + // actually has a slot for one of the group's members + if (row.slot.empty() && !row.layer_type.empty()) row.slot = group_suffix_slot(row.role, t.name); if (row.slot.empty()) { const auto why = table.why.find(row.role); row.why = why != table.why.end() ? why->second diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index 30d00b5cb..e871d748e 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5231,6 +5231,20 @@ void test_container_placement() { check(r.rows[5].slot.empty() && r.rows[5].role.empty(), "placement: a tensor with no role is counted apart"); check(r.no_role == 1, "placement: no_role counts the vocabulary gap, not the deliberate refusal"); check(r.unplaced == 2, "placement: unplaced counts the two decisions, not the vocabulary gap"); + { + // the group roles: the destination is the tensor's own suffix, and only the members the engine has a slot for move + const std::vector g = { + t("model.layers.0.pre_feedforward_layernorm.weight"), + t("model.layers.0.post_feedforward_layernorm.weight"), + t("model.layers.0.inp_gate.weight"), + }; + const pi::PlacementReport gr = pi::container_placement(g, lt); + check(gr.rows[0].role == "gemma_local", "placement: the gemma group is one role"); + check(gr.rows[0].slot == "pre_feedforward_layernorm.weight", "placement: the suffix IS the destination (pre)"); + check(gr.rows[1].slot == "post_feedforward_layernorm.weight", "placement: and for post"); + check(gr.rows[2].slot.empty(), "placement: a member with no engine slot stays unplaced"); + check(gr.placed == 2 && gr.unplaced == 1, "placement: two placed, one held, for the three members"); + } } void test_container_roles_moe_and_singular_layer() { From 79811cf47f7a5d8b702052aea9a5632004403c55 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 12:53:07 -0400 Subject: [PATCH 03/13] tests: the unknown bucket is CHECKED, in CI's own invocation (review of #2291) The review's mutant was right and it is the important kind: the result this PR exists for -- every container having an empty unknown bucket -- was printed and never asserted, so removing {"ssm", ...} left the suite green (rc 0) while the box printed UNKNOWN: ssm. Two changes: * in the real-container leg, collect the roles that hit the "does not know" default and check that the set is empty, naming them in the failure; * a NO-PACK arm for the roles the containers actually carry (ssm, gemma_local, container_extra, the four share_*, q_norm, k_norm, moe_router): each must be explained, none may be defaulted, and no reason may be a bare placeholder. The mutant now fails where CI runs: $ ./npu_pack_import_test # no pack: what ctest runs FAIL slots: the table explains ssm rather than defaulting it rc=1 Also spelled out the last placeholder: k_norm's reason still began "same: ..." (inherited from #2285) and read exactly like the bare "same" this PR removed from the shared-expert entries. --- npu/pack_import.cpp | 2 +- tests/npu_pack_import_test.cpp | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index 7b89f810d..69450e88a 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -3574,7 +3574,7 @@ static const struct { const char* role; const char* slot; } kRoleSlots[] = { static const struct { const char* role; const char* why; } kRoleNoSlot[] = { {"qkv_proj", "the slot prefix is the LAYER TYPE's (`linear_attn.` or `self_attn.`), so it needs the second key"}, {"q_norm", "the engine's normaliser slots are per-architecture; none is verified for this role yet"}, - {"k_norm", "same: the engine's `k_norm` string is a config field, not a verified weight slot"}, + {"k_norm", "the engine's `k_norm` is a config field, not a verified weight slot (the q_norm case again)"}, {"gate_up_proj", "fused in some containers, two slots in the engine; needs a measured split"}, {"moe_router", "the engine reads `mlp.router.bias`; the weight slot is not verified"}, {"share_gate_exps", "the shared expert's gate slot differs per architecture; none is verified in npu/forward/src"}, diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index e871d748e..fd14e4d9e 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -40,6 +40,7 @@ #include "q4nx.h" #include "q4nx_device.h" +#include #include #include @@ -5202,6 +5203,15 @@ static void test_container_placement_real(const std::string& dir, const std::vec } std::printf("\n"); check(r.placed + r.unplaced + r.no_role == int64_t(c.tensors.size()), std::string("placement: accounts for every tensor in ") + dir); + // REVIEW of #2291: the result this PR exists for -- an empty unknown bucket on a real container -- was printed and + // never checked, so a removed refusal entry still exited 0. Now it fails where it happens, with the role named. + std::set unknown; + for (const auto& row : r.rows) + if (!row.slot.empty() || row.role.empty()) continue; + else if (row.why.find("does not know") != std::string::npos) unknown.insert(row.role); + std::string names; + for (const auto& u : unknown) names += (names.empty() ? "" : ", ") + u; + check(unknown.empty(), std::string("placement: no role is unknown to the table in ") + dir + " (" + names + ")"); } void test_container_placement() { @@ -5231,6 +5241,23 @@ void test_container_placement() { check(r.rows[5].slot.empty() && r.rows[5].role.empty(), "placement: a tensor with no role is counted apart"); check(r.no_role == 1, "placement: no_role counts the vocabulary gap, not the deliberate refusal"); check(r.unplaced == 2, "placement: unplaced counts the two decisions, not the vocabulary gap"); + { + // REVIEW of #2291: the containers' whole role vocabulary, no pack needed, because CI runs this binary with no + // pack. A role that reaches the "this table does not know" default, or a reason that is a bare placeholder, is a + // hole -- and this arm is what makes removing a refusal entry fail in CI rather than print UNKNOWN on the box. + pi::ContainerRoles roles_c; + for (const char* role : {"ssm", "gemma_local", "container_extra", "share_gate_exps", "share_up_exps", + "share_down_exps", "shared_expert_gate", "q_norm", "k_norm", "moe_router"}) + roles_c.counts[role] = 1; + const pi::RoleSlotReport slots_c = pi::container_role_slots(roles_c); + for (const auto& kv : slots_c.why) { + check(kv.second.find("does not know") == std::string::npos, + "slots: the table explains " + kv.first + " rather than defaulting it"); + check(kv.second.rfind("same", 0) != 0 && kv.second != "same", + "slots: and the reason for " + kv.first + " is a reason, not a placeholder"); + check(kv.second.size() > 20, "slots: and it is long enough to be one (" + kv.first + ")"); + } + } { // the group roles: the destination is the tensor's own suffix, and only the members the engine has a slot for move const std::vector g = { From f32081e1766af08f085298ddb85b86c5cbe591d3 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 13:05:19 -0400 Subject: [PATCH 04/13] npu-pack: the ssm family, mapped one for one onto the engine slots that exist (engine#1788, 3c) ssm was the biggest refusal and the one that mattered most: 210 tensors on the 35B, the acceptance case. Unlike gemma_local, the ENGINE HAS A SLOT FOR EVERY MEMBER -- grepping npu/forward/src finds the whole family, and its literal names match the classifier's own ssm needles one for one: ssm_alpha_proj -> linear_attn.ssm_alpha_proj.weight ssm_dt -> linear_attn.ssm_dt.bias ssm_beta_proj -> linear_attn.ssm_beta_proj.weight ssm_norm -> linear_attn.ssm_norm.weight ssm_conv1d -> linear_attn.ssm_conv1d.weight ssm_out_proj -> linear_attn.ssm_out_proj.weight ssm_a / .ssm.a -> linear_attn.ssm_a (suffix-only: `ssm_a` is a PREFIX of `ssm_alpha_proj`, so a find() would shadow alpha and beta -- the substring defect in its exact form) Measured, put / held before -> after: MiniCPM 35B 303/430 -> 513/220 MiniCPM 1B 183/174 -> 345/12 gemma3-270m 201/36 -> 201/36 (unchanged: no ssm family) gemma4-e2b 390/319 -> 390/319 bolt-e4b 467/382 -> 467/382 And the ssm refusal's own reason is corrected: it used to say the role "covers four destinations with four shapes", which stopped being true the moment six of them mapped -- an unmatched member is now a name not yet seen, not a destination that does not exist. --- npu/pack_import.cpp | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index 69450e88a..9141f1903 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -3438,18 +3438,35 @@ std::string header_witness_refusal(int vote_hdrs, bool declared_qk) { // The two GROUP roles (ssm, gemma_local) carry several destinations each, and the destination is in the tensor's own // suffix. Two of gemma_local's seven stems have a slot in npu/forward/src -- the einsum-free `pre/post_feedforward_ // layernorm` pair -- and the rest have none, so this table places exactly those two and leaves the others refused. -static const struct { const char* role; const char* suffix; const char* slot; } kGroupSuffixSlots[] = { - {"gemma_local", "pre_feedforward_layernorm.weight", "pre_feedforward_layernorm.weight"}, - {"gemma_local", "post_feedforward_layernorm.weight", "post_feedforward_layernorm.weight"}, +// engine#1788, 3c: `ssm` is the other group, and unlike gemma_local the engine has a slot for EVERY member -- measured by +// grepping npu/forward/src for the ssm family, whose literal names match the classifier's own needles one for one. This is +// the 35B's largest bucket (210 tensors), so it is the placement's biggest single win. +static const struct { const char* role; const char* needle; const char* slot; bool suffix_only; } kGroupSuffixSlots[] = { + {"gemma_local", "pre_feedforward_layernorm.weight", "pre_feedforward_layernorm.weight", true}, + {"gemma_local", "post_feedforward_layernorm.weight", "post_feedforward_layernorm.weight", true}, + {"ssm", "ssm_alpha_proj", "linear_attn.ssm_alpha_proj.weight", false}, + {"ssm", "ssm_beta_proj", "linear_attn.ssm_beta_proj.weight", false}, + {"ssm", "ssm_conv1d", "linear_attn.ssm_conv1d.weight", false}, + {"ssm", "ssm_dt", "linear_attn.ssm_dt.bias", false}, + {"ssm", "ssm_norm", "linear_attn.ssm_norm.weight", false}, + {"ssm", "ssm_out_proj", "linear_attn.ssm_out_proj.weight", false}, + // the `a` member has no `.weight` on either side, and its name is a PREFIX of `ssm_alpha_proj`, so it is matched as a + // suffix only -- a `find()` here would shadow alpha and beta, which is the substring defect in its exact form. + {"ssm", "ssm_a", "linear_attn.ssm_a", true}, + {"ssm", ".ssm.a", "linear_attn.ssm_a", true}, }; static std::string group_suffix_slot(const std::string& role, const std::string& tensor) { for (const auto& kv : kGroupSuffixSlots) { if (role != kv.role) continue; - const std::string suffix = kv.suffix; - if (tensor.size() >= suffix.size() && - tensor.compare(tensor.size() - suffix.size(), suffix.size(), suffix) == 0) + const std::string suffix = kv.needle; + if (kv.suffix_only) { + if (tensor.size() >= suffix.size() && + tensor.compare(tensor.size() - suffix.size(), suffix.size(), suffix) == 0) + return kv.slot; + } else if (tensor.find(suffix) != std::string::npos) { return kv.slot; + } } return std::string(); } @@ -3584,7 +3601,7 @@ static const struct { const char* role; const char* why; } kRoleNoSlot[] = { // Measured on the real containers (35B: 210 tensors): `ssm` is not one destination but FOUR -- dt.bias, conv1d, norm // and a, with four different shapes. It needs a fourth key (the tensor's own suffix), not a slot, so it is refused // here on purpose rather than mapped to whichever of the four looked most plausible. - {"ssm", "one role covering dt.bias, conv1d, norm and a -- four destinations with four shapes, so it needs the tensor's own suffix"}, + {"ssm", "the members this table does not match: the six that do map one for one onto linear_attn.ssm_* slots, so an unmatched one is a name not yet seen, not a destination that does not exist"}, // Measured on gemma3/gemma4/bolt. `gemma_local` is likewise a GROUP: pre/post_feedforward_layernorm (both of which DO // have engine slots), inp_gate, layer_output_scale, per_layer_projection, post_layernorm and per_layer_model_proj. // Refusing the group is honest; the three with slots need the suffix key to be told apart from the four without. From 9f9fbcf6344367369cd061ec553cd8c5598734a2 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 13:17:43 -0400 Subject: [PATCH 05/13] npu-pack: the MoE destinations are the CONTAINER spelling, not the EngineConverter one (engine#1788, 3c) moe_container.h documents TWO naming conventions and they disagree on every MoE name. This table was using the EngineConverter spelling, and a container is the FLM shape -- moe_format() says so ("a container with neither is the FLM shape") and the 35B proves it on disk: `gate_exps_proj` occurs, `experts.gate_proj` does not. So the placement was naming slots the engine would not look for in a pack of this genre, which fails silently as zero experts rather than as an error. gate/up/down_exps -> mlp.{gate,up,down}_exps_proj.weight (was mlp.experts.*) moe_router -> moe_router.weight (was refused) share_*_exps -> mlp.share_{gate,up,down}_exps_proj.weight shared_expert_gate -> mlp.share_router.weight Measured, put / held: MiniCPM 35B 513/220 -> 713/20 the 20 held are the q_norm/k_norm pairs, config fields in this engine MiniCPM 1B 345/12 -> 345/12 already only the norm pairs gemma3/4, bolt unchanged no MoE, and gemma_local's unplaceable five Also: my first attempt at this edit removed the five entries I had just added instead of the five refusal rows -- both tables now edited by exact text, and the refusal table no longer contains roles that are placed. --- npu/pack_import.cpp | 20 ++++++++++++-------- tests/npu_pack_import_test.cpp | 5 ++--- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index 9141f1903..8e463be5d 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -3581,10 +3581,19 @@ static const struct { const char* role; const char* slot; } kRoleSlots[] = { {"gate_proj", "mlp.gate_proj.weight"}, {"up_proj", "mlp.up_proj.weight"}, {"down_proj", "mlp.down_proj.weight"}, - {"gate_exps", "mlp.experts.gate_proj.weight"}, - {"up_exps", "mlp.experts.up_proj.weight"}, - {"down_exps", "mlp.experts.down_proj.weight"}, + {"gate_exps", "mlp.gate_exps_proj.weight"}, + {"up_exps", "mlp.up_exps_proj.weight"}, + {"down_exps", "mlp.down_exps_proj.weight"}, {"shortconv_in_proj", "shortconv.in_proj.weight"}, + // engine#1788, 3c: the MoE names are FORMAT-DEPENDENT, and moe_container.h documents both conventions. A container is + // the FLM shape (`moe_format()`: "a container with neither is the FLM shape"), and the 35B proves it on disk -- + // `gate_exps_proj` occurs, `experts.gate_proj` does NOT. So the destinations below are FLM spellings; the + // EngineConverter spellings (mlp.experts.*, mlp.gate.weight, mlp.shared_expert.*) are a different pack format. + {"moe_router", "moe_router.weight"}, + {"share_gate_exps", "mlp.share_gate_exps_proj.weight"}, + {"share_up_exps", "mlp.share_up_exps_proj.weight"}, + {"share_down_exps", "mlp.share_down_exps_proj.weight"}, + {"shared_expert_gate", "mlp.share_router.weight"}, }; // The roles this slice does NOT place, each with the reason. A number here is honest; a guessed slot is not. @@ -3593,11 +3602,6 @@ static const struct { const char* role; const char* why; } kRoleNoSlot[] = { {"q_norm", "the engine's normaliser slots are per-architecture; none is verified for this role yet"}, {"k_norm", "the engine's `k_norm` is a config field, not a verified weight slot (the q_norm case again)"}, {"gate_up_proj", "fused in some containers, two slots in the engine; needs a measured split"}, - {"moe_router", "the engine reads `mlp.router.bias`; the weight slot is not verified"}, - {"share_gate_exps", "the shared expert's gate slot differs per architecture; none is verified in npu/forward/src"}, - {"share_up_exps", "the shared expert's up slot; same open question as the gate"}, - {"share_down_exps", "the shared expert's down slot; same open question as the gate"}, - {"shared_expert_gate", "the shared-expert router gate; no slot with this name exists in the engine"}, // Measured on the real containers (35B: 210 tensors): `ssm` is not one destination but FOUR -- dt.bias, conv1d, norm // and a, with four different shapes. It needs a fourth key (the tensor's own suffix), not a slot, so it is refused // here on purpose rather than mapped to whichever of the four looked most plausible. diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index fd14e4d9e..50b5082c8 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5246,8 +5246,7 @@ void test_container_placement() { // pack. A role that reaches the "this table does not know" default, or a reason that is a bare placeholder, is a // hole -- and this arm is what makes removing a refusal entry fail in CI rather than print UNKNOWN on the box. pi::ContainerRoles roles_c; - for (const char* role : {"ssm", "gemma_local", "container_extra", "share_gate_exps", "share_up_exps", - "share_down_exps", "shared_expert_gate", "q_norm", "k_norm", "moe_router"}) + for (const char* role : {"ssm", "gemma_local", "container_extra", "q_norm", "k_norm", "gate_up_proj"}) roles_c.counts[role] = 1; const pi::RoleSlotReport slots_c = pi::container_role_slots(roles_c); for (const auto& kv : slots_c.why) { @@ -6339,7 +6338,7 @@ static void test_container_role_slots() { r.counts["q_proj"] = 1; r.counts["gate_exps"] = 1; r.counts["input_norm"] = 1; const RoleSlotReport s = container_role_slots(r); check(s.slot_of.at("q_proj") == "self_attn.q_proj.weight", "slots: the attention query projection"); - check(s.slot_of.at("gate_exps") == "mlp.experts.gate_proj.weight", "slots: the routed gate is the expert gate"); + check(s.slot_of.at("gate_exps") == "mlp.gate_exps_proj.weight", "slots: the routed gate, in the CONTAINER (FLM) spelling"); check(s.slot_of.at("input_norm") == "input_layernorm.weight", "slots: the pre-attention norm"); check(s.no_slot.empty(), "slots: three placed roles leave nothing unexplained"); } From d2d0698c17d04ea4b428da3e0de2514337b0ceb0 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 13:21:25 -0400 Subject: [PATCH 06/13] npu-pack: q_norm and k_norm ARE weight slots -- the placement is now complete on the acceptance case I refused these two for two PRs on the strength of a grep that found only the rmsnorm debug label ("q_norm") and concluded they were config fields. model.c names them explicitly, in both container spellings: "model.layer{s}.%d.self_attn.q_norm.weight" -> q_norm_weight "model.layer{s}.%d.self_attn.k_norm.weight" -> k_norm_weight Placed, and the two refusals removed. Measured, put / held / unnamed: MiniCPM 35B 733 / 0 / 0 of 733 <- the acceptance case: every tensor has a destination MiniCPM 1B 357 / 0 / 0 of 357 gemma3-270m 237 / 0 / 0 of 237 gemma4-e2b 460 / 249 / 0 of 709 the 249 are gemma_local's five members with no engine slot bolt-e4b 551 / 298 / 0 of 849 So three of the five containers place completely, and the two that do not are held only by tensors this engine genuinely has no field for -- which is the refusal doing its job rather than a gap in the table. --- npu/pack_import.cpp | 7 +++++-- tests/npu_pack_import_test.cpp | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index 8e463be5d..f7e5414d9 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -3585,6 +3585,11 @@ static const struct { const char* role; const char* slot; } kRoleSlots[] = { {"up_exps", "mlp.up_exps_proj.weight"}, {"down_exps", "mlp.down_exps_proj.weight"}, {"shortconv_in_proj", "shortconv.in_proj.weight"}, + // engine#1788, 3c: I refused these two for two PRs on the strength of a grep that found only the rmsnorm debug LABEL + // ("q_norm"), and called them config fields. They are weight slots, and model.c names them explicitly: + // "model.layer{s}.%d.self_attn.q_norm.weight" / "… .k_norm.weight" (singular and plural both probed) + {"q_norm", "self_attn.q_norm.weight"}, + {"k_norm", "self_attn.k_norm.weight"}, // engine#1788, 3c: the MoE names are FORMAT-DEPENDENT, and moe_container.h documents both conventions. A container is // the FLM shape (`moe_format()`: "a container with neither is the FLM shape"), and the 35B proves it on disk -- // `gate_exps_proj` occurs, `experts.gate_proj` does NOT. So the destinations below are FLM spellings; the @@ -3599,8 +3604,6 @@ static const struct { const char* role; const char* slot; } kRoleSlots[] = { // The roles this slice does NOT place, each with the reason. A number here is honest; a guessed slot is not. static const struct { const char* role; const char* why; } kRoleNoSlot[] = { {"qkv_proj", "the slot prefix is the LAYER TYPE's (`linear_attn.` or `self_attn.`), so it needs the second key"}, - {"q_norm", "the engine's normaliser slots are per-architecture; none is verified for this role yet"}, - {"k_norm", "the engine's `k_norm` is a config field, not a verified weight slot (the q_norm case again)"}, {"gate_up_proj", "fused in some containers, two slots in the engine; needs a measured split"}, // Measured on the real containers (35B: 210 tensors): `ssm` is not one destination but FOUR -- dt.bias, conv1d, norm // and a, with four different shapes. It needs a fourth key (the tensor's own suffix), not a slot, so it is refused diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index 50b5082c8..f18825dfa 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5246,7 +5246,7 @@ void test_container_placement() { // pack. A role that reaches the "this table does not know" default, or a reason that is a bare placeholder, is a // hole -- and this arm is what makes removing a refusal entry fail in CI rather than print UNKNOWN on the box. pi::ContainerRoles roles_c; - for (const char* role : {"ssm", "gemma_local", "container_extra", "q_norm", "k_norm", "gate_up_proj"}) + for (const char* role : {"ssm", "gemma_local", "container_extra", "gate_up_proj"}) roles_c.counts[role] = 1; const pi::RoleSlotReport slots_c = pi::container_role_slots(roles_c); for (const auto& kv : slots_c.why) { From 056205b5cb56308372700250f78caba8eca910e6 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 13:35:20 -0400 Subject: [PATCH 07/13] tests: a role may not be both placed and refused, and the five refusals are pinned (review of #2291) The review named a hazard review caught and code should catch: during one of my edits this file briefly held two role->destination tables with the same keys and different values, and container_role_slot() would have returned whichever came first. The observable invariant now has an arm: no role is in both slot_of and why, and every role is in exactly one bucket. The five refusals are pinned by name, and the mix is the insight: two are about the SECOND KEY (qkv_proj needs the layer type, gate_up_proj a measured split) and three are GROUP roles (ssm, gemma_local, container_extra) that have no single destination at role granularity even though their members are placed per tensor by suffix. Role-level refusal and per-tensor placement are different answers and both are correct. Also pinned: the MoE destinations are the CONTAINER (FLM) spelling -- gate_exps -> mlp.gate_exps_proj.weight and moe_router -> moe_router.weight -- so a quiet revert to the EngineConverter names, which the engine would not look for in a pack of this genre, fails here instead of producing a pack with zero experts. --- tests/npu_pack_import_test.cpp | 37 ++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index f18825dfa..c53bd7469 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5214,6 +5214,42 @@ static void test_container_placement_real(const std::string& dir, const std::vec check(unknown.empty(), std::string("placement: no role is unknown to the table in ") + dir + " (" + names + ")"); } +// REVIEW of #2291 named a hazard that review caught and code should catch: during one edit this file briefly held two +// role->destination tables with the SAME keys and DIFFERENT values, and container_role_slot() would have returned +// whichever came first. The observable invariant is that no role is both placed and refused -- asserted here once, over +// the whole report, so a duplicated or contradictory entry fails without needing a real container. +static void test_role_tables_disagree_nowhere() { + using namespace onebit::npu::pack_import; + ContainerRoles r; + for (const char* role : {"embed", "final_norm", "input_norm", "post_norm", "lm_head", "q_proj", "k_proj", "v_proj", + "o_proj", "q_norm", "k_norm", "self_attn_gate_proj", "gate_proj", "up_proj", "down_proj", + "gate_up_proj", "qkv_proj", "moe_router", "gate_exps", "up_exps", "down_exps", + "share_gate_exps", "share_up_exps", "share_down_exps", "shared_expert_gate", + "shortconv_in_proj", "ssm", "gemma_local", "container_extra"}) + r.counts[role] = 1; + const RoleSlotReport s = container_role_slots(r); + for (const auto& kv : s.slot_of) { + check(!s.why.count(kv.first), "tables: " + kv.first + " is not both placed and refused"); + } + check(s.placed.size() + s.no_slot.size() == r.counts.size(), "tables: every role is in exactly one bucket"); + // the MoE destinations must be the CONTAINER spelling; the EngineConverter one is a different pack format + check(s.slot_of.count("gate_exps") && s.slot_of.at("gate_exps") == "mlp.gate_exps_proj.weight", + "tables: the routed gate is the FLM spelling, not mlp.experts.*"); + check(s.slot_of.count("moe_router") && s.slot_of.at("moe_router") == "moe_router.weight", + "tables: the router is moe_router.weight, not mlp.gate.weight"); + // two refusals are correct and both are about the SECOND KEY, not about a missing slot: qkv_proj needs the layer type, + // gate_up_proj needs a measured split. Anything else appearing here is a regression, so the set is pinned exactly. + // Five refusals, and the mix is the point: two are about the SECOND KEY (qkv_proj needs the layer type, + // gate_up_proj needs a measured split) and three are GROUP roles (ssm, gemma_local, container_extra) which have no + // single destination at role granularity even though their members are placed per tensor by suffix. Role-level + // refusal plus per-tensor placement are two different answers, and both are correct. + { std::string list; for (const auto& r : s.no_slot) list += r + " "; + check(s.no_slot.size() == 5, "tables: five roles are refused at role granularity (got: " + list + ")"); } + check(s.no_slot[0] == "container_extra" && s.no_slot[1] == "gate_up_proj" && s.no_slot[2] == "gemma_local" && + s.no_slot[3] == "qkv_proj" && s.no_slot[4] == "ssm", + "tables: and they are exactly the pinned five, so a new refusal cannot slip in unnoticed"); +} + void test_container_placement() { using pi::Q4nxTensor; const auto t = [](const std::string& n) { Q4nxTensor x; x.name = n; x.dtype = "I8"; return x; }; @@ -6456,6 +6492,7 @@ int main(int argc, char** argv) { test_container_roles_gemma_local(); test_container_roles_moe_and_singular_layer(); test_container_placement(); + test_role_tables_disagree_nowhere(); if (argc > 4) { std::vector lt; std::string all = argv[4]; From a30efd96f343bf2555bb1b1624db2ec7b524de80 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 13:43:51 -0400 Subject: [PATCH 08/13] tests: a placed slot is checked against the rank and dtype the engine documents (3c, engine#1788) A placement can reach the right NAME with the wrong shape, and the forward then reads the tensor as something else. The rank and dtype are architecture-independent even where the extents are not, and model.h documents them per field, so the real-container leg now asserts them on every placed row: moe_router.weight 2-D, BF16 *exps_proj.weight (routed and shared) 3-D, I8 Measured on the 35B, the router lands at [2048,256] BF16 -- bit for bit the shape model.h annotates for that field -- and every expert stack is 3-D I8 with K=4736, the container's own hidden size. bolt-e4b passes the same arms on the slots it places. PlacementRow carries shape and dtype now, so the check is against the tensor that was actually placed rather than against the name alone. --- npu/pack_import.cpp | 2 ++ npu/pack_import.h | 2 ++ tests/npu_pack_import_test.cpp | 27 +++++++++++++++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index f7e5414d9..521dc9f23 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -3479,6 +3479,8 @@ PlacementReport container_placement(const std::vector& tensors, for (const auto& t : tensors) { PlacementRow row; row.tensor = t.name; + row.shape = t.shape; + row.dtype = t.dtype; const auto it = roles.role_of.find(t.name); if (it == roles.role_of.end()) { row.why = "no role recognised"; ++out.no_role; out.rows.push_back(row); continue; } row.role = it->second; diff --git a/npu/pack_import.h b/npu/pack_import.h index c006bac9a..789e8d7a4 100644 --- a/npu/pack_import.h +++ b/npu/pack_import.h @@ -416,6 +416,8 @@ struct PlacementRow { std::string role; std::string slot; // "" when there is no destination std::string why; // the reason, when slot is empty + std::vector shape; // carried so a placement can be checked against the shape the engine documents + std::string dtype; }; struct PlacementReport { std::vector rows; diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index c53bd7469..d0125391a 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5194,6 +5194,33 @@ static void test_container_placement_real(const std::string& dir, const std::vec for (const auto& kv : held_by_reason) std::printf(" held: %lld x %s\n", (long long)kv.second, kv.first.c_str()); std::map roles_seen; for (const auto& row : r.rows) if (!row.role.empty()) roles_seen[row.role]++; + // REVIEW of #2291 direction: the engine documents a rank and a dtype per slot (model.h), and both are + // architecture-independent -- a router that is 3-D or an expert that is BF16 is wrong for every model. + { + std::map seen; + for (const auto& row : r.rows) { + if (row.slot.empty() || seen.count(row.slot)) continue; + std::string sh = "["; + for (size_t i = 0; i < row.shape.size(); ++i) sh += (i ? "," : "") + std::to_string(row.shape[i]); + sh += "]"; + seen[row.slot] = sh + " " + row.dtype; + std::printf(" slot %-42s %s %s\n", row.slot.c_str(), sh.c_str(), row.dtype.c_str()); + } + } + // The rank and dtype are architecture-independent even though the extents are not: model.h documents the router as a + // 2-D BF16 tensor and the expert stacks as 3-D I8. A placement that reached the right NAME with the wrong shape would + // load a tensor the forward reads as something else, so these are asserted per placed row on the real container. + for (const auto& row : r.rows) { + if (row.slot.empty()) continue; + if (row.slot == "moe_router.weight") { + check(row.shape.size() == 2, "placement: the router is 2-D in " + dir); + check(row.dtype == "BF16", "placement: and BF16 in " + dir); + } + if (row.slot.find("exps_proj") != std::string::npos) { + check(row.shape.size() == 3, "placement: an expert stack is 3-D (" + row.slot + ") in " + dir); + check(row.dtype == "I8", "placement: and I8 (" + row.slot + ") in " + dir); + } + } std::printf(" roles:"); for (const auto& kv : roles_seen) std::printf(" %s=%lld", kv.first.c_str(), (long long)kv.second); std::printf("\n"); std::printf(" UNKNOWN:"); From 42d0e4d4e28fd8eb8c61d7e20f98f2a07fe5180c Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 14:03:09 -0400 Subject: [PATCH 09/13] npu-pack: layer-qualify the destination, using the container own prefix (engine#1788, 3c) The collision check -- run before it was written as an arm -- said the placement was not writable: 733 tensors pointed at 26 destinations, because every slot was a role-shaped name with no layer. The engine looks up "model.layer{s}.%d.", so the destination is the source tensor's own prefix through the layer digits plus the slot. Using the SOURCE's prefix rather than choosing a dialect is the point: model.c probes both "model.layers.%d." and "model.layer.%d." because MiniCPM spells it singular, so a placement that echoes the container cannot disagree with it. model.layer.0.input_layernorm.weight [2048] BF16 model.layer.0.linear_attn.qkv_proj.weight [256,8,4736] I8 model.layer.0.linear_attn.ssm_a [32] F32 MiniCPM 35B and 1B and gemma3 now have a UNIQUE destination per placed tensor. Two containers still do not, and the collision check named exactly which: gemma4/bolt: model.embed_tokens.weight <- model.embed_tokens.weight + model.embed_tokens.weight.scale model.norm.weight <- model.norm.weight + model.per_layer_proj_norm.weight Both are mapping questions, not layer artifacts, and both are the silent kind: a .scale companion is not the embedding, and per_layer_proj_norm has its own slot in the engine. Deliberately left for the next commit with the check that names them, rather than shipping a red arm here. --- npu/pack_import.cpp | 7 +++++++ tests/npu_pack_import_test.cpp | 8 ++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index 521dc9f23..8b43b28b1 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -3479,6 +3479,7 @@ PlacementReport container_placement(const std::vector& tensors, for (const auto& t : tensors) { PlacementRow row; row.tensor = t.name; + std::string layer_prefix; row.shape = t.shape; row.dtype = t.dtype; const auto it = roles.role_of.find(t.name); @@ -3494,6 +3495,7 @@ PlacementReport container_placement(const std::vector& tensors, if (end > start) { const int64_t layer = std::atoll(t.name.substr(start, end - start).c_str()); row.layer = std::to_string(layer); + layer_prefix = t.name.substr(0, end) + "."; if (layer >= 0 && layer < int64_t(layer_types.size())) row.layer_type = layer_types[size_t(layer)]; } } @@ -3501,6 +3503,11 @@ PlacementReport container_placement(const std::vector& tensors, // a group role carries its destination in the tensor's own suffix -- the fourth key, applied where the engine // actually has a slot for one of the group's members if (row.slot.empty() && !row.layer_type.empty()) row.slot = group_suffix_slot(row.role, t.name); + // engine#1788, 3c: a per-layer slot must be LAYER-QUALIFIED or every layer collides on one destination (the + // collision check found 733 tensors pointing at 26 names). The prefix is the CONTAINER's own spelling through + // the layer digits, because model.c probes both -- "model.layers.%d.…" and "model.layer.%d.…" -- and MiniCPM + // uses the singular. Using the source's own prefix keeps the two agree without this code choosing a dialect. + if (!row.slot.empty() && !row.layer.empty()) row.slot = layer_prefix + row.slot; if (row.slot.empty()) { const auto why = table.why.find(row.role); row.why = why != table.why.end() ? why->second diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index d0125391a..30c96aa17 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5296,10 +5296,10 @@ void test_container_placement() { "placement: placed + unplaced + no-role accounts for every tensor"); check(r.rows[0].slot == "model.embed_tokens.weight", "placement: a tensor with no layer still gets a slot"); check(r.rows[0].layer.empty() && r.rows[0].layer_type.empty(), "placement: and reports that it has no layer"); - check(r.rows[1].slot == "linear_attn.qkv_proj.weight", "placement: the fused qkv on the LINEAR layer"); + check(r.rows[1].slot == "model.layers.0.linear_attn.qkv_proj.weight", "placement: the fused qkv on the LINEAR layer, layer-qualified"); check(r.rows[2].slot.empty(), "placement: the same role on the FULL-attention layer has no destination"); check(r.rows[2].why.size() > 10, "placement: and says why"); - check(r.rows[3].slot == "self_attn.q_proj.weight", "placement: the query projection is layer-type-independent"); + check(r.rows[3].slot == "model.layers.1.self_attn.q_proj.weight", "placement: the query projection is layer-type-independent, layer-qualified"); check(r.rows[4].slot.empty() && r.rows[4].role == "gate_up_proj", "placement: gate_up_proj is still refused"); check(r.rows[5].slot.empty() && r.rows[5].role.empty(), "placement: a tensor with no role is counted apart"); check(r.no_role == 1, "placement: no_role counts the vocabulary gap, not the deliberate refusal"); @@ -5329,8 +5329,8 @@ void test_container_placement() { }; const pi::PlacementReport gr = pi::container_placement(g, lt); check(gr.rows[0].role == "gemma_local", "placement: the gemma group is one role"); - check(gr.rows[0].slot == "pre_feedforward_layernorm.weight", "placement: the suffix IS the destination (pre)"); - check(gr.rows[1].slot == "post_feedforward_layernorm.weight", "placement: and for post"); + check(gr.rows[0].slot == "model.layers.0.pre_feedforward_layernorm.weight", "placement: the suffix IS the destination (pre), layer-qualified"); + check(gr.rows[1].slot == "model.layers.0.post_feedforward_layernorm.weight", "placement: and for post, layer-qualified"); check(gr.rows[2].slot.empty(), "placement: a member with no engine slot stays unplaced"); check(gr.placed == 2 && gr.unplaced == 1, "placement: two placed, one held, for the three members"); } From 71377eb55a2f541a8df080f20955c694760a4917 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 14:14:54 -0400 Subject: [PATCH 10/13] npu-pack: a .scale companion is not its weight, and per_layer_proj_norm is not the final norm (3c, engine#1788) Both found by the collision check naming two source tensors at one destination on gemma4/bolt: model.embed_tokens.weight <- model.embed_tokens.weight + model.embed_tokens.weight.scale model.norm.weight <- model.norm.weight + model.per_layer_proj_norm.weight The engine reads BOTH .scale names as literals of their own, so a scale companion gets role weight_scale and an IDENTITY destination (its own name), which is what the forward looks up. per_layer_proj_norm gets its own role and its own top-level slot; it was landing on the model final norm only because the general norm rule ran first. The .scale rule is placed FIRST in the classifier, before the needle tables, because that is the same ordering defect as attn_q versus attn_q_norm (engine#2097): the general rule was matching model.embed_tokens.weight.scale by substring before anything could ask whether it was a companion. gemma4, bolt and the 35B now have a unique destination per placed tensor. The 1B does not, and the check named why: it carries PAIRED dtype variants -- linear_attn.ssm_alpha_proj.bf16.weight beside linear_attn.ssm_alpha_proj.weight -- so both reach one slot. That is a source-format question (which of the pair the forward wants), not a role question, and it is the next commit rather than a guess here. --- npu/pack_import.cpp | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index 8b43b28b1..acdacd149 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -3507,7 +3507,8 @@ PlacementReport container_placement(const std::vector& tensors, // collision check found 733 tensors pointing at 26 names). The prefix is the CONTAINER's own spelling through // the layer digits, because model.c probes both -- "model.layers.%d.…" and "model.layer.%d.…" -- and MiniCPM // uses the singular. Using the source's own prefix keeps the two agree without this code choosing a dialect. - if (!row.slot.empty() && !row.layer.empty()) row.slot = layer_prefix + row.slot; + if (row.role == "weight_scale") row.slot = t.name; // a scale companion goes to its OWN name + else if (!row.slot.empty() && !row.layer.empty()) row.slot = layer_prefix + row.slot; if (row.slot.empty()) { const auto why = table.why.find(row.role); row.why = why != table.why.end() ? why->second @@ -3594,6 +3595,7 @@ static const struct { const char* role; const char* slot; } kRoleSlots[] = { {"up_exps", "mlp.up_exps_proj.weight"}, {"down_exps", "mlp.down_exps_proj.weight"}, {"shortconv_in_proj", "shortconv.in_proj.weight"}, + {"per_layer_proj_norm","model.per_layer_proj_norm.weight"}, // engine#1788, 3c: I refused these two for two PRs on the strength of a grep that found only the rmsnorm debug LABEL // ("q_norm"), and called them config fields. They are weight slots, and model.c names them explicitly: // "model.layer{s}.%d.self_attn.q_norm.weight" / "… .k_norm.weight" (singular and plural both probed) @@ -3740,6 +3742,11 @@ ContainerRoles container_roles(const std::vector& tensors) { role = "vision"; else if (n.find("audio.") != std::string::npos) role = "audio"; + // engine#1788, 3c: FIRST, before any needle table -- a `.scale` companion must never be filed as the weight it + // accompanies, and the `embed` rule matches `model.embed_tokens.weight.scale` by substring. This is the same + // ordering defect as `attn_q` vs `attn_q_norm` (engine#2097): the general rule ran before the specific one. + else if (n.size() > 6 && n.compare(n.size() - 6, 6, ".scale") == 0) + role = "weight_scale"; if (role.empty()) { bool matched = false; for (const auto& r : kRules) { @@ -3751,6 +3758,13 @@ ContainerRoles container_roles(const std::vector& tensors) { // "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). + // engine#1788, 3c: a `.scale` companion is NOT the weight it accompanies, and the engine reads it by its own + // name -- "model.embed_tokens.weight.scale" is a literal in npu/forward. Filed as `embed` it would be written + // over the embedding, or its scales where the embedding belongs, with no error. Matched first. + // and `per_layer_proj_norm` has its own top-level slot; it is not the model's final norm. + if (!matched && role.empty() && n.find("per_layer_proj_norm") != std::string::npos) { + role = "per_layer_proj_norm"; matched = true; + } 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 From 20fe08c172fddce1cf49d5df41fcbbec89d3f9e4 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 14:21:35 -0400 Subject: [PATCH 11/13] npu-pack: dtype-tagged copies are refused, and every destination is now unique on all five containers (3c) The 1B ships each ssm weight twice, once with a .bf16. marker, and both reached one slot. The engine has no .bf16. literal at all, so that copy has no destination and must not reach the untagged one: it becomes role dtype_variant, refused with that reason, from the same first position in the classifier as .scale (before every needle table). And the collision check, which caught the placement being non-writable and then all three mapping classes, now runs as an arm on every real container: placement: every placed tensor has its own destination in Measured, put / held / unnamed, and the check passing on all five: MiniCPM 35B 733 / 0 / 0 gemma3-270m 237 / 0 / 0 MiniCPM 1B 321 / 36 / 0 gemma4-e2b 461 / 248 / 0 bolt-e4b 552 / 297 / 0 The 1B held count went UP by 36 and that is the fix: 36 .bf16. copies that were silently overwriting their untagged siblings are now named and refused. The placement is layer-resolved, dialect-correct, rank/dtype-verified and collision-free -- which is the minimum for writing bytes without loading one tensor over another. --- npu/pack_import.cpp | 6 ++++++ tests/npu_pack_import_test.cpp | 16 +++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index acdacd149..791cfca45 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -3626,6 +3626,7 @@ static const struct { const char* role; const char* why; } kRoleNoSlot[] = { {"gemma_local", "a group: pre/post_feedforward_layernorm have engine slots, inp_gate / layer_output_scale / per_layer_projection / post_layernorm / per_layer_model_proj do not -- separating them needs the suffix key"}, // Measured on gemma4/bolt. These ARE recognised and deliberately have no destination: `rope_freqs.weight` is a file the // forward only FLAGS (model_config.h: has_rope_freqs_file) and the per-layer embeddings have no slot of their own. + {"dtype_variant", "a dtype-tagged (`.bf16.`) copy of a weight the container also ships untagged; the engine has no `.bf16.` literal, so this copy has no destination and must not reach the untagged slot"}, {"container_extra", "recognised so it is not a gap, and deliberately not written: a table the forward only flags (rope_freqs) or a per-layer embedding with no slot"}, }; @@ -3747,6 +3748,11 @@ ContainerRoles container_roles(const std::vector& tensors) { // ordering defect as `attn_q` vs `attn_q_norm` (engine#2097): the general rule ran before the specific one. else if (n.size() > 6 && n.compare(n.size() - 6, 6, ".scale") == 0) role = "weight_scale"; + // engine#1788, 3c: a dtype-TAGGED companion (a `.bf16.` copy of a weight the engine also ships untagged). Same + // first-position rule as `.scale`, and the opposite outcome: the engine has no `.bf16.` literal at all, so this + // copy has no destination and must not reach the untagged slot -- which is what the 1B collision showed. + else if (n.find(".bf16.") != std::string::npos) + role = "dtype_variant"; if (role.empty()) { bool matched = false; for (const auto& r : kRules) { diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index 30c96aa17..a7aaa5532 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5196,6 +5196,20 @@ static void test_container_placement_real(const std::string& dir, const std::vec for (const auto& row : r.rows) if (!row.role.empty()) roles_seen[row.role]++; // REVIEW of #2291 direction: the engine documents a rank and a dtype per slot (model.h), and both are // architecture-independent -- a router that is 3-D or an expert that is BF16 is wrong for every model. + { + // Two source tensors reaching one destination is silent data loss: the pack carries one, the forward reads the + // other. This check is what caught the placement being non-writable (733 tensors / 26 names) and then all three + // mapping classes: a `.scale` companion filed as its weight, per_layer_proj_norm filed as the final norm, and the + // 1B's `.bf16.` copies reaching the untagged slot. + std::map per_slot; + for (const auto& row : r.rows) if (!row.slot.empty()) per_slot[row.slot]++; + std::vector dup; + for (const auto& kv : per_slot) if (kv.second > 1) dup.push_back(kv.first); + std::string dl; + for (const auto& d : dup) dl += d + " "; + check(per_slot.size() == size_t(r.placed), + "placement: every placed tensor has its own destination in " + dir + " (" + dl + ")"); + } { std::map seen; for (const auto& row : r.rows) { @@ -5309,7 +5323,7 @@ void test_container_placement() { // pack. A role that reaches the "this table does not know" default, or a reason that is a bare placeholder, is a // hole -- and this arm is what makes removing a refusal entry fail in CI rather than print UNKNOWN on the box. pi::ContainerRoles roles_c; - for (const char* role : {"ssm", "gemma_local", "container_extra", "gate_up_proj"}) + for (const char* role : {"ssm", "gemma_local", "container_extra", "gate_up_proj", "dtype_variant"}) roles_c.counts[role] = 1; const pi::RoleSlotReport slots_c = pi::container_role_slots(roles_c); for (const auto& kv : slots_c.why) { From 21786bf095ff18a73458a778b116f153261f4cd6 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 14:52:28 -0400 Subject: [PATCH 12/13] tests: arm the .scale rule and its ORDERING, and per_layer_proj_norm (review of #2291) The review's mutant was exact: deleting the .scale branch left every arm green, even though its own comment says the cost is a pack that writes scales over the embedding with no error. Four arms now, and the third is the one that also fails if the branch is refactored INTO the needle loop -- which is the change that would restore the defect silently: 1. container_roles: the .scale companion is weight_scale, not embed 2. its destination is its own name (model.embed_tokens.weight.scale), which q4nx_forward reads 3. a per-layer companion takes its own name, so it cannot land in the down_proj slot 4. per_layer_proj_norm is not the final norm and has its own top-level slot The mutant (delete the branch) now fails four arms: FAIL scale: the .scale companion is NOT filed as embed FAIL scale: its destination is its OWN name, which q4nx_forward reads FAIL scale: and it does not collide with the embedding FAIL scale: a per-layer companion is weight_scale too --- tests/npu_pack_import_test.cpp | 47 ++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index a7aaa5532..114c56776 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5291,6 +5291,52 @@ static void test_role_tables_disagree_nowhere() { "tables: and they are exactly the pinned five, so a new refusal cannot slip in unnoticed"); } +// REVIEW of #2291: the `.scale` branch and its ORDERING were both unarmed -- deleting the branch left every arm green, +// even though its own comment says the cost is a pack that writes scales over the embedding with no error. These arms +// fail if the branch is deleted AND if it is moved inside the needle loop, which is the refactor that would restore the +// defect silently. +static void test_scale_and_proj_norm_roles() { + using pi::Q4nxTensor; + const auto t = [](const std::string& n) { Q4nxTensor x; x.name = n; x.dtype = "BF16"; return x; }; + + // 1. the classifier: a .scale companion is its own role, not the weight it accompanies + { + const std::vector v = {t("model.embed_tokens.weight"), t("model.embed_tokens.weight.scale")}; + const pi::ContainerRoles r = pi::container_roles(v); + check(r.role_of.at("model.embed_tokens.weight") == "embed", "scale: the embedding is still the embedding"); + check(r.role_of.at("model.embed_tokens.weight.scale") == "weight_scale", + "scale: the .scale companion is NOT filed as embed"); + } + // 2. the destination: identity, and it is a literal the forward reads + { + const std::vector v = {t("model.embed_tokens.weight"), t("model.embed_tokens.weight.scale")}; + const pi::PlacementReport p = pi::container_placement(v, {}); + check(p.rows[1].slot == "model.embed_tokens.weight.scale", + "scale: its destination is its OWN name, which q4nx_forward reads"); + check(p.rows[0].slot != p.rows[1].slot, "scale: and it does not collide with the embedding"); + } + // 3. the ordering, as a negative: nothing ending .scale may be placed under a WEIGHT role's slot + { + const std::vector v = {t("model.layers.0.mlp.down_proj.weight"), + t("model.layers.0.mlp.down_proj.weight.scale")}; + const pi::PlacementReport p = pi::container_placement(v, {"full_attention"}); + check(p.rows[1].role == "weight_scale", "scale: a per-layer companion is weight_scale too"); + check(p.rows[1].slot == "model.layers.0.mlp.down_proj.weight.scale", + "scale: identity, so it cannot land in the down_proj slot"); + check(p.rows[0].slot == "model.layers.0.mlp.down_proj.weight", "scale: and the weight keeps its own slot"); + } + // 4. per_layer_proj_norm is not the model's final norm (same class, one level simpler) + { + const std::vector v = {t("model.norm.weight"), t("model.per_layer_proj_norm.weight")}; + const pi::ContainerRoles r = pi::container_roles(v); + check(r.role_of.at("model.norm.weight") == "final_norm", "proj_norm: model.norm is the final norm"); + check(r.role_of.at("model.per_layer_proj_norm.weight") == "per_layer_proj_norm", + "proj_norm: per_layer_proj_norm is NOT the final norm"); + const pi::PlacementReport p = pi::container_placement(v, {}); + check(p.rows[1].slot == "model.per_layer_proj_norm.weight", "proj_norm: and it has its own top-level slot"); + } +} + void test_container_placement() { using pi::Q4nxTensor; const auto t = [](const std::string& n) { Q4nxTensor x; x.name = n; x.dtype = "I8"; return x; }; @@ -6533,6 +6579,7 @@ int main(int argc, char** argv) { test_container_roles_gemma_local(); test_container_roles_moe_and_singular_layer(); test_container_placement(); + test_scale_and_proj_norm_roles(); test_role_tables_disagree_nowhere(); if (argc > 4) { std::vector lt; From 6db001b40fb8229e8a66ac43d2f798b215e8e07a Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 15:13:11 -0400 Subject: [PATCH 13/13] tests: pin every layer-independent destination, because CI runs with no pack (self-review of #2291) The reviews of this PR kept finding the same class -- a rule whose deletion is invisible -- and applying that to my own leg found it again: the real-container leg arms the destination table only when a pack is given, and ctest runs this binary with NO pack. So a wrong destination for any single role was invisible in CI. Changing one character of o_proj's slot now fails: FAIL tables: o_proj -> self_attn.o_proj.weight (got: self_attn.o_proj.weight ) All 25 layer-type-independent destinations are pinned exactly, with the value printed on failure. --- tests/npu_pack_import_test.cpp | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index 114c56776..d622252a4 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5286,6 +5286,31 @@ static void test_role_tables_disagree_nowhere() { // refusal plus per-tensor placement are two different answers, and both are correct. { std::string list; for (const auto& r : s.no_slot) list += r + " "; check(s.no_slot.size() == 5, "tables: five roles are refused at role granularity (got: " + list + ")"); } + // REVIEW of #2291, applied to myself: the real-container leg arms the table only where a pack is given, and CI runs + // with NO pack -- so before this, a wrong destination for any single role (o_proj, up_exps, ssm_conv1d …) was + // invisible in CI. Every layer-type-independent destination is pinned here, exactly. + { + const std::map want = { + {"embed", "model.embed_tokens.weight"}, {"final_norm", "model.norm.weight"}, + {"input_norm", "input_layernorm.weight"}, {"post_norm", "post_attention_layernorm.weight"}, + {"lm_head", "lm_head.weight"}, {"q_proj", "self_attn.q_proj.weight"}, + {"k_proj", "self_attn.k_proj.weight"}, {"v_proj", "self_attn.v_proj.weight"}, + {"o_proj", "self_attn.o_proj.weight"}, {"q_norm", "self_attn.q_norm.weight"}, + {"k_norm", "self_attn.k_norm.weight"}, {"self_attn_gate_proj", "self_attn.gate_proj.weight"}, + {"gate_proj", "mlp.gate_proj.weight"}, {"up_proj", "mlp.up_proj.weight"}, + {"down_proj", "mlp.down_proj.weight"}, {"gate_exps", "mlp.gate_exps_proj.weight"}, + {"up_exps", "mlp.up_exps_proj.weight"}, {"down_exps", "mlp.down_exps_proj.weight"}, + {"shortconv_in_proj", "shortconv.in_proj.weight"}, {"per_layer_proj_norm", "model.per_layer_proj_norm.weight"}, + {"moe_router", "moe_router.weight"}, {"share_gate_exps", "mlp.share_gate_exps_proj.weight"}, + {"share_up_exps", "mlp.share_up_exps_proj.weight"}, {"share_down_exps", "mlp.share_down_exps_proj.weight"}, + {"shared_expert_gate", "mlp.share_router.weight"}, + }; + for (const auto& kv : want) { + check(s.slot_of.count(kv.first) && s.slot_of.at(kv.first) == kv.second, + "tables: " + kv.first + " -> " + kv.second + " (got: " + + (s.slot_of.count(kv.first) ? s.slot_of.at(kv.first) : std::string("")) + ")"); + } + } check(s.no_slot[0] == "container_extra" && s.no_slot[1] == "gate_up_proj" && s.no_slot[2] == "gemma_local" && s.no_slot[3] == "qkv_proj" && s.no_slot[4] == "ssm", "tables: and they are exactly the pinned five, so a new refusal cannot slip in unnoticed");