Draft: parse tool_calls arguments#4921
Conversation
Add parsing for JSON-encoded arguments in tool_calls.
There was a problem hiding this comment.
Code Review
This pull request modifies _transform_messages in xinference/model/llm/utils.py to automatically parse JSON-encoded tool call arguments into dictionaries, facilitating easier iteration within Jinja2 templates. Reviewers pointed out that this global conversion is a breaking change for templates expecting string-based arguments and suggested implementing a fromjson filter as a safer, opt-in alternative. Additionally, feedback suggests adding more robust type checking to ensure that the function objects and parsed results are indeed dictionaries before processing them.
| # Parse JSON-encoded arguments in tool_calls to dicts, | ||
| # so Jinja2 templates can iterate them with |items. | ||
| if new_message.get("tool_calls"): | ||
| tool_calls = [] | ||
| for tc in new_message["tool_calls"]: | ||
| tc = dict(tc) | ||
| func = tc.get("function") | ||
| if func and isinstance(func.get("arguments"), str): | ||
| func = dict(func) | ||
| try: | ||
| func["arguments"] = json.loads(func["arguments"]) | ||
| except (json.JSONDecodeError, TypeError): | ||
| pass | ||
| tc["function"] = func | ||
| tool_calls.append(tc) | ||
| new_message["tool_calls"] = tool_calls |
There was a problem hiding this comment.
This change introduces a breaking change for models whose chat templates expect tool_calls arguments to be a JSON string. As noted in the PR description, templates using string concatenation or assuming a string type will fail. Instead of a global conversion in _transform_messages, a safer approach would be to add a fromjson filter to the Jinja2 environment in _compile_jinja_template. This allows templates that need structured data to opt-in without breaking others. If this global change is desired, it should likely be made conditional based on the model family or template requirements.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
https://github.com/xorbitsai/inference/actions/runs/26015899223/job/76465909355?pr=4921 Could you fix the error? |
Okay, it is fixed. |
|
@qinxuye Can you merge this PR? Models that cannot handle arguments correctly should be very few and most of them are outdated. |
Okay, I will review this. |
| pass | ||
| tc["function"] = func | ||
| tool_calls.append(tc) | ||
| new_message["tool_calls"] = tool_calls |
There was a problem hiding this comment.
This global conversion is too broad for _transform_messages. That helper feeds vLLM, MLX, SGLang, and several multimodal paths, while the OpenAI-compatible message shape and existing tests expect tool_calls[].function.arguments to remain a JSON string. Converting it to a dict here changes the message contract for every template, including templates that concatenate or tojson the argument string. Please keep _transform_messages preserving the input shape and make the JSON parsing opt-in in the template path, for example via a Jinja fromjson filter or a model/template-specific branch.
There was a problem hiding this comment.
Yes, I know this. Actually in the initial version in #4914, I implemented a from_json filter. However, this way requires modifying templates of many existing models (including future models?).
At the moment, I don't have a good solution. By comparison, directly parsing it seems a bit simpler.
There was a problem hiding this comment.
A safer way to keep this scoped is:
- Keep
_transform_messagespreserving the OpenAI-compatible message shape, sotool_calls[].function.argumentsstays a JSON string. - Add an opt-in filter in
_compile_jinja_template, e.g.jinja_env.filters["fromjson"] = lambda s: json.loads(s) if isinstance(s, str) else swith defensive fallback. - Update only the templates that need structured arguments to use something like
{% set args = tool_call.arguments | fromjson %}before iteratingargs.items(). - Keep the existing preservation test, and add a focused template-rendering test showing that a template using
fromjsoncan iterate JSON string arguments.
This avoids changing every backend/template caller at once while still supporting templates that require dict-style argument access.
Add parsing for JSON-encoded arguments in tool_calls. Tested with open-zread and Qwen3.5 models.
Close #4914 .
Note: This is a draft PR and it works on my computer. I hope the subsequent modifications can be made by the official maintainers.
TODO
I made a quick investigation in
llm_family.jsonand it seemed that there are four kinds of handling logics in chat templates:tool_call.arguments | tojsonDianJin-R1-32B,HuatuoGPT-o1-70B,InternVL3-78B,Ovis2-34B,QwQ-32B,XiYanSQL-QwenCoder-32B,Fin-R1,Qwen1.5-Chat,Qwen2-Instruct,Qwen2.5-Coder-32B-Instruct,Qwen2.5-Instruct,GPT-OSS,KAT-V1-40B,DeepSeek-V3.1{%- set arguments = tool['function']['arguments'] %}{%- if arguments is not string %}...DeepSeek-R1-0528,Qwen3,Qwen3-Instruct,Qwen3-Thinking,Baichuan-M2-32B,Qwen3-VL-Instruct,Qwen3-VL-Thinking,Qwen3-Next-Instruct,Qwen3-Next-Thinking,Qwen3-Omni-Instruct,Qwen3-Omni-Thinking,DeepSeek-V3.2,Kimi-K2.5argumentsas string'\n' + tool['function']['arguments'] + '\n'DeepSeek-Prover-V2-7B,DeepSeek-V2.5,DeepSeek-V3-0324,DeepSeek-R1,DeepSeek-R1-0528-Qwen3-8B,DeepSeek-R1-Distill-Llama-8B,DeepSeek-R1-Distill-Qwen-32B,Skywork-OR1-7B,QwenLong-L1-32Bargumentsas dict{%- for arg_name, arg_val in tool_call.arguments | items %}Llama-3.1-70B-Instruct,Qwen3-Coder,GLM-4.5,GLM-4.5V,Seed-OSS-36B-Instruct,MiniMax-M2,GLM-4.6,GLM-4.7,GLM-4.7-Flash,MiniMax-M2.5,GLM-5,Qwen3.5,Qwen3.6,MiniMax-M2.7,GLM-5.1So models that treat arguments as string will be unable to handling function callings if this PR is merged but the chat template remains the same.