Repository navigation
npu: count a gated q_proj's 2*QD rows, so check_q4nx_dir stops calling every attn_output_gate pack padded (Fixes #1285) - #1286
Conversation
…g every attn_output_gate pack padded `dims_from_config` expected `num_attention_heads * head_dim` rows for `self_attn.q_proj.weight`, but with `attn_output_gate` the stored projection emits q AND the output gate (2*QD rows). The mismatch made the geometry check report a 512-padded grid and fail `--strict` on 8 of the 18 Q4NX directories on this box -- including the real d3 pack (#796) with 6 warnings and every 27B qwen3_5 directory with 16 -- for containers that decode and answer correctly. `QG = 2*QD` when config.json sets `attn_output_gate` (JSON true or 1, as the forward's q4nx_config_int reads it); `expected_rc` uses it for q_proj. Self-test gains the d3 case in both directions (quiet when gated, still warning when not), and docs/npu.md says so where the checker is described. Fixes #1285
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 39bead7a — reproduced on the real 27B pack this box holds: the shipped checker calls a correct container broken (rc 1 on main), and this PR makes the same directory pass, while a non-gated container behaves identically to main.
Before/after, same directory, same command, only the tool differs (~/models/npu/bonsai2-27b-pb16i, whose config.json sets attn_output_gate; the pack the 313/410 ms rows in the leaderboard are about):
origin/main tool: FAIL model.layers.47.self_attn.q_proj.weight: holds 7680 tiles, the model's dimensions
imply 3840 - columns padded 5120 -> 10240 (the 512-column grid) … (engine#1097) [×6 layers]
chunk layout NOT verified rc 1
this PR's tool: chunk layout verified: every quantized tensor's shape names a known chunk kind rc 0
7680/3840 = 2, which is the whole point: a gated q_proj emits q and the output gate, so its rows are 2 * num_attention_heads * head_dim. The old code used QD and then explained a correct grid as padding — i.e. --strict failed on the real container, which is exactly the class of false red the check exists to prevent.
The negative side is unchanged: ~/models/npu/d3-synth-0.6b/ (no attn_output_gate) gives chunk layout verified … at both heads, so the new branch is inert for the containers that do not need it.
The self-test grew the arm and the anti-vacuity control for it: python3 tools/check_q4nx_dir.py --self-test rc 0 with … and a gated (attn_output_gate) q_proj's 2*QD rows are the model's own grid — and the arm refuses to pass silently, because it also asserts the same tensor shape does warn when attn_output_gate is absent ("the same q_proj is quiet without attn_output_gate, so the case proves nothing"). That is the right shape: a case that cannot fail would be worthless here.
Two details I checked because they are how this breaks later:
- the boolean is read the way the forward reads it — a JSON
trueor1(gate is True or gate == 1), matchingq4nx_config_int, so a checkpoint that spells it as an integer is not silently treated as ungated; QGis carried in the samedims_from_config()dict the rest of the expectations use, soq_projando_projstay consistent (o_projremains(H, QD)— its input is q's pre-gate width, and the diff correctly leaves it alone).
Docs are in the PR (docs/npu.md names the rule and both issues); tool + self-test + doc in one change.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at acac5938 — main merges only; the reviewed content is byte-identical to the head I verdicted.
Checked rather than assumed: the PR's own added lines (own diff vs merge-base, which excludes everything main brought in) hash to 260cec477c38 at the head I reviewed and to the same value here, so no line of this PR's change moved — only its base did (the branch now sits on newer main).
Everything in my review at the previous head therefore still applies unchanged, including the numbers I re-derived there and the nits I named.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4f33529cceadac32758f1672d90d369ed663bf88. A small, correct fix, tested in both directions, and the 18-directory claim reproduces on a wider set.
Checked here (host only, no device)
- Scope:
git diff --stat $(merge-base) 4f33529cis 2 files, +31/−5 (tools/check_q4nx_dir.py,docs/npu.md), as the body says. MERGEABLE. tools/check_q4nx_dir.py --self-testat the head: passes, rc 0.- I tested the test: with
qg = qd * 2 ifmutated toqg = qd if, the self-test fails withattn_output_gate did not double q_proj's rows: {'QD': 2048, 'QG': 2048, …}, rc 1. The case also asserts that the same table still warns under ungated dims, so it cannot pass vacuously. - The checker now agrees with the forward.
npu/forward/src/q4nx_forward.cpp:757readsattn_output_gatefromconfig.jsonwithq4nx_config_int, andq4nx_pack.h:187-192documents q_proj as q|gate. Treating JSONtrueand1alike matches that.o_projstays(H, QD), which is right: the gate is consumed before o_proj. - Sweep, old (
origin/main) vs new,--strict, over everymodel.q4nxdirectory under~/models,~/lband~/stage-c26. That is 31 directories, a superset of the body's 18:
| dir | gate | WARN | rc |
|---|---|---|---|
models/npu/d3-lite-conv |
true | 6 → 0 | 1 → 0 |
lb/i796-fresh, lb/i796-fresh-noplan |
true | 6 → 0 | 1 → 0 |
bonsai2-27b-NPU, -NPU-b8, -pb16i, -pb16i-unrot, -t2s2, -t2s2-serve, bonsai2-t2s2-perrole876 |
1 | 16 → 0 each | 1 → 0 |
models/npu/d3-lite-bf16 |
true | 0 → 0 | 0 → 0 |
| the other 19 | absent | unchanged | unchanged |
No directory gains a warning or changes rc in the wrong direction.
- The new
docs/npu.mdsentence is in the same PR as the behaviour.copyright.py --checkrc 0.
Notes, not blocking
gate is True or gate == 1treats any other nonzero int as ungated, while the forward treats any nonzero as gated (if (attn_gate_)). No config on the box uses another value, butbool(gate)(excluding strings) would match the forward exactly.- #1275 edits the same file (the self-test tail and the success line). The two may conflict textually when the second one lands; they don't conflict logically.
Fixes #1285 is accurate: the false WARN / --strict rc 1 on gated containers is gone, measured.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ecebe03510f05736e0c6adf4eaa30fd506417fab. Main merges only since my READY at 4f33529c: the PR diff's git patch-id --stable against its merge-base is identical (298bfe339e36) at both heads, and there are no non-merge commits of its own. Re-checked at this head: tools/check_q4nx_dir.py --self-test rc 0. The full review at 4f33529c stands.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Fixes #1285 (found while closing out the pack side of #796).
tools/check_q4nx_dir.pyderives each weight's expected tile grid fromconfig.json, and forself_attn.q_proj.weightit expectednum_attention_heads * head_dimrows. Withattn_output_gatethestored projection emits q and the output gate, so it holds
2 *that many rows — so the geometry checkreported a phantom 512-padded grid and
--strictexited 1 on correct containers. That is the inverse of whatthe tool is for: it told an operator to distrust a pack that decodes and answers.
Measured (header reads only, no device, no ticket), before → after
tools/check_q4nx_dir.py <dir> --strictover all 18 Q4NX model directories on this box:attn_output_gated3-lite-conv(the realvllm-sr/d3-litepack, #796)bonsai2-27b-NPU,-NPU-b8,-pb16i,-pb16i-unrot,-t2s2,-t2s2-serve,bonsai2-t2s2-perrole8768 of 18 containers change; none gains a warning.
The container is right (two independent checks)
2*QD.d3-lite-convmodel.layers.3.self_attn.q_proj.weightisI8 [512, 5120]= 512 tiles of 32×256 = 4096 rows against
QD = 8 × 256 = 2048. The container's ownplan.jsonsays thatlayer's fused QKV is
K=1024 N=5120, andN - 2*KVD = 5120 - 2*512 = 4096is the q|gate pair — the numberq4nx_derive_designsbuilds the ELF set from./v1/systemoneon the array and 10/10 underNPU_INFER_HOST_GEMM=1(npu: a real d3 pack (vllm-sr/d3-lite, 0.8B-GDN) answering through /v1/systemone — converter transforms, name map, and the 7-design ELF set #796 rungs 2 and 1). Six tensors that answer cannot be "failing to decode".Change (2 files, 31+/5−)
dims_from_configcarriesQG = 2*QDwhenconfig.jsonsetsattn_output_gate(JSONtrueor1— theforward reads it with
q4nx_config_int, so a bool and an int must mean the same thing);expected_rcusesQGforq_proj.o_projis unchanged: a gated layer's attention output is stillQDwide, and the d3container holds exactly
(H, QD)there.q4nx_dir_check) in both directions: a d3q_projof[512, 5120]tiles isquiet under gated dims, and the same table still warns under ungated dims — so the case only passes when the
gate is what suppresses the warning.
docs/npu.md, where the checker is described. (Not thenoncausal_full_attentionsentence —that one is in the open npu: the forward is causal-only, so a noncausal_full_attention pack (d3-lite) cannot be reproduced — measured, not inferred #847 PR.)
Verification
tools/check_q4nx_dir.py --self-test→ passes; tested the test: withqg = qdrestored it fails onattn_output_gate did not double q_proj's rows: {'QD': 2048, 'QG': 2048, …}, restored it passes.--strictrc 1 → 0 on both gated families.tools/safetensors_to_q4nx.py pack ~/models/npu/d3-lite-src … --arch qwen3_5→ 320 tensors / 820,971,656 B, weight bytes byte-identical tothe shipped
d3-lite-conv(sha256 313ac4f9…),verify320/320 ok, the three GDN transforms still exacton real weights (
ssm_a=-exp(A_log)18/18,conv1dtap-major 18/18 — a flatten differs in 18/18 —alpha/beta zero-padded 18/18),
decision→decision.q4nx [255, 1024] f32. Old checker: 6 WARN / rc 1. Newchecker: 0 WARN / rc 0.
tools/copyright.py --checkrc 0. Device never opened in any of the above.