[https://nvbugs/6384951][fix] Fix incorrect error reporting#15732
[https://nvbugs/6384951][fix] Fix incorrect error reporting#157322ez4bz wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds Pydantic-based fallback validation for raw tool-call lists in Tool-Call Fallback Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unittest/llmapi/apps/test_chat_utils.py (1)
120-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
-> Noneto the new test methods.These newly added test functions omit return annotations. As per coding guidelines, “Always annotate functions with return types (use
Noneif no return).”🤖 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/apps/test_chat_utils.py` around lines 120 - 213, The new test methods in test_chat_utils.py are missing explicit return annotations. Update each added test function, including test_assistant_message_tool_call_missing_function, test_assistant_message_tool_call_invalid_json_arguments, test_assistant_message_tool_call_non_object_function, test_assistant_message_tool_call_non_object_item, and test_assistant_message_tool_call_preserves_extra_fields, to declare a None return type so they comply with the function annotation guideline.Source: Coding guidelines
tensorrt_llm/serve/chat_utils.py (1)
280-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Python 3.10 built-in generics here. Replace
Optional[...],Union[...],List[...], andDict[...]with|andlist/dictin the new fallback helpers to match the repo’s Python 3.10 target and keep the annotations consistent.Proposed annotation cleanup
- name: Optional[str] = None - arguments: Optional[Union[str, Dict[str, Any]]] = None + name: str | None = None + arguments: str | dict[str, Any] | None = None @@ -_FALLBACK_TOOL_CALLS_ADAPTER = TypeAdapter(List[_FallbackToolCall]) +_FALLBACK_TOOL_CALLS_ADAPTER = TypeAdapter(list[_FallbackToolCall]) @@ -def _format_tool_call_error(errors: List[Dict[str, Any]]) -> str: +def _format_tool_call_error(errors: list[dict[str, Any]]) -> str: @@ -def _validate_fallback_tool_calls( - tool_calls: List[Any]) -> List[Dict[str, Any]]: +def _validate_fallback_tool_calls( + tool_calls: list[Any]) -> list[dict[str, Any]]:🤖 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/chat_utils.py` around lines 280 - 316, Update the new fallback helper annotations in chat_utils.py to use Python 3.10 built-in generics and unions instead of Optional, Union, List, and Dict. Adjust the type hints on _FallbackFunctionCall, _FallbackToolCall, _FALLBACK_TOOL_CALLS_ADAPTER, _format_tool_call_error, and _validate_fallback_tool_calls to use list, dict, and | consistently with the repo’s Python 3.10 target.Sources: Coding guidelines, Learnings
🤖 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/chat_utils.py`:
- Around line 361-368: The argument parsing in chat_utils.py is incorrectly
gated on truthiness, so empty strings skip JSON decoding and non-dict JSON like
"[]" or "0" can be accepted. Update the function handling tool call arguments so
it distinguishes None from string values in item["function"]["arguments"],
always attempts json.loads for string inputs, and then validates the parsed
result is a dict/object before assigning it back. Keep the existing ValueError
path in the same argument-normalization flow so invalid JSON and non-object
payloads are both rejected consistently.
In `@tests/unittest/llmapi/apps/test_chat_utils.py`:
- Around line 130-150: Expand parse_chat_message_content coverage in
test_chat_utils by adding cases for empty string arguments and valid non-object
JSON values like [] or 0 in
test_assistant_message_tool_call_invalid_json_arguments or nearby tests. The
current invalid-JSON assertion only covers malformed syntax; update the tests to
verify the tool_calls[0].function.arguments normalization path rejects empty and
non-object JSON too, with ValueError matches that make the failure reason
explicit.
---
Nitpick comments:
In `@tensorrt_llm/serve/chat_utils.py`:
- Around line 280-316: Update the new fallback helper annotations in
chat_utils.py to use Python 3.10 built-in generics and unions instead of
Optional, Union, List, and Dict. Adjust the type hints on _FallbackFunctionCall,
_FallbackToolCall, _FALLBACK_TOOL_CALLS_ADAPTER, _format_tool_call_error, and
_validate_fallback_tool_calls to use list, dict, and | consistently with the
repo’s Python 3.10 target.
In `@tests/unittest/llmapi/apps/test_chat_utils.py`:
- Around line 120-213: The new test methods in test_chat_utils.py are missing
explicit return annotations. Update each added test function, including
test_assistant_message_tool_call_missing_function,
test_assistant_message_tool_call_invalid_json_arguments,
test_assistant_message_tool_call_non_object_function,
test_assistant_message_tool_call_non_object_item, and
test_assistant_message_tool_call_preserves_extra_fields, to declare a None
return type so they comply with the function annotation guideline.
🪄 Autofix (Beta)
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: 9aacfde3-16a1-4684-a87c-3e5e9d7de868
📒 Files selected for processing (2)
tensorrt_llm/serve/chat_utils.pytests/unittest/llmapi/apps/test_chat_utils.py
2ez4bz
changed the title
2ez4bz
force-pushed
the
dev-nvbug-6384951
branch
from
51ccb9e to
b2efdab
Compare
|
/bot run |
|
PR_Github #56466 [ run ] triggered by Bot. Commit: |
|
PR_Github #56466 [ run ] completed with state
|
2ez4bz
force-pushed
the
dev-nvbug-6384951
branch
from
b2efdab to
25dc000
Compare
|
/bot run |
|
PR_Github #56729 [ run ] triggered by Bot. Commit: |
|
PR_Github #56729 [ run ] completed with state
|
|
/bot run |
|
PR_Github #56823 [ run ] triggered by Bot. Commit: |
|
PR_Github #56823 [ run ] completed with state
|
|
/bot run |
|
PR_Github #56858 [ run ] triggered by Bot. Commit: |
|
PR_Github #56858 [ run ] completed with state
|
|
/bot run |
|
PR_Github #56969 [ run ] triggered by Bot. Commit: |
|
PR_Github #56969 [ run ] completed with state
|
|
/bot run |
* Why? tau2-bench required a workaround that could misreport errors. * What? This commit fixes that with typed checks and adds tests for it. Signed-off-by: William Zhang <133824995+2ez4bz@users.noreply.github.com>
|
/bot run |
|
PR_Github #57109 [ run ] triggered by Bot. Commit: |
|
PR_Github #57109 [ run ] completed with state
|
|
/bot run |
|
PR_Github #57132 [ run ] triggered by Bot. Commit: |
|
PR_Github #57132 [ run ] completed with state
|
|
/bot run |
|
PR_Github #57236 [ run ] triggered by Bot. Commit: |
|
PR_Github #57236 [ run ] completed with state
|
|
/bot run |
|
PR_Github #57249 [ run ] triggered by Bot. Commit: |
|
PR_Github #57249 [ run ] completed with state
|
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #57340 [ run ] triggered by Bot. Commit: |
|
PR_Github #57340 [ run ] completed with state
|
|
/bot run |
|
PR_Github #57482 [ run ] triggered by Bot. Commit: |
|
PR_Github #57482 [ run ] completed with state
|
|
/bot run |
|
PR_Github #57516 [ run ] triggered by Bot. Commit: |
5 participants
Summary by CodeRabbit
Description
tau2-bench required a workaround that could misreport errors.
This commit fixes that with typed checks and adds tests for it.
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.