Skip to content

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

Closed
janhilgard wants to merge 1 commit into
waybarrios:mainfrom
janhilgard:fix/reasoning-parser-finalize-guard
Closed

janhilgard wants to merge 1 commit into
waybarrios:mainfrom
janhilgard:fix/reasoning-parser-finalize-guard

Conversation

@janhilgard

Copy link
Copy Markdown
Collaborator

Supersedes #653 with the CI failure resolved.

Background

@Thump604 opened #653 on 2026-07-28 to flush parser-buffered terminal text (e.g. Gemma 4's truncated <|channel> markers) before emitting the terminal chunk of a stream. The fix is correct and needed — Gemma 4's finalize_stream() already implements the fallback, stream_chat_completion just wasn't calling it.

CI on that PR broke four existing streaming tests:

tests/test_server.py::TestStreamChatCompletion::test_reasoning_stream_emits_structured_tool_calls
tests/test_server.py::TestStreamChatCompletion::test_reasoning_stream_redirects_gemma4_tool_marker
tests/test_server.py::TestStreamChatCompletion::test_reasoning_stream_skips_tool_parser_until_markup_appears
tests/test_server.py::TestStreamChatCompletion::test_response_format_stream_promotes_reasoning_json_to_content

All four failed with AttributeError: 'FakeReasoningParser'/'ReasoningOnlyParser' object has no attribute 'finalize_stream'.

Root cause

Those tests declare inline test fixtures (FakeReasoningParser, ReasoningOnlyParser) that duck-type extract_reasoning_streaming and reset_state but don't inherit from ReasoningParser. The base class provides finalize_stream() as a documented no-op, so any parser that does inherit gets the default automatically. #653 called reasoning_parser.finalize_stream() unconditionally, which broke the duck-typed fixtures.

The alternatives are:

  1. Add hasattr / getattr guard in _resolve_reasoning_stream_delta.
  2. Add finalize_stream = lambda self: None stubs to every fake fixture.

I picked (1). The base class's own docstring establishes finalize_stream as an optional extension point ("Default implementation is a no-op"), so the calling code should tolerate parsers that don't implement it — that keeps the interface contract loose for third-party parsers as well, not only tests.

Change

One-line change in _resolve_reasoning_stream_delta (vllm_mlx/server.py):

# Before (from #653):
final_delta = reasoning_parser.finalize_stream()

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

Everything else is @Thump604's original diff, unchanged. Authorship on this commit is preserved as Thump604 <...>; I'm listed as Co-Authored-By for the guard change.

Validation

Ran the 5 relevant tests locally (Python 3.14, macOS ARM64, uv-installed dev extras against fresh clone):

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

The four previously-failing tests now pass without modification — the guard tolerates duck-typed fixtures. The new test (@Thump604's Gemma flush regression) still passes as designed.

Not claimed

Same non-claims as #653:

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

Followup

Please close #653 after merging this. The intent is identical; this is just a rebased single-commit variant with the CI regression fixed and the base-class no-op contract respected.

Closes #653.

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.

Guard the invocation so third-party or test-only reasoning parsers
that duck-type the interface without inheriting from ReasoningParser
still stream correctly (default no-op behavior).

## 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 added 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

- Full test suite passes on apple-silicon; the four previously
  failing tests
  (test_reasoning_stream_emits_structured_tool_calls,
  test_reasoning_stream_redirects_gemma4_tool_marker,
  test_reasoning_stream_skips_tool_parser_until_markup_appears,
  test_response_format_stream_promotes_reasoning_json_to_content)
  now pass because the getattr guard tolerates duck-typed test
  fixtures without finalize_stream.
- The new test
  (test_gemma_stream_flushes_pending_content_before_length_finish)
  exercises the intended Gemma 4 buffered-tail flush.

Closes waybarrios#653.

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

Copy link
Copy Markdown
Collaborator Author

Closing in favor of #653 — I pushed the same fix directly to the PR branch (amended into the original commit, preserving @Thump604 as author with co-author credit for the guard change). See #653 for the updated PR.

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