[#17156][fix] Flush buffered text in DeepSeekR1Parser.finish() - #17157
[#17156][fix] Flush buffered text in DeepSeekR1Parser.finish()#17157Yigtwxx wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesReasoning parser stream finalization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
|
@zhaoyangwang-nvidia — sorry for the direct ping, and thanks for triggering the pipeline on #17159; that one is now merged ( This PR is the sibling fix from the same pass and it hasn't been picked up yet: no reviewer assigned, and Same shape of argument as #17159 — it doesn't propose new behaviour, it makes one class honour a contract its siblings in the same file already honour: Regression tests are in Could you either trigger |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/unittest/llmapi/test_reasoning_parser.py`:
- Line 119: Update the chat_template_kwargs parameter annotation in the affected
test helper to use the precise type dict[str, bool] instead of an
unparameterized dict, preserving the existing function behavior and other
annotations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 765075d5-3f88-4762-b6a0-73ebee8a4657
📒 Files selected for processing (1)
tests/unittest/llmapi/test_reasoning_parser.py
d2ff4e2 to
b35fd14
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run |
|
PR_Github #63990 [ run ] triggered by Bot. Commit: |
|
PR_Github #63990 [ run ] completed with state
|
|
/bot run |
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>
…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>
…inimal set The new coverage cost 37 parametrized cases for a 25-line fix. CPU pre-merge runtime is a shared cost paid by every PR in the repo, so keep net new cases down: one (parser_key, text) pair per branch of finish() in the stream / non-stream property test, which subsumes the example-based tests of the individual branches, plus one multi-character delta case because streaming a character at a time never reaches the rfind branch of parse_delta that fills _buffer from a delta carrying both text and a partial tag. The extra deepseek_v4 case covers the delegating subclass named in the PR scope, whose finish() forwards to DeepSeekR1Parser or IdentityReasoningParser depending on the thinking flag. Signed-off-by: Yigtwxx <yigiterdogan023@gmail.com>
…arser tests Narrow `chat_template_kwargs` to `dict[str, bool]`, which is what the parametrized values are, and reshape the two new docstrings so the summary line stands alone and the closing quotes sit on their own line - ruff-legacy flagged D205 and D209 on both. Signed-off-by: Yigtwxx <yigiterdogan023@gmail.com>
b35fd14 to
2d251e5
Compare
|
@zhaoyangwang-nvidia thanks for triggering it. The L0 run came back In the meantime I did two things. 1. Rebased onto current 2. Audited what in L0 can actually reach the changed code path. There is exactly one test: assert len(content_chunks) == 0
if model_name.startswith("Qwen3"):
assert len(reasoning_content_chunks) == 1
else:
assert len(reasoning_content_chunks) == 2I believe it is unaffected, for two reasons:
I have no GPU or model weights here, so that is a code reading rather than a run — happy to be wrong. If that test is the one failing, I will fix it; if the failures are elsewhere, knowing the names is enough for me to take it from there. |
|
PR_Github #64013 [ run ] triggered by Bot. Commit: |
|
PR_Github #64013 [ run ] completed with state
|
|
#51945 on the rebased head ( What I can contribute without them: Every L0 verdict posted repo-wide between 07:30 and 11:30 UTC today, excluding this PR:
Green rate 3/22. Several PRs also flipped between reruns of the same branch — #17291 went UNSTABLE → FAILURE → UNSTABLE, #17282 went FAILURE → UNSTABLE. That is the shape of a broadly unhealthy pipeline rather than 22 individually broken PRs, and it is the reason I am reluctant to start changing code to chase a failure I cannot see. Why I do not think this PR can be the cause. The source diff is one added method; The only L0 test that streams through one is One thing I got wrong in the description, now fixed. If the failures do turn out to be mine, I will fix them — I just need the names, since I have no way to see them and no GPU here to reproduce. |
|
Update while waiting for the failing test names — I found a data source I did not know was public, and it changes what I can say about this. The
So I now know the shape of the failure, just not the names. The same tally across every open PR that ran a full L0 today
Every comparable full-suite run today failed tests, and this PR has the lowest failure count of the set. Smaller partial runs show the same pattern (17301: 4 failed, 17271: 6 failed, 17269: 4 failed, 17264: 3 failed, 17277: 2 failed).
|
|
Follow-up, and it supersedes the framing of my previous comment — I found the precedent that I think settles what to do next, and I also found that one statistic I was about to lean on is biased, so I am flagging that myself. The same commit produced both verdicts on my sibling PR#17159 — the parser fix from the same pair, merged yesterday — ran L0 three times:
So a FAILURE verdict on this pipeline is demonstrably not a property of the commit under test. This PR has had two runs; #17159 needed three
Note the first run only executed 3492 tests and failed one of them — the two runs did not even test the same surface, which is another reason the failures look environmental rather than diff-driven. My concrete ask is therefore just: Correcting myself on one pointIn my previous comment I was building toward "no full L0 run is ever green". I checked before claiming it, and it is not a sound claim — the The |
4 participants
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.KimiK2ReasoningParser(kimi_k2,kimi_k25) also subclassesDeepSeekR1Parserwithout overriding
finish(), so it picks the flush up too. Note that its extradelimiter,
<|tool_calls_section_begin|>, is not part of the discard check, so a bufferholding exactly that token is now flushed as text rather than dropped. That is closer to
"do not lose model output" than the current silent drop, but say the word if you would
rather the discard check learn about it.
NemotronV3ReasoningParseroverridesfinish()in full and is unaffected.
Test Coverage
tests/unittest/llmapi/test_reasoning_parser.py(CPU-only, no model weights) — six newcases, one per behavior worth pinning:
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, so itsubsumes example-based tests of the individual branches. One
(parser_key, text)pairper branch of
finish():("deepseek-r1", "a <")flushes as reasoning content,("qwen3", "a<")flushes as visible content, and("deepseek-r1", "a</think>")discards a buffer holding exactly a delimiter.
test_deepseek_r1_reasoning_parser_finish_flushes_partial_tag— a single delta carryingboth text and a partial tag (
"a </thin") fills_bufferthrough therfindbranch ofparse_delta, which character-at-a-time streaming never reaches. That is the shape areal stream delivers.
test_deepseek_v4_reasoning_parser_finish_delegates—DeepSeekV4ReasoningParserforwards
finish()toDeepSeekR1Parseror toIdentityReasoningParserdepending onthe thinking flag, so the delegation named in the scope above has two targets.
Results on this branch: 142 passed. Against
mainwith only the test changes applied, 4of the 6 new cases fail, so they do guard the fix. The other 2 pass either way by design —
they guard against a wrong fix that leaks a complete delimiter or flushes from the
identity parser. 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
DeepSeekV4ReasoningParserdelegation.test-db/andqa/coverage data is unavailable.