Skip to content

serve: refuse --npu-opt on a model-generic container, where the per-op forward cannot use it - #1991

Merged
bong-water-water-bong merged 1 commit into
mainfrom
fix/npuopt-container-bypass
Nov 10, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
fix/npuopt-container-bypass

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

The residual of #1868, found by running W6 and getting the wrong route twice without being told.

#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.

How it showed up

per-op                        11,943 ms / 12,010 ms    "The first person to walk on the Moon was Neil Armstrong…"
--npu-opt dx=<elf dir>       12,169 ms                 identical reply
1bit serve: …/w6-llama3bi19.q4nx is a model-generic container: serving it through the per-op forward

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:

--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 own model directory). Drop it, or serve the model
directory that route expects.

Verified both directions, on the real container

  • 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 exactly 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.

…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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-opt unconditionally (git grep npu-opt outside serve.cpp: docs and npu_private_route_test.cpp only), 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:

  1. Docs. docs/serve.md (the --npu-opt KEY=VALUE paragraph 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.
  2. No arm. #1868's half has one in tests/npu_private_route_test.cpp; this half lives in run_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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY cb21235

This closes the remaining half of the --npu-opt story, and the failure it names is the one that is hardest to notice:

with an add-on linked the option was still accepted here and then bypassed by the CONTAINER TYPE, silently — a W6 run asked for the fused route, got
per-op, and produced identical numbers, which reads as "no gain" rather than "wrong route". Refusing is the only answer that cannot be misread.

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
expensive than a crash because someone concludes the feature does not help. #1868 covered the no-add-on case; this covers the container that cannot
consume it.

Verified:

  • it refuses rather than warns — the block ends in return 2;, so the run stops instead of serving the wrong route with a message nobody reads
    mid-startup;
  • the message lists the keys it was given (for (const auto& kv : o.npu_opts) keys += … substr(0, kv.find('='))), so "Drop it, or serve the model
    directory that route …" is actionable against the exact option the operator typed;
  • the branch is the model-generic-container path specifically (the per-op forward), so a private route's own directory keeps --npu-opt working — no
    regression for the case the option exists for.

ci_targets is untouched (one file, no target). Nothing requested: a refusal that names the option, the reason and the alternative is the complete form.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 678917b — the completion of #1868, and the sentence that identifies the failure mode is "produced identical numbers".

// engine#1740 (W6): --npu-opt is for a PRIVATE route, and this branch is the per-op forward, which
// cannot consume one. #1868 made the option refuse when no add-on is linked; with an add-on linked
// option was still accepted here and then bypassed by the CONTAINER TYPE, silently -- a W6 run ask[ed]
// the fused route, got per-op, and produced *identical* numbers, which reads as "no gain" rat[her than]

Why identical numbers is the give-away

A 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 --npu-opt — is covered by one of them.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 678917b: READY — the refusal is the right answer, and the reason given in the comment is the important one: an option that is accepted and then bypassed produces identical numbers, which reads as "no gain" rather than "wrong route". That is a measurement-integrity bug, not a UX one, and refusing is the only outcome that cannot be misread.

One precision note on the message. app/serve.cpp:3385 parses the options before this branch, and the parsed opts is an input to npu_model_kind(o.model, opts) — i.e. --npu-opt participates in classifying the model, not only in the route the forward then takes. There are no opts. reads after that line, so "the option has no effect here" is true for the forward and not literally true for the process. Two words would close it: the option has no effect on the per-op forward. As written, a reader could conclude the flag is inert — and then be surprised when a wrong key changes which route is chosen.

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 serve invocation with a generic container. Given the incident it fixes was silent, an arm would be worth more here than in most places: a green suite that never exercises the refusal is how the option got accepted-and-bypassed in the first place.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 678917b: READY — the option was accepted and then bypassed by a container type, and the symptom was identical numbers

This is the most insidious failure shape in the queue tonight, and the body names it: #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 because a model-generic Q4NX container is routed to the per-op forward. The result: "the fused route, got per-op, and produced identical numbers, which reads as 'no gain' rather than 'wrong route'." A user who runs W6 and gets the same throughput with and without --npu-opt will conclude the optimisation does not help — and the actual answer is that the flag never did anything. That is worse than an error, because it produces a measurement that is confidently wrong.

"Refusing is the only answer that cannot be misread" is exactly right, and it is the same judgement as #1668's load-mode observation: an option that silently degrades makes the tally the header mislabels.

Three asks, in order:

  1. The refusal must distinguish the two "cannot use it here" cases, since they need different user action: no add-on linked (npu: refuse --npu-opt when the build links no private route, instead of storing an option nothing reads #1868) means install/rebuild; model-generic container (this) means the pack cannot take the private route at all, and no rebuild will change that. If both share one message, the user will chase the wrong one — worth the container's type and what would be needed instead.
  2. A positive arm that the fused route is actually taken on a container that can consume it: the whole value of the check is the pairing, and a test suite that only asserts refusals would pass on a build where --npu-opt never worked anywhere.
  3. Log the chosen route at startup (one line: route: fused / route: per-op (model-generic container)), so the W6 measurement that found this is reproducible from a log rather than from remembering which flag was passed. That converts the discovery into a permanent property.

And one meta-point worth putting in the body: this was found by running W6 and getting the wrong route twice without being told — i.e. by a measurement that produced a plausible null result. The general lesson is that "no gain" and "no effect" are indistinguishable from output alone, which is the same argument for the /proc/<pid>/maps control in #1913 and for printing the load mode in the preflight header.

@bong-water-water-bong
bong-water-water-bong merged commit f7ecac4 into main Nov 10, 2026
9 of 10 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the fix/npuopt-container-bypass branch November 10, 2026 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant