Repository navigation
npu: refuse a rope scaling the route does not apply, instead of serving plain RoPE (closes #1087) - #1090
Conversation
…ng plain RoPE
A model that declares rope_scaling {yarn|linear|longrope|dynamic} was accepted at both front doors and
served theta^(-i/half), which is right only inside the model's original window: the reply stays fluent,
is wrong past it, and no line anywhere says the model was served something other than it asked for. The
size of that error is documented for the case that IS implemented (q4nx_rope.h: ignoring llama3 on
Llama-3.2-1B gives KL 0.0058 at ctx 104 rising to 0.0252 at 1497).
- q4nx_rope.h: q4nx_rope_declared_type() reads the declared type from config.json (both HF spellings,
'' when the block is absent or null) and q4nx_rope_type_supported() decides: default/none plain,
llama3 only with usable numbers, longrope only with the short_factor the forward reads, everything
else refused.
- q4nx_forward.cpp: init_impl fails the load with the type named, before any kernel or device use and
on the init_host path too.
- tools/gguf_to_q4nx.py: refuse_unsupported_rope() at the top of pack(), before the output directory
exists, and in config() so no other command bypasses it.
- tests: a 13-case config table in npu_rope_llama3_test (including a null block followed by another
block that does carry a type: still nothing declared) and three synthetic GGUF packs in
gguf_to_q4nx_test asserting non-zero exit, the type named, and no model.q4nx written.
Measured, same reproducer both ways: the converter wrote a container with rope_scaling dropped before
and refuses now; on the forward, a real 1.2 GB container whose config.json says yarn reached 'ready on
http://127.0.0.1:18099 (full ELF)' with rope_type_named=0 before and fails the load now, while the same
container with its real llama3 block still loads. Closes #1087.
…rter, and which two rules the forward does apply The behaviour changed in the previous commit is user-visible (a conversion stops instead of writing a container that would be served plain RoPE, and a serve fails at load rather than at request time), so it belongs where the GGUF route and its config.json contract are documented.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at a1c4cac — behaviour verified at 0992f65 (the only change on top is docs/npu.md +11/−0), both doors reproduce, and the guard does not cost a good model its load.
Converter door, reproduced with my own GGUF writer rather than the PR's fixture — six tiny GGUFs, qwen3.rope.scaling.type set to each value, converted at this head:
plain rc=1 out=no-claim-holds (gate passes; fails later: my synthetic file has no tensors)
none rc=1 out=yes (gate passes — 'none' is plain rope)
default rc=1 out=yes (gate passes)
yarn rc=1 out=no qwen3.rope.scaling.type is 'yarn': this converter has no RoPE scaling rule ...
linear rc=1 out=no ... 'linear' ...
longrope rc=1 out=no ... 'longrope' ...
dynamic rc=1 out=no ... 'dynamic' ...
Every refused case names the type and leaves no output directory — the ordering claim (refuse_unsupported_rope() before os.makedirs) holds in the code and in the run, and config() calls it too, so verify cannot bypass it.
Forward door, on real containers, under one npu-hold ticket (NPU_INFER_HOST_GEMM=1, no device dispatch):
yarn block rc=1 config.json declares rope scaling "yarn", which this forward does not implement
(only llama3 and longrope short_factor are), so it would be served plain RoPE and
reply wrongly past the model's original window (engine#1048, #1087)
llama3, factor 0 rc=1 declares rope_type "llama3" but its numbers are not usable (factor > 0, ...)
CONTROL valid llama3 block -> ctrLlama ready on http://127.0.0.1:18612 (full ELF) (served, then stopped)
CONTROL no block -> 844-out-clean ready on http://127.0.0.1:18613 (full ELF) (served, then stopped)
The two controls are the half that matters for a refusal PR: a legitimate llama3 container and a plain one still load and serve, so this is not a guard that refuses the models it claims to support. The unusable-llama3 case gets its own message instead of the generic one, which is the right split — it is a broken block, not an unimplemented rule.
./npu_rope_llama3_test → all llama3 rope checks passed; ctest -R "npu_rope_llama3|npu_forward_loader" → 2/2; build clean at -DONEBIT_NPU=ON, 17 warnings, none from q4nx_rope.h, q4nx_forward.cpp or the new test.
Two nits, both latent (neither is reachable with any file on this box) — the guard is a substring scanner and both edges are in it:
- It can fail open.
q4nx_rope_declared_type()searches 800 bytes past the"rope_scaling"key; arope_typethat sits beyond that window reads as no block, i.e.supported=1, i.e. served plain — the exact silent failure the PR exists to stop. Probed directly:
E rope_type 900B into the block {"rope_scaling": {"pad": "xxx…900", "rope_type": "yarn"}} declared=(none) supported=1
G rope_type 700B into the block same with 700 bytes declared=yarn supported=0
Reachable in principle for a LongRoPE/YaRN block serialised with its factor arrays before the type key; not reachable here — the only config on this box with a block is Llama-3.2-1B and its rope_type is at +145 bytes, and the ordering (factor, high_freq_factor, low_freq_factor, original_max_position_embeddings, then rope_type) is worth knowing because it is the shape that would eventually cross 800.
2. It can false-positive. The "type" fallback is searched from the key, not inside the object, so an object with no type followed within 800 bytes by another block's "type" declares that neighbour's type: {"rope_scaling": {"factor": 8.0}, "quantization_config": {"type": "fp8"}} → declared=fp8, refused. Fails closed, but with a confusing name in the message.
Both are fixed by bounding the search to the object's matching } instead of a fixed 800 bytes — a dozen lines, and it makes the scanner's window a property of the JSON rather than a magic number. Not a blocker: the null trap that is reachable (every config without a block) is handled correctly, and {"rope_scaling": null, "other": {"type": "yarn"}} reads as no block in my probe too.
Scope the PR states and I agree with: npu/pack.cpp fill_rope (the packed lane) still has no scaling rule — this only stops a scaled GGUF reaching it. Worth saying plainly in the PR body that the packed lane would also need one, which it does.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
…, not 800 bytes Review found the scanner wrong in both directions (PR #1090): a block that serialises its factor arrays before the type key can push "rope_type" past a fixed window, which reads as NO block, i.e. supported, i.e. served plain -- the exact silent failure this guard exists to stop (it failed OPEN); and a block with no type at all could reach a neighbour's "type" inside the window and refuse a model that asked for nothing. Braces are now matched through strings, so the window is a property of the JSON: a "}" inside a string, a nested array or a nested object cannot end the block early, and an unterminated block declares nothing rather than guessing. Tests cover all four: the review's two probes (a type 1,000 bytes into the block; {"rope_scaling": {"factor": 8.0}, "quantization_config": {"type": "fp8"}}), a nested mrope_section before the type, and a '}' inside a string. The first three FAIL on the old scanner and pass now. Re-verified end to end at this head (host GEMM, no device): the yarn container is still refused, a yarn block padded with 1,000 bytes before its type key is refused, and a real llama3 container still serves ('ctr-llama ready on http://127.0.0.1:18091').
|
Both nits fixed at
Your two probes, plus two more, are tests in End to end at this head ( The control still serves, so the tightened scanner did not turn into a guard that refuses good models. One process note for whoever reads the log: my first attempt at the padded-block run bound port 18097 for its 11 s |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at dac3a06 — both nits are fixed the way I asked, the fix has teeth, and nothing that worked before changed.
Re-ran my own probe suite against the new scanner, old vs new, same nine inputs:
OLD (0992f65) NEW (dac3a06)
E rope_type 900B into the block declared=(none) sup=1 declared=yarn sup=0 <- the fail-open, closed
F no type, neighbour "type": fp8 declared=fp8 sup=0 declared=(none) sup=1 <- the false refusal, closed
A no block / B yarn / C null trap / D `{}` / G 700B / H text_config / I longrope <- all unchanged
So it fixes exactly the two directions I raised and leaves the seven cases that were already right alone.
The regression guard has teeth, which is the part I actually care about. Compiling the new test file against the old header fails, and against the new one passes:
$ gcc -I<old header> tests/npu_rope_llama3_test.c -lm && ./a.out
FAIL a type 1,000 bytes into the block is still read (fails closed)
FAIL ... and is refused
FAIL a block with no type does not take a neighbour's type
FAIL ... and is not refused
FAIL an unterminated block declares nothing rather than guessing
rc=1
$ gcc -I<new header> … && ./a.out
all llama3 rope checks passed rc=0
Three cases, five assertions, all of which the old scanner answered wrongly — so this is a test that would have caught the bug rather than a test that describes the fix.
Edges the new brace-walk gets right, probed directly because a hand-written JSON scanner lives or dies on them: a } inside a string value ("note": "a } here" → still yarn), an escaped quote before the block's brace (yarn → linear case correct), a nested object before the type key (dynamic), and a type value containing a brace (ya}rn). Braces are matched through strings with escape tracking, so the block's window is its own closing brace rather than a magic 800.
One residual direction, stated because it is the last one: an unterminated block ({"rope_scaling": {"rope_type": "yarn" and then end-of-file) declares nothing, i.e. fails open. I would not change it — a config that truncated mid-block is a broken file that the rest of the loader also reads, and the code's own comment ("nothing trustworthy to refuse") is a defensible call. Worth knowing, not worth a third round.
Everything from my READY at a1c4cac still stands unchanged: the converter refuses yarn/linear/longrope/dynamic by name with no output directory, none/default/absent pass, the forward refuses with a named message at load (host-GEMM, device never opened), and the two controls — a valid llama3 block and a plain container — still load and serve. The delta since then is the scanner, its tests and docs/npu.md; no behaviour outside the two edge cases moved.
|
A branch-hygiene lane merged
|
A rope scaling the route does not apply is now refused by name, at both front doors
Closes #1087.
Two entry points accepted a model that declares a RoPE scaling block they do not implement, and neither said so:
tools/gguf_to_q4nx.py(the GGUF → packed-container route) read*.rope.freq_baseonly, sorope.scaling.type = yarn | linear | longrope | dynamicwas dropped: the container was written withrope_thetaalone and the packed lane computes
theta^(-i/half)(npu/pack.cpp fill_rope), which is right only inside themodel's original window.
npu/forwardimplementsllama3(npu: a pack that declares rope_type "llama3" is served plain RoPE — 17 of 32 dim pairs run up to 32x fast (Llama-3.x) #1048) and LongRoPEshort_factor, and nothing read any other type, so acontainer whose
config.jsondeclares one was loaded and served as if the block were absent.The failure mode is the reason this is worth a PR at all: the reply stays fluent and is wrong past the original
window, and no line in any log says the model was served something other than it asked for. The size of it is measured
for the case that is implemented —
npu/forward/include/q4nx_rope.h: ignoring the llama3 rule on Llama-3.2-1B givesKL 0.0058 at ctx 104, 0.0107 at 754, 0.0252 at 1497 against the same checkpoint with it applied.
Refuse rather than implement: YaRN's attention-factor schedule, dynamic NTK and look-up scaling are each a feature of
their own, and a guessed rule is worse than a named refusal.
The change
npu/forward/include/q4nx_rope.h— two header-only functions next to the llama3 rule:q4nx_rope_declared_type()(the declared type fromconfig.json, both HF spellingsrope_type/type,""when theblock is absent or
null) andq4nx_rope_type_supported()(""/default/none= plain;llama3only with usablenumbers;
longropeonly with theshort_factorthe forward reads; everything else refused).npu/forward/src/q4nx_forward.cpp—init_implcalls them after the two existing readers and fails the load with amessage naming the type. That is before any kernel or device use, and it is the path
init_hostshares, so theoffline host-GEMM route refuses the same way.
tools/gguf_to_q4nx.py—refuse_unsupported_rope(), called at the top ofpack()before the output directory iscreated (so no half-written container) and again in
config()(so no other command can bypass it).tests/npu_rope_llama3_test.cgains a 13-case table over the config shapes that exist in the wild, includingthe trap this reader could have fallen into —
"rope_scaling": nullfollowed by another block that does carry a"type"must still declare nothing.tests/gguf_to_q4nx_test.pypacks synthetic GGUF writer output for all threerefused types and asserts a non-zero exit, the type named in the message, and no
model.q4nxleft behind.Measured before / after, same reproducer
Converter,
tests/gguf_to_q4nx_test.py's synthetic qwen3 GGUF plusw.add_rope_scaling_type(YARN):Forward, host-GEMM route (
NPU_INFER_HOST_GEMM=1, no device opened) against a copy of a real container whoseconfig.jsonsays{"type": "yarn", "factor": 4.0, ...}instead of the llama3 block:OLD no guard: [semantics] per-type rope: full(theta=500000 rot=0) sliding(theta=0 rot=0) rope_type_named=0
1bit forward-serve: yarn-ctr ready on http://127.0.0.1:18099 (full ELF) <- served, block ignored
NEW guard: 1bit forward-serve: forward init failed: config.json declares rope scaling "yarn", which this
forward does not implement (only llama3 and longrope short_factor are), so it would be served plain
RoPE and reply wrongly past the model's original window (engine#1048, #1087) rc 1
CONTROL the same container with its real llama3 block: "ctr-llama ready on http://127.0.0.1:18098"
Tests:
Not a regression for anything on this box
Every GGUF here declares no scaling block (checked with the
ggufreader,rope.scaling.typeNonefor all 12:Qwen3-0.6B/4B, Qwen3-Coder-30B-A3B, Qwen3.5-4B, Qwen3.6-35B-A3B, bonsai2, zaya1-8b, BlackMamba-2.8B, Zamba2-2.7B,
ZR1-1.5B), and the two pack-imported families (
rope_type: llama3and no block at all) are on the supported side ofthe decision, which the table test pins.
Scope, stated rather than implied
npu/pack.cpp fill_rope(the packed lane route) still has no scaling rule of its own; this PR only stops a GGUF with adeclared scaling from reaching it. Implementing a rule there is the other half of #1087 and is not started here — worth
noting that the lane's
fill_ropeis hard-wired to 128 dim pairs (64 cos + 64 sin), so it could only ever serve ahead_dim-128 model anyway.
Nothing merged, tagged or published.