fix(reasoning): stop leaking split think tags and rescanning output by janhilgard · Pull Request #677 · waybarrios/vllm-mlx

janhilgard · 2026-08-03T09:12:49Z

Two problems in BaseThinkingReasoningParser's streaming path. Both are invisible for models whose tokenizer has the think tags as single tokens — which is why they have gone unnoticed — and both are unavoidable for models that spell the tags out in ordinary tokens.

Split tags leaked into the reasoning stream

A tag arriving across delta boundaries was detected against the accumulated text, but by then the fragments had already gone out: delta_text.find(end_token) returns -1 for a partial tag, so the code fell through to treating the whole delta as reasoning. Streaming weighing it</think>Answer. five characters at a time produced:

reasoning: "weighing it</thi"
content:   "nk>Answer."

Per-token cost grew with output length

While the reasoning block was open, every delta ran start_token in current_text and end_token in current_text across the whole accumulated output. That is O(N²) over a generation — despite the docstring stating the state machine avoided exactly that. Measured at 50x the tokens:

parser	before
`deepseek_r1`	1023x
`qwen3`	~1170x

Perfectly linear would be 50x.

One change fixes both

A delta whose tail could still grow into a tag is withheld rather than emitted. That makes a completing tag always present in the text at hand, so there is nothing left to search the accumulated output for — the leak and the rescan disappear together.

Withholding is applied to what is about to be emitted rather than to the raw delta. Text that follows a tag completing in the same delta is then covered too; otherwise a fragment is stranded when the phase moves on and resurfaces out of order at the end of the stream. finalize_stream() flushes anything still withheld, so a generation that stops on a bare < does not drop it.

DeepSeekR1ReasoningParser no longer overrides the streaming method. It existed to catch an end token arriving without a start token, which the base class handles itself now; the override only duplicated that while scanning the accumulated text for the start token on every delta — which is what kept deepseek_r1 quadratic even after the base class was fixed. Its complete-output override, the actual leniency this parser is known for, is untouched.

After: 63x for deepseek_r1 and 64x for qwen3 at 50x the tokens.

Verification

Behaviour is unchanged on real traffic. I replayed four completions captured from a running Qwen3.6-27B server through both the old and new parsers, split on the model's own token boundaries rather than arbitrary character chunks: identical reasoning and content in all eight parser/sample combinations, and 1.3x faster at 1198 tokens. The gain scales with output length — at 1200 tokens the quadratic term is still small.

The new tests are checked both ways: 32 of 40 fail against the previous implementation and all 40 pass against this one. They cover split start and end tags at every chunk size from 1 to 16, truncated tag prefixes at end of stream, and a scaling assertion that fails loudly on quadratic growth.

tests/test_tool_call_promotion.py::test_stream_large_chunks caught a first attempt that withheld only the think tags and not <tool_call>, which broke promotion when an explicit <think> handed the remainder of a delta to the thinking phase. That marker is withheld too now.

Full CI test list passes; the only failures in my environment are four pre-existing ones that need mlx, identical on a clean checkout of main.

janhilgard · 2026-08-03T09:14:57Z

@Thump604 — review requested. One thing worth flagging before this lands.

This PR and #676 touch the same class hierarchy and the merge order matters. #676 adds DeepSeekV4ReasoningParser, which subclasses DeepSeekR1ReasoningParser and does its own withholding for the DSML marker. An earlier revision of the fix here stranded withheld text when a tag completed in the same delta, which broke that parser on real model output while every unit test still passed — I only caught it by replaying captured DeepSeek-V4 completions with both branches applied together.

The current revision is verified against that combination: 145 DeepSeek-V4 tests plus a pipeline replay over 23 real completions, clean. But if this merges before #676, the V4 branch will need a rebase and that combined check re-run rather than just a green CI.

The scaling assertion in TestStreamingCostIsFlat has a deliberately loose bound (200x for a 20x token increase). It is there to fail on quadratic growth, which measured over 1000x, not to police small regressions on a loaded runner. Tighten it if you would rather it be strict.

Thump604

Reviewed the current head as a standalone parser fix. The pending-fragment state keeps split <think> / </think> markers out of emitted channels without rescanning accumulated output; tool-call buffering and end-of-stream flush behavior remain covered. I also ran tests/test_reasoning_parser.py at the exact head: 162 passed. No blocking findings. This approval is for #677 only; #676 still needs a rebase and combined DeepSeek replay after this lands.

DeepSeek-V4 has no Jinja chat template, so without a prompt encoder the prompt is built by plain concatenation and the model sees a format it was never trained on. It also emits tool calls in its own DSML markup rather than JSON. This adds the encoder and both parsers, plus registration and CLI wiring. The DSML tool parser is a scanner rather than a regex, because the `string="true|false"` attribute means a string parameter may legitimately contain quotes, angle brackets or a JSON-looking payload; `TestParameterTyping::test_string_value_may_contain_markup_like_text` pins that. The streaming path is the subtle part. `<｜DSML｜tool_calls>` has no token id of its own — only the bare `｜DSML｜` does — so the marker always straddles delta boundaries, and two obvious implementations are both wrong: detecting completion against the delta rather than the accumulated text drops the calls entirely, and emitting marker fragments as they arrive leaks markup to the client and then repeats the whole marker. Both are covered at chunk sizes 1 through 128. Two integration bugs that only appear once this is wired into the server: - `SUPPORTS_NATIVE_TOOL_FORMAT` must be True. With False, `extract_multimodal_content` flattens `role="tool"` into `"[Tool Result (id)]: ..."` and assistant `tool_calls` into `"[Calling tool: name(...)]"` *before* the encoder runs, so a multi-turn tool conversation reaches the model as prose and the encoder's own `<tool_result>`/DSML handling never fires. - With native format preserved, `api/utils.py` json-loads `arguments` in place. The encoder loaded it again, and `json.loads` on a mapping raises, so every parameter collapsed into one bogus `arguments` entry — the model saw a malformed call in its own history. It now accepts either form. Benchmark (`benchmarks/bench_deepseek_v4.py`), both serving paths: Prompt encoding 4 messages -> 0.022 ms 130 messages -> 0.335 ms Single stream 100 tok -> 0.0061 ms/tok 5000 tok -> 0.0380 ms/tok DSML parser alone 100 tok -> 0.0018 ms/tok 5000 tok -> 0.0028 ms/tok Batched decode 1 concurrent -> 0.0107 16 concurrent -> 0.0107 ms/tok Batching costs nothing per token; each request holds independent parser state. Verified end to end on DeepSeek-V4-Flash-0731 MXFP4 (283.8B, M3 Ultra): 11/11 over HTTP with `--tool-call-parser deepseek_v4 --reasoning-parser deepseek_v4` — `finish_reason=tool_calls`, arguments as JSON objects, reasoning split into `reasoning_content`, no DSML in user-visible content, parallel calls intact, and a tool-result round trip where the model uses the returned values. Scope note: this was previously one branch carrying engine changes as well. Those are now waybarrios#679 (SimpleEngine ownership), waybarrios#680 (BatchedEngine owner thread) and waybarrios#681 (prefix cache on non-trimmable KV), and chunked prefill is dropped in favour of waybarrios#648. This PR is the DeepSeek slice alone. Merge order: this depends on nothing, but waybarrios#677 changes the reasoning base class this parser inherits from. Checked merged with waybarrios#677 rather than only alongside it: 307 targeted tests and 2479 repo-wide, clean. Repo suite on this branch alone: 2439 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

DeepSeek-V4 has no Jinja chat template, so without a prompt encoder the prompt is built by plain concatenation and the model sees a format it was never trained on. It also emits tool calls in its own DSML markup rather than JSON. This adds the encoder and both parsers, plus registration and CLI wiring. The DSML tool parser is a scanner rather than a regex, because the `string="true|false"` attribute means a string parameter may legitimately contain quotes, angle brackets or a JSON-looking payload; `TestParameterTyping::test_string_value_may_contain_markup_like_text` pins that. The streaming path is the subtle part. `<｜DSML｜tool_calls>` has no token id of its own — only the bare `｜DSML｜` does — so the marker always straddles delta boundaries, and two obvious implementations are both wrong: detecting completion against the delta rather than the accumulated text drops the calls entirely, and emitting marker fragments as they arrive leaks markup to the client and then repeats the whole marker. Both are covered at chunk sizes 1 through 128. Two integration bugs that only appear once this is wired into the server: - `SUPPORTS_NATIVE_TOOL_FORMAT` must be True. With False, `extract_multimodal_content` flattens `role="tool"` into `"[Tool Result (id)]: ..."` and assistant `tool_calls` into `"[Calling tool: name(...)]"` *before* the encoder runs, so a multi-turn tool conversation reaches the model as prose and the encoder's own `<tool_result>`/DSML handling never fires. - With native format preserved, `api/utils.py` json-loads `arguments` in place. The encoder loaded it again, and `json.loads` on a mapping raises, so every parameter collapsed into one bogus `arguments` entry — the model saw a malformed call in its own history. It now accepts either form. Benchmark (`benchmarks/bench_deepseek_v4.py`), both serving paths: Prompt encoding 4 messages -> 0.022 ms 130 messages -> 0.335 ms Single stream 100 tok -> 0.0061 ms/tok 5000 tok -> 0.0380 ms/tok DSML parser alone 100 tok -> 0.0018 ms/tok 5000 tok -> 0.0028 ms/tok Batched decode 1 concurrent -> 0.0107 16 concurrent -> 0.0107 ms/tok Batching costs nothing per token; each request holds independent parser state. Verified end to end on DeepSeek-V4-Flash-0731 MXFP4 (283.8B, M3 Ultra): 11/11 over HTTP with `--tool-call-parser deepseek_v4 --reasoning-parser deepseek_v4` — `finish_reason=tool_calls`, arguments as JSON objects, reasoning split into `reasoning_content`, no DSML in user-visible content, parallel calls intact, and a tool-result round trip where the model uses the returned values. Scope note: this was previously one branch carrying engine changes as well. Those are now waybarrios#679 (SimpleEngine ownership), waybarrios#684 (BatchedEngine owner thread) and waybarrios#683 (prefix cache on non-trimmable KV), and chunked prefill is dropped in favour of waybarrios#648. This PR is the DeepSeek slice alone. Merge order: this depends on nothing, but waybarrios#677 changes the reasoning base class this parser inherits from. Checked merged with waybarrios#677 rather than only alongside it: 307 targeted tests and 2479 repo-wide, clean. Repo suite on this branch alone: 2439 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

waybarrios · 2026-08-16T17:27:03Z

Nice work on the parser. I added a small follow-up because finalize_stream() existed, but the server streaming paths were not calling it.

Buffered marker prefixes are now flushed with the final output across Chat Completions, Anthropic Messages, and Responses.

This matters when generation ends with something like thinking</thi or a bare <. Those final characters could be dropped even though the stream completed normally.

I also added regression tests for all three API paths.

Two problems in BaseThinkingReasoningParser's streaming path, both invisible for models whose tokenizer has the think tags as single tokens and both unavoidable for models that spell them out. **Split tags leaked.** A tag arriving across delta boundaries was detected against the accumulated text, but the fragments had already been emitted: `delta_text.find(end_token)` returned -1, so the code fell through to treating the whole delta as reasoning. Streaming `weighing it</think>Answer.` five characters at a time produced reasoning `weighing it</thi` and content `nk>Answer.`. **Cost grew with output length.** While the reasoning block was open, every delta ran `start_token in current_text` and `end_token in current_text` over the whole accumulated output — quadratic over a generation, despite the docstring claiming the state machine avoided exactly that. Measured at 50x the tokens: 1023x the time for deepseek_r1, ~1170x for qwen3. Both fall out of one change. A delta whose tail could still grow into a tag is withheld rather than emitted, which means a tag completing now is always contained in the text at hand and there is nothing left to search the accumulated output for. Withholding is applied to what is about to be emitted rather than to the raw delta, so text following a tag that completes in the same delta is covered too — otherwise a fragment is stranded when the phase moves on and resurfaces out of order at the end of the stream. `finalize_stream()` flushes anything still withheld, so a generation that stops on a bare `<` does not drop it. `DeepSeekR1ReasoningParser` no longer overrides the streaming method. It existed to catch an end token arriving without a start token, which the base class now handles itself; the override only duplicated it while scanning the accumulated text for the start token on every delta, which is what kept deepseek_r1 quadratic after the base class was fixed. Scaling at 50x the tokens is now 63x for deepseek_r1 and 64x for qwen3, against 50x for perfectly linear. Behaviour is unchanged. Verified by replaying four real completions from a Qwen3.6-27B server through both the old and new parsers, split on the model's own token boundaries: identical reasoning and content in all eight parser/sample combinations, 1.3x faster at 1198 tokens. The new tests fail against the previous implementation — 32 of 40 — and pass against this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

janhilgard · 2026-08-19T16:07:55Z

Rebased on `main` — #679 and #684 landing put this back into conflict.

Six conflicts in `server.py`, three in `tests/test_server.py`. The server ones split cleanly: the new streaming helpers you added (`_prepare_streaming_reasoning_parser`, `_request_tool_definitions`, `_streaming_json_fence_stripper`) and this PR's `_extract_streaming_reasoning_delta` are independent additions, so both are kept; the remaining five are the same guard in different functions, where this branch's version is a superset (it adds the `use_reasoning`/`output_finished` pair so a finished output can still flush). Verified with an AST pass that both names are assigned in every function that reads them — the guard appears in three separate functions and a missed definition would only surface at runtime.

The test file needed more than a merge. Resolving it mechanically spliced one test's signature into the middle of another's body, which pytest surfaced as "async def functions are not natively supported" rather than as a syntax error. I rebuilt it from `main`'s copy and re-applied only what this branch actually adds.

That turned out to be one test, not two: `test_build_tool_parser_returns_request_local_instances` is the same test you kept as `test_build_tool_parser_returns_a_fresh_instance_per_stream`, so it is dropped rather than duplicated. The remaining one, `test_reasoning_stream_flushes_partial_marker_before_finish`, is mutation-checked — stubbing out `finalize_stream` fails it with `assert 'thinking' == 'thinking</thi'`, which is the leak this PR exists to prevent.

Repo suite: 2555 passed, 4 failed — the same four that fail on clean `upstream/main` (`test_shutdown_canceled_prepare_error_unwinds_to_unloaded`, the two `_SpecPrefillCancelled` cases, and the ffmpeg smoke test, which needs a binary this machine lacks). Lint and black clean under the CI invocation.

* feat: add DeepSeek-V4-Flash support DeepSeek-V4 has no Jinja chat template, so without a prompt encoder the prompt is built by plain concatenation and the model sees a format it was never trained on. It also emits tool calls in its own DSML markup rather than JSON. This adds the encoder and both parsers, plus registration and CLI wiring. The DSML tool parser is a scanner rather than a regex, because the `string="true|false"` attribute means a string parameter may legitimately contain quotes, angle brackets or a JSON-looking payload; `TestParameterTyping::test_string_value_may_contain_markup_like_text` pins that. The streaming path is the subtle part. `<｜DSML｜tool_calls>` has no token id of its own — only the bare `｜DSML｜` does — so the marker always straddles delta boundaries, and two obvious implementations are both wrong: detecting completion against the delta rather than the accumulated text drops the calls entirely, and emitting marker fragments as they arrive leaks markup to the client and then repeats the whole marker. Both are covered at chunk sizes 1 through 128. Two integration bugs that only appear once this is wired into the server: - `SUPPORTS_NATIVE_TOOL_FORMAT` must be True. With False, `extract_multimodal_content` flattens `role="tool"` into `"[Tool Result (id)]: ..."` and assistant `tool_calls` into `"[Calling tool: name(...)]"` *before* the encoder runs, so a multi-turn tool conversation reaches the model as prose and the encoder's own `<tool_result>`/DSML handling never fires. - With native format preserved, `api/utils.py` json-loads `arguments` in place. The encoder loaded it again, and `json.loads` on a mapping raises, so every parameter collapsed into one bogus `arguments` entry — the model saw a malformed call in its own history. It now accepts either form. Benchmark (`benchmarks/bench_deepseek_v4.py`), both serving paths: Prompt encoding 4 messages -> 0.022 ms 130 messages -> 0.335 ms Single stream 100 tok -> 0.0061 ms/tok 5000 tok -> 0.0380 ms/tok DSML parser alone 100 tok -> 0.0018 ms/tok 5000 tok -> 0.0028 ms/tok Batched decode 1 concurrent -> 0.0107 16 concurrent -> 0.0107 ms/tok Batching costs nothing per token; each request holds independent parser state. Verified end to end on DeepSeek-V4-Flash-0731 MXFP4 (283.8B, M3 Ultra): 11/11 over HTTP with `--tool-call-parser deepseek_v4 --reasoning-parser deepseek_v4` — `finish_reason=tool_calls`, arguments as JSON objects, reasoning split into `reasoning_content`, no DSML in user-visible content, parallel calls intact, and a tool-result round trip where the model uses the returned values. Scope note: this was previously one branch carrying engine changes as well. Those are now #679 (SimpleEngine ownership), #684 (BatchedEngine owner thread) and #683 (prefix cache on non-trimmable KV), and chunked prefill is dropped in favour of #648. This PR is the DeepSeek slice alone. Merge order: this depends on nothing, but #677 changes the reasoning base class this parser inherits from. Checked merged with #677 rather than only alongside it: 307 targeted tests and 2479 repo-wide, clean. Repo suite on this branch alone: 2439 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix DeepSeek V4 streaming lifecycle and reasoning profiles (#676) * Finalize empty DeepSeek V4 streaming deltas safely (#676) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Wayner Barrios <waybarrios@gmail.com>

…waybarrios#687 aclosing kept Rebase onto upstream `22efb47` (5 commits past `4b654c0`). Six conflict stops, all in three files. This commit carries the non-mechanical parts. - Upstream waybarrios#677 adds `not _thinking_disabled(...)` to the chat-completions streaming gate — exactly the skip patch #27 exists to prevent (gemma-4 emits `<|channel>thought` with thinking off; skipping the parser leaks raw markers into content). Clause dropped in both places; upstream's `or output_finished` and `_extract_streaming_reasoning_delta` helper kept, since waybarrios#677 now withholds a trailing partial tag and flushes it via `finalize_stream()`. - The Responses-path marker latch is rewired rather than re-applied: upstream's `use_reasoning` is computed once per output as "parser and thinking on", which would silently defeat a latch that can engage mid-stream. It is now derived from the latch and re-armed when the latch trips. - Upstream waybarrios#687 (`aclosing` on delegated streams) is KEPT as a wanted delta — it is not superseded by our patches, and it closes the inner generator when a client disconnects. Fork's `strip_effort_fallback` (waybarrios#76) re-applied inside it. - `test_public_stream_chat_close_cleans_nested_generate` given a fork-aware fixture instead of a skip: it stubs `engine._model.stream_generate`, but the fork's text route calls `mlx_lm.stream_generate` directly to pass a `prompt_cache`, so the real entry point ran and died on a MagicMock prompt. With both stubbed it passes and verifies fork abort cleanup. - Removed three upstream imports confirmed dead in the fork's `simple.py`: `OrderedDict` (upstream's own system-KV LRU; we delegate to SystemKVManager) and `ThreadPoolExecutor` (upstream's generation-executor machinery). Suite: 3315 passed / 31 skipped / 30 deselected; ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFxkYqZwQgVp5QDNfpU6Hp

janhilgard requested a review from Thump604 August 3, 2026 09:14

janhilgard mentioned this pull request Aug 3, 2026

feat: add DeepSeek-V4-Flash support #676

Merged

janhilgard self-assigned this Aug 3, 2026

Thump604 approved these changes Aug 8, 2026

View reviewed changes

janhilgard and others added 2 commits August 19, 2026 18:01

Flush buffered reasoning at stream completion

1e143dc

janhilgard force-pushed the fix/thinking-parser-streaming branch from 7af9003 to 1e143dc Compare August 19, 2026 16:05

waybarrios merged commit 0d93f84 into waybarrios:main Aug 26, 2026
9 checks passed

Navigation Menu

fix(reasoning): stop leaking split think tags and rescanning output - #677

fix(reasoning): stop leaking split think tags and rescanning output#677
waybarrios merged 2 commits into
waybarrios:mainfrom
janhilgard:fix/thinking-parser-streaming

janhilgard commented Aug 3, 2026

Uh oh!

janhilgard commented Aug 3, 2026

Uh oh!

Thump604 left a comment

Uh oh!

waybarrios commented Aug 16, 2026

Uh oh!

janhilgard commented Aug 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

janhilgard commented Aug 3, 2026

Split tags leaked into the reasoning stream

Per-token cost grew with output length

One change fixes both

Verification

Uh oh!

janhilgard commented Aug 3, 2026

Uh oh!

Thump604 left a comment

Choose a reason for hiding this comment

Uh oh!

waybarrios commented Aug 16, 2026

Uh oh!

janhilgard commented Aug 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants