Skip to content

fix(simple-engine): preserve streaming finish reasons - #681

Merged
waybarrios merged 2 commits into
waybarrios:mainfrom
Thump604:604/issue-628-natural-stop
Aug 11, 2026
Merged

waybarrios merged 2 commits into
waybarrios:mainfrom
Thump604:604/issue-628-natural-stop

Conversation

@Thump604

@Thump604 Thump604 commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Natural generator exhaustion currently emits a final chunk with no finish reason. Strict OpenAI clients treat that as a truncated stream.

This stamps natural exhaustion as stop. It also fixes the engine-enforced token-limit fallback so a backend chunk with no reason is reported as length, while preserving explicit backend reasons.

Tests cover natural and empty exhaustion, token limits, exceptions, and request cleanup.

This adopts and extends #629 because the required regression patch could not be added to its fork-owned branch. Miguel Vilhena is credited as co-author.

Verification: 2,301 passed, 24 skipped.

Refs #628

Co-authored-by: Miguel Vilhena <mvmories@gmail.com>
janhilgard added a commit to janhilgard/vllm-mlx that referenced this pull request Aug 8, 2026
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>
@Thump604
Thump604 marked this pull request as ready for review August 8, 2026 12:33
@Thump604
Thump604 requested a review from janhilgard August 8, 2026 12:33
funkymonkeymonk added a commit to funkymonkeymonk/vllm-mlx that referenced this pull request Aug 9, 2026
…he finished output

A finished=True engine output consumed by a parser `continue` in
stream_chat_completion (e.g. gemma4 emits the complete tool call in one
delta, then a bare <turn|> end-of-turn token as the terminal delta,
which the tool parser suppresses) ends the stream with the tool_calls
chunk (finish_reason=null) followed by [DONE] and no finish_reason
chunk. Strict OpenAI clients (pi coding agent) abort with
"Stream ended without finish_reason" and retry the turn.

Track whether any emitted chunk carried a *non-null* finish_reason;
after the streaming loop, if the engines finished output was
suppressed, emit the terminal chunk (finish_reason="tool_calls" when
tool calls were detected, else the engine's reason or "stop").

Also cherry-pick the engine-side fix from waybarrios#681 (Thump604) so that
SimpleEngine stamps finish_reason="stop" on natural-stop streaming
epilogue instead of leaving it null.

Refs waybarrios#672

@janhilgard janhilgard left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Reproduced both halves against main and confirmed the fix, driving _stream_generate_impl with a backend that exhausts without ever emitting a finished chunk — the shape #628 describes.

main   natural exhaustion  ->  finish_reason=None   finished=True
main   token limit         ->  finish_reason=None
#681   natural exhaustion  ->  finish_reason='stop'
#681   token limit         ->  finish_reason='length'

Both were None on main, so a strict client sees every stream as truncated, not just the natural-stop case. The issue title says "length works", which is true at the API level but not here — the length case was equally unset at this layer.

The root cause is worth spelling out because it is easy to reintroduce: getattr(chunk, "finish_reason", "stop") supplies its default only when the attribute is missing, never when it is present and None. Backends set it to None routinely, so the default was dead code. Splitting the lookup from the fallback is the fix, and deriving length from completion_tokens >= max_tokens rather than assuming stop is the right way round.

Crediting Miguel Vilhena as co-author while adopting #629 is the right handling of a fork-owned branch.

Merge note

This conflicts with #679, which is approved and touches the same block. The conflict is textual only — #679 re-indents that region into a _generation_worker_in_use() context and rewrites the tail's terminal chunk — but the semantic resolution needs stating, or a rebase will silently drop one intent:

  • #679 yields finish_reason="abort" from that tail when a stream is cut short by stop().
  • #681 yields "stop" from the same tail on natural exhaustion.

Combined, it should be "abort" if aborted_by_stop else "stop". Picking either branch wholesale loses the other. Merges cleanly against #690 with no conflict.

CI green on all nine jobs.

@waybarrios

Copy link
Copy Markdown
Owner

I carefully reviewed and it is all set. Dont see any major flaw or issue here.

@waybarrios
waybarrios merged commit 60bc821 into waybarrios:main Aug 11, 2026
9 checks passed
waybarrios added a commit that referenced this pull request Aug 15, 2026
…he finished output (#673)

* fix(server): emit terminal finish_reason chunk when parsers swallow the finished output

A finished=True engine output consumed by a parser `continue` in
stream_chat_completion (e.g. gemma4 emits the complete tool call in one
delta, then a bare <turn|> end-of-turn token as the terminal delta,
which the tool parser suppresses) ends the stream with the tool_calls
chunk (finish_reason=null) followed by [DONE] and no finish_reason
chunk. Strict OpenAI clients (pi coding agent) abort with
"Stream ended without finish_reason" and retry the turn.

Track whether any emitted chunk carried a *non-null* finish_reason;
after the streaming loop, if the engines finished output was
suppressed, emit the terminal chunk (finish_reason="tool_calls" when
tool calls were detected, else the engine's reason or "stop").

Also cherry-pick the engine-side fix from #681 (Thump604) so that
SimpleEngine stamps finish_reason="stop" on natural-stop streaming
epilogue instead of leaving it null.

Refs #672

* fix(server): avoid duplicate terminal stream usage

---------

Co-authored-by: Wayner Barrios <waybarrios@gmail.com>
Co-authored-by: Thump604 <thump@cosmiccooler.org>
TimotejLabsky added a commit to TimotejLabsky/vllm-mlx that referenced this pull request Aug 18, 2026
…ng, wanted deltas, stream-global restore

engine/simple.py was kept wholesale over upstream's c65c356/d7c1c98
rewrites (PATCHES.md 2026-08-17 rebase note). This commit restores the
upstream deltas we do want and reconciles the plumbing that assumed
upstream's engine:

- accept upstream waybarrios#574's prefix_trie_cache* kwargs fail-closed (raise if
  enabled — the fork's system-KV supersedes the trie; no silent no-op)
- upstream waybarrios#681: stamp finish_reason=length at the token-budget cutoff
  when the backend chunk carries None
- upstream waybarrios#686 folded into fork semantics: _import_text_model_classes
  candidate chain with gemma4/qwen3 family rules; unknown families raise
  instead of guessing qwen3_5
- engine_core: keep upstream's owns_worker teardown guard AND the fork's
  waybarrios#49 SSD flush; getattr-guard close_ssd_tier for duck-typed schedulers
- fix(streams): snapshot the pre-bind generation-stream globals at first
  worker bind and restore them in SimpleEngine.stop() — a retired worker
  otherwise leaves mlx_lm/mlx_vlm generation_stream naming a dead
  thread's stream (surfaced by upstream waybarrios#702's parity tests)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TimotejLabsky added a commit to TimotejLabsky/vllm-mlx that referenced this pull request Aug 23, 2026
…ng, wanted deltas, stream-global restore

engine/simple.py was kept wholesale over upstream's c65c356/d7c1c98
rewrites (PATCHES.md 2026-08-17 rebase note). This commit restores the
upstream deltas we do want and reconciles the plumbing that assumed
upstream's engine:

- accept upstream waybarrios#574's prefix_trie_cache* kwargs fail-closed (raise if
  enabled — the fork's system-KV supersedes the trie; no silent no-op)
- upstream waybarrios#681: stamp finish_reason=length at the token-budget cutoff
  when the backend chunk carries None
- upstream waybarrios#686 folded into fork semantics: _import_text_model_classes
  candidate chain with gemma4/qwen3 family rules; unknown families raise
  instead of guessing qwen3_5
- engine_core: keep upstream's owns_worker teardown guard AND the fork's
  waybarrios#49 SSD flush; getattr-guard close_ssd_tier for duck-typed schedulers
- fix(streams): snapshot the pre-bind generation-stream globals at first
  worker bind and restore them in SimpleEngine.stop() — a retired worker
  otherwise leaves mlx_lm/mlx_vlm generation_stream naming a dead
  thread's stream (surfaced by upstream waybarrios#702's parity tests)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TimotejLabsky added a commit to TimotejLabsky/vllm-mlx that referenced this pull request Aug 27, 2026
…ng, wanted deltas, stream-global restore

engine/simple.py was kept wholesale over upstream's c65c356/d7c1c98
rewrites (PATCHES.md 2026-08-17 rebase note). This commit restores the
upstream deltas we do want and reconciles the plumbing that assumed
upstream's engine:

- accept upstream waybarrios#574's prefix_trie_cache* kwargs fail-closed (raise if
  enabled — the fork's system-KV supersedes the trie; no silent no-op)
- upstream waybarrios#681: stamp finish_reason=length at the token-budget cutoff
  when the backend chunk carries None
- upstream waybarrios#686 folded into fork semantics: _import_text_model_classes
  candidate chain with gemma4/qwen3 family rules; unknown families raise
  instead of guessing qwen3_5
- engine_core: keep upstream's owns_worker teardown guard AND the fork's
  waybarrios#49 SSD flush; getattr-guard close_ssd_tier for duck-typed schedulers
- fix(streams): snapshot the pre-bind generation-stream globals at first
  worker bind and restore them in SimpleEngine.stop() — a retired worker
  otherwise leaves mlx_lm/mlx_vlm generation_stream naming a dead
  thread's stream (surfaced by upstream waybarrios#702's parity tests)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TimotejLabsky added a commit to TimotejLabsky/vllm-mlx that referenced this pull request Sep 23, 2026
…ng, wanted deltas, stream-global restore

engine/simple.py was kept wholesale over upstream's c65c356/d7c1c98
rewrites (PATCHES.md 2026-08-17 rebase note). This commit restores the
upstream deltas we do want and reconciles the plumbing that assumed
upstream's engine:

- accept upstream waybarrios#574's prefix_trie_cache* kwargs fail-closed (raise if
  enabled — the fork's system-KV supersedes the trie; no silent no-op)
- upstream waybarrios#681: stamp finish_reason=length at the token-budget cutoff
  when the backend chunk carries None
- upstream waybarrios#686 folded into fork semantics: _import_text_model_classes
  candidate chain with gemma4/qwen3 family rules; unknown families raise
  instead of guessing qwen3_5
- engine_core: keep upstream's owns_worker teardown guard AND the fork's
  waybarrios#49 SSD flush; getattr-guard close_ssd_tier for duck-typed schedulers
- fix(streams): snapshot the pre-bind generation-stream globals at first
  worker bind and restore them in SimpleEngine.stop() — a retired worker
  otherwise leaves mlx_lm/mlx_vlm generation_stream naming a dead
  thread's stream (surfaced by upstream waybarrios#702's parity tests)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TimotejLabsky added a commit to TimotejLabsky/vllm-mlx that referenced this pull request Sep 25, 2026
…ng, wanted deltas, stream-global restore

engine/simple.py was kept wholesale over upstream's c65c356/d7c1c98
rewrites (PATCHES.md 2026-08-17 rebase note). This commit restores the
upstream deltas we do want and reconciles the plumbing that assumed
upstream's engine:

- accept upstream waybarrios#574's prefix_trie_cache* kwargs fail-closed (raise if
  enabled — the fork's system-KV supersedes the trie; no silent no-op)
- upstream waybarrios#681: stamp finish_reason=length at the token-budget cutoff
  when the backend chunk carries None
- upstream waybarrios#686 folded into fork semantics: _import_text_model_classes
  candidate chain with gemma4/qwen3 family rules; unknown families raise
  instead of guessing qwen3_5
- engine_core: keep upstream's owns_worker teardown guard AND the fork's
  waybarrios#49 SSD flush; getattr-guard close_ssd_tier for duck-typed schedulers
- fix(streams): snapshot the pre-bind generation-stream globals at first
  worker bind and restore them in SimpleEngine.stop() — a retired worker
  otherwise leaves mlx_lm/mlx_vlm generation_stream naming a dead
  thread's stream (surfaced by upstream waybarrios#702's parity tests)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants