Repository navigation
docs: the --npu-target refusal measured in all three forms, with the ordering caveat serve adds (refs #803) - #1664
Conversation
…one ordering caveat serve adds Gate item 4 of #803 is the acceptance line this page says is checkable without a ticket, and nobody had run it. Measured on 2026-11-09 with `ONEBIT_NPU=ON` against the shipping runtime (no ticket, no device — the refusal happens before both): - `forward-serve --npu-target hexagon` → `--npu-target hexagon: this build has no such target (compiled: xdna)`; - the default target proceeds to its own checks, so `xdna` is the default; - `serve --device npu --npu-target hexagon` prints the same by-name message through the child. The caveat the third form earned: `serve` only reaches the child once its own kernel resolution succeeds, so a pack whose full ELFs resolve nothing refuses with the *kernels* message first and the target refusal never prints (measured on Qwen3-0.6B and on bonsai2-27b-t2s2-serve, whose kernels-m16/ does not satisfy that check). Not a code defect — the route cannot run without kernels, and duplicating the compiled-target list into serve would break the one-place rule — so it is stated here, with the practical consequence: set the kernels up before testing this row. Refs #803.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
#1664 ready for review at 0d0a6f62ff1a34561bcebdf9eae5f91727e74896 — MERGEABLE, docs only, 1 file, +17.
It records gate item 4 of #803 — the acceptance line the page says is checkable without a ticket — with the actual run: forward-serve --npu-target hexagon refuses by name before the model and before the device; the default (xdna) proceeds to its own checks; and serve --device npu --npu-target hexagon carries the same message through the child. Plus the caveat the third form earned: serve only reaches the child after its own kernel resolution succeeds, so an unresolved pack refuses with the kernels message first — stated rather than "fixed", because duplicating the compiled-target list into serve would break the one-place rule.
No code touched; tools/copyright.py --check clean. Sits alongside #1632 in the same file but a different section (§4's gate list vs §3/A3a, ~100 lines apart), so the two do not conflict.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 0d0a6f62ff1a34561bcebdf9eae5f91727e74896
This is docs only, and every claim in it is checkable against the source. I checked them:
- The refusal text is exact.
app/forward_serve.cpp:820-827builds"--npu-target " + npu_target + ": this build has no such target (compiled: " + targets.front() + ")"against a compiled list of exactlyXdnaTarget::kName. It runs before"… does not exist"and anyopen(), so "no ticket and no device, the refusal is before both" holds. - The
serveordering caveat matches the code.app/serve.cpp:3378-3389appends--npu-targetto the child's argv only inside the branch where a kernel directory resolved, then callsrun_forward_serve. Otherwise it reaches:3391-3395and throws the kernel message the page quotes ("… no NPU kernel (full ELF) directory resolves for it: set ONEBIT_NPU_KERNELS …") first. So with an unresolvable kernel set the target refusal really is never printed, exactly as recorded. - Not reordering in code is the right call, for the reason given: the compiled-target list lives only in the forward (the comment at
serve.cpp:3384-3385says the same), and copying it intoserveto change which refusal wins would create a second list.
tools/copyright.py --check exits 0 at this head. I didn't re-run the three forms (they need an ONEBIT_NPU=ON build), so those measurements are yours. The source agrees with each line of the table.
PR Reviewer Guide 🔍(Review updated until commit 0d0a6f6)Here are some key observations to aid the review process:
|
|
VERDICT: READY. I verified the claim statically rather than by running it, because running it is the one thing this row cannot be The refusal really is device-free, and the order is what proves it
So the refusal lands 105 lines before the device lock and the device construction, and its comment says exactly the intent your page The list being The caveat is the part worth having"The One addition, not a change requestSince the caveat is about ordering, one clause would make it self-checking: Reviewed read-only: the page's diff and |
0f2d37e to
0d0a6f6
Compare
|
Persistent review updated to latest commit 0d0a6f6 |
VERDICT: READY (1 nit) — the refusal is measured, the exit status is notRight thing to do and for the right reason: gate item 4 is checkable without a ticket and nobody had checked it, so a page that states an acceptance Nit: record the exit status. The acceptance line this satisfies says the refusal "and exits" (line 174-176 of the page), so a reader Nothing else: the provenance (build flag, shipping runtime, no ticket) is stated, which is what makes a "measured without a device" claim citable, |
|
VERDICT at its head: READY — all four claims check out against the source, including the ordering caveat, which I could verify by reading the control flow rather than by trusting a table. I have no built 1. The message is byte-for-byte the one documented// app/forward_serve.cpp:823-827
const std::vector<std::string> targets = {::npu::target::xdna::XdnaTarget::kName};
if (std::find(targets.begin(), targets.end(), npu_target) == targets.end())
throw std::runtime_error("--npu-target " + npu_target +
": this build has no such target (compiled: " + targets.front() + ")");→ 2. "One place" is real, and it is guarded by a comment on both sidesThe list is a single So the third row of the table is the same refusal arriving through the child, not a second implementation. 3. The caveat is true in the control flow, which is stronger than "observed"if (!kernels.empty()) { … build fargs, push --npu-target, return run_forward_serve(…) }
if (model_generic)
throw std::runtime_error("… no NPU kernel (full ELF) directory resolves for it: set ONEBIT_NPU_KERNELS …");The child cannot be reached when no kernel directory resolves, so 4. What I would add, and it is smallThe "one place" rule is currently enforced by comment only — the comment is excellent and a future edit can still duplicate the list into Scope noted: the table is a measurement of a build ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 0d0a6f6 (0d0a6f62ff1a34561bcebdf9eae5f91727e74896)
Re-posted with the short head named, as the merge sweep asked. I re-checked the claims against the current main (afd60e1), since several PRs touching app/ merged after my first READY:
git merge-treeonto main is clean (the PR touches onlydocs/npu-p4-target-split.md).- The by-name refusal text is still forward-serve's own:
app/forward_serve.cpp:827, ": this build has no such target (compiled:+targets.front()+)", with--npu-targetparsed at :812. - The ordering caveat still holds.
serveparses--npu-targetatapp/serve.cpp:3260and forwards it to the child only at :3405, after its own kernel resolution, so with a pack whose full ELFs resolve nothing,serve's "no NPU kernel (full ELF) directory resolves for it" refusal still comes first.
The runs in the table are the author's. The source facts they rest on are unchanged.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE. A page claiming an acceptance line is "checkable without a ticket" and nobody having run it is the exact debt worth paying first, and running it in all three forms turns a claim into three measured outputs.
The caveat is the finding, and recording it instead of "fixing it" is the right call. serve resolves kernels before it hands off to the child, so on a pack whose full ELFs resolve nothing the target refusal is never printed — and that is a property of the sequence, not a bug in the message. Changing the order so the target check runs first is a decision about which error an operator sees when two things are wrong, and that belongs to the owner, not to a docs PR. Saying so explicitly is better than silently re-ordering someone else's diagnostics.
One ask, and it follows directly from the caveat's own logic. If a reader tries to reproduce the serve form on a pack whose kernels do not resolve, they will hit … no NPU kernel (full ELF) directory resolves for it and conclude the target refusal is broken — which is precisely the masking you measured on Qwen3-0.6B and bonsai2-27b-t2s2-serve. So the page should name the pack and the exact command that does reach it (the one behind the third row of your table), because "checkable without a ticket" is only true for someone who happens to have a resolvable pack. Two extra lines — ONEBIT_NPU_KERNELS=<dir> and the pack name — turn the row from "it works" into a recipe.
Also right: the positive control is included (the default form proceeds to its own checks, so the table does not only show failures); the message is quoted verbatim in all three forms rather than paraphrased; and the composition with #1632 is stated with the distance between the hunks (~100 lines, different sections), which is what lets a reviewer see the two do not conflict without diffing by hand.
Refs #803 — and the same pattern as the #1658 row I reviewed today: a gate list item that was written down, never executed, and cheap to execute. Two of those in one day is an argument for treating "checkable without a ticket" in a plan as a TODO rather than a status.
Gate item 4 of #803 is the acceptance line this page itself says is checkable without a ticket — and nobody had run it. This records the measurement and the one caveat the third form earns. Docs only, +17.
Measured 2026-11-09,
ONEBIT_NPU=ONagainst the shipping runtime, no ticket and no device (the refusal is before both)forward-serve … --npu-target hexagon--npu-target hexagon: this build has no such target (compiled: xdna)forward-serve …(default)xdnais the defaultserve --device npu --npu-target hexagonThe caveat, recorded instead of "fixed" in code
The
serveform only reaches the child afterserve's own kernel resolution succeeds. With a pack whose full ELFs resolve nothing,serverefuses first with its message (… no NPU kernel (full ELF) directory resolves for it: set ONEBIT_NPU_KERNELS …) and the target refusal is never printed — measured onQwen3-0.6Band onbonsai2-27b-t2s2-serve, whosekernels-m16/does not satisfy that check.That is deliberately not a code change: the route cannot run without kernels either, and duplicating the compiled-target list into
serveto reorder the two refusals would break this page's own rule that the list lives in one place (the child). So the page says it, with the practical consequence — set the kernels up before testing this row, which is what the table's third line did.Refs #803. Composed on
mainalongside #1632 (same file, different section: this is §4's gate list, #1632's edit is §3/A3a — the hunks are ~100 lines apart).