From 5c02cad2a84842d9d1f85f4e55f9f3c1440ba59a Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 07:46:09 -0400 Subject: [PATCH 1/2] npu-pack: the classifier fixes and the container import refusal, on main (engine#1788) The behaviour half of my #2187, ported to `main` because it does not need slice 3b: #2270 already landed on main from a main base, and this is the same move -- the parts that classify and refuse do not touch the plan fields 3b adds. - `final_norm`'s guard tested `layers.` and MiniCPM-V spells its blocks `layer.N`, so 30 per-layer linear_attn.ssm_norm tensors were filed as the model norm. - `self_attn.gate_proj` is an ATTENTION output gate, and it was filed as the dense `gate_proj` (name precedent: model.h's `self_attn_gate_proj_weight`). - the eight MoE families (share_* first), mapped to the fields model.h already reads. - gemma3/gemma4/bolt recognition: `pre_/post_feedforward_layernorm`, `inp_gate`, `per_layer_projection`, `layer_output_scale`, `post_layernorm`, `per_layer_model_proj` (role `gemma_local` -- no model.h field exists for them) and `rope_freqs`/`per_layer_token_embd` (role `container_extra`, known for a different reason). Measured: gemma3 36 -> 0 unassigned, gemma4 319 -> 0, bolt 382 -> 0, MiniCPM-V unchanged at 0. - `npu-pack import` on a container pack now refuses BY NAME ("733 tensor(s); importing it needs the container-genre writer (engine#1786, item F1)") instead of falling through to the ONNX importer's "cannot open model.onnx". Arms: the attention gate vs the FFN gate, the eight MoE families with the shared spelling not double-counting, the five gemma families recognised, and that a norm the forward reads is not swallowed by the recognisers. --- npu/pack_import.cpp | 62 ++++++++++++++++++++++++++++++-- tests/npu_pack_import_test.cpp | 66 ++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 2 deletions(-) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index 362dcc5bb..80f31d3cd 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -3507,9 +3507,28 @@ ContainerRoles container_roles(const std::vector& tensors) { {"k_proj", "k_proj"}, {"k_proj", "attn_k"}, {"k_proj", "wk."}, {"v_proj", "v_proj"}, {"v_proj", "attn_v"}, {"v_proj", "wv."}, {"o_proj", "o_proj"}, {"o_proj", "attn_output"}, {"o_proj", "wo."}, + // engine#1788: an ATTENTION output gate is not the FFN gate. MiniCPM-V 4.7 carries `attn_output_gate: true`, + // so every one of its 30 `self_attn.gate_proj.weight` tensors was filed under `gate_proj` -- the dense-MLP role + // -- and a consumer would have wired an attention gate into the FFN path. The forward already keeps them apart + // (model.h: `gate_proj_weight` vs `self_attn_gate_proj_weight`), so this uses its name and must precede the + // `gate_proj` needle. + {"self_attn_gate_proj", "self_attn.gate_proj"}, {"gate_proj", "gate_proj"}, {"gate_proj", "ffn_gate"}, {"up_proj", "up_proj"}, {"up_proj", "ffn_up"}, - {"down_proj", "down_proj"}, {"down_proj", "ffn_down"}, + {"down_proj", "down_proj"}, {"down_proj", "ffn_down"}, // engine#1788 acceptance (F1 slice 3a gap): the MoE families MiniCPM-V 4.7 names, mapped to the roles the NPU + // forward ALREADY reads -- npu/forward/include/model.h carries up_exps_weight / gate_exps_weight / + // down_exps_weight / share_{up,gate,down}_exps_weight / moe_router_weight / shared_expert_gate_weight with + // those exact container names in their comments. So this is translation to an existing vocabulary, not a new + // one. The `share_*` spellings come first because `share_gate_exps_proj` contains `gate_exps_proj`. + {"share_gate_exps", "share_gate_exps_proj"}, + {"share_up_exps", "share_up_exps_proj"}, + {"share_down_exps", "share_down_exps_proj"}, + {"gate_exps", "gate_exps_proj"}, + {"up_exps", "up_exps_proj"}, + {"down_exps", "down_exps_proj"}, + {"moe_router", "moe_router"}, + {"shared_expert_gate","shared_expert_gate"}, + {"input_norm", "input_layernorm"},{"input_norm", "attn_norm"}, {"input_norm", "attention_norm"}, {"post_norm", "post_attention_layernorm"}, {"post_norm", "ffn_norm"}, {"final_norm", "final_layernorm"}, @@ -3519,6 +3538,24 @@ ContainerRoles container_roles(const std::vector& tensors) { // and `model.mtp.*` is a draft head rather than a model. static const char* kSsm[] = {"linear_attn.", "ssm_alpha_proj", "ssm_beta_proj", "ssm_conv1d", "ssm_dt", "ssm_norm", "ssm_out_proj"}; + // engine#1788: gemma3/gemma4/bolt carry five tensor families this engine's forward does NOT read -- sandwich norms + // (`pre_/post_feedforward_layernorm`), an input gate, a per-layer projection (and its `_prefill` sibling) and a layer + // output scale. Nothing here has a field in npu/forward/include/model.h, so they are RECOGNISED rather than mapped: + // `unassigned` has to keep meaning "nothing knows this", and these are known -- measured on gemma4-e2b (319 of 709) + // and bolt-e4b (382 of 849), which were the two packs the classifier could not finish. + // `post_layernorm` and `per_layer_model_proj` are here for a measured reason, not a guess: gemma4-e2b carries + // BOTH `post_attention_layernorm` (35, classified `post_norm`) and `post_layernorm` (35), so the second cannot be + // the forward's post-attention norm -- they are different tensors in the same model. gemma3-270m has no + // `post_layernorm` at all, which is why only gemma4/bolt were left with it. + static const char* kGemmaLocal[] = {"pre_feedforward_layernorm", "post_feedforward_layernorm", "inp_gate", + "layer_output_scale", "per_layer_projection", "post_layernorm", + "per_layer_model_proj"}; + // engine#1788: the last three on gemma4/bolt are recognised for a different reason than the gemma ones, so they get + // their own role. `rope_freqs.weight` is a file the forward only FLAGS (`model_config.h`: has_rope_freqs_file), and + // `per_layer_token_embd` (with its `.scale` sibling) is the per-layer token embedding that our ONNX path reads as + // `embed_tokens_per_layer` (engine#1725) but the container path has no field for. Recognised, not mapped: neither + // has a field to fill, and calling them "unassigned" would say nothing knows them when the opposite is true. + static const char* kContainerExtras[] = {"rope_freqs", "per_layer_token_embd"}; static const char* kMtp[] = {"model.mtp."}; ContainerRoles out; for (const auto& t : tensors) { @@ -3536,8 +3573,20 @@ ContainerRoles container_roles(const std::vector& tensors) { matched = true; break; } - if (!matched && n.find("norm.weight") != std::string::npos && n.find("layers.") == std::string::npos) + // "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). + 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 + if (!matched && role.empty()) { + for (const char* e : kContainerExtras) + if (n.find(e) != std::string::npos) { role = "container_extra"; matched = true; break; } + } + if (!matched && role.empty()) { + for (const char* g : kGemmaLocal) + if (n.find(g) != std::string::npos) { role = "gemma_local"; matched = true; break; } + } if (!matched && role.empty()) { for (const char* s : kSsm) if (n.find(s) != std::string::npos) { role = "ssm"; matched = true; break; } @@ -4756,6 +4805,15 @@ Report import_pack(const std::string& pack_dir, const std::string& out_dir, cons // Census yes, writer no (see the note where graph_weights is decided): this genre's weights are 1-D external-data // blocks, and until the repacker exists an import would write a container nothing can read. Refused here rather // than in plan(), because `info` must keep reporting the genre it can now see. + // engine#1786 (item F1) / engine#1788: a container-genre pack has no `model.onnx` by design, so without a branch here + // the first byte fails with "cannot open /model.onnx" -- a missing-file message about a pack that has no graph + // at all. Refused by name, the same way the external-data genre below is, until the container-genre writer exists. + if (p.container_tensors >= 0) { + throw std::runtime_error( + "this pack is a safetensors container (model.q4nx): " + std::to_string(p.container_tensors) + + " tensor(s); importing it needs the container-genre writer (engine#1786, item F1), which the importer does " + "not have yet. `1bit npu-pack info` reports what the container holds"); + } if (p.graph_packed_siblings > 0) { // The genre has a writer now (engine#1503): decode -> convert -> split by role -> the same I8 tile chunks, // with its own function because import_pack is built around the .fconst envelope. diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index 6a62ba855..227d98bc5 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5177,6 +5177,70 @@ void test_container_role_layer_types() { "role+layer: a layer index past layer_types is counted as out of range"); } +void test_container_roles_moe_and_singular_layer() { + const auto t = [](const std::string& n) { pi::Q4nxTensor x; x.name = n; x.dtype = "I8"; return x; }; + const auto roles = [&](std::initializer_list names) { + std::vector v; + for (const char* n : names) v.push_back(t(n)); + return pi::container_roles(v); + }; + + // 1. the eight MoE families, spelled as the container spells them + { + const auto r = roles({"model.layer.0.moe_router.weight", + "model.layer.0.shared_expert_gate.weight", + "model.layer.0.mlp.gate_exps_proj.weight", + "model.layer.0.mlp.up_exps_proj.weight", + "model.layer.0.mlp.down_exps_proj.weight", + "model.layer.0.mlp.share_gate_exps_proj.weight", + "model.layer.0.mlp.share_up_exps_proj.weight", + "model.layer.0.mlp.share_down_exps_proj.weight"}); + check(r.unassigned.empty(), "35b: the MoE families are named, not unassigned"); + check(r.role_of.at("model.layer.0.moe_router.weight") == "moe_router", "35b: moe_router"); + check(r.role_of.at("model.layer.0.shared_expert_gate.weight") == "shared_expert_gate", + "35b: shared_expert_gate is its own role, not a projection"); + check(r.role_of.at("model.layer.0.mlp.gate_exps_proj.weight") == "gate_exps", "35b: gate_exps_proj"); + check(r.role_of.at("model.layer.0.mlp.share_gate_exps_proj.weight") == "share_gate_exps", + "35b: share_gate_exps_proj is the shared expert, not the routed gate"); + check(r.counts.at("gate_exps") == 1 && r.counts.at("share_gate_exps") == 1, + "35b: the shared spelling does not also count as the routed one"); + } + + // 2. a per-layer norm under the singular block spelling stays a layer norm: it must NOT become final_norm + { + const auto r = roles({"model.norm.weight", "model.layer.0.linear_attn.ssm_norm.weight", + "model.layer.0.input_layernorm.weight"}); + check(r.role_of.at("model.norm.weight") == "final_norm", "35b: the top-level norm is final_norm"); + check(r.counts.at("final_norm") == 1, "35b: exactly one final_norm, even with `model.layer.N.` blocks"); + check(r.role_of.at("model.layer.0.linear_attn.ssm_norm.weight") == "ssm", + "35b: a per-layer ssm_norm is ssm, which is what kSsm says it is"); + } +} + +void test_container_roles_gemma_local() { + const auto t = [](const std::string& n) { pi::Q4nxTensor x; x.name = n; x.dtype = "BF16"; return x; }; + const auto roles = [&](std::initializer_list names) { + std::vector v; + for (const char* n : names) v.push_back(t(n)); + return pi::container_roles(v); + }; + { + const auto r = roles({"model.layers.0.pre_feedforward_layernorm.weight", + "model.layers.0.inp_gate.weight", "model.layers.0.inp_gate.weight_prefill", + "model.layers.0.per_layer_projection.weight", "model.layers.0.layer_output_scale.weight"}); + check(r.unassigned.empty(), "gemma local: the families this forward does not read are recognised"); + check(r.counts.at("gemma_local") == 5, "gemma local: all five land on one role"); + } + { + // the forward's own norms must still win: this is the shadowing the ordering could cause + const auto r = roles({"model.layers.0.post_attention_layernorm.weight", + "model.layers.0.input_layernorm.weight"}); + check(r.role_of.at("model.layers.0.post_attention_layernorm.weight") == "post_norm" && + r.role_of.at("model.layers.0.input_layernorm.weight") == "input_norm", + "gemma local: a norm the forward reads is not swallowed by the recogniser"); + } +} + void test_container_roles() { const auto t = [](const std::string& n) { pi::Q4nxTensor x; x.name = n; x.dtype = "I8"; return x; }; const auto roles = [&](std::initializer_list names) { @@ -6218,6 +6282,8 @@ int main(int argc, char** argv) { test_q4nx_container(golden_dir.empty() ? "." : golden_dir); test_container_geometry(); test_container_roles(); + test_container_roles_gemma_local(); + test_container_roles_moe_and_singular_layer(); test_container_role_layer_types(); test_container_plan(golden_dir.empty() ? "." : golden_dir); test_hybrid_pack_import(golden_dir.empty() ? "." : golden_dir); From a05c4680925e3f6c64df17bf0bfd5c2981bbcf53 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 08:34:40 -0400 Subject: [PATCH 2/2] tests: an arm for the attention gate, so removing the rule is red (engine#2277 review) The review's mutant was right: the body listed an attention-gate arm that the diff did not contain, so deleting {self_attn_gate_proj, "self_attn.gate_proj"} left the suite green while MiniCPM-V's 30 attention gates went back to being the dense FFN gate. Now: self_attn.gate_proj.weight -> self_attn_gate_proj, mlp.gate_proj.weight -> gate_proj, and counts.at("gate_proj") == 1; removing the rule fails two of those three. --- tests/npu_pack_import_test.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index 227d98bc5..182dbe612 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -5185,6 +5185,16 @@ void test_container_roles_moe_and_singular_layer() { return pi::container_roles(v); }; + // 1a. engine#2277 review: an ATTENTION output gate is not the FFN gate, and this is the arm that says so -- + // without the `self_attn_gate_proj` rule (main's behaviour) the first of these is filed as `gate_proj` and the + // suite stayed green, which is what the review found. + { + const auto r = roles({"model.layer.0.self_attn.gate_proj.weight", "model.layer.0.mlp.gate_proj.weight"}); + check(r.role_of.at("model.layer.0.self_attn.gate_proj.weight") == "self_attn_gate_proj", + "35b: an attention output gate is not the FFN gate"); + check(r.role_of.at("model.layer.0.mlp.gate_proj.weight") == "gate_proj", "35b: the FFN gate is still gate_proj"); + check(r.counts.at("gate_proj") == 1, "35b: the attention gate does not also count as gate_proj"); + } // 1. the eight MoE families, spelled as the container spells them { const auto r = roles({"model.layer.0.moe_router.weight",