[#17156][fix] Flush buffered text in DeepSeekR1Parser.finish() - #17157
[#17156][fix] Flush buffered text in DeepSeekR1Parser.finish()#17157Yigtwxx wants to merge 2 commits into
Conversation
parse_delta withholds a trailing fragment that could still grow into a <think>/</think> tag. DeepSeekR1Parser never overrode finish(), so when a stream ended while such a fragment was buffered the characters were silently dropped from content or reasoning_content. Override finish() to emit the withheld text, attributing it to the block it was withheld in. A buffer holding exactly a complete tag is a delimiter rather than model output and is still discarded. NemotronV3ReasoningParser and Gemma4ReasoningParser already implement this flush; this brings the shared base parser in line with them. Signed-off-by: Yigtwxx <yigiterdogan023@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesReasoning parser end-of-stream handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unittest/llmapi/test_reasoning_parser.py (2)
78-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd complete type annotations to the new test functions.
Add
-> Noneto each test function. Uselist[str]fordelta_texts.As per coding guidelines, "Annotate every function" and "use precise ... types."
Also applies to: 95-96, 108-109, 119-119, 135-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/llmapi/test_reasoning_parser.py` around lines 78 - 79, Update the new test functions, including test_deepseek_r1_reasoning_parser_finish_flushes_reasoning and the additional functions at the referenced locations, with complete annotations: use list[str] for delta_texts and add -> None to each function signature.Source: Coding guidelines
65-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the test-only constant private.
R1_AT_START_KEYSonly supports parametrization in this module. Rename it to_R1_AT_START_KEYS.As per coding guidelines, "Prefix non-public names with
_."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/llmapi/test_reasoning_parser.py` around lines 65 - 67, Rename the test-only constant R1_AT_START_KEYS to _R1_AT_START_KEYS and update every reference to it in the module, preserving its parametrization behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unittest/llmapi/test_reasoning_parser.py`:
- Around line 78-79: Update the new test functions, including
test_deepseek_r1_reasoning_parser_finish_flushes_reasoning and the additional
functions at the referenced locations, with complete annotations: use list[str]
for delta_texts and add -> None to each function signature.
- Around line 65-67: Rename the test-only constant R1_AT_START_KEYS to
_R1_AT_START_KEYS and update every reference to it in the module, preserving its
parametrization behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 88d07722-0ea2-4ab1-9d81-098fc96cceae
📒 Files selected for processing (2)
tensorrt_llm/llmapi/reasoning_parser.pytests/unittest/llmapi/test_reasoning_parser.py
…t helpers Follow-up on review feedback: the tests added for DeepSeekR1Parser.finish() lacked return annotations and used a bare list type, and the parser-key constant is module-internal. CODING_GUIDELINES requires every function to be annotated and non-public names to be prefixed with an underscore. Signed-off-by: Yigtwxx <yigiterdogan023@gmail.com>
Description
Fixes #17156.
DeepSeekR1Parser.parse_deltawithholds a trailing fragment that could still grow intoa
<think>/</think>tag, keeping it inself._bufferuntil the next delta arrives.BaseReasoningParser.finish()exists so a parser can flush that state when the streamends, and the serving layer calls it (
serve/postprocess_handlers.py:177,serve/responses_utils.py:963).DeepSeekR1Parsernever overrodefinish(), so astream that ended while a fragment was buffered silently dropped those characters from
contentorreasoning_content— for example a response ending in a literal<, orone truncated by
max_tokenspartway through</thin.This adds the missing
finish()override: the withheld text is emitted and attributedto the block it was withheld in (reasoning content inside a reasoning block, visible
content otherwise). A buffer holding exactly a complete tag is a delimiter rather than
model output, so it is still discarded, which keeps the existing behavior for a stray
closing tag arriving as the final delta.
This is a conformance fix rather than a behavior change: two sibling parsers in the same
file already implement exactly this flush —
NemotronV3ReasoningParser.finish()andGemma4ReasoningParser.finish()— andGemma4ReasoningParser.finish()has the sameshape as the implementation added here.
parse_deltais untouched.Scope: every parser key backed by
DeepSeekR1Parser(deepseek-r1,qwen3,qwen3_5,laguna,minimax_m2,minimax_m2_append_think) plus the subclassesMiniMaxM3ReasoningParser(minimax_m3) andDeepSeekV4ReasoningParser(
deepseek_v4), whosefinish()delegates to the base parser and was a no-op until now.NemotronV3ReasoningParseroverridesfinish()in full and is unaffected.Test Coverage
tests/unittest/llmapi/test_reasoning_parser.py(CPU-only, no model weights):test_deepseek_r1_reasoning_parser_finish_flushes_reasoning— a withheld partialclosing tag is emitted as reasoning content at end of stream. Parametrized over the
keys that stream from inside the reasoning block:
deepseek-r1,qwen3_5,minimax_m2,minimax_m2_append_think.qwen3_5andminimax_m2_append_thinkwerenot referenced by any test in this file before.
test_deepseek_r1_reasoning_parser_finish_flushes_content— a withheld partial openingtag is emitted as visible content, for
qwen3andlaguna.test_deepseek_r1_reasoning_parser_finish_drops_complete_tag— a buffer holding only adelimiter does not leak into the output.
test_deepseek_r1_reasoning_parser_finish_is_idempotent— a secondfinish()returnsan empty result.
test_deepseek_r1_reasoning_parser_stream_matches_non_stream— streaming one characterat a time and then finishing produces the same
content/reasoning_contentsplit asparse()on the whole text. This is the contract the missing flush violated.Results on this branch: 173 passed. Against
mainwith only the test changes applied,19 of the new cases fail, so they do guard the fix. All 136 pre-existing cases in the
file pass unchanged.
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.Dev Engineer Review
DeepSeekR1Parser.finish()flushes buffered text at end-of-stream.<think>and</think>tags remain discarded.reasoning_contentorcontentfield.QA Engineer Review
finish().test-db/andqa/coverage data is unavailable.