Skip to content

[None][fix] use tool parser for named tool choice responses#12052

Open
youngzheng-oss wants to merge 1 commit intoNVIDIA:mainfrom
youngzheng-oss:youngz/fix-named-tool-choice-parser-parity
Open

[None][fix] use tool parser for named tool choice responses#12052
youngzheng-oss wants to merge 1 commit intoNVIDIA:mainfrom
youngzheng-oss:youngz/fix-named-tool-choice-parser-parity

Conversation

@youngzheng-oss
Copy link

@youngzheng-oss youngzheng-oss commented Mar 9, 2026

Description

  • remove named-tool-choice special handling in chat postprocessors
  • always run tool output through apply_tool_parser for parity with auto tool choice

Test coverage

  • tests/unittest/llmapi/apps/_test_openai_tool_call.py

Summary by CodeRabbit

  • Bug Fixes
    • Tool call handling is now unified and consistent between streaming and non-streaming response processing modes.
    • Tool calls are reliably parsed and integrated into messages regardless of the processing path.
    • Improved stability and consistency when handling tool calls across all post-processing scenarios.

Route named tool choice through the same parser flow as auto tool choice so tool-call arguments and finish reasons stay consistent.

Signed-off-by: Young Zheng <young.zheng@baseten.co>
Made-with: Cursor
@youngzheng-oss youngzheng-oss requested a review from a team as a code owner March 9, 2026 20:37
@youngzheng-oss youngzheng-oss requested a review from Superjomn March 9, 2026 20:37
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 9, 2026

📝 Walkthrough

Walkthrough

The pull request consolidates tool-call handling across streaming and non-streaming post-processing paths in a single file. Tool parsing is now applied uniformly regardless of tool_choice type, and tool_call_id generation is restricted to first chunks only. Duplicate branching logic is eliminated in favor of a single, consistent flow for both functions.

Changes

Cohort / File(s) Summary
Tool-call handling unification
tensorrt_llm/serve/postprocess_handlers.py
Refactored yield_first_chat and chat_response_post_processor to consistently apply tool parsing and construct tool call entries. Removed conditional branching based on tool_choice type; tool_call_id now generated only for first chunks. DeltaMessage and ChatMessage construction consolidated to single path for both functions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description provides a concise explanation of the changes but is missing required sections from the template such as PR title format, detailed problem/solution explanation, and the PR checklist. Add a proper PR title following the template format [ticket/issue][type] Summary, expand the description section with more context about why this change is needed, and ensure the PR checklist section is included with appropriate items reviewed.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: applying the tool parser to named tool choice responses for consistency with auto tool choice handling.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tensorrt_llm/serve/postprocess_handlers.py`:
- Around line 219-250: apply_tool_parser currently lacks access to
args.tool_choice so when a forced named tool
(ChatCompletionNamedToolChoiceParam) is used and the model emits only argument
deltas, resulting calls lose their function name (call.name is None) and
streaming/non-streaming behavior breaks; fix by updating apply_tool_parser to
accept the forced tool name (extract from args.tool_choice when it is a
ChatCompletionNamedToolChoiceParam) and propagate it into returned call objects,
or if you prefer minimal changes, after apply_tool_parser returns, inspect calls
in the postprocess flow (the loop building DeltaToolCall/DeltaFunctionCall) and
if call_item.name is missing but args.tool_choice is a named tool, restore
function_name from args.tool_choice before creating the DeltaFunctionCall so
DeltaToolCall.id/name are preserved.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0a5bb47d-6e2d-4266-9f34-0b2844115b36

📥 Commits

Reviewing files that changed from the base of the PR and between 35ccedd and 2dcda86.

📒 Files selected for processing (1)
  • tensorrt_llm/serve/postprocess_handlers.py

@pengbowang-nv
Copy link
Collaborator

Hi @LinPoly , could you review this PR as it is related to tool parser code? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community want to contribute PRs initiated from Community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants