Repository navigation
tools: check_q4nx_dir names a weight grid padded away from the model's own tiles - #1099
Conversation
…s own tiles The FastFlowLM converter pads intermediate_size up to its 512-column grid (8960 -> 9216 on both sides), and every chunk-kind check still passes: the chunks are valid, the declared bytes are the tiles that kind needs. What changes is the tile count -- 1728 where the model's dimensions imply 1680 -- and this engine's reader derives the chunk size from the unpadded grid, so q4nx_dequant_tensor_chunked() is handed cb = 8847360/1680 = 5266 B, which is not a chunk kind, and the container loads and then fails every request with "forward step failed: dequant failed" (engine#1097, measured on a Qwen2.5-1.5B finetune: 84 anomalies, 28 layers x 3 MLP tensors). A directory carries its own config.json, so the tile count can be checked against it without dequantizing anything: rows/cols per weight from hidden_size, num_attention_heads, num_key_value_heads, head_dim, intermediate_size, vocab_size, then tiles = prod(shape[:-1]) against the model's own (rows/32)(cols/256). A mismatch names which side was padded and to what, and says what the engine will do with it. Warnings by default, failures under --strict; a directory without config.json skips the check, and the default path is unchanged. FLM repack (8960 padded to 9216): 84 WARN, rc 0 (rc 1 with --strict) published ZR1-1.5B-npu-q4km-v2: 0 WARN, rc 0 (rc 0 with --strict) --self-test grows three rows (the model's own grid is quiet; a 512-padded MLP grid warns, both sides, with the padding named; a non-quantized tensor is never a geometry warning). Also verified: --against on the published container still reports layout A corr +1.0000, and tests/gguf_to_q4nx_test.py PASS. This is the gate, not the fix: the forward change is npu/forward's (npu/forward/src/q4nx_dequant.c uses cols/256 as the tile stride, so the padded width has to be threaded through, which is the owner's call).
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at f76d579b — the detection reproduces on a real container, the default path is untouched, and the MoE exclusion is right by construction. One nit, and it is the same fail-open family this tool exists to prevent.
The padded-grid check, reproduced independently. The FLM repack dir is not on this box, so I made the geometry real instead of trusting the row: a symlinked copy of the published ZR1-1.5B-npu-q4km-v2 container (config hidden 1536 / IM 8960 / 28 layers) with intermediate_size rewritten to the padded 9216:
$ check_q4nx_dir.py /tmp/cqtest
WARN model.layers.0.mlp.down_proj.weight: holds 1680 tiles, the model's dimensions imply 1728 -
columns padded 9216 -> 8960 (the 512-column grid); this engine derives the chunk size from the
unpadded grid, so the tensor fails to decode (engine#1097)
WARN model.layers.0.mlp.gate_proj.weight: … rows padded 9216 -> 8960 …
WARN model.layers.0.mlp.up_proj.weight: … rows padded 9216 -> 8960 …
$ check_q4nx_dir.py /tmp/cqtest --strict -> rc 1
84 WARN
84 is exactly the count you quoted for the FLM repack, and it is 28 layers × 3 MLP tensors — so the number is structural, not incidental. Two things I liked more than the count: the message names which side was padded, and it is right about it — rows for gate_proj/up_proj (they produce IM) and columns for down_proj (it contracts over IM). That is the difference between a warning someone can act on and one they learn to ignore.
The other claims, checked:
published ZR1-1.5B-npu-q4km-v2 0 WARN, rc 0 default and --strict
directory with no config.json geometry check skipped, chunk-layout still verified, rc 0
--self-test PASS, including "a 512-padded grid is named as padded while
the model's own grid is quiet"
every line removed by this PR exactly one, inside the self-test's own summary string —
so "no behaviour change on the default path" is true of the code,
not just of the intent
expected_rc() names only lm_head + the 7 per-layer projections, so an expert
stack resolves to None and is skipped: MoE untouched by construction
The nit: a config.json that exists but is unusable fails open, silently. dims_from_config() returns None on any missing key, and geometry_warnings() returns [] when it has no dims — the same branch as "no config.json at all". So a container with a partial config gets no geometry check and no mention of it:
$ cat /tmp/cqtest/config.json # hidden_size + intermediate_size only, and it still claims the padded 9216
$ check_q4nx_dir.py /tmp/cqtest
chunk layout verified: every quantized tensor's shape names a known chunk kind
rc=0 # the 84 warnings above are simply gone, with no note
The tool's own thesis is that a silent skip is the disease — so I would distinguish the two cases in one line: "config.json present but the dims this check needs are missing (num_attention_heads, …): geometry not checked". Same shape as the --strict story, just for input it cannot interpret, and it is what the no-config path already prints implicitly by being a documented skip.
I did not verify --against on the published container beyond the self-test row, and I have no FLM-repacked container here to run the row you measured — the 84 comes from reproducing the geometry, not from your file.
|
Preparing review... |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Re-checked at bf79db2d: the delta from f76d579b is a rebase onto the merged work (#1094's serve.cpp/unified.cpp, #1093's console-tui, #1086's moe_expert_chunks.py) — not a fix for the nit, and the nit still reproduces:
$ python3 check_q4nx_dir.py /tmp/cq2 # config.json = {hidden_size, intermediate_size: 9216}, i.e. the padded grid
chunk layout verified: every quantized tensor's shape names a known chunk kind
(no geometry warning, no note) rc=0
So the tool still fails open on a config.json it cannot read the dims from, in the same silent way as "no config at all". Verdict stays READY — it does not gate the merge and the check is correct on every complete config — but the one-line distinction is worth taking while the PR is open, because the failure it guards against ("container loads, every request fails with dequant failed") is exactly what a silent skip hides.
For the record on the rebase: mergeStateStatus is UNKNOWN at this moment (GitHub still computing), so I am not treating the head as verified for merge order; the earlier blob-level review at f76d579b stands for the file's content since the tool itself is unchanged in this push.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
tools/check_q4nx_dir.pyverifies chunk kinds and declared byte counts, and it passed a container that cannot answer asingle request. This adds the one thing it was not looking at: whether a quantized tensor's tile count matches the
geometry the model's own dimensions imply.
Root cause it catches (engine#1097)
1bit serve -m <gguf> --device npurepacks through FastFlowLM's converter, which padsintermediate_sizeup to its512-column grid — 8960 → 9216 on both sides. Every chunk check still passes: the chunks are the right kind and the
declared bytes are the tiles that kind needs. What changes is the tile count, 1728 where the model's dimensions imply
1680, and the reader derives the chunk size from the unpadded grid (
q4nx_forward.cpp:180,dequant_any), so it computescb = 8847360 / 1680 = 5266B — not a chunk kind — falls through to the legacy non-chunked dequantizer and every requestanswers
{"error":{"message":"forward step failed: dequant failed"}}in ~7 ms. Measured on a Qwen2.5-1.5B finetune:84 anomalies = 28 layers × 3 MLP tensors, and the model loads fine, which is why the failure looked like a forward bug.
What the check does
A model directory carries its own
config.json, so the tile count can be compared against it with no dequantizing:rows/colsper weight fromhidden_size,num_attention_heads,num_key_value_heads,head_dim,intermediate_size,vocab_size; thentiles = prod(shape[:-1])against the model's own(rows/32)·(cols/256). A mismatch names which sidewas padded and to what, and says what the engine will do with it:
--strictturns them into failures, for a gate that must not pass such a container;config.jsonskips the check (and an unreadable one says so once);lm_head,q/k/v/o_proj,gate/up/down_proj); MoE stacks and anything else areleft alone, so no false positives on containers this tool does not model.
Checks
~/.cache/1bit/q4nx/ZR1-1.5B-Q4_K_M, IM 8960 padded to 9216)--strictZR1-1.5B-npu-q4km-v2(the same model,gguf_to_q4nx.pylayout)--strict--self-test(3 new rows: the model's own grid is quiet, a 512-padded MLP grid warns on both sides with the padding named, a non-quantized tensor is never a geometry warning)--againston the published container, unchanged pathlayout A corr +1.0000, success line as beforetests/gguf_to_q4nx_test.pytools/copyright.py --checkScope
This is the gate, not the fix. The forward change is
npu/forward's:q4nx_dequant_tensor_chunked()usescols / 256as the tile stride (npu/forward/src/q4nx_dequant.c:101-109), so an unpaddedcbwould decode the wrong tilesrather than fail cleanly — the padded width has to be threaded through the reader and reach the design's K as well
(
q4nx_pack.c:141), which is a padding decision I am not making in someone else's file. Reported with the audit inengine#1097; no
closes.