Repository navigation
[#20991][fix] Poolside v1 tool parser: keep argument text that has no strict JSON form - #20992
LeizhenZhang wants to merge 1 commit into
Conversation
…has no strict JSON form _parse_argument_value let three exceptions out of detect_and_parse and parse_streaming_increment: - OverflowError from int(parsed) when json.loads returns infinity (1e999, Infinity) for an integer argument - OverflowError from float(parsed) for an integer with more than 308 digits in a number argument - RecursionError from json.loads on deeply nested arrays or objects It also returned NaN and infinity, which json.dumps writes as NaN and Infinity, so the arguments string was not valid JSON. A decoded value is now kept only when json.dumps spells it as strict JSON one level deep, the depth it has inside the arguments object. Otherwise the raw argument text is kept, as glm4_parser.parse_arguments already does for the same arg_key/arg_value format. Signed-off-by: LeizhenZhang <zhangleizhen645@gmail.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Strict JSON validation and fallback tensorrt_llm/serve/tool_parser/poolside_v1_parser.py, tests/unittest/llmapi/apps/test_tool_parsers.py |
The parser validates decoded and converted values as strict JSON. If validation or conversion fails, it retains the trimmed input text. Tests cover numeric overflow, non-finite values, oversized integers, and deeply nested arrays in one-shot and streaming modes. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Merge Risk: 
15f60
-
No merge-blocking issue is established; the change is mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description check The description clearly explains the problem, root cause, fix, test coverage, known setup-related failures, and checklist status. It is complete and directly aligned with the pull request changes. Title check The title clearly summarizes the main change: retaining raw argument text when Poolside v1 tool values have no strict JSON representation. Linked Issues check Issue #20991requires Poolside v1 parsing to return tool calls for numeric overflow, non-strict numeric values, and deeply nested values in both parsing paths. The change catchesRecursionErrorand …Out of Scope Changes check The pull request changes only Poolside v1 argument parsing and its focused CPU-only tests. The helper and exception handling support the #20991behavior, and the tests verify both required parsing pat…Full details: Linked Issues check
-
Explanation
Issue
#20991requires Poolside v1 parsing to return tool calls for numeric overflow, non-strict numeric values, and deeply nested values in both parsing paths. The change catchesRecursionErrorandOverflowError, validates decoded values withjson.dumps(..., allow_nan=False), and retains raw text when strict JSON cannot represent the value. The new parameterized test covers nine values throughdetect_and_parseandparse_streaming_increment, including strict-JSON rejection ofNaNandInfinity.
Full details: Out of Scope Changes check
-
Explanation
The pull request changes only Poolside v1 argument parsing and its focused CPU-only tests. The helper and exception handling support the
#20991behavior, and the tests verify both required parsing paths. No unrelated production behavior or unrelated test scope is introduced.
-
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
svc-trtllm-gh-bot
added
the
Community want to contribute
|
Following @karljang's review on #21005 to consolidate closely related changes, this Poolside v1 fix is now part of #20994 (commit |
Dev Engineer Review
The parser retains raw argument text when a decoded value cannot be serialized as strict JSON. It also handles recursion and overflow errors during parsing. These changes target the linked issue’s reported failure cases. No current review findings or test execution results were supplied.
QA Engineer Review
tests/unittest/llmapi/apps/test_tool_parsers.pyaddstest_values_without_strict_json_keep_textfor numeric overflow, non-finite values, and deeply nested arrays in one-shot and streaming modes. The test file is listed intests/integration/test_lists/test-db/l0_cpu.yml. Coverage verdict: sufficient for the reported cases; test execution status is unavailable.Per-File QA Perspective
tensorrt_llm/serve/tool_parser/poolside_v1_parser.py: Verify that unrepresentable values retain their raw text and that recursion or overflow errors do not escape one-shot or streaming parsing.tests/unittest/llmapi/apps/test_tool_parsers.py: The added parameterized test covers the reported numeric, non-finite, and nested-array cases in both parsing modes. The file is listed in the CPU CI test list.Description
Fixes #20991.
Problem
PoolsideV1ToolParserraises on some argument values the model can write.1e999,Infinity,-InfinityOverflowError: cannot convert float infinity to integerOverflowError: int too large to convert to float[RecursionErrorfromjson.loadsNaN,1e999,[1e999]NaNorInfinity, which strict JSON parsers rejectThe exception leaves both
detect_and_parseandparse_streaming_increment. A non-streaming chat request then fails with HTTP 400 and the generated text is lost. A streaming request ends without its last chunks and withoutdata: [DONE].Root cause
_parse_argument_valueintensorrt_llm/serve/tool_parser/poolside_v1_parser.py:json.loads(value)is guarded by(json.JSONDecodeError, ValueError), soRecursionErrorpasses through.int(parsed)andfloat(parsed)are guarded by(TypeError, ValueError).json.loadsreturnsfloat("inf")for1e999andInfinity,int(inf)raisesOverflowError, and so doesfloat()of a very long integer.Fix
_parse_argument_valuekeeps a decoded value only whenjson.dumps(..., allow_nan=False)can write it one level deep, which is the depth it has inside the arguments object. Otherwise it keeps the raw argument text.RecursionErrorfromjson.loadsandOverflowErrorfrom the numeric conversions are caught. This is the ruleglm4_parser.parse_argumentsalready applies to the same<arg_key>/<arg_value>format (1e309andNaNkeep their text there). Values that decode to strict JSON parse exactly as before.The one-level-deep check matters at the decoder's nesting limit. On CPython 3.12 an array nested 9997 levels decodes, and then the
json.dumpsof the arguments object, one level deeper, raisesRecursionError. A sweep of nesting depths from 1 to 3000 and 9980 to 10010 (arrays and objects, both parse paths) raises nothing after the change.Test Coverage
New CPU-only test
TestPoolsideV1ToolParser::test_values_without_strict_json_keep_textintests/unittest/llmapi/apps/test_tool_parsers.py. It covers nine values (integer1e999,-Infinity,NaN, number1e999,NaN,1.e999, a 400-digit integer, array[1e999], 100000 nested arrays) throughdetect_and_parseandparse_streaming_increment, and decodes the arguments with aparse_constantthat rejectsNaNandInfinity.Results (CPU, tensorrt_llm 1.4.0rc1 wheel with the tool_parser package loaded from the checkout):
OverflowError, two raiseRecursionError, and ten returnNaNorInfinityin the arguments string.test_tool_parsers.pyon main: 896 passed, 4 failed.test_tool_parsers.pywith the fix: 914 passed, 4 failed.The same four tests fail on main and with the fix (
TestQwen3CoderRequiredTools::test_required_stream_matches_complete_response[10000-True]and threeTestQwen3CoderParameterRecovery::test_chat_recovery_policy_preserves_required_argumentscases). They go throughpostprocess_handlersand the reasoning parser, which this local setup takes from the 1.4.0rc1 wheel, so they come from the setup.Open PR #19917 changes one line of
_get_argument_typein the same file. The two changes touch different lines.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.