Skip to content

fix(server): flush reasoning parser at stream end - #653

Closed
Thump604 wants to merge 1 commit into
waybarrios:mainfrom
Thump604:604/gemma4-stream-finalizer-upstream
Closed

Thump604 wants to merge 1 commit into
waybarrios:mainfrom
Thump604:604/gemma4-stream-finalizer-upstream

Conversation

@Thump604

Copy link
Copy Markdown
Collaborator

Summary

Flush parser-buffered terminal text before emitting a streamed completion's
terminal chunk.

Gemma 4's reasoning parser already implements finalize_stream() for a
truncated channel marker, but stream_chat_completion() did not invoke it.
The finalizer is now called only for the terminal output and its content or
reasoning is merged into that terminal delta.

Reproduction

With Gemma 4 reasoning enabled, a stream that ends at finish_reason=length
after the parser has buffered a partial channel marker could omit that buffered
tail from the client stream.

The regression simulates a Gemma thought-to-response transition that ends at a
length boundary. It verifies the thought text, the buffered final content, and
the terminal length reason all reach the emitted stream.

Scope

  • Preserves template selection, sampling, request limits, and scheduler behavior.
  • Does not change Jobs integration, model configuration, or constrained decoding.
  • Does not claim that every length-bound Gemma generation produces final content;
    it preserves parser-buffered terminal content when it exists.

Validation

  • pytest -q tests/test_server.py -k 'gemma_stream_flushes_pending_content_before_length_finish or stream_without_parser_flags_keeps_plain_text'
  • ruff check vllm_mlx/server.py tests/test_server.py --select E,F,W --ignore E402,E501,E731,F811,F841
  • Live Gemma 4 streaming boundary evidence is retained locally at
    /opt/ai-runtime/run/gemma4-stream-finalizer-certification/20260728T183900Z-live-manifest-replay/summary.json.

@janhilgard

Copy link
Copy Markdown
Collaborator

Pushed a fix into the PR branch directly (I was on the assignee list and the branch had maintainerCanModify: true, so this seemed like the least disruptive path — happy to revert if you'd rather it be a stacked review).

What changed (a6ebbf6 → 3f675d8, amended into your existing commit):

Only one line in _resolve_reasoning_stream_delta. The four failing tests all use inline duck-typed fixtures (FakeReasoningParser, ReasoningOnlyParser) that don't inherit from ReasoningParser and therefore don't get the base class's finalize_stream() no-op:

# Before:
final_delta = reasoning_parser.finalize_stream()

# After:
finalize = getattr(reasoning_parser, "finalize_stream", None)
final_delta = finalize() if callable(finalize) else None

Why guard instead of patching the fixtures: ReasoningParser.finalize_stream()'s own docstring says "Default implementation is a no-op" — that establishes it as an optional extension point. The caller should tolerate parsers that don't implement it, not just to unblock these tests but to keep the contract loose for third-party or vendored parsers. Fixing the guard once is less fragile than adding stubs to every fake fixture forever.

Validation (Python 3.14, macOS ARM64, fresh clone of Thump604/vllm-mlx@604/gemma4-stream-finalizer-upstream after push):

tests/test_server.py::TestStreamChatCompletion::test_reasoning_stream_emits_structured_tool_calls PASSED
tests/test_server.py::TestStreamChatCompletion::test_reasoning_stream_redirects_gemma4_tool_marker PASSED
tests/test_server.py::TestStreamChatCompletion::test_reasoning_stream_skips_tool_parser_until_markup_appears PASSED
tests/test_server.py::TestStreamChatCompletion::test_response_format_stream_promotes_reasoning_json_to_content PASSED
tests/test_server.py::TestStreamChatCompletion::test_gemma_stream_flushes_pending_content_before_length_finish PASSED
5 passed in 1.72s

All four previously-failing tests now pass, and your new test_gemma_stream_flushes_pending_content_before_length_finish regression still passes as intended.

Authorship preserved as yours on the commit; I added Co-Authored-By: janhilgard for the guard change.

CI should re-run automatically on the force-push. If you'd prefer a stacked commit or a separate PR structure, no problem — say the word and I'll revert my amend and open it as a follow-up instead.

@janhilgard
janhilgard force-pushed the 604/gemma4-stream-finalizer-upstream branch from 3f675d8 to 093365e Compare July 30, 2026 15:35
@janhilgard

Copy link
Copy Markdown
Collaborator

Rebased onto current main (94c008b) and fixed a defect that the rebase exposed. 3f675d8 → 093365e.

The problem

This PR was written against 0dd1157, where stream_chat_completion used the module-level _reasoning_parser. #644 landed today at 09:56 and switched those call sites to a request-local instance from _build_reasoning_parser(engine).

Git merges both cleanly — no conflict — but the result was split-brained:

reasoning_parser = _build_reasoning_parser(engine)          # from #644, per-request

delta_msg = reasoning_parser.extract_reasoning_streaming(…)  # local  — buffers here

resolved_delta = _resolve_reasoning_stream_delta(
    _reasoning_parser, output, delta_msg, request            # global — finalizes here
)

Extraction buffers into the request-local parser; finalization calls finalize_stream() on the module-level global, which never saw a token. So the buffered tail is discarded and this PR silently does nothing — while also reintroducing the cross-request shared state that #644 removed.

The getattr guard I added in my earlier push makes it worse to spot: _reasoning_parser is None at module scope, so the guard returns None and the whole thing fails silently. No exception, no log.

Verified, not inferred

Test-merged this PR onto 94c008b and ran your own regression:

tests/test_server.py::…::test_gemma_stream_flushes_pending_content_before_length_finish

assert content == "FINAL<|chan"
E   AssertionError: assert 'FINAL' == 'FINAL<|chan'

The <|chan tail — exactly what the finalizer exists to recover — was dropped. Your test was correct all along; it just could not fail on the old base, because there the global was the parser doing the buffering.

CI stayed green because it last ran 2026-07-29 22:08, about thirteen hours before #644 merged. The combination was never tested.

Fix

One line in stream_chat_completion:

 resolved_delta = _resolve_reasoning_stream_delta(
-    _reasoning_parser, output, delta_msg, request
+    reasoning_parser, output, delta_msg, request
 )

After it, on the rebased branch:

  • test_gemma_stream_flushes_pending_content_before_length_finish — passes
  • -k "stream or reasoning" — 25 passed
  • the four tests my earlier getattr guard addressed — 5 passed
  • black clean

Authorship preserved as yours; Co-Authored-By added for the rebase and the fix.

Worth checking on neighbours

#652 and #654 also touch streaming parser state and were both authored against pre-#644 main. Same silent-merge shape is plausible there — clean merge, green CI from before #644, wrong instance afterwards. I have not test-merged them; flagging so someone does before they land.

More generally: any PR whose CI predates 09:56 today and touches stream_chat_completion deserves a re-run rather than trust in the existing green tick.

Credit where due — this was surfaced by a scheduled agent run doing the audit, not by me reading the diff. I had pushed to this branch yesterday and did not catch it.

Co-Authored-By: janhilgard <jan.hilgard@gmail.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Co-Authored-By: janhilgard <jan.hilgard@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@janhilgard janhilgard closed this Jul 30, 2026
@janhilgard
janhilgard force-pushed the 604/gemma4-stream-finalizer-upstream branch from 093365e to 93c85d3 Compare July 30, 2026 15:50
@Thump604

Copy link
Copy Markdown
Collaborator Author

@janhilgard The corrective rebase at 93c85d3 addresses the post-#644 request-local parser mismatch, but #653 was closed afterward and upstream main does not yet contain the terminal-finalizer wiring. Was the closure intentional because a replacement is planned? If not, please reopen this branch so CI can validate the corrected current-main diff.

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.

2 participants