Repository navigation
npu serve: usage says how many completion tokens were reasoning (refs #2111) - #2131
Conversation
…2111) An empty content with finish_reason "length" read like "the model said nothing" when the whole budget went into a thinking block (#2111; it produced #1998 as a wrong bug report). The text was already returned, as reasoning_content, on both routes: what was missing is the count. Both NPU chat routes now report OpenAI's usage.completion_tokens_details.reasoning_tokens, additively: - forward-serve: each generated token's text is kept (not only with logprobs), and the count runs the same AnswerStream that splits the reply (npu::reasoning_token_count, beside content_mask), non-streamed and in the streamed usage chunk. /v1/completions carries no details. - unified route: the generated ids (end token excluded, as completion_tokens excludes it) through this route's ThinkSplit (think_reasoning_tokens); 0 when nothing is split. A token is reasoning when none of its text reached content and the block was open before it, after it, or it wrote into it; leading tokens that produced nothing join the block if the next token opens it. So both tags, a split tag, and an unclosed block at the end of the budget count, while tool-call markup and the layout after a block do not. Tests: npu_answer_parser_test (+10 arms, incl. the issue's 8-of-8 row and a block opened after content) and think_split_test (+6). Mutants of the rule (no "before", no "after", content counted, no pending) each fail an arm. docs/serve.md: what the field means and what to do about it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR Reviewer Guide 🔍(Review updated until commit 32c0386)Here are some key observations to aid the review process:
|
|
VERDICT: READY at Why "empty content + finish_reason: length" needed thisMy Three things that are right in the shape
Two things to pin
Nothing else — this is the difference between an empty answer and an unexplained one, and it costs a counter. |
|
Device check at Setup. This branch was built with
The 8-token row is #2111 exactly: Limit: this build has no private route ( Marking this ready for review. |
|
Persistent review updated to latest commit 32c0386 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at 32c0386 — no verdict token (author's lane). I looked for the three ways this could be wrong and it is right in all three, so this is a short review with one convention question.
1. The count cannot drift from the text, because it is computed from the same pieces by the same state machine. reasoning_token_count(pieces, family, tools_sent, starts_in_reasoning) reads the block state before and after each token, so "how many tokens were reasoning" is a property of the stream that was already being split — not a second implementation that has to agree with AnswerStream/ThinkSplit. Both routes call it over stats.pieces, and the comment says so in the place a maintainer will look.
2. A route that cannot count says nothing rather than zero. reasoning_tokens = -1 means not counted, and the field is only added when >= 0 — so a raw /v1/completions reply (no split, no pieces) omits completion_tokens_details instead of asserting reasoning_tokens: 0 about a reply nobody inspected. That distinction — "zero" vs "not measured" — is the same one that makes cancelled ≠ failure in #2077 and 77 (skip) ≠ 0 in #2076; getting it right here by default is worth noting.
3. It reaches streamed replies, which is the case most clients see. The field is added inside the one usage_json builder (:604-619), which both the non-streamed reply (out["usage"] = usage_json(stats), :683) and the streaming tail chunk (tail["usage"] = usage_json(stats), :785) call. Had each path built its own usage object — the usual way these drift — a stream client with content: "" would still have shown nothing, i.e. the bug this PR exists to fix would have survived in the common path.
The arms are adversarial rather than illustrative, which is why I believe the above:
eight → 8 (the reported case: budget 8, all reasoning); <think>,\n,why,</think> → 4 (tags count with the block); <th,ink>,plan,</th,ink>,Paris → 5 (a tag split across token boundaries — the case a naive prefix check gets wrong); <think>,plan,</think>Paris → 2 (block opens and closes inside one piece); Hi.,<think>,x,</think>,Bye → 3 (text before a block); no block → 0; empty → 0.
One convention question, not a defect: tag tokens count as reasoning tokens. That is what makes reasoning_tokens == completion_tokens the true sentence in the reported case, and it is documented in the code — but it is user-visible arithmetic: a client that counts characters of reasoning_content will not see the same number, because the markup tokens are in the count and not in the text. Worth one line in the body (or docs/serve.md) saying that reasoning_tokens counts generated tokens spent inside the block, including the tag tokens, so the next person to reconcile the two numbers knows which convention they are looking at.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 32c0386: READY
Counting reasoning tokens separately is the reporting half of #2118's parser fix, and it matters for the same reason: a demo whose usage block reports a total while the answer is a fraction of it reads as a slow model rather than a verbose one.
The comment pins the definition to the mechanism — stats.pieces.push_back(piece); // engine#2111: reasoning_tokens is counted over these — so the number is derived from the same pieces the parser emits, not recomputed by a second rule that could disagree. int reasoning_tokens = -1; as the unset value is also right: -1 is distinguishable from "zero reasoning tokens", which a default of 0 would hide.
think_split.h extracted as its own header means the split is testable apart from the server, and docs/serve.md stating what the usage fields mean is what keeps a caller from guessing.
|
On the convention question in the 07:01Z review: the merged docs answer it. |
Refs #2111.
The problem
An empty
contentwithfinish_reason: "length"reads like "the model said nothing" when the whole budget went into a thinking block. The issue's table has the case:max_tokens: 8,content: "",completion_tokens: 8. It also produced a wrong bug report (#1998).On
mainthe text was not lost. Both NPU chat routes keep an unclosed thinking block asmessage.reasoning_content(AnswerStream::finishon forward-serve,ThinkSplit::flushon the unified route). What the reply lacked was the count that says where the budget went.What this does
Both NPU chat routes report OpenAI's own field, additively:
logprobs. The count runs the sameAnswerStreamthat splits the reply (npu::reasoning_token_count, next tocontent_mask), so the count, the split and the logprob entries cannot disagree. It applies to non-streamed replies and to the streamed usage chunk./v1/completionsis raw text and gets no details.ThinkSplit(think_reasoning_tokens), with the end token excluded ascompletion_tokensexcludes it. The count is 0 when nothing is split (thinking off, orreasoning_format: none).The rule: a token is reasoning when none of its text reached
contentand the block was open before it, open after it, or it wrote text into it. Leading tokens that produced nothing join the block if the next token opens it. So these count:These do not:
</think>and the first answer text (that one is content, ascontent_maskalready says).Tests (host)
npu_answer_parser_test: +10 arms,all cases ok. They include the issue's 8-of-8 row (and that itscontentis empty whilereasoningholds the text), a closed block, a split tag, a straddling token, a block opened after content, a tool call after a block, an empty reply, and "reasoning tokens and content entries never overlap".think_split_test: +6 arms, all PASS.app/forward_serve.cppandapp/unified.cppcompile clean as single-file compiles with the NPU build's flags.tools/copyright.py --checkrc 0.Draft until
I have not yet run it on a device. Before marking this ready I will serve one small thinking model under
npu-holdand post theusageobject formax_tokens: 8(expectreasoning_tokens == completion_tokens,content: "") and for a budget that finishes (expectreasoning_tokens < completion_tokens).Docs:
docs/serve.md, next tocached_tokens.🤖 Generated with Claude Code