Repository navigation
[Bugfix] Fix DeepSeekV32 tool parser incorrect type conversion for array/object parameters - #30797
fangtaosong wants to merge 3 commits into
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a bug in the DeepSeekV32ToolParser that caused incorrect serialization of array and object parameters. The fix, which involves looking up parameter types from the tool schema and using json.loads for complex types, is well-implemented. I particularly appreciate the refactoring that introduced the _get_param_type helper function, which centralizes type lookup logic and removes code duplication between streaming and non-streaming parsing. I've identified one high-severity issue regarding robustness against malformed tool definitions in the new helper function and provided a suggestion to prevent potential TypeError exceptions.
| and hasattr(tool.function, "parameters") | ||
| ): | ||
| params = tool.function.parameters | ||
| if isinstance(params, dict) and "properties" in params: |
There was a problem hiding this comment.
The code doesn't validate that params['properties'] is a dictionary before it's used. If a user provides a malformed tool definition where properties is not a dictionary (e.g., a list), the subsequent code that assumes it's a dictionary could raise a TypeError. To make this more robust against such malformed inputs, you should ensure params['properties'] is a dictionary.
| if isinstance(params, dict) and "properties" in params: | |
| if isinstance(params, dict) and isinstance(params.get("properties"), dict): |
…ray/object parameters
The DeepSeekV32 tool parser was incorrectly serializing array and object
type parameters as strings instead of their proper JSON types. This caused
tool calls with array parameters (e.g., todos: [{...}]) to be serialized
as escaped strings rather than actual JSON arrays.
Root cause:
- The parser defaulted to "string" type when parameter type lookup failed
- Type conversion logic processed string type before object/array types
- Parameters were double-encoded when json.dumps was called on string values
Fix:
- Add _get_param_type() helper to look up parameter types from tool schema
- Update _parse_invoke_params() to accept tool_name and request for type lookup
- Reorder _convert_param_value() to handle object/array types first
- For unknown types, attempt JSON parsing before falling back to string
Co-authored-by: Xinyu Lu <luxinyu221@mails.ucas.ac.cn>
Signed-off-by: Taosong Fang <constfrost@foxmail.com>
Signed-off-by: 10053 <1005394403@qq.com>
dda32a9 to
1a95436
Compare
|
With this fix, it is now possible to successfully complete complex, multi-turn code editing tasks (spanning dozens of rounds) in Claude Code using DeepSeek V3.2 deployed via vLLM. |
| if not tool_name or not request or not request.tools: | ||
| return "unknown" | ||
|
|
||
| for tool in request.tools: |
There was a problem hiding this comment.
This change doesn’t seem to be compatible with the Responses API.
Could you help test it with the Responses API?
Signed-off-by: Taosong Fang <constfrost@foxmail.com>
509bcfb to
a7e62ee
Compare
|
Hi @fangtaosong, sorry about this — I have a few other things to take care of over the next few days, and I’ll be able to review this PR after that. |
|
Hi @chaunceyjiang, checking in on this PR. It's been a few weeks. Could you please take a look at this bugfix for DeepSeekV3.2 tool parsing when you have a moment? Please let me know if you need any updates from my side. |
|
I'm encountering the same issue in array tool parse. Any updates on this PR? |
It doesn't seem so. Can the above changes resolve your issue? |
|
Gentle bump @chaunceyjiang, any chance to review this soon? |
|
I’m currently addressing another high-priority issue with dsv32 tool calls. After that’s done, I’ll come back and take a look at your PR. @fangtaosong |
There was a problem hiding this comment.
Thanks for the work, the PR does fix the bug where the array/object parameters were double encoded as strings.
However, it introduces a regression - _get_param_type assumes schema["type"] is always a string, but JSON Schema allows type arrays (e.g., {"type": ["integer", "null"]}). In that case it returns a list, and _convert_param_value calls .lower() on it → AttributeError crash. The old code wouldn't hit this because it never called _get_param_type.
I updated the PR back to your queue for changes.
|
A note - the minimax m2 parser has more comprehensive schema handling implemented, while it goes beyond the scope of bug listed in the PR, it's worth aligning with that approach, which will include coverage on anyOf/oneOf/allOf handling, enum handling, etc. |
|
Closing as fixed in #43019 |
Add parser-level regression coverage for DeepSeek V3.2/V4 DSML tool arguments whose JSON Schema types are expressed through composed schemas such as anyOf and allOf. These cases cover the customer-observed wait object and JSON Patch array shapes that previously surfaced as JSON strings when the parser only read direct parameter type fields. Current vLLM main already contains the source behavior through vllm-project#43019, which replaced the DeepSeek parser's private conversion path with shared schema helpers. This branch is therefore intentionally scoped to coverage that locks the composed-schema behavior on the V3.2 superclass path and the V4 parser alias path. Why this is not duplicating existing upstream work: vllm-project#43019 fixed the parser behavior and added broader coercion tests, but did not include the customer-shaped composed-schema parser regression cases covered here. Earlier related work (vllm-project#30797, vllm-project#41531, vllm-project#42879) was adjacent but did not fully cover this semantic case. Tests: PYTHONPATH=/Users/aeldeib/code/inference/vllm-dsv4-composed-tool-args-ace /Users/aeldeib/code/inference/vllm/.venv/bin/python -m pytest tests/tool_parsers/test_deepseekv32_tool_parser.py tests/tool_parsers/test_deepseekv4_tool_parser.py -q -k composed_schema -> 4 passed, 50 deselected. uvx ruff check tests/tool_parsers/test_deepseekv32_tool_parser.py tests/tool_parsers/test_deepseekv4_tool_parser.py vllm/tool_parsers/deepseekv32_tool_parser.py -> passed. uvx ruff format --check tests/tool_parsers/test_deepseekv32_tool_parser.py tests/tool_parsers/test_deepseekv4_tool_parser.py vllm/tool_parsers/deepseekv32_tool_parser.py -> passed. AI assistance was used to investigate production traces, search related vLLM work, and prepare this coverage. A human submitter must review and defend every changed line before opening an upstream PR. Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Ace Eldeib <aeldeib@coreweave.com>
4 participants
The
DeepSeekV32ToolParserwas incorrectly serializing array and object type parameters as strings instead of their proper JSON types. This caused tool calls with complex parameters (e.g.,todos: [{...}]) to be serialized as escaped strings (e.g.,"todos": "[{...}]") rather than actual JSON arrays/objects.This issue breaks integration with downstream applications (such as Claude Code) that perform strict type validation on tool arguments.
Root cause:
json.dumpswas called on the already stringified values during the final tool call construction.Fix:
_get_param_type()helper to look up parameter types from theChatCompletionRequesttools schema._parse_invoke_params()to accepttool_nameandrequestfor type lookup._convert_param_value()and the parsing logic (both streaming and non-streaming) to attemptjson.loadswhen the expected type isarray/object, or as a fallback when the type is unknown.Test Plan
Verified the fix by integrating DeepSeek-V3.2 with Claude Code (Anthropic's CLI tool), which heavily relies on
TodoWritetool calls containing array parameters (todos).Steps:
TodoWritetool (requires a list of objects).Test Result
Before Fix:
The client receives the
todosparameter as a string. Validation fails.{ "type": "assistant", "message": { "id": "chatcmpl-9068c14695bca837", "type": "message", "role": "assistant", "content": [ { "type": "tool_use", "id": "chatcmpl-tool-ad2bf0b87531d72b", "name": "TodoWrite", "input": { "todos": "[{\"content\": \"Explore Django codebase to understand i18n_patterns and locale handling\", \"status\": \"in_progress\", \"activeForm\": \"Exploring Django codebase for i18n_patterns locale handling\"}, {\"content\": \"Find the specific code that handles language prefix matching\", \"status\": \"pending\", \"activeForm\": \"Finding language prefix matching code\"}, {\"content\": \"Reproduce the issue to confirm the bug\", \"status\": \"pending\", \"activeForm\": \"Reproducing the i18n_patterns issue\"}, {\"content\": \"Analyze the root cause of the bug\", \"status\": \"pending\", \"activeForm\": \"Analyzing root cause of locale parsing\"}, {\"content\": \"Implement fix for language locales with script and region\", \"status\": \"pending\", \"activeForm\": \"Implementing fix for language locales with script and region\"}, {\"content\": \"Test the fix with the problematic locales\", \"status\": \"pending\", \"activeForm\": \"Testing fix with problematic locales\"}]" } } ], "model": "deepseek_v3.2", "stop_reason": "tool_use", "usage": { "input_tokens": 17052, "output_tokens": 254 }, "context_management": null }, "parent_tool_use_id": null, "session_id": "a8f618b7-d1d1-4306-9a6a-083f837a001e", "uuid": "5f4e31ae-801a-44af-bcff-921caa6b94f0" } { "type": "user", "message": { "role": "user", "content": [ { "type": "tool_result", "content": "<tool_use_error>InputValidationError: TodoWrite failed due to the following issue:\nThe parameter `todos` type is expected as `array` but provided as `string`</tool_use_error>", "is_error": true, "tool_use_id": "chatcmpl-tool-ad2bf0b87531d72b" } ] }, "parent_tool_use_id": null, "session_id": "a8f618b7-d1d1-4306-9a6a-083f837a001e", "uuid": "d79441e5-0476-4a68-9698-c208ff98e58d", "tool_use_result": "InputValidationError: [\n {\n \"code\": \"invalid_type\",\n \"expected\": \"array\",\n \"received\": \"string\",\n \"path\": [\n \"todos\"\n ],\n \"message\": \"Expected array, received string\"\n }\n]" }After Fix:
The client receives the
todosparameter as a JSON Array. Validation succeeds.{ "type": "assistant", "message": { "id": "chatcmpl-adb360f4bba4c60d", "type": "message", "role": "assistant", "content": [ { "type": "text", "text": "I'll help you investigate and fix the internationalization issue where `i18n_patterns` doesn't support locale containing both script and region (like `en-latn-us`). Let me start by exploring the Django codebase to understand how i18n patterns handle language codes.\n\n" } ], "model": "deepseek_v3.2", "stop_reason": "tool_use", "usage": { "input_tokens": 16689, "output_tokens": 306 }, "context_management": null }, "parent_tool_use_id": null, "session_id": "2aa341be-b82c-4098-ad5e-8f81bc9a4ef2", "uuid": "bf244b3a-7287-4221-bd09-b8aeab9eaf85" } { "type": "assistant", "message": { "id": "chatcmpl-adb360f4bba4c60d", "type": "message", "role": "assistant", "content": [ { "type": "tool_use", "id": "chatcmpl-tool-b58ebfb23bb032e6", "name": "TodoWrite", "input": { "todos": [ { "content": "Explore Django codebase to understand i18n patterns and language code handling", "status": "in_progress", "activeForm": "Exploring Django codebase to understand i18n patterns and language code handling" }, { "content": "Reproduce the issue locally to confirm the problem", "status": "pending", "activeForm": "Reproducing the issue locally to confirm the problem" }, { "content": "Identify the root cause of the bug", "status": "pending", "activeForm": "Identifying the root cause of the bug" }, { "content": "Implement a fix for the issue", "status": "pending", "activeForm": "Implementing a fix for the issue" }, { "content": "Test the fix with various language code formats", "status": "pending", "activeForm": "Testing the fix with various language code formats" }, { "content": "Update tests if necessary", "status": "pending", "activeForm": "Updating tests if necessary" } ] } } ], "model": "deepseek_v3.2", "stop_reason": "tool_use", "usage": { "input_tokens": 16689, "output_tokens": 306 }, "context_management": null }, "parent_tool_use_id": null, "session_id": "2aa341be-b82c-4098-ad5e-8f81bc9a4ef2", "uuid": "860260f5-5dfc-4b12-9c57-6592c3adcec0" }Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.