Skip to content

fix(server): surface reasoning_content on non-streaming chat - #359

Merged
inureyes merged 2 commits into
mainfrom
fix/issue-352-gemma4-reasoning-content
Jun 18, 2026
Merged

inureyes merged 2 commits into
mainfrom
fix/issue-352-gemma4-reasoning-content

Conversation

@inureyes

Copy link
Copy Markdown
Member

Summary

gemma-4-12b-it (model_type: gemma4_unified) carries the <|channel> / <channel|> thinking markers, so on startup the server resolves a think-marker pair and defaults the enable_thinking chat-template kwarg to true, mirroring ml-explore/mlx-lm#1114. Per the maintainer decision this default stays as-is: the marker detection in src/tokenizer/mod.rs and set_default_enable_thinking in src/server/startup.rs are untouched. The real bug is that the non-streaming /v1/chat/completions response silently discarded the thinking scratchpad, so a verbose thinking model looked like it returned an empty or truncated answer at a normal max_tokens.

This surfaces the reasoning as reasoning_content on the non-streaming response, which closes the gap for every thinking family at once (qwen3 <think> and gemma4 <|channel>), and documents the thinking default plus a recommended higher max_tokens for gemma4_unified.

What changed

  • src/server/types/response.rs: add an optional reasoning_content: Option<String> to the response ChatMessage (#[serde(skip_serializing_if = "Option::is_none")], so non-thinking responses keep their exact wire shape), matching the OpenAI/vLLM convention and the streaming delta.reasoning_content. Add a with_reasoning_content chained setter that mirrors with_cached_tokens.
  • src/server/routes/chat.rs: add extract_reasoning_content, which replays the raw generation through the same StreamFilter the streaming path constructs, then wire it onto all three non-streaming return points (tool-calls, tools-enabled-no-calls, plain). The existing content computation (strip_unclosed_primed_thinking / clean_structural_tokens / tool-call parsing) is unchanged; reasoning is purely additive.
  • docs/supported-models.md: document the gemma4_unified thinking default, the recommended max_tokens (at least 512), the reasoning_content surfacing on both streaming and non-streaming paths, and that per-request/CLI/env enable_thinking=false disables it.
  • docs/block-diffusion.md: correct the stale claim that non-streaming responses strip reasoning_content.

StreamFilter reuse (streaming / non-streaming parity)

Rather than write a new marker parser, extract_reasoning_content builds the same StreamFilter the streaming SSE path builds (primed-open vs fresh, keyed on is_prompt_primed_open_thinking), feeds the whole generated string once, calls flush(), and accumulates the reasoning pieces. The filter state machine is deterministic over the concatenated input, so feeding the whole string yields the same reasoning/content split as feeding it token-by-token. A unit test (reasoning_split_identical_whole_vs_chunked) locks this equivalence, which is what guarantees streaming and non-streaming surface identical reasoning. Per-request chat_template_kwargs.enable_thinking, --chat-template-kwargs, and LLAMA_ARG_CHAT_TEMPLATE_KWARGS precedence is untouched (merge_server_and_request unchanged).

Real-model validation (gemma-4-12b-it-4bit, temperature 0)

Startup log confirms the intended default is preserved: Tokenizer recognizes a think marker pair; defaulting chat_template kwarg enable_thinking=true (upstream PR #1114) think_start=Some("<|channel>thought") think_end=Some("<channel|>").

Prompt "What is a hash table? Answer in one sentence." at max_tokens=400, default thinking:

finish_reason: stop
completion_tokens: 275
has reasoning_content key: True
reasoning_content len: 980
reasoning_content (head): "*   Topic: Hash table. ... *Draft 1:* A hash table is a data structure that maps keys to values by using a hash function ..."
content: "A hash table is a data structure that maps keys to values by using a hash function to compute an index into an array, allowing for efficient data storage and retrieval."

Same prompt with chat_template_kwargs={"enable_thinking": false} (per-request precedence preserved):

finish_reason: stop
completion_tokens: 33
has reasoning_content key: False
content: "A hash table is a data structure that maps keys to values by using a hash function to compute an index into an array, allowing for efficient data retrieval and storage."

This also answers the issue open question 3: the over-thinking is marker-delimited (<|channel>thought ... <channel|>), since the filter captured the full 980-char block into reasoning_content while content holds only the answer (an empty content plus finish_reason=length at a tiny budget was the tokens being stripped as a marked thinking block, not leaked unmarked text).

This fixes qwen3 too

qwen3-4b-4bit thinking request "What is 17 times 23? Answer in one sentence." at max_tokens=512, temperature 0:

finish_reason: stop
completion_tokens: 375
has reasoning_content key: True
reasoning_content len: 932
reasoning_content (head): " Okay, the user is asking for 17 multiplied by 23 ... 340 + 51 equals 391 ..."
content: "17 times 23 equals 391."

Test plan

  • cargo check --lib --tests --features metal,accelerate
  • cargo clippy --lib --tests --features metal,accelerate -- -D warnings
  • cargo test -p mlxcel --lib --features metal,accelerate for the new tests: server::routes::chat::tests::extract_reasoning_* (5), server::routes::chat::tests::reasoning_split_identical_whole_vs_chunked, server::types::response::tests::reasoning_content_* (2). All 8 pass.
  • cargo fmt --check
  • Real-model smoke on gemma-4-12b-it-4bit and qwen3-4b-4bit (above)

Closes #352

gemma-4-12b-it (model_type gemma4_unified) carries the `<|channel>` / `<channel|>` thinking markers, so the server defaults `enable_thinking=true` on startup, mirroring ml-explore/mlx-lm#1114. That default is intentional and stays. The real bug is that the non-streaming `/v1/chat/completions` response silently discarded the thinking scratchpad: the streaming path already split reasoning into `delta.reasoning_content`, but the non-streaming `ChatMessage` had no such field, so for any thinking model (qwen3 `<think>`, gemma4 `<|channel>`) the reasoning was dropped and a verbose model looked like it returned an empty answer.

Add an optional `reasoning_content` field to the non-streaming response `ChatMessage` (omitted when None, matching the OpenAI/vLLM convention and the streaming delta), and populate it by replaying the raw generation through the same `StreamFilter` the streaming path uses. Feeding the whole string in one `feed()` accumulates the same reasoning/content split as feeding it token-by-token because the filter state machine is deterministic over the concatenated input, so streaming and non-streaming now surface identical reasoning. This is additive: the existing content computation (strip_unclosed_primed_thinking, clean_structural_tokens, tool-call parsing) is unchanged, and per-request `chat_template_kwargs.enable_thinking`, `--chat-template-kwargs`, and `LLAMA_ARG_CHAT_TEMPLATE_KWARGS` still win.

The fix lands on all three non-streaming return points (tool-calls, tools-enabled-no-calls, and the plain response) via a `with_reasoning_content` chained setter that mirrors `with_cached_tokens`.

Document the gemma4_unified thinking default and a recommended `max_tokens` of at least 512 in docs/supported-models.md, and correct docs/block-diffusion.md which claimed non-streaming responses strip reasoning.

Validated on the real gemma-4-12b-it-4bit checkpoint (temperature 0). "What is a hash table? Answer in one sentence." returns reasoning_content with the 980-char thinking block and content with the one-sentence answer at 275 completion tokens; the same request with chat_template_kwargs={"enable_thinking": false} omits reasoning_content and answers in 33 tokens. A qwen3-4b-4bit thinking request also surfaces reasoning_content, confirming the general fix.
@inureyes inureyes added type:bug Bug fixes, error corrections, or issue resolutions priority:medium Medium priority area:core mlxcel-core: MLX FFI, primitives, KV cache, layers area:inference Generation, sampling, decoding (incl. speculative, DRY) status:review Under review labels Jun 18, 2026
The bare-leading-close pre-pass in strip_thinking handled only the Gemma 4 <channel|> primed case, not a primed <think> that emits a close-only </think>. On a Qwen-family checkpoint that primes <think>, the non-streaming chat content leaked the reasoning text and the literal </think> marker, while /v1/responses, /v1/anthropic, and the chat streaming filter all split it correctly. Generalize the pre-pass to both (open, close) pairs so a close marker with no preceding open is stripped for </think> as well, matching the established split convention.
@inureyes

Copy link
Copy Markdown
Member Author

Follow-up commit 8402a75e1 addresses the HIGH finding from review: the non-streaming chat content leaked the reasoning text and a literal </think> for a primed-open <think> checkpoint that emits a close-only </think> (no opening tag in the output). The bare-leading-close pre-pass in strip_thinking handled only the Gemma 4 <channel|> primed case; it is now generalized to both (open, close) pairs, so chat non-streaming matches the split already done by /v1/responses, /v1/anthropic, and the chat streaming filter.

Added parser tests strip_thinking_primed_qwen_close_only and clean_structural_tokens_primed_qwen_close_only. Verified locally under the pinned toolchain (rust 1.93.1, matching CI): clippy -D warnings clean, 47 parser tests pass, the 5 extract_reasoning tests and the whole-vs-chunked parity test pass, fmt clean.

@inureyes inureyes added status:done Completed and removed status:review Under review labels Jun 18, 2026
@inureyes
inureyes merged commit a9e7080 into main Jun 18, 2026
5 checks passed
@inureyes
inureyes deleted the fix/issue-352-gemma4-reasoning-content branch June 18, 2026 15:57
inureyes added a commit that referenced this pull request Jun 20, 2026
Audio I/O on the OpenAI /v1/audio/* surface (Whisper speech-to-text #371,
Kokoro-82M text-to-speech #374, endpoint plumbing #368), reasoning_content on
non-streaming chat (#359), pre-M5 decode +8-12% via a hardware-gated op cap
(#360), Turbo4Asym decode rerouted through dequant-SDPA (#369), batched
sampling fixes (#339, #340, #344, #356), and the Turbo KV advisor/docs aligned
to the measured sweep with ADR 0002 (#343, #376).

README: consolidate the v0.3.0 and v0.3.1 highlights under a single v0.3 entry
and add v0.3.2.
@inureyes inureyes added this to the 0.3 milestone Jun 21, 2026
@inureyes inureyes self-assigned this Aug 31, 2026
be-free-snowboy pushed a commit to freestylejs/snowbrain-mlxcel that referenced this pull request Oct 27, 2026
)

* fix(server): surface reasoning_content on non-streaming chat

gemma-4-12b-it (model_type gemma4_unified) carries the `<|channel>` / `<channel|>` thinking markers, so the server defaults `enable_thinking=true` on startup, mirroring ml-explore/mlx-lm#1114. That default is intentional and stays. The real bug is that the non-streaming `/v1/chat/completions` response silently discarded the thinking scratchpad: the streaming path already split reasoning into `delta.reasoning_content`, but the non-streaming `ChatMessage` had no such field, so for any thinking model (qwen3 `<think>`, gemma4 `<|channel>`) the reasoning was dropped and a verbose model looked like it returned an empty answer.

Add an optional `reasoning_content` field to the non-streaming response `ChatMessage` (omitted when None, matching the OpenAI/vLLM convention and the streaming delta), and populate it by replaying the raw generation through the same `StreamFilter` the streaming path uses. Feeding the whole string in one `feed()` accumulates the same reasoning/content split as feeding it token-by-token because the filter state machine is deterministic over the concatenated input, so streaming and non-streaming now surface identical reasoning. This is additive: the existing content computation (strip_unclosed_primed_thinking, clean_structural_tokens, tool-call parsing) is unchanged, and per-request `chat_template_kwargs.enable_thinking`, `--chat-template-kwargs`, and `LLAMA_ARG_CHAT_TEMPLATE_KWARGS` still win.

The fix lands on all three non-streaming return points (tool-calls, tools-enabled-no-calls, and the plain response) via a `with_reasoning_content` chained setter that mirrors `with_cached_tokens`.

Document the gemma4_unified thinking default and a recommended `max_tokens` of at least 512 in docs/supported-models.md, and correct docs/block-diffusion.md which claimed non-streaming responses strip reasoning.

Validated on the real gemma-4-12b-it-4bit checkpoint (temperature 0). "What is a hash table? Answer in one sentence." returns reasoning_content with the 980-char thinking block and content with the one-sentence answer at 275 completion tokens; the same request with chat_template_kwargs={"enable_thinking": false} omits reasoning_content and answers in 33 tokens. A qwen3-4b-4bit thinking request also surfaces reasoning_content, confirming the general fix.

* fix(server): strip primed-open close-only </think> from chat content

The bare-leading-close pre-pass in strip_thinking handled only the Gemma 4 <channel|> primed case, not a primed <think> that emits a close-only </think>. On a Qwen-family checkpoint that primes <think>, the non-streaming chat content leaked the reasoning text and the literal </think> marker, while /v1/responses, /v1/anthropic, and the chat streaming filter all split it correctly. Generalize the pre-pass to both (open, close) pairs so a close marker with no preceding open is stripped for </think> as well, matching the established split convention.
be-free-snowboy pushed a commit to freestylejs/snowbrain-mlxcel that referenced this pull request Oct 27, 2026
Audio I/O on the OpenAI /v1/audio/* surface (Whisper speech-to-text lablup#371,
Kokoro-82M text-to-speech lablup#374, endpoint plumbing lablup#368), reasoning_content on
non-streaming chat (lablup#359), pre-M5 decode +8-12% via a hardware-gated op cap
(lablup#360), Turbo4Asym decode rerouted through dequant-SDPA (lablup#369), batched
sampling fixes (lablup#339, lablup#340, lablup#344, lablup#356), and the Turbo KV advisor/docs aligned
to the measured sweep with ADR 0002 (lablup#343, lablup#376).

README: consolidate the v0.3.0 and v0.3.1 highlights under a single v0.3 entry
and add v0.3.2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core mlxcel-core: MLX FFI, primitives, KV cache, layers area:inference Generation, sampling, decoding (incl. speculative, DRY) priority:medium Medium priority status:done Completed type:bug Bug fixes, error corrections, or issue resolutions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(server): gemma-4-12b-it defaults to enable_thinking, yielding verbose thinking and truncated/empty answers at normal max_tokens

1 participant