[None][fix] Forward reasoning_effort to the chat template - #17553
[None][fix] Forward reasoning_effort to the chat template#17553joerowell wants to merge 2 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:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe change expands accepted reasoning-effort values, forwards explicit values to chat templates, preserves existing template kwargs, and prevents unsupported Harmony values from raising ChangesReasoning effort support
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatCompletionRequest
participant OpenAIServer
participant ChatTemplate
Client->>ChatCompletionRequest: send reasoning_effort
ChatCompletionRequest->>OpenAIServer: request and chat_template_kwargs
OpenAIServer->>OpenAIServer: merge explicit reasoning_effort
OpenAIServer->>ChatTemplate: render with merged kwargs
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: 2
🧹 Nitpick comments (1)
tensorrt_llm/serve/openai_server.py (1)
208-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a precise request type annotation.
Annotate
requestasChatCompletionRequest. This helper reads Pydantic request fields and is part of the chat-template contract.Proposed fix
-def _chat_template_kwargs_with_effort(request) -> dict: +def _chat_template_kwargs_with_effort( + request: ChatCompletionRequest) -> dict[str, Any]:As per coding guidelines, “Annotate every function” and “use precise types instead of
dict/object/Anywhere applicable.”🤖 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 `@tensorrt_llm/serve/openai_server.py` around lines 208 - 232, Update _chat_template_kwargs_with_effort to annotate request as ChatCompletionRequest, reusing the existing import or adding the appropriate import if needed. Keep the current return annotation and behavior unchanged.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.
Inline comments:
In `@tensorrt_llm/serve/harmony_adapter.py`:
- Around line 1922-1929: Add an "xhigh" entry to the str_to_effort mapping used
by reasoning-effort resolution, mapping it to ReasoningEffort.HIGH alongside
"max", so both requests resolve identically while preserving existing fallback
behavior.
In `@tests/unittest/llmapi/apps/test_reasoning_effort_resolution.py`:
- Around line 32-104: Run the repository-configured YAPF and ruff formatters on
the test file containing make_request, _chat_template_kwargs_with_effort, and
maybe_transform_reasoning_effort, then commit all formatter-generated changes
without altering the test behavior.
---
Nitpick comments:
In `@tensorrt_llm/serve/openai_server.py`:
- Around line 208-232: Update _chat_template_kwargs_with_effort to annotate
request as ChatCompletionRequest, reusing the existing import or adding the
appropriate import if needed. Keep the current return annotation and behavior
unchanged.
🪄 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: 184d25ea-16fd-4cd1-b28c-a42e51c2cffc
📒 Files selected for processing (4)
tensorrt_llm/serve/harmony_adapter.pytensorrt_llm/serve/openai_protocol.pytensorrt_llm/serve/openai_server.pytests/unittest/llmapi/apps/test_reasoning_effort_resolution.py
601faaf to
0d55bbc
Compare
|
/bot run |
|
PR_Github #67484 [ run ] triggered by Bot. Commit: |
|
PR_Github #67484 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #67496 [ run ] triggered by Bot. Commit: |
|
PR_Github #67496 [ run ] completed with state
|
|
/bot run |
|
PR_Github #67542 [ run ] triggered by Bot. Commit: |
|
PR_Github #67542 [ run ] completed with state
|
|
/bot run |
|
PR_Github #67562 [ run ] triggered by Bot. Commit: |
|
PR_Github #67562 [ run ] completed with state
|
|
/bot run |
|
PR_Github #67579 [ run ] triggered by Bot. Commit: |
|
PR_Github #67579 [ run ] completed with state
|
0856a36 to
59f2c50
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 |
|
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. |
|
PR_Github #69196 [ run ] triggered by Bot. Commit: |
|
PR_Github #69196 [ run ] completed with state
|
|
/bot run |
|
PR_Github #69225 [ run ] triggered by Bot. Commit: |
|
PR_Github #69225 [ run ] completed with state
|
b5c7ae6 to
7fed60a
Compare
|
/bot run |
|
PR_Github #69728 [ run ] triggered by Bot. Commit: |
|
PR_Github #69728 [ run ] completed with state
|
| "low": ReasoningEffort.LOW, | ||
| "medium": ReasoningEffort.MEDIUM, | ||
| "high": ReasoningEffort.HIGH | ||
| "high": ReasoningEffort.HIGH, |
There was a problem hiding this comment.
Does "minimal" also need to be in this dictionary?
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #71285 [ run ] triggered by Bot. Commit: |
|
PR_Github #71285 [ run ] completed with state
|
|
/bot run |
|
PR_Github #71348 [ run ] triggered by Bot. Commit: |
|
PR_Github #71348 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #71444 [ run ] triggered by Bot. Commit: |
|
PR_Github #71444 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #71575 [ run ] triggered by Bot. Commit: |
|
PR_Github #71575 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #71616 [ run ] triggered by Bot. Commit: |
|
PR_Github #71616 [ run ] completed with state
|
Signed-off-by: Joe Rowell <joerowell4@gmail.com>
Signed-off-by: Joe Rowell <joerowell4@gmail.com>
2e1735f to
f9d2901
Compare
|
/bot run |
|
PR_Github #71992 [ run ] triggered by Bot. Commit: |
|
PR_Github #71992 [ run ] completed with state
|
6 participants
Dev Engineer Review
reasoning_effortforwards to chat templates.chat_template_kwargs.none,minimal,low,medium,high,xhigh, andmax.low,medium, andhighdirectly. It mapsxhighandmaxto high reasoning.Noneinstead of raisingKeyError.QA Engineer Review
Added CPU-only tests:
test_a_sent_level_reaches_the_template_unchangedtest_an_unsent_effort_is_not_forwardedtest_a_sent_level_overrides_chat_template_kwargstest_chat_template_kwargs_still_work_on_their_owntest_other_template_kwargs_are_preservedtest_the_request_kwargs_are_not_mutatedtest_harmony_tolerates_every_level_the_field_acceptstest_harmony_still_maps_the_levels_it_ownsNo corresponding
tests/integration/test_lists/,test-db/, orqa/coverage entry was found. Verdict: needs follow-up.