[#17146][fix] Resolve reasoning mode from the rendered prompt - #17305
Conversation
47118cf to
616da4e
Compare
|
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:
WalkthroughThe change adds prompt-based thinking-state resolution, Poolside V1 parser support, Qwen3.5 and MiniMax M2 registrations, Laguna alias mapping, server integration, and expanded tests. ChangesReasoning parser flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ChatTemplate
participant OpenAIServer
participant ReasoningParserFactory
participant PoolsideV1ReasoningParser
ChatTemplate->>OpenAIServer: Render chat prompt
OpenAIServer->>ReasoningParserFactory: Resolve prefilled thinking state
ReasoningParserFactory->>PoolsideV1ReasoningParser: Check prompt markers
PoolsideV1ReasoningParser-->>ReasoningParserFactory: Return thinking state
ReasoningParserFactory-->>OpenAIServer: Return true, false, or None
OpenAIServer->>OpenAIServer: Update thinking and enable_thinking
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/unittest/llmapi/test_reasoning_parser.py (1)
370-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated opt-in test.
test_resolve_prefilled_thinking_opted_inrepeatstest_alias_resolves_identicallywithparser="poolside_v1". The prompt, tails, and expectations are identical, andtest_resolve_prefilled_thinkingalso covers both tails. The negative opt-in case is covered separately bytest_resolve_prefilled_thinking_requires_opt_in.♻️ Proposed removal
-@pytest.mark.parametrize(("tail", "expected"), [(R1_START, True), - (R1_END, False)]) -def test_resolve_prefilled_thinking_opted_in(tail: str, expected): - assert ReasoningParserFactory.resolve_prefilled_thinking( - "poolside_v1", f"<assistant>{tail}") is expected - -🤖 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 370 - 374, Remove the redundant test_resolve_prefilled_thinking_opted_in test, retaining test_alias_resolves_identically, test_resolve_prefilled_thinking, and test_resolve_prefilled_thinking_requires_opt_in as the existing coverage.
🤖 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 `@tensorrt_llm/llmapi/reasoning_parser.py`:
- Around line 96-97: Add a return annotation to the class method
ReasoningParserFactory.keys, using the appropriate mapping view type for the
object returned by cls._parsers.keys(). Preserve the existing implementation and
behavior.
In `@tensorrt_llm/serve/openai_server.py`:
- Around line 1571-1585: Update the request-processing flow around
ReasoningParserFactory.resolve_prefilled_thinking so the prompt-resolved
thinking and enable_thinking values are applied before
add_thinking_budget_logits_processor runs. Ensure the budget processor for
poolside_v1 and laguna receives the resolved mode rather than stale request
kwargs, while preserving consistent parser selection during postprocessing.
In `@tests/unittest/llmapi/test_reasoning_parser.py`:
- Around line 400-413: Update test_resolve_prefilled_thinking_requires_opt_in to
first assert each parser name is registered in ReasoningParserFactory._parsers,
then retain the existing None assertions for every tail value. This ensures the
test validates opt-in behavior rather than passing for an unregistered name.
- Around line 377-397: Update test_resolved_mode_overrides_stale_thinking_kwarg
to include a scenario where only enable_thinking is set to False while stale
thinking=True remains, and assert that the parser places the visible text in
reasoning_content rather than content. Retain the existing case verifying that
clearing both keys yields normal content, so the test covers the parser’s OR
behavior and proves both keys must be cleared.
---
Nitpick comments:
In `@tests/unittest/llmapi/test_reasoning_parser.py`:
- Around line 370-374: Remove the redundant
test_resolve_prefilled_thinking_opted_in test, retaining
test_alias_resolves_identically, test_resolve_prefilled_thinking, and
test_resolve_prefilled_thinking_requires_opt_in as the existing coverage.
🪄 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: 80888a38-a585-4a62-af3e-642c43dc6998
📒 Files selected for processing (7)
docs/source/developer-guide/telemetry.mdtensorrt_llm/llmapi/llm_args.pytensorrt_llm/llmapi/reasoning_parser.pytensorrt_llm/serve/openai_server.pytensorrt_llm/usage/llm_args_golden_manifest.jsontests/unittest/api_stability/references/trtllm_serve_cli.yamltests/unittest/llmapi/test_reasoning_parser.py
|
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. |
616da4e to
b62ee09
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unittest/llmapi/test_reasoning_parser.py (2)
416-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the non-opted-in parser list from the registry.
The hardcoded list does not cover parsers registered later. If a new parser sets
resolves_thinking_from_promptby mistake, this test still passes. Compute the list fromReasoningParserFactory.keys()minus the opted-in names. The registration guard at Line 429 then becomes unnecessary.♻️ Proposed refactor
-@pytest.mark.parametrize("parser", [ - "deepseek-r1", "deepseek_v4", "qwen3", "qwen3_5", "minimax_m2", - "minimax_m3", "nemotron-v3", "nano-v3", "gemma4", "kimi_k2", "kimi_k25" -]) +_OPTED_IN_PARSERS = {"poolside_v1", "laguna"} + + +@pytest.mark.parametrize( + "parser", + sorted(set(ReasoningParserFactory.keys()) - _OPTED_IN_PARSERS)) def test_resolve_prefilled_thinking_requires_opt_in(parser: str): """Parsers that have not opted in must never be resolved from the prompt. `deepseek_v4` shares the base class and `nemotron-v3` / `nano-v3` also read `enable_thinking`, so without the flag they would silently pick up a mode the server inferred. """ - # Otherwise a typo or a dropped registration passes vacuously, since an - # unknown name also resolves to None. - assert parser in ReasoningParserFactory.keys() for tail in (R1_START, R1_END, ""): assert ReasoningParserFactory.resolve_prefilled_thinking( parser, f"<assistant>{tail}") is None🤖 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 416 - 432, Update test_resolve_prefilled_thinking_requires_opt_in to derive its parameterized parser list from ReasoningParserFactory.keys(), excluding the registered parsers that explicitly opt in via resolves_thinking_from_prompt. Remove the redundant registry-membership assertion, while preserving the existing assertions that each non-opted-in parser resolves to None for all tested prompt tails.
370-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant opt-in test.
test_resolve_prefilled_thinking_opted_inrepeats assertions that already exist. Lines 353-354 coverpoolside_v1with both markers, andtest_alias_resolves_identicallycovers the same two tails forpoolside_v1andlaguna. This test adds no new coverage.♻️ Proposed removal
-@pytest.mark.parametrize(("tail", "expected"), [(R1_START, True), - (R1_END, False)]) -def test_resolve_prefilled_thinking_opted_in(tail: str, expected): - assert ReasoningParserFactory.resolve_prefilled_thinking( - "poolside_v1", f"<assistant>{tail}") is expected - -🤖 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 370 - 374, Remove the redundant test_resolve_prefilled_thinking_opted_in test and its parametrized cases; retain the existing coverage for poolside_v1 markers and alias behavior in the surrounding tests.
🤖 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`:
- Around line 283-457: Add finalization coverage to
test_poolside_v1_reasoning_parser_stream for an unterminated </think> prefix
such as “reason</th”. Feed the fragment through parse_delta, then call finish()
and assert the buffered suffix is emitted as reasoning content rather than
dropped, with no visible content.
---
Nitpick comments:
In `@tests/unittest/llmapi/test_reasoning_parser.py`:
- Around line 416-432: Update test_resolve_prefilled_thinking_requires_opt_in to
derive its parameterized parser list from ReasoningParserFactory.keys(),
excluding the registered parsers that explicitly opt in via
resolves_thinking_from_prompt. Remove the redundant registry-membership
assertion, while preserving the existing assertions that each non-opted-in
parser resolves to None for all tested prompt tails.
- Around line 370-374: Remove the redundant
test_resolve_prefilled_thinking_opted_in test and its parametrized cases; retain
the existing coverage for poolside_v1 markers and alias behavior in the
surrounding tests.
🪄 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: 821cb581-67c1-439f-ade6-410365d6bc21
📒 Files selected for processing (7)
docs/source/developer-guide/telemetry.mdtensorrt_llm/llmapi/llm_args.pytensorrt_llm/llmapi/reasoning_parser.pytensorrt_llm/serve/openai_server.pytensorrt_llm/usage/llm_args_golden_manifest.jsontests/unittest/api_stability/references/trtllm_serve_cli.yamltests/unittest/llmapi/test_reasoning_parser.py
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/source/developer-guide/telemetry.md
- tests/unittest/api_stability/references/trtllm_serve_cli.yaml
- tensorrt_llm/usage/llm_args_golden_manifest.json
- tensorrt_llm/llmapi/llm_args.py
- tensorrt_llm/serve/openai_server.py
- tensorrt_llm/llmapi/reasoning_parser.py
DomBrown
added
the
api-compatible
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Approach looks right — reading the mode off the rendered prompt is the only thing that works when the template prefills the marker, and gating it on resolves_thinking_from_prompt with a test that every other registered parser stays at None is the right way to keep it contained. The rename matches what tool_parser_factory.py:38 already does, so doing it here is fine.
One ask before merge: the parsers that opt into resolves_thinking_from_prompt are only correct on the code path that can actually see a rendered prompt (openai_chat with add_generation_prompt). On every other path — offline LLM API, and the disagg generation server, which receives prompt_token_ids — laguna/poolside_v1 now silently falls back to IdentityReasoningParser and returns reasoning text as content. That is a behavior regression relative to today, and it fails silently: no error, no log line, just a response with the wrong field populated. Please add a guard so an unsupported path is visible rather than quietly wrong. Concretely:
- A one-time
logger.warningwhen a parser withresolves_thinking_from_prompt = Trueis constructed without a resolvable mode (nothinking/enable_thinkinginchat_template_kwargs), naming the parser and saying the mode could not be resolved so output will not be split. - A startup-time check in the disagg generation server: if
reasoning_parseropts into prompt resolution and the server role is generation, that combination can never work today. I'd lean toward a hard error there rather than a warning, since it's a static config mistake that produces wrong output on every request, but a loud startup warning is acceptable if you'd rather not break existing deployments in a bugfix PR.
I wouldn't hard-fail the offline LLM path — callers that pass chat_template_kwargs themselves are legitimate and shouldn't be blocked — so a warning is the right level there.
Two gaps also worth a line in the PR description: the laguna key is not behavior-preserving, and the fix doesn't reach disaggregated serving. Details inline.
b62ee09 to
2a6bce9
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. |
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`:
- Around line 288-294: Add complete type annotations to all 14 new
reasoning-parser test functions in
tests/unittest/llmapi/test_reasoning_parser.py at lines 288-294, 305-316,
326-333, 336-341, 347-349, 360-362, 365-367, 372-374, 384-399, 407-415, 418-434,
445-454, 461-473, and 481-496: add -> None return annotations, use dict[str,
bool] for kwargs, list[str] for stream fixtures, and bool or bool | None for
expected values as applicable.
🪄 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: 50931dfd-1840-4f75-8af0-fea07f415824
📒 Files selected for processing (7)
docs/source/developer-guide/telemetry.mdtensorrt_llm/llmapi/llm_args.pytensorrt_llm/llmapi/reasoning_parser.pytensorrt_llm/serve/openai_server.pytensorrt_llm/usage/llm_args_golden_manifest.jsontests/unittest/api_stability/references/trtllm_serve_cli.yamltests/unittest/llmapi/test_reasoning_parser.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tensorrt_llm/usage/llm_args_golden_manifest.json
- tests/unittest/api_stability/references/trtllm_serve_cli.yaml
- tensorrt_llm/llmapi/llm_args.py
- tensorrt_llm/serve/openai_server.py
- docs/source/developer-guide/telemetry.md
zhaoyangwang-nvidia
left a comment
zhaoyangwang-nvidia
left a comment
There was a problem hiding this comment.
Reviewed the change. The approach (resolving the rendered mode from the prompt, behind an opt-in flag) is sound and the unit tests are thorough. What blocks me is the laguna semantics change: it now resolves to a kwargs-driven parser, while the fix only covers the "OpenAI chat endpoint + server-rendered prompt + add_generation_prompt=True" path. Offline LLM API users and the disagg generation server regress from "splits on an emitted </think>" to "everything is content" — as @brnguyen2 already pointed out; I checked and I agree those are real. Either give those paths an actual fix, or keep the old behavior and warn loudly when the mode cannot be resolved, rather than silently degrading. The rest below are non-blocking.
2a6bce9 to
7000db3
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. |
7000db3 to
5fafc13
Compare
|
Thanks both for the reviews. @zhaoyangwang-nvidia I think yours was read against an earlier state. Disagg is properly fixed now too, following your suggestion, so the mode is relayed from the context worker rather than inferred by the generation worker. Minor thing: on main Rebased onto current main as of this morning, which also picks up #17157. Could you take another look? |
|
/bot run |
DomBrown
left a comment
DomBrown
left a comment
There was a problem hiding this comment.
Approving from API perspective
|
PR_Github #64606 [ run ] triggered by Bot. Commit: |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Re-reviewed after the three fixes. The unresolved-mode fallback in PoolsideV1ReasoningParser.__init__ restores the pre-PR laguna behavior exactly, so the base-class swap no longer changes any path that isn't newly resolved — that was my main concern and it's settled.
I traced the disagg relay rather than deploying it, and the hop holds up: _get_ctx_request forces stream: False (openai_disagg_service.py:214), so the ctx worker always lands in chat_response_post_processor — the one handler you stamp — and _get_gen_request copies ctx_response.choices[0].disaggregated_params wholesale (openai_disagg_service.py:231) before overwriting only request_type/schedule_style/conversation_id/ctx_usage. So the field survives.
That relay does rest on an unstated dependency, though: the streaming post-processor deliberately does not stamp, which is correct only because of the forced stream: False. Rather than a comment there, I'd put a once-per-process logger.warning on the consuming side — gen worker, nothing rendered, opted-in parser, and no relayed value — since that single condition catches every route into the silently-wrong mode (a future orchestration path that doesn't go through _get_ctx_request, a rolling upgrade where the ctx worker predates the field, a hand-crafted generation_only request), not just the streaming one. Details in the [openai_server.py:1585](https://github.com/NVIDIA/TensorRT-LLM/pull/17305/files#diff-2eb783dbd719ea8b2067105279f5a50460168065912d0efd2a3b2cdc0b3a78e5R1585) comment; it's the same if as the opt-in gate.
Remaining comments are narrow — the relay isn't gated on the parser opting in, two of the new tests re-implement the server logic rather than calling it (so they pass by construction), and resolve_prefilled_thinking's docstring under-specifies both why a suffix test is sound and what its three return values mean.
Description hygiene: the PR body still describes only the prompt-resolution flag and the rename. The new DisaggregatedParams.resolved_thinking field and the ctx→gen relay are a protocol addition that reviewers of a disagg deployment will care about; please add a line. The rename mirrors tool_parser/poolside_v1_parser.py and its "laguna" -> "poolside_v1" alias map, so it's consistent with what's already there — no complaint. No user-facing docs mention laguna, and the telemetry table/manifest/CLI reference are all updated, so nothing else is owed on docs.
|
PR_Github #64606 [ run ] completed with state
|
64b091a to
b674f86
Compare
Laguna templates prefill <think> or </think>, so the model never emits the opening marker and the request kwargs cannot tell the parser which mode was rendered. Read it off the prompt instead, behind an opt-in flag so only poolside_v1 is affected. Adds poolside_v1, keeping laguna as an alias. Signed-off-by: Joe Rowell <joerowell4@gmail.com>
b674f86 to
a958fba
Compare
|
/bot run |
|
PR_Github #65019 [ run ] triggered by Bot. Commit: |
|
PR_Github #65019 [ run ] completed with state
|
|
/bot run |
|
PR_Github #65038 [ run ] triggered by Bot. Commit: |
|
PR_Github #65038 [ run ] completed with state
|
|
/bot run |
|
PR_Github #65101 [ run ] triggered by Bot. Commit: |
|
/bot run |
|
PR_Github #65268 [ run ] triggered by Bot. Commit: |
|
PR_Github #65101 [ run ] completed with state |
|
PR_Github #65268 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #65326 [ run ] triggered by Bot. Commit: |
|
PR_Github #65326 [ run ] completed with state |
7 participants
Laguna templates prefill
<think>or</think>, so the model never emits the opening marker and the request kwargs cannot tell the parser which mode was rendered. Thus, we should read it from the prompt instead.To make this change minimal, I've kept it behind an opt-in flag so only poolside_v1 is affected.
As an aside, I've renamed the laguna parser to poolside_v1, to keep it consistent with the rest of the world. Happy to defer this to a follow-up PR, but figured I'd do it in one go. We keep the alias, so it shouldn't break anything.
Dev Engineer Review
<think>or</think>markers.poolside_v1and retainedlagunaas a compatibility alias.qwen3_5,minimax_m2, andminimax_m2_append_thinkregistrations.thinkingandenable_thinking.QA Engineer Review
tests/unittest/llmapi/test_reasoning_parser.py.tests/integration/test_lists/entries were modified.Description
Test Coverage
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.