Repository navigation
npu: layer_types is matched exactly, so a kind the forward cannot run is refused instead of run as GDN (#1156) - #1172
Conversation
… is refused instead of run as GDN (#1156) The #1124 guard refuses an unknown layer kind by name, but it decided "known" with strstr, so its width was the substring: chunked_linear_attention, mamba2_linear and nonlinear_attention all contain "linear" and were mapped to Q4NX_LAYER_LINEAR, i.e. the forward would serve attention math it does not have through the GDN recurrence - fluent, silently wrong at every position, which is the failure mode #1124 exists to remove. sliding_window_attention hit the same branch through "sliding". Exact match on the three spellings that name attention math this forward has (HF's own layer_types values: full_attention, sliding_attention, linear_attention); everything else stays Q4NX_LAYER_OTHER and is refused by name. tests/q4nx_layer_kinds_test.c pins both sides: the three known kinds still map as before, and a spelling that merely contains a known kind is refused and quoted.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
The fix is exactly what #1156 asked for, and I checked the two things that decide whether it is safe: the spellings it now refuses are not in any container here, and the ones it keeps are the ones this box actually uses.
My own probe from the issue, rebuilt against this head (same q4nx_semantics.c, two-entry layer_types):
mamba2_linear -> OTHER unknown='mamba2_linear'
chunked_linear_attention -> OTHER unknown='chunked_linear_attention'
nonlinear_attention -> OTHER unknown='nonlinear_attention'
conv_attention -> OTHER unknown='conv_attention'
linear_attention -> LINEAR(GDN) unknown=''
sliding_attention -> SLIDING unknown=''
full_attention -> FULL unknown=''
so the four that used to be served through the GDN recurrence (or as sliding) are refused by name, which is the shape #1124 established and #1087 before it. The PR's own test agrees: q4nx_layer_kinds_test -> passed (0 failures), including a substring match is quoted by name, so the container is refused instead of served.
No container on this box regresses. Every layer_types spelling across the 19 Q4NX containers under ~/models/npu/:
known full_attention 11
known linear_attention 10
known sliding_attention 3 (counting the mixed containers)
REFUSED conv_attention 1 <- npu/Qwen3-0.6B-layerkind-probe, the deliberate fixture
The one spelling that changes behaviour and is plausible in the wild is sliding_window_attention, which was SLIDING and is now OTHER. Nothing here uses it, our converter writes only full_attention/linear_attention (scripts/repack_gguf.py:169), and the failure mode is a named refusal at load rather than silent wrong maths - so the cost is discoverable in one message, which is the trade this PR is making deliberately.
One inconsistency this leaves behind, worth one line somewhere (not a blocker, and not introduced here): tools/npu_plan.py:85-87 accepts a wider set than the forward -
if t in ("linear_attention", "linear", "gdn"): out.append("gdn")
elif t in ("full_attention", "sliding_attention", "attention"): out.append("attn")and the forward refuses linear, gdn and attention (probe above). So a container declaring "linear" gets a plan.json claiming the gdn role and is then refused at load. gdn/attention were already in that state before this PR ("gdn" contains no linear/sliding, so it was OTHER before too); linear joins them now that the match is exact. The generator and the loader having two accepted-spelling lists is the kind of drift this PR is about, and one of them should be the other's authority.
PR Reviewer Guide 🔍(Review updated until commit 64fa749)Here are some key observations to aid the review process:
|
…ot evidence against
Found by sweeping the box rather than by review: both real repack-cache directories
(~/.cache/1bit/q4nx/Qwen3-0.6B-Q4_K_M and ZR1-1.5B-Q4_K_M) were written by a repack older than the
field, so their stamp has no `tile_order` key. The previous commit read `fields.get("tile_order", "")`
and fell through to the fatal branch, so `npu_plan.py` refused two directories that work today, with the
nonsense message `tile_order='' -- the repack measured these tiles as NOT the engine-1 order`.
A stamp without the key records provenance and says nothing about the tiles, so it takes the same path as
no stamp at all: default engine-1, silent. The fatal branch is now only reachable from a stamp that
actually names a different order.
Re-measured: both cache dirs rc=0, tile_order=engine-1, 0 bytes of stderr; 19 directories with
config.json+model.q4nx on this box all plan cleanly except Qwen3-0.6B-layerkind-probe, whose
`conv_attention` refusal is identical on origin/main (pi-npufeat's #1172 fixture, not this change).
Test gains the verbatim pre-field stamp as a case (16 checks).
…ot evidence against
Found by sweeping the box rather than by review: both real repack-cache directories
(~/.cache/1bit/q4nx/Qwen3-0.6B-Q4_K_M and ZR1-1.5B-Q4_K_M) were written by a repack older than the
field, so their stamp has no `tile_order` key. The previous commit read `fields.get("tile_order", "")`
and fell through to the fatal branch, so `npu_plan.py` refused two directories that work today, with the
nonsense message `tile_order='' -- the repack measured these tiles as NOT the engine-1 order`.
A stamp without the key records provenance and says nothing about the tiles, so it takes the same path as
no stamp at all: default engine-1, silent. The fatal branch is now only reachable from a stamp that
actually names a different order.
Re-measured: both cache dirs rc=0, tile_order=engine-1, 0 bytes of stderr; 19 directories with
config.json+model.q4nx on this box all plan cleanly except Qwen3-0.6B-layerkind-probe, whose
`conv_attention` refusal is identical on origin/main (pi-npufeat's #1172 fixture, not this change).
Test gains the verbatim pre-field stamp as a case (16 checks).
|
Persistent review updated to latest commit 64fa749 |
…ot evidence against
Found by sweeping the box rather than by review: both real repack-cache directories
(~/.cache/1bit/q4nx/Qwen3-0.6B-Q4_K_M and ZR1-1.5B-Q4_K_M) were written by a repack older than the
field, so their stamp has no `tile_order` key. The previous commit read `fields.get("tile_order", "")`
and fell through to the fatal branch, so `npu_plan.py` refused two directories that work today, with the
nonsense message `tile_order='' -- the repack measured these tiles as NOT the engine-1 order`.
A stamp without the key records provenance and says nothing about the tiles, so it takes the same path as
no stamp at all: default engine-1, silent. The fatal branch is now only reachable from a stamp that
actually names a different order.
Re-measured: both cache dirs rc=0, tile_order=engine-1, 0 bytes of stderr; 19 directories with
config.json+model.q4nx on this box all plan cleanly except Qwen3-0.6B-layerkind-probe, whose
`conv_attention` refusal is identical on origin/main (pi-npufeat's #1172 fixture, not this change).
Test gains the verbatim pre-field stamp as a case (16 checks).
…ot evidence against
Found by sweeping the box rather than by review: both real repack-cache directories
(~/.cache/1bit/q4nx/Qwen3-0.6B-Q4_K_M and ZR1-1.5B-Q4_K_M) were written by a repack older than the
field, so their stamp has no `tile_order` key. The previous commit read `fields.get("tile_order", "")`
and fell through to the fatal branch, so `npu_plan.py` refused two directories that work today, with the
nonsense message `tile_order='' -- the repack measured these tiles as NOT the engine-1 order`.
A stamp without the key records provenance and says nothing about the tiles, so it takes the same path as
no stamp at all: default engine-1, silent. The fatal branch is now only reachable from a stamp that
actually names a different order.
Re-measured: both cache dirs rc=0, tile_order=engine-1, 0 bytes of stderr; 19 directories with
config.json+model.q4nx on this box all plan cleanly except Qwen3-0.6B-layerkind-probe, whose
`conv_attention` refusal is identical on origin/main (pi-npufeat's #1172 fixture, not this change).
Test gains the verbatim pre-field stamp as a case (16 checks).
…#1110) (#1187) * npu_plan: the plan's tile_order is a measurement, not a constant (engine#1110) `npu/plan/plan.h` refuses every `container.tile_order` but "engine-1", and that is the only thing between a container whose tiles are another order and a clean HTTP 200 with noise in it. But `tools/npu_plan.py` wrote "engine-1" for whatever directory it was pointed at, unconditionally, so the guard was green on exactly the containers it exists for. Measured (Qwen3-0.6B-Q4_K_M, converter FLM_Q4NX_Converter d1d5232, no device): the repack refuses the directory -- "matches NO layout: closest is flm with worst max|err| 2.474e+00" -- leaves its tiles on disk, and `npu_plan.py` on that same directory then wrote `container.tile_order = "engine-1"`, which the loader accepts. One command undid the refusal. The repack holds both the source GGUF and the tiles, so it is the only step that can measure. Now: - it writes the measured verdict into `repack-stamp.txt` on its REFUSING path too (`tile_order=<id>` via `tile_order_id()`, plus the verdict line and `refused=tile order`), so the measurement survives the refusal; - `npu_plan.py` reads that stamp (`tile_order_claim()`) before it derives anything: `engine-1` claims engine-1 and says it was measured, `unverified` still claims engine-1 (the loader accepts nothing else) but says out loud that nothing checked the tiles, and any other id is fatal -- no plan is written; - a directory with no stamp keeps the previous silent default, because those predate the field (`--check` on the box's real plans: ZR1-1.5B-npu-q4km-q8 and Qwen3-0.6B still "matches the model dir"). Test `tests/npu_plan_tile_order_test.py` (ctest `npu_plan_tile_order`, stdlib only) pins the mapping and the joint between the two files -- the ids the repack stamps are the ids the planner refuses. refs #1110 (the remaining jaw of it); no engine change, no device. * npu_plan: a stamp written before tile_order existed is no evidence, not evidence against Found by sweeping the box rather than by review: both real repack-cache directories (~/.cache/1bit/q4nx/Qwen3-0.6B-Q4_K_M and ZR1-1.5B-Q4_K_M) were written by a repack older than the field, so their stamp has no `tile_order` key. The previous commit read `fields.get("tile_order", "")` and fell through to the fatal branch, so `npu_plan.py` refused two directories that work today, with the nonsense message `tile_order='' -- the repack measured these tiles as NOT the engine-1 order`. A stamp without the key records provenance and says nothing about the tiles, so it takes the same path as no stamp at all: default engine-1, silent. The fatal branch is now only reachable from a stamp that actually names a different order. Re-measured: both cache dirs rc=0, tile_order=engine-1, 0 bytes of stderr; 19 directories with config.json+model.q4nx on this box all plan cleanly except Qwen3-0.6B-layerkind-probe, whose `conv_attention` refusal is identical on origin/main (pi-npufeat's #1172 fixture, not this change). Test gains the verbatim pre-field stamp as a case (16 checks). --------- Co-authored-by: pi-onebp <onebp@strixhalo.local>
Root cause
npu/forward/src/q4nx_semantics.cdecides whether alayer_typesentry names a layer kind this forward can run.For
full_attentionit compared exactly, but for the other two it asked a substring:So the #1124 guarantee — refuse a layer kind this forward does not implement, by name — was exactly as wide as the
substring. Measured with a probe linking this file and a two-entry
layer_types(full_attention,<spelling>):linear_attention(HF's GDN spelling)LINEAR(correct)LINEAR(correct)sliding_attentionSLIDING(correct)SLIDING(correct)full_attentionFULL(correct)FULL(correct)mamba2_linearLINEAR,unknown=''OTHER,unknown='mamba2_linear'chunked_linear_attentionLINEAR,unknown=''OTHER,unknown='chunked_linear_attention'nonlinear_attentionLINEAR,unknown=''OTHER,unknown='nonlinear_attention'sliding_window_attentionSLIDINGOTHER,unknown='sliding_window_attention'chunked_attention/conv_attentionOTHER(refused)OTHER(refused)Before, the four bold rows were served through the GDN recurrence or the sliding path and the container was accepted —
fluent output, wrong at every position, nothing in the log. That is the failure mode #1124 removed, re-opened by the
substring.
Fix
Exact match on the three spellings that name attention math this forward actually has — HF's own
layer_typesvaluesfull_attention,sliding_attention,linear_attention. Everything else falls through toQ4NX_LAYER_OTHER, which theforward already refuses with the spelling quoted (
q4nx_forward.cpp: "config.json layer_types names "…", a layer kindthis forward does not implement …"). Seven lines, no new state, no change to the refusal path.
Verification
tests/q4nx_layer_kinds_test.c: new case 5 pins the substring family (all four refused and the first one quoted, hybridcount included) and keeps the known-kind rows, so the fix cannot pass by matching nothing.
passed (0 failures), 20assertions; the binary is the same
add_test(NAME q4nx_layer_kinds …)target CI already runs, andq4nx_semantics.ccompiles-Wall -Wextraclean.layer_types[2]to expectQ4NX_LAYER_LINEARmakes it printFAILED (1 failure), so the newrows really execute.
~/models/npu/*parsedthrough the patched
q4nx_semantics_from_config— every one resolves toOTHER=0,refused_as=''(bonsai-2 27B 16 full +48 linear, d3-lite 6 + 18, the Qwen3/ZR1/MoE dirs without
layer_types). The only refusal on the box isQwen3-0.6B-layerkind-probe(conv_attention), which was already refused before this change.tools/copyright.py --checkrc 0 (both files already carry the notice).Deliberately not changed
tools/npu_plan.py:85-87accepts three extra aliases (linear,gdn,attention) that this parser never saw. Aligning themwould either widen the forward to guess what
attentionmeans, or start refusing configs whose plan file generates today —neither is this issue. If the two should share one list, that is a one-line change in whichever file is declared the source,
and it is worth an owner call rather than a quiet guess here.
closes #1156