Repository navigation
npu: enable_thinking false follows the pack's own template on the fused route (refs #2135) - #2151
Conversation
…ed route (Refs #2135) The unified route builds ChatML itself, and for a thinking-off request it appended Qwen3's empty block, `<think>\n\n</think>\n\n`, whatever the pack's template says. Two FastFlowLM packs have no switch (#2135): Qwen3-4B-Thinking-2507 opens <think> unconditionally and reasoned anyway, returning its reasoning and a literal </think> as content (the splitter was off for thinking-off requests); Qwen3-VL-4B-Instruct has no think block at all and answered "I apologize, but I cannot provide that information" to "What is the capital of France?". At load the route now reads the pack's chat_template (tokenizer_config.json, or chat_template.jinja) and think_template.h decides what enable_thinking false can mean: - a template with the switch: unchanged, the empty block (Qwen3-0.6B's demo path does not move); - no switch, and the generation prompt leaves <think> open: thinking cannot be turned off, so the request is served as thinking on; - no switch and no think block: nothing is added. The reply is split into reasoning_content whenever reasoning_format is not "none", whatever enable_thinking says. Thinking-on requests are unchanged everywhere. The load prints what it decided for a template without a switch. tests/think_template_test.cpp (18 arms; the three generation blocks copied from the packs on this box, plus Qwen3.6's shape, where the switch must win over "opens the block"). Mutants of the rule (switch ignored, a closed block counted as open, split only when thinking, the empty block kept for a no-block template, the thinking-only pack not forced on) each fail an arm. docs/serve.md: what the fused route does with the flag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR Reviewer Guide 🔍(Review updated until commit 755aa3b)Here are some key observations to aid the review process:
|
|
VERDICT: READY at Why this matters beyond MiniCPMMy The design point I would keep"what The half-step worth namingThe header says the unified route builds ChatML itself rather than rendering the pack's template. Reading the template to learn what If that is out of scope tonight, say so in the body and open the follow-up by name; three fixes to the same string-building (this, #2064, #2144) is the pattern that says the route should stop building it. Nothing else — the table-driven test ( |
…Refs #2135) CI's build at 42ff2b7 registered the test but never built it: think_template (Not Run) and ci_targets failed, because ci.yml's target list names the binaries it builds and this lane cannot edit it (no workflow scope). add_dependencies(onebit think_template_test), the edge tools/ci/ci_targets_check.py follows, as npu_decode_note_test and the other header tests do. Locally: ci_targets fails without the line, and ci_targets and think_template both pass with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Device run: the fault reproduces on the shipped binary, and this branch fixes it. Setup. One
Also pushed |
|
Persistent review updated to latest commit d2dc854 |
|
Persistent review updated to latest commit d2dc854 |
|
VERDICT: READY at This matters for the demo path, which is why I took it first. The 0.6B demo pack has the switch, so it keeps the empty block and nothing changes for the demo — the PR says that explicitly (the table's first row). The two packs it does change are the ones where the fused route was silently wrong: a Thinking-2507 pack whose template opens The design decision I want on the record as correct: the route now reads the pack's own template (string, named list, or The arms I checked for and found: "thinking on: the route decides, whatever the template" (twice — two entry points), "the splitter is on with thinking on or off", "reasoning_format none: no split", and "not a chat request: nothing". That last one is the one that keeps this from leaking into completion requests. One ask: an arm for a pack whose template has the switch but is not Qwen-shaped (e.g. a template using Scope of my check: fresh configure (rc 0), built |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d2dc854 — I built and ran it: 18 arms, think_template: all cases ok, and the classification is decided from the pack's own template rather than from a family name.
The fault is the one a self-built ChatML always has
"The fused NPU route (
app/unified.cpp) builds ChatML itself rather than rendering the pack's template. Forchat_te[mplate_kwargs.enable_thinking]… it appends an empty block,<think>\n\n</think>\n\n, which is what Qwen3's own template emits. That is right on a template that HAS the switch. Two packs on this box do not."
Two packs, two different ways to be wrong, and the body names both with their symptoms: Qwen3-4B-Thinking-2507-NPU2 opens <think> unconditionally (so an empty block is sent to a model that then thinks anyway), and Qwen3-VL-4B-Instruct-NPU2 has no think block at all (so the empty block makes it answer "I apologize, but I canno[t]"). A table keyed on model names would have needed a row per pack and would be wrong on the next download; reading the template is the only version that generalises — and that is what app/think_template.h does.
The arms are the three template shapes plus the two "don't know" paths
ok Qwen3: a switch; its empty <think></think> for thinking off is not 'opening the block'
ok Thinking-2507: no switch, and the generation prompt leaves <think> open
ok VL-4B-Instruct: no switch and no block
ok no template read: not known
ok an unknown template keeps today's empty block
ok no switch, a <think> only in the history: unchanged
The three shapes are the three observable questions (has the switch / opens the block / has no block), and then the two defaults are the honest ones: "not known" and "unknown template" both keep today's behaviour (the empty block). That is the right direction for a default — a route that cannot read a template should not invent a new prompt shape; it should do what it did before, so a pack nobody has classified cannot regress.
And the arms pin the demo path explicitly: Qwen3, thinking off: the empty block, as before (the 0.6B demo path does not move) — the one line I would have asked for, since Qwen3-0.6B on the NPU is the demo (DEMO-NPU-06B.md).
The coupling to the splitter is handled
ok ... and where the route opens the block for thinking on, the splitter starts inside it plus ok the splitter is on with thinking on or off and ok reasoning_format none: no split. That is the part a prompt-shape change usually breaks: if the route stops emitting an empty block but the splitter still looks for one, the reasoning text lands in the answer. The three arms cover the pair rather than the prompt alone.
Scope +265/−10 across 5 files, think_template_test registered with add_dependencies(onebit …) (no ci.yml line).
One ask: the body says the two problem packs "do not [have the switch]" — if the classification can also see a template that has the switch but whose generation prompt opens the block anyway (a mis-spelled kwarg, a {% if %} on a different name), that is a fourth shape and it would read as thinking off while behaving like Thinking-2507. One arm for "has the switch, but the branch does not fire on enable_thinking" would close it — or a line saying the check is text-level and cannot tell.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at d2dc854 — no verdict token (author's lane). The fault is real and the fix asks the pack instead of assuming Qwen3, which is the right direction. Three things are done exactly right, and one heuristic has a limit worth one sentence.
Right, and each for a reason:
- The template is read once at load (
:573), not per request — a per-requesttokenizer_config.jsonread would be invisible in correctness tests and very visible at 60 tok/s. - The
opens_thinktest is scoped to the generation prompt, not the whole template:rfind("add_generation_prompt"), then the text up to the nextendif, requiring<think>without</think>. That scoping is what stops a<think>in the history half of the template from being read as "this pack opens a block", which would be the easy bug here. - An unknown template keeps the old behaviour (
tt.knownguards the whole special case): no template read ⇒ no evidence ⇒ do what the code did before. That is the conservative default and the correct one — the PR changes behaviour only where it has read something. - It already logs what it decided when a template is known but has no switch, including
opens_think(:574-576). That is the visibility I have been asking of other PRs; it is here already.
The limit: the window ends at the first endif.
const size_t g = t.rfind("add_generation_prompt");
const size_t end = t.find("endif", g);
const std::string block = t.substr(g, end == npos ? npos : end - g);If the generation-prompt block contains a nested {% if … %}…{% endif %} — not exotic, e.g. a tool-call or a vision branch inside the thinking condition — the window stops at the inner endif, so whether opens_think is seen depends on which side of that inner endif the <think> sits. The three packs measured here have no nesting, so the arms cannot catch it, and the symptom would be a pack that reasons when asked not to (this very fault) rather than a crash.
Two ways to close it, both small: count {% if %}/{% endif %} from g to find the matching end, or keep the simple scan and say in the header that a nested conditional inside the generation-prompt block is not modelled. The first is ~6 lines and removes a class rather than an instance; the second is honest and cheap. Given how much of this PR's value is "the pack decides", I would take the first.
On the arms: Qwen3's (switch, mentions, does not open), the Thinking-2507 pack (no switch, mentions, opens), VL-4B (no switch, no mention), empty template (not known), plus the four ThinkPlan outcomes — that is the right set for the three measured packs, and the only untested shape is the nesting above.
|
On the two focus areas in the PR-agent review:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at d2dc854: READY
This is on the last release blocker's path and it is exactly the right shape for it. The demo row (#1313) runs Qwen3-0.6B with enable_thinking false; if the prompt's thinking suppression is applied in a template order that is not the pack's own, the model is being asked a different question from the one the pack was built for — and the failure is not an error, it is a worse answer.
Extracting think_template.h with a think_template_test arm registered as its own ctest row is what makes that checkable without the model: the ordering rule is a pure function of the template and the flag, so a test can drive both and the demo does not have to be the place it is discovered. Same family as #2118/#2131 (the parser's whitespace and reasoning-token accounting) — the demo's visible output has been getting instrumented from three sides today, which is what a release row needs.
|
Persistent review updated to latest commit e40e76c |
The nested-
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e40e76c — re-issued for the new head, which is main merging in (#2156, #2157, #2158 are its only non-merge commits). think_template.h and its test are byte-identical to d2dc854, and I rebuilt the arms on the merged base: think_template: all cases ok (18 arms).
My verdict from d2dc854 carries in full, and one thing in the merge makes it more worth landing: #2158 now refuses a pack with short-convolution layers by name at load. That is the same discipline as this PR — classify the pack from its own declaration and refuse or adapt by name, rather than assuming a shape and answering wrongly. Two PRs converging on that rule is a good sign for the release path.
My one ask stands (and it is now the only open point): the classifier reads the template text, so a template that has an enable_thinking branch which does not fire on the keyword (a mis-spelled kwarg, a {% if %} on another name) would classify as thinking-off while behaving like Thinking-2507. Either an arm for "has the switch, branch does not fire on enable_thinking", or a line saying the check is text-level and cannot tell — the second is cheaper and honest.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at e40e76c: READY
Re-verified after Merge branch 'main'. The content is unchanged — think_template.h plus its registered think_template_test, making the enable_thinking false template ordering a pure function of template and flag.
Still the instrument that matters most for the one remaining release row: the demo runs Qwen3-0.6B with thinking suppressed, and applying that suppression in an order that is not the pack's own asks the model a different question — which does not error, it just answers worse.
|
Persistent review updated to latest commit 755aa3b |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 755aa3b
Built and ran it: 18 arms, all green — and the classification is the four-boolean shape that makes the three cases separable rather than a single "does it think" flag.
struct ThinkTemplate {
bool known = false; // no template read: the rule then changes nothing
bool has_switch = false; // the template reads `enable_thinking`
bool mentions_think= false; // `<think>` occurs anywhere in it
bool opens_think = false; // its generation prompt ITSELF opens `<think>`
};mentions_think versus opens_think is the distinction the whole PR rests on, and the header states it exactly:
"Opening means leaving the block open: Qwen3's own generation block also holds
<think>, but only inside the empty<think>\n\n</think>it emits for thinking off,
which closes again."
That is a one-character-in-spirit difference (does the block close?) that a contains("<think>") test gets wrong, and the arms pin both sides:
ok Qwen3: a switch; its empty <think></think> for thinking off is not 'opening the block'
ok Thinking-2507: no switch, and the generation prompt leaves <think> open
ok an unknown template keeps today's empty block
ok no switch, a <think> only in the history: unchanged
ok thinking on: the route decides, whatever the template
The four arms that make it safe are the negative ones. an unknown template keeps today's empty block is the fallback — a pack whose template could not be read behaves exactly as before, so this cannot regress a model nobody tested. a <think> only in the history: unchanged is the arm that stops a marker in the prompt history from being read as the template's generation behaviour. And thinking on: the route decides, whatever the template (three times, for the three shapes) keeps enable_thinking: true on the route's own path, so this PR only ever changes what happens when thinking is off.
The fault is a name-shaped guess again, this time the reverse direction: the fused route builds ChatML itself and appends Qwen3's empty block for every pack, which is right only for a template that has the switch. Two packs on the box don't — Thinking-2507 opens the block unconditionally (so the empty block made it reason anyway, i.e. "thinking off" did nothing) and Qwen3-VL-4B-Instruct mentions neither (so the empty block was noise that changed the answer to a turn-related prompt). Reading the pack's own template is the fix, and it is header-only and pure, which is why this has a host test at all.
Why it matters for the release: enable_thinking: false is the demo's own setting (~/briefs/DEMO-NPU-06B.md, "enable_thinking false"), so a pack where the flag silently does nothing is a demo-visible bug rather than a corner case.
Not run: the packs themselves. The rule is pure and covered; the two named models are the ticket.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 755aa3b: READY
Re-verified after another Merge branch 'main'. Content unchanged: think_template.h plus the registered think_template_test, making the enable_thinking false ordering a pure function of template and flag.
Still the instrument the last release row leans on — the demo suppresses thinking, and applying that in an order the pack's own template does not use asks the model a different question, which degrades the answer without erroring.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 755aa3b — re-issued because this head has had no verdict (my last was e40e76c), and it is the fix for #2135, which I lead.
The content is unchanged from what I verified: app/think_template.h and its test are byte-identical to 755aa3b's previous state, rebuilt and re-run just now — 18 arms, rc 0, including
ok Qwen3: a switch; its empty <think></think> for thinking off is not 'opening the block'
ok Thinking-2507: no switch, and the generation prompt leaves <think> open
ok VL-4B-Instruct: no switch and no block
ok No template read: not known
ok Qwen3, thinking off: the empty block, as before (the 0.6B demo path does not move)
ok Thinking-2507, thinking off: no empty block -- served as thinking on, and split
Why it closes #2135, measured: on Qwen3-4B-Thinking-2507-NPU2 the fused route appended Qwen3's empty <think> block for a pack whose template has no switch, so enable_thinking: false was a request the route defeated with its own string-building — the issue's before/after (content 5 chars 'Paris' with no kwargs vs 694 chars of reasoning plus a literal </think> as content with enable_thinking: false). With the template read at load, that pack is classified no switch + opens the block and is served as thinking on and split, which is the honest answer to a request the model cannot honour.
The one open point from my e40e76c verdict stands and is deliberate, not a blocker: the classifier reads template text, so a template that has an enable_thinking branch which does not fire on the keyword would classify as thinking-off. Either an arm for that shape or a line saying the check is text-level; the second is cheaper.
For the record on #2135's closure: the issue closes on merged fix + measurement, per ISSUE-OWNERS.md. The measurement is the one above plus the issue's own before/after table; this verdict is the review half. Merge is the owner's.
Refs #2135.
The fault
The fused NPU route (
app/unified.cpp) builds ChatML itself rather than rendering the pack's template. Forchat_template_kwargs.enable_thinking: falseit appended Qwen3's empty block,<think>\n\n</think>\n\n, whatever the pack. #2135 measured two FastFlowLM packs that have no such switch, on the shipped build:Qwen3-4B-Thinking-2507-NPU2: its generation prompt opens<think>\nunconditionally. Sent the empty block, it reasons anyway, and with the splitter off for thinking-off requests its reasoning and a literal</think>come back ascontent.Qwen3-VL-4B-Instruct-NPU2: no think block at all. Sent the empty block, it answers "I apologize, but I cannot provide that information …" to "What is the capital of France?".What this does
At load the route reads the pack's
chat_template(fromtokenizer_config.json, either a string or a named list, or fromchat_template.jinja).app/think_template.h(new, header-only, pure) decides whatenable_thinking: falsecan mean:enable_thinking: false<think>open (Thinking-2507)reasoning_contentwheneverreasoning_formatis notnone, whateverenable_thinkingsays. This is the issue's "cheap half". A reply with no block passes through unchanged.1bit unified: the chat template has no enable_thinking switch; enable_thinking false adds nothing to the prompt.Tests (host)
think_template_test(new ctest row): 18 arms,think_template: all cases ok. The three generation blocks are copied from the packs on this box (Qwen3-0.6B, Thinking-2507, VL-4B). Qwen3.6's shape is included too: a switch that opens the block only when thinking is on, where the switch must win.think_template_of()run on the three packs' actualtokenizer_config.jsonfiles:<think></think>counted as opening the block;tools/copyright.py --checkrc 0;file_create_checkclean.Draft until the device run
This branch is built with the dx add-on at the engine's pinned private revision (
ONEBIT_NPU_PRIVATE= npu-kernels17ed5be7). It serves all three packs on the fused route next to the shipped c32 RC binary (as "before"): both load, and the branch prints the line above for VL-4B. The answers are not measured yet, because every request in my ticket gotnpu runtime: device error (code 102): No such device, from the RC binary too. Anflm-real serveprocess holds/dev/accel0outside any ticket (amdxdna … Init hwctx failed, ret -19); it is on the board. Before marking this ready I will post, for each pack, the reply with no kwargs and withenable_thinking: false, before and after, from onenpu-holdticket.🤖 Generated with Claude Code