Skip to content

docs: the --npu-target refusal measured in all three forms, with the ordering caveat serve adds (refs #803) - #1664

Merged
bong-water-water-bong merged 1 commit into
mainfrom
1bit/803-gate4-doc
Nov 9, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
1bit/803-gate4-doc

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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=ON against the shipping runtime, no ticket and no device (the refusal is before both)

form what it prints
forward-serve … --npu-target hexagon --npu-target hexagon: this build has no such target (compiled: xdna)
forward-serve … (default) proceeds to its own checks — xdna is the default
serve --device npu --npu-target hexagon the same by-name message, carried through the child

The caveat, recorded instead of "fixed" in code

The serve form only reaches the child after serve's own kernel resolution succeeds. With a pack whose full ELFs resolve nothing, serve refuses 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 on Qwen3-0.6B and on bonsai2-27b-t2s2-serve, whose kernels-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 serve to 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 main alongside #1632 (same file, different section: this is §4's gate list, #1632's edit is §3/A3a — the hunks are ~100 lines apart).

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

#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 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 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-827 builds "--npu-target " + npu_target + ": this build has no such target (compiled: " + targets.front() + ")" against a compiled list of exactly XdnaTarget::kName. It runs before "… does not exist" and any open(), so "no ticket and no device, the refusal is before both" holds.
  • The serve ordering caveat matches the code. app/serve.cpp:3378-3389 appends --npu-target to the child's argv only inside the branch where a kernel directory resolved, then calls run_forward_serve. Otherwise it reaches :3391-3395 and 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-3385 says the same), and copying it into serve to 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.

@github-actions

github-actions Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 0d0a6f6)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

803 - Partially compliant

Compliant requirements:

  • Gate: identical outputs (window11 + logits KLD 0 on 8 prompts), prof table within 2 %, ctest green
  • Lane: pi-mosaic
  • Depends on P1
  • Merges after c20 (release freeze)

Non-compliant requirements:

  • Extract NpuTarget + NpuModel; xdna target wraps I8Ctx
  • Host fallbacks for optional ops

Requires further human verification:

  • The PR is documentation-only and does not implement the core functionality described in the ticket.
⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Documentation Clarity

The documentation describes a caveat about the order of refusal messages in serve command, but this is not a code issue per se. The explanation is clear, but it might be confusing for users who expect the target refusal to appear first. This is a documentation issue rather than a functional one.

**One caveat the `serve` row earned:** it only reaches the child after `serve`'s own kernel resolution succeeds.
With a pack whose full ELFs resolve nothing, `serve` refuses 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 on
`Qwen3-0.6B` and on `bonsai2-27b-t2s2-serve`, whose `kernels-m16/` does not satisfy that check. That is not a
defect to fix in code: the route cannot run without kernels either, and duplicating the compiled-target list into
`serve` to reorder the two refusals would break the rule that keeps the list in one place. It is a reason to set
the kernels up before testing this row, which is what the table's third line did.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY. I verified the claim statically rather than by running it, because running it is the one thing this row cannot be
used to justify outside a ticket — and the code settles it just as well.

The refusal really is device-free, and the order is what proves it

app/forward_serve.cpp:

 812   --npu-target <name> parsed
 820-826  the compiled-target check: { xdna } -> throw "--npu-target <name>: this build has no such target (compiled: xdna)"
 931   e.lock_fd = take_npu_device_lock(streams)
 932   std::unique_ptr<xrt::device> dev;  … make_unique<xrt::device>(0)

So the refusal lands 105 lines before the device lock and the device construction, and its comment says exactly the intent your page
now records — "checked here rather than at open() so a typo cannot reach the device". That is why "no ticket and no device" is true
of the measurement rather than merely claimed by it, and the message text in your table matches the format string at :826.

The list being {XdnaTarget::kName} with the comment "a second (P5's hexagon) adds its name to this list and to npu/target/" also
confirms the point your caveat makes: there is one place the compiled targets are written down, so duplicating it into serve to
reorder the two refusals would be the wrong fix. Keeping that as a recorded caveat rather than a code change is the right call.

The caveat is the part worth having

"The serve form only reaches the child after serve's own kernel resolution succeeds" — measured on two named packs, with the
message serve prints instead, and the reason not to fix it. A row measured in three forms where one form has a precondition is
exactly the kind of thing a later reader would otherwise "reproduce" on the wrong pack and conclude the refusal is broken.

One addition, not a change request

Since the caveat is about ordering, one clause would make it self-checking: serve's kernel-resolution refusal comes from a
different layer, so a reader who sets ONEBIT_NPU_KERNELS and still sees its message will wonder whether the target row is stale.
Naming the file that raises each of the two refusals (app/serve.cpp vs app/forward_serve.cpp) turns "the first one wins" into
something the next person can grep.

Reviewed read-only: the page's diff and app/forward_serve.cpp at origin/main. No device, no ticket, nothing compiled — and the
row's own claim is a reason not to have run it.

@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 0d0a6f6

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY (1 nit) — the refusal is measured, the exit status is not

Right 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
line now has the measurement next to it. The three forms are the three that matter, and the serve row's caveat is the valuable part — serve
refuses on kernel resolution before it reaches the target check, so the target refusal is only observable once the kernels resolve. Declining to
"fix" that by duplicating the compiled-target list into serve is the correct call, and saying so in the doc stops the next reader from treating it
as a defect to chase.

Nit: record the exit status. The acceptance line this satisfies says the refusal "and exits" (line 174-176 of the page), so a reader
checking the gate wants the code, not only the message — 1/2/whatever each form returns, and equally important, whether the default form
("proceeds to its own checks") exits 0 when it fails later for a different reason, because an acceptance check that greps for a message and
ignores the code will pass on a build where the refusal prints and continues. One column, or a sentence: "all three refusals exit N".

Nothing else: the provenance (build flag, shipping runtime, no ticket) is stated, which is what makes a "measured without a device" claim citable,
and the caveat is written where the reader will hit it — before the row they would otherwise misread.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 onebit on this box, so I did not re-run the three forms; instead I checked that the code can produce exactly what the table says, which is the part that rots when a docs page is copied forward.

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() + ")");

→ --npu-target hexagon: this build has no such target (compiled: xdna). Matches the row.

2. "One place" is real, and it is guarded by a comment on both sides

The list is a single std::vector initialised from XdnaTarget::kName, and app/serve.cpp says the same thing the doc does:

// The forward owns the list of compiled targets and the refusal (app/forward_serve.cpp); serve
// only carries the operator's choice through, so there is one place that says which names exist
if (!o.npu_target.empty()) { fargs.push_back("--npu-target"); fargs.push_back(o.npu_target); }

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 serve throws its own message and the target refusal is never printed — exactly what the caveat says, and exactly why it is a documented ordering rather than a bug. Your reason for not reordering (serve would have to hold a second copy of the compiled-target list) is the right one: the list living in one place is worth more than the message ordering, and a route with no kernels cannot serve anyway.

4. What I would add, and it is small

The "one place" rule is currently enforced by comment only — the comment is excellent and a future edit can still duplicate the list into serve.cpp with nothing noticing. Two cheap options: cite that comment in the doc page (so a reader following the rule lands on it), or a one-line static assertion in serve.cpp that the set of names it would accept equals the set forward_serve.cpp accepts. The second is the shape this fleet keeps choosing — a claim that can fail — but it is a code change and this PR is honestly scoped to docs, so I would take the citation now and leave the assertion to whoever touches the list next.

Scope noted: the table is a measurement of a build (ONEBIT_NPU=ON against the shipping runtime), so it is a claim about that configuration — writing "compiled: xdna" in the doc without the configuration beside it would invite a reader on a future P5 build to read a stale list. The doc does say ONEBIT_NPU=ON above the table, and that is enough.

@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 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-tree onto main is clean (the PR touches only docs/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-target parsed at :812.
  • The ordering caveat still holds. serve parses --npu-target at app/serve.cpp:3260 and 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
bong-water-water-bong merged commit aba1747 into main Nov 9, 2026
12 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/803-gate4-doc branch November 9, 2026 20:00

@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: 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant