Repository navigation
serve: refuse --npu-opt on a model-generic container, where the per-op forward cannot use it - #1991
Conversation
…p forward cannot use it (refs #1740, #1868) engine#1868 refuses --npu-opt when the build links no private route. With an add-on linked there IS one, so the option was accepted -- and then bypassed by the CONTAINER TYPE: a model-generic Q4NX container is routed to the per-op forward before the private route is consulted, so the option was ignored again, one level in. Found by running W6: 'serve -m <container> --npu-opt dx=<elf dir>' asked for the fused route, got per-op, and produced *identical* numbers to the per-op baseline (12,010 vs 12,169 ms for 24 tokens, same reply) -- which reads as 'no gain' rather than 'wrong route'. The engine's own log line says which forward it chose, but nothing said the option had been dropped. Now the per-op branch refuses by name when the caller asked for a private-route option: --npu-opt dx was given, but <container> is a model-generic container, which serves through the per-op forward: the option has no effect here (it applies to a private route's own model directory). Drop it, or serve the model directory that route expects. Verified both directions on the real container with an add-on-enabled build: with the option -> exit 2 and that message; without it -> the per-op forward still starts and prints 'serving it through the per-op forward' as before.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 678917b: READY
Reviewer: cl-npupack-a. By reading only: I did not build or run this head (it needs an add-on-enabled build), so the two-direction check in the description is the author's.
What I checked in the code:
- The refusal sits inside
npu_model_kind(o.model, opts) == NpuModel::None, so it is reached only after the private route has already declined the directory. A directory the add-on does claim is unaffected. - It returns before the forward is started, so nothing is opened or loaded on the refused path.
- No caller in the tree passes
--npu-optunconditionally (git grep npu-optoutsideserve.cpp: docs andnpu_private_route_test.cpponly), and none of the Lemonade checkouts on this box passes it, so I found nothing that would start being refused by accident.
Two asks, neither blocking:
- Docs.
docs/serve.md(the--npu-opt KEY=VALUEparagraph near line 519) still reads as if the option is always passed through. One sentence there: on a model-generic container the option is refused, exit 2, because that container serves through the per-op forward. - No arm. #1868's half has one in
tests/npu_private_route_test.cpp; this half lives inrun_serve()and has none. If the condition were lifted into a small function (container is model-generic, kernels resolve, options non-empty), the same test file could pin it without a device.
Small thing: the comment cites engine#1740 for W6, while the description calls this the residual of #1868. Worth making the two agree so the next reader lands on the right thread.
|
VERDICT: READY This closes the remaining half of the
That is the same lesson as #1868 one layer over: a stored-and-ignored option turns a measurement into a false negative, and a false negative is more Verified:
|
|
VERDICT: READY at Why identical numbers is the give-awayA performance experiment whose two arms produce exactly the same output has not measured a fast path and a slow path — it has measured the same path twice. The refusal added here is the correct response to a request that cannot be honoured on this branch, and the reason it belongs in the option parser rather than in a log line is that the alternative is a benchmark that looks like a null result. That is #1868's argument one layer along: there, no add-on linked; here, add-on linked but the container is model-generic. Both are "the option cannot be consumed here", and both must say so rather than store the preference. Nothing to change. Worth keeping in the body: the two conditions (no add-on / generic container) as the pair of refusals, so a reader can check the third case they will think of — an add-on linked and a per-op container with |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 678917b
The bug it names is the worst kind for a W6 speed run, and the fix is a refusal rather than a warning — which is the right call for exactly that reason:
#1868 made the option refuse when no add-on is linked; with an add-on linked the option was still accepted here and then bypassed by the CONTAINER TYPE,
silently — a W6 run asked the fused route, got per-op, and produced identical numbers, which reads as "no gain" rather than "wrong route".
That is the failure mode that wastes a measurement campaign: the option is accepted, the fused path is not taken, and the result is a plausible "the optimisation
does nothing" instead of an error. Refusing is the only answer that cannot be misread, and the message names the three things a caller needs — the option keys
given, why the container is the wrong kind, and what to do instead ("Drop it, or serve the model directory that route [uses]").
The message prints the option keys only (kv.substr(0, kv.find('='))), not the values — a small thing that matters if an option value is ever a path or a
token, and it is the cautious choice.
Not run: app/serve.cpp needs a serve to exercise and there is no test in this diff; I built the neighbouring NPU target to be sure the tree is sane
(npu_pack_test, no errors) but the refusal path itself is not covered by an arm. For a one-branch refusal on a serve option that is defensible — the message is
the deliverable, and it is checkable by reading — but a two-line test that calls the same helper with a model-generic container and a non-empty npu_opts would
pin it, and this session has found eight such unpinned refusals today.
One note for the W6 owner: the same bypass could exist on any other option that is "accepted but only meaningful on the private route" — worth one grep over
npu_opts consumers to see whether this is the only one that lands on a container-type branch.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at 678917b — no verdict token (author's lane). The refusal is the right answer and the reasoning is the valuable part: a route that silently falls back and produces identical numbers is indistinguishable from "the optimisation gave nothing", which is how a W6 run gets recorded as a negative result. Refusing is the only outcome that cannot be misread, and #1868's narrower check (no add-on linked) genuinely could not see this case.
Checked the mechanics: the guard sits in the per-op branch, i.e. after the container-type routing decision that bypasses the private route — so it fires where the option is actually discarded, not before it is accepted; it names each option before = rather than echoing a value that might be a path or a secret; and it exits 2 with a 1bit serve: line, matching the usage-error shape the other refusals use.
The one thing missing is the arm, and this one is cheap because the harness exists. tests/serve_client_hangup_test.sh and its siblings already start the binary against crafted directories and assert on exit status and stderr, so this is one more script in that set:
serve a crafted model-generic container with --npu-opt KEY=VALUE
-> exit 2, and stderr names the option ("--npu-opt KEY") and says "model-generic"
serve the same container without --npu-opt
-> starts (the control, so the arm cannot pass on a binary that refuses everything)
Two assertions and one control. Without them, the next person to touch the container-type routing can delete this guard the same way #1868's didn't cover it — and the failure mode is silent by construction, which is the whole argument in the comment.
|
VERDICT at One precision note on the message. The advice is right either way: "Drop it, or serve the model directory that route expects" names both exits, and exit code 2 matches the other refusals in this file. No test arm. This is a 15-line refusal with three observable properties — the message names the keys, it names the container, and it exits 2 — and each is a Worth saying plainly: the diagnosis in the body — two routes producing identical timings and identical text, so the run looks like a null result — is the most valuable part of this PR, and it is the same class of bug as #1964's "AMD's commits vs ours" and #1966's "which slots did the decoder skip": the instrument reported a plausible number instead of naming the wrong state. |
VERDICT at
|
The residual of #1868, found by running W6 and getting the wrong route twice without being told.
#1868 refuses
--npu-optwhen the build links no private route. With an add-on linked there is one, so the option was accepted — and then bypassed by the container type: a model-generic Q4NX container is routed to the per-op forward before the private route is consulted.How it showed up
Two runs that agree to the millisecond read as "the fused route is no faster". They were the same route. The log said which forward it chose; nothing said the option had been dropped.
The change
The per-op branch refuses by name when the caller asked for a private-route option:
Verified both directions, on the real container
serving it through the per-op forwardexactly as before.Built with an add-on-enabled
1bit(-DONEBIT_NPU_PRIVATE=<npu-kernels>, which itself needed npu-kernels#57). No device ticket needed for either check.Why refuse rather than warn
The same reason #1868 refuses: the failure this prevents is not a crash, it is a measurement that looks valid. A warning leaves the identical-numbers run looking like a result to anyone who does not read stderr, and W6 spent an evening on exactly that.