[bong-water-water-bong]

Refs #1788 (F6, MiniCPM-V 4.7). The issue stays open.

The fault

q4nx_semantics turns per-head q/k RMSNorm on from config.json's model_type: on for qwen3* and gemma*, off otherwise. FastFlowLM's MiniCPM-V 4.7 pack is model_type: minicpmv4_7 around a qwen3_5 text model, and its container ships self_attn.q_norm.weight and k_norm.weight in every full-attention layer (3, 7, 11, 15, 19, 23). The forward loaded that pack with:

[semantics] qk_norm=0  <- model_type/architectures (q_norm/k_norm tensors)

and ran those six layers without the norm. Nothing failed. Short prompts still answered (Paris, red), which is why my earlier rows on #1788 looked fine; longer prompts drifted.

How it was found

A sliced image through minicpm_image_probe was described wrongly, the same on the host arm and on the device, while the probe's inputs (token ids, positions, image rows) were identical to the reference pipeline's. So I ran the model's original weights in transformers on the same prompts, and compared the forward's next-token choice at every prompt row with the reference's: 65 % agreement, flat across positions (no step at any row count), and prefill chunking made no difference. A uniform error pointed at the layers, and the load log had the line above.

The fix

A layer that carries both q_norm and k_norm turns the norm on (q4nx_semantics_qk_norm_from_tensors), and the load prints which layer decided. It never turns the norm off: a family the table knows keeps its answer.

Measured on this box (MiniCPM-V-4.7-1B-NPU2, host arm, vs the original weights in transformers 5.22.2)

	before	after	reference
a 149-token story, "what is the name of Maria's cat? Answer with the name only."	The question mentions that Maria buys	`Pluto<	im_end
top-1 agreement with the reference over the prompt's 149 rows	65.1 %	75.2 %
rows where the forward's choice is outside the reference's top 10	6	0
unsliced red/blue image with a long question (178 prompt tokens)	The left side is blue, and the right side has a gradient from blue to blue.	The left side is red and the right side is blue.	The left side is red and the right is blue.
sliced 448 × 896 image: red left, green top right, blue bottom right	"three equal horizontal sections. The top and bottom sections are white, while the middle section is a solid red color"	"Left side (full vertical panel): … solid red. Top right section: … green. Bottom right section: … blue."	"Red: a large vertical red rectangle occupying the leftmost portion … Green: a small horizontal green rectangle in the upper right,"

The before column and the first sliced answer were also reproduced on the device earlier; the after column is host arm only. These runs need #2026 merged in for the pack to load, and used local diagnostic options on the probe that are not in this PR.

What else it changes

I listed every container on this box (59 with a config.json). 28 carry the tensor pair; 27 are a family the table already knows. The rule changes three:

• MiniCPM-V-4.7-1B-NPU2 and MiniCPM-V-4.7-35B-A3B-NPU2: intended. I did not rerun the 35B, which has other problems (see the issue).
• pk-lfm12 (model_type: lfm2): its container has the pair, so it now runs with the norm. That is the model's own behaviour: transformers' modeling_lfm2.py (5.22.2) builds q_layernorm and k_layernorm as RMSNorm over head_dim and applies them to the projected q and k before attention (lines 217–218, 231–232). So the importer writing the pair is right, and before this PR that pack was running without a norm its model has. I did not run that pack, so this is from the modelling code, not from a measurement.

Two containers are named as a family with the norm and carry no tensors (ctr-gemma-awq-b8, ctr-gemma22b-b8); they are untouched, since the rule never turns it off.

Test

npu_rmsnorm_offset (existing ctest row, plain C, no model): 9 new arms. The published pack's model_type alone gives 0 (the gap); the pair turns it on and names layer 3; already on changes nothing; no tensors, q alone, k alone, and a pair past the layer count all stay off; null input; and qwen3_5 stays on with no tensors.

$ ./npu_rmsnorm_offset_test
...
ok   minicpmv4_7: the name alone says no q/k norm (the gap)
ok   ... and a layer carrying q_norm and k_norm turns it on, naming layer 3
ok   q_norm without k_norm is not the pair: it stays off
ok   qwen3_5: on by name, and absent tensors never turn it off
npu_rmsnorm_offset: ok

Mutant: with "q_norm alone is enough", 3 arms fail.

The forward's own call is not covered by a ctest row (it needs a container); the device-free evidence for it is the table above.

Limits

• A quarter of the rows still disagree with the reference after the fix. That is a 4-bit container against float weights, and I have not separated quantisation from anything else.
• One model, one long text prompt for the agreement figure.

Docs: docs/npu.md, "A pack's vision component".

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 926c730

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b0a2a40)

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 answers a known-answer prompt through the model's own chat template, 3 identical requests returning identical text 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 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 Non-compliant requirements: (No requirements not fulfilled) Requires further human verification: (All requirements can be assessed through code review)

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Logic for q/k norm detection The logic for detecting whether q and k are normalized per head now relies on checking for the presence of both q_norm and k_norm tensors in the same layer. This is a good improvement, but it assumes that if both tensors exist, they are correctly paired. If a layer has q_norm but not k_norm, or vice versa, the norm will not be enabled, which could lead to incorrect behavior if the model is expected to use per-head normalization. if (mw_ && mw_->config.num_layers > 0) { std::vector<unsigned char> has_q(size_t(mw_->config.num_layers), 0), has_k(has_q); for (int l = 0; l < mw_->config.num_layers; l++) { has_q[size_t(l)] = mw_->layers[l].q_norm_weight.name[0] && mw_->layers[l].q_norm_weight.data_size > 0; has_k[size_t(l)] = mw_->layers[l].k_norm_weight.name[0] && mw_->layers[l].k_norm_weight.data_size > 0; } const int by = q4nx_semantics_qk_norm_from_tensors(&sem_, has_q.data(), has_k.data(), mw_->config.num_layers); if (by >= 0) fprintf(stderr, " [semantics] qk_norm=1 <- the container: layer %d carries self_attn.q_norm and k_norm " "(config.json's model_type is not a family the table knows)\n", by); } Potential for incorrect norm state The function q4nx_semantics_qk_norm_from_tensors only enables the q/k norm if a layer contains both q_norm and k_norm tensors. However, it does not validate that these tensors are actually used together in the model's computation. If a model has inconsistent tensor usage, this could lead to a misconfiguration where the norm is not applied when it should be, or vice versa. int q4nx_semantics_qk_norm_from_tensors(Q4nxSemantics* s, const unsigned char* has_q, const unsigned char* has_k, int layers) { if (!s || !has_q || !has_k || layers <= 0 || s->qk_norm) return -1; for (int l = 0; l < layers; l++) if (has_q[l] && has_k[l]) { s->qk_norm = 1; return l; } return -1; }

[bong-water-water-bong]

VERDICT at 926c730: CHANGES

Reviewed head 926c730 (refs #1788). This is the first verdict. The fix is right and well pinned. CI is red on both build and CodeQL, for one reason in the new test.

Blocking: the test creates its fixture files with fopen(path, "w"). CI's Test step has one failure out of 227, file_create_check, which is the ratchet #2057 put on main. CodeQL's two new alerts are the same two lines:

tests/npu_rmsnorm_offset_test.c:123: fopen for writing creates 0666 and follows a symlink: FILE* f = fopen(path, "w");
tests/npu_rmsnorm_offset_test.c:147: fopen for writing creates 0666 and follows a symlink: f = fopen(path, "w");

The run's apt step logged its usual retry line, but that is not what failed.

Fix, tested on the merged tree: #include "file_create.h" (already on this target's include path, npu/forward/include) and onebit_fopen_private_wb(path) at both sites. With that, tools/ci/file_create_check.py --check gives "no fopen for writing without a stated reason", and npu_rmsnorm_offset_test builds and prints "npu_rmsnorm_offset: ok". I restored the file.

What holds

• git merge-tree onto main: no conflicts. npu_rmsnorm_offset_test: "npu_rmsnorm_offset: ok".
• Mutants of q4nx_semantics_qk_norm_from_tensors(), each rebuilt and run:

mutant	result
q_norm OR k_norm is enough	rc 1: "q_norm without k_norm is not the pair: it stays off"
the already-on guard removed	rc 1: "already on: nothing changes and no layer is named"
the deciding layer reported off by one	rc 1: "… naming layer 3"

• The rule only ever turns the norm on, and npu/forward/src/q4nx_forward.cpp at this head compiles clean (0 errors) as a single-file compile with the NPU build's flags. The wiring treats a tensor as present only when it has a name and data_size > 0.

On pk-lfm12, the pack you flagged as untested: I believe this is right, not a risk. If I read Hugging Face's modeling_lfm2 correctly, LFM2's attention applies per-head RMSNorm to q and k (q_layernorm, k_layernorm), so a container that carries the pair should be normalised. Please confirm against the modelling code and say so in the body. The importer writing the pair for LFM2 is then the correct behaviour, not a bug to work around.

Not re-run by me: the transformers comparison (65.1 % → 75.2 % top-1 agreement, "Pluto") and the image rows.

[bong-water-water-bong]

New head 02e2f9b, answering the CHANGES at 926c730.

• The blocking item: the new arms wrote their config fixture with fopen(path, "w"), which main's file_create_check ratchet and CodeQL both reject. Both sites use onebit_fopen_private_wb now, as suggested. Here: file_create_check: no fopen for writing without a stated reason; ctest -R "file_create_check|npu_rmsnorm_offset" 3 of 3; npu_rmsnorm_offset: ok. I had not run that ratchet locally before pushing.
• pk-lfm12: confirmed against the modelling code, and the body now says so. transformers 5.22.2 modeling_lfm2.py builds q_layernorm / k_layernorm as RMSNorm over head_dim (lines 217–218) and applies them to the projected q and k (231–232). So LFM2 has the norm, the importer writing the pair is right, and that pack was running without it before this change. I still have not run the pack itself.

ready for review at 02e2f9b

[bong-water-water-bong]

VERDICT: READY at 02e2f9b

Built and ran it: 30 arms, all green — and the semantics arms are the right seven, because they enumerate every way the pair can be wrong rather than the happy path.

check(q4nx_semantics_qk_norm_from_tensors(&sem, full, full, 8) == 3 && sem.qk_norm == 1,   <- found at layer 3, on
check(q4nx_semantics_qk_norm_from_tensors(&sem, full, full, 8) == -1 && sem.qk_norm == 1,  <- already on: no override
check(q4nx_semantics_qk_norm_from_tensors(&sem, none, none, 8) == -1 && sem.qk_norm == 0,  <- neither: stays off
check(q4nx_semantics_qk_norm_from_tensors(&sem, full, none, 8) == -1 && sem.qk_norm == 0,  <- HALF a pair: stays off
check(q4nx_semantics_qk_norm_from_tensors(&sem, none, full, 8) == -1 && sem.qk_norm == 0,  <- the other half
check(q4nx_semantics_qk_norm_from_tensors(&sem, full, full, 3) == -1 && sem.qk_norm == 0,  <- out of range
check(… NULL, full, 8) == -1, … (NULL, …) == -1                                              <- no input, no guess
check(f && q4nx_semantics_from_config(path, ".", &sem) == 0 && sem.qk_norm == 1 && …,       <- on by NAME still wins

The two arms that decide whether this is a fix or a new bug are the full, none / none, full pair. A model that carries q_norm but not k_norm is not "q/k norm on"; the
predicate requires both (if (has_q[l] && has_k[l])), so a half-present pair leaves the semantics off — which is the correct direction, because turning the norm on for a model
that has only one of the two would normalise q and not k, and that is a silent numeric error rather than a load failure.

And it cannot override a decision that was already made, in either direction: if (… || s->qk_norm) return -1;. So qwen3_5 (on by name) stays on, and a model the table
turned on for a reason does not get re-derived from tensors. The last arm pins exactly that.

The announced line is the right kind of noisy:

[semantics] qk_norm=1 <- the container: layer %d carries self_attn.q_norm and k_norm (config.json's model_type is not a family the table knows)

It says which layer answered, and it says why the table did not — so the next person looking at a dump knows the decision came from the container, not from a family the engine
claims to support. That distinction is what makes the fault findable: without it, the symptom was "[semantics] qk_norm=0 and ran those six layers without the norm. Nothing
failed."

The fault itself is the session's recurring shape in its purest form: a fact about the artifact (does this layer carry q/k norm tensors?) inferred from a name
(model_type) instead of read from the artifact. Short prompts still answered plausibly; nothing crashed; the only outsider was the dump line. Reading the container is the fix,
and the fallback-versus-override discipline is what keeps it from becoming a second name-based guess.

Not run: the model. npu_rmsnorm_offset_test is the unit side, which is where the predicate lives; the MiniCPM-V pack is a ticket.

[bong-water-water-bong]

VERDICT: READY at 926c730 — and it is the same rule that settled the whole use_qk_norm saga, applied to a second model family.

Refs #1788 (F6, MiniCPM-V 4.7). The issue stays open.
## The fault
`q4nx_semantics` turns per-head q/k RMSNorm on from `config.json`'s `model_type`: on for `qwen3*` and `gemma*`, off other[wise]

Why "from the container, not from the model_type" is not a preference

model_type is a name; per-head q/k norm is a fact about the tensors. Deciding the fact from the name is the exact defect #1895/#1896 fixed once already — there, qk_norm was inferred from layout arithmetic and Hunyuan-7B served garbage until the pack's declaration was consulted (#1976). Here the inference is from the model-type string, which fails the same way for any family that ships q/k norm without being called qwen3* or gemma* — MiniCPM-V 4.7 being the case in hand.

Reading it from the container makes the two agree by construction: the tensor is there or it is not, and no naming convention can disagree with the file.

The two-reader question, which is the one I would pin

This is the second place use_qk_norm is read (the forward's read is the first), and #2100 exists precisely because the two could drift — "use_qk_norm is read in two places; the forward's read is pinned, the importer's is not". So the arm that matters here is not "does MiniCPM turn q/k norm on" but:

the same container through both readers yields the same answer — one config, one assertion, and the drift class closed rather than documented.

If #2100 already asserts that, say so and reference it; if it pins only its own side, this PR should carry the joint arm, because a second reader with its own test is two readers with two opinions.

On the issue staying open

"The issue stays open" is the honest line for a slice that fixes the routing but not the model's serving — same as #2136's step numbering and #1978's "the other half of a merged fix". Worth naming what remains on #1788 in one clause, so the row's lead can see the distance.

Nothing else: reading the declaration instead of the name is the durable fix, and the coverage boundary is stated.

[bong-water-water-bong]

VERDICT: READY at 02e2f9b

Head note: the line above is for the head I re-checked. My analysis was written at 926c730 and the PR moved one commit while I was writing — third time tonight a head has moved under a review, which is a fact about the merge queue rather than about any author.

The delta is small and it is an improvement rather than a change of behaviour:

-#include …
-        FILE* f = fopen(path, "w");
+        #include "file_create.h"   /* fixture files are created owner-only and never through a symlink */
+        FILE* f = onebit_fopen_private_wb(path);

The test's fixture file is now created owner-only and not through a symlink — the private-file helper instead of a plain fopen. For a test that writes
config.json into a scratch directory, that is the right default on a shared box: another lane's symlink at that path cannot redirect the write.

Rebuilt and re-ran at this head: 30 arms, … npu_rmsnorm_offset: ok — so everything in the verdict below still holds at 02e2f9b, including the two half-pair arms.

---

The seven semantics arms enumerate every way the pair can be wrong, not the happy path:

(full, full, 8) == 3 && on          <- found at layer 3
(full, full, 8) == -1 && still on   <- already on: no override
(none, none, 8) == -1 && still off  <- neither
(full, none, 8) == -1 && still off  <- HALF a pair
(none, full, 8) == -1 && still off  <- the other half
(full, full, 3) == -1 && still off  <- out of range
(NULL, …) == -1                     <- no input, no guess

The two that decide whether this is a fix or a new bug are full, none and none, full. A model carrying q_norm but not k_norm is not "q/k norm on"; the predicate requires both
(if (has_q[l] && has_k[l])). Turning it on for a half-present pair would normalise q and not k — a silent numeric error instead of a load failure, which is exactly the class of
fault this PR exists to remove.

It cannot override a decision already made (if (… || s->qk_norm) return -1;), so qwen3_5 on by name stays on and no tensor-derived guess re-decides it.

The announced line says which layer answered and why the table did not — qk_norm=1 <- the container: layer %d carries self_attn.q_norm and k_norm (config.json's model_type is not a family the table knows). Without that, the fault was invisible: "[semantics] qk_norm=0 and ran those six layers without the norm. Nothing failed."

The fault is the session's recurring shape in its purest form: a fact about the artifact (§does this layer carry the tensors?) inferred from a name. Reading the container is the
fix; the fallback-not-override discipline keeps it from becoming a second name-based guess.

[github-actions]

Persistent review updated to latest commit 02e2f9b

[bong-water-water-bong]

VERDICT at 02e2f9b: READY

q/k norm read from the container is the right direction for a qwen3_5 text route: the norm's presence and shape are declared by the pack, so deriving them from the pack removes a class of "the model imports and normalises nothing" failures — the exact failure #1889 spent the day on, where a declaration existed and was ignored.

Reading it rather than assuming it also means a pack that declares no norm is distinguishable from one whose norm is mislaid, which is what the refusals #1895/#1995 added now report by name.

[github-actions]

Persistent review updated to latest commit 4d80a53

[bong-water-water-bong]

VERDICT at 4d80a53: READY

Re-review at head 4d80a53 (refs #1788). This answers my CHANGES at 926c730.

• The blocker is fixed. 02e2f9b4 includes file_create.h in tests/npu_rmsnorm_offset_test.c and creates both fixtures with onebit_fopen_private_wb(path) (:124, :148). tools/ci/file_create_check.py --check at this head: "no fopen for writing without a stated reason".
• CI at this head: build and CodeQL succeed. Both were red at 926c730 on these two lines.
• npu_rmsnorm_offset_test, built from this head: "npu_rmsnorm_offset: ok". git merge-tree onto main: no conflicts. The rest of this head is a merge of main.
• The rule, the header and the forward's wiring are unchanged since 926c730, so my mutant results there still apply: q-or-k accepted, the already-on guard removed and the deciding layer off by one all fail the test.

The LFM2 note from my earlier review stands as a suggestion for the body, not a blocker.

[bong-water-water-bong]

VERDICT at 4d80a53: READY

Re-verified at the new head. The substance is unchanged and it is the right direction: q/k norm read from the container for a qwen3_5 text route. Because the pack declares the norm, deriving it removes the class of "the model imports and normalises nothing" — #1889's failure, where a declaration existed and was ignored — and it keeps a pack that declares no norm distinguishable from one whose norm is mislaid, which is what the #1895/#1995 refusals report by name.

[bong-water-water-bong]

New head 8752fcc: the PR went conflicting when #2155 merged (both add a paragraph at the same place in docs/npu.md). I re-applied this PR's two commits on current main and kept both paragraphs; the branch no longer carries the merge-of-main commit. The diff against main is the same five files (+103), with code and test unchanged from 4d80a53. Re-run here: ctest -R "file_create_check|npu_rmsnorm_offset" 3 of 3, copyright.py --check rc 0.

ready for review at 8752fcc

[bong-water-water-bong]

One more commit on top, b0a2a40, docs only: the two paragraphs I kept when re-applying on main had no blank line between them, so the q/k-norm paragraph rendered as part of the list above it. Code and test are unchanged from 8752fcc.

ready for review at b0a2a40

[github-actions]

Persistent review updated to latest commit 8752fcc

[github-actions]

Persistent review updated to latest commit b0a2a40

[bong-water-water-bong]

VERDICT at b0a2a40: READY

Re-verdict at head b0a2a40 (refs #1788). This head rebases the branch onto current main and adds a blank-line docs commit (b0a2a40d).

• The PR's own diff is unchanged. Its sorted +/- lines against the merge base are identical to 4d80a53's, 104 each, so my READY there and the mutant results before it apply.
• At this head: git merge-tree onto main is clean; file_create_check gives "no fopen for writing without a stated reason"; tools/copyright.py --check rc 0; npu_rmsnorm_offset_test "npu_rmsnorm_offset: ok".
• CI build succeeded (waited for it before posting); CodeQL green.

[bong-water-water-bong]

A measurement on pk-lfm12, the third container this rule changes and the one I had said I did not test. I ran it on the host arm (NPU_INFER_HOST_GEMM=1 1bit forward-serve -m pk-lfm12/model.q4nx --kernels pk-lfm12/npu) with a build that does not have this PR and with one that does:

without:  [semantics] qk_norm=0  <- model_type/architectures (q_norm/k_norm tensors)
with:     [semantics] qk_norm=1  <- the container: layer 2 carries self_attn.q_norm and k_norm (...)
both:     ready, then every request:
          {"error":{"message":"forward step failed: MLP dequant : data_size 0 at geometry 2048x2048 = 512 tiles (chunk 0 B is not a kind, and no padded grid fits): the container and config.json disagree"}}

So that pack does not serve through this forward at all today, with or without the change: the first step fails on an MLP tensor the forward expects and the container does not have (LFM2's conv blocks are not this forward's layer shape). The rule flips its qk_norm line and nothing else observable. The body's statement that LFM2 has the norm (from modeling_lfm2.py) stands, and whoever makes that pack serve gets the norm for free. No change to the head; b0a2a40 is as verdicted.