Revert "studio: tool calling for Llama-3, Mistral, Gemma 4 on safetensors + MLX (#5615)"#5619
Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies the tool-call parser by removing support for several model-specific formats, including Llama-3, Mistral, and Gemma 4, to focus on a unified XML-based approach. Review feedback highlights two regressions: the regex for function and parameter names is now too restrictive, and the JSON parsing logic no longer handles non-dictionary arguments or validates for empty tool names as robustly as the previous implementation.
| _TC_FUNC_START_RE = re.compile(r"<function=(\w+)>\s*") | ||
| _TC_END_TAG_RE = re.compile(r"</tool_call>") | ||
| _TC_FUNC_CLOSE_RE = re.compile(r"\s*</function>\s*$") | ||
| _TC_PARAM_START_RE = re.compile(r"<parameter=([\w\.\-]+)>\s*") | ||
| _TC_PARAM_START_RE = re.compile(r"<parameter=(\w+)>\s*") |
There was a problem hiding this comment.
The regular expressions for parsing function and parameter names have been restricted from [\w\.\-]+ to \w+. This is a regression, as it no longer supports names containing dots or hyphens, which was possible before.
| _TC_FUNC_START_RE = re.compile(r"<function=(\w+)>\s*") | |
| _TC_END_TAG_RE = re.compile(r"</tool_call>") | |
| _TC_FUNC_CLOSE_RE = re.compile(r"\s*</function>\s*$") | |
| _TC_PARAM_START_RE = re.compile(r"<parameter=([\w\.\-]+)>\s*") | |
| _TC_PARAM_START_RE = re.compile(r"<parameter=(\w+)>\s*") | |
| _TC_FUNC_START_RE = re.compile(r"<function=([\w\.\-]+)>\s*") | |
| _TC_END_TAG_RE = re.compile(r"</tool_call>") | |
| _TC_FUNC_CLOSE_RE = re.compile(r"\s*</function>\s*$") | |
| _TC_PARAM_START_RE = re.compile(r"<parameter=([\w\.\-]+)>\s*") |
| obj = json.loads(json_str) | ||
| tc = { | ||
| "id": f"call_{id_offset + len(tool_calls)}", | ||
| "type": "function", | ||
| "function": { | ||
| "name": obj.get("name", ""), | ||
| "arguments": obj.get("arguments", {}), | ||
| }, | ||
| } | ||
| if isinstance(tc["function"]["arguments"], dict): | ||
| tc["function"]["arguments"] = json.dumps( | ||
| tc["function"]["arguments"] | ||
| ) | ||
| tool_calls.append(tc) |
There was a problem hiding this comment.
This implementation for parsing JSON tool calls has a couple of regressions compared to the original _parse_tool_call_json function that was removed:
- Incorrect handling of
arguments: It no longer correctly handlesargumentsthat are not a dictionary or a string. The previous logic would wrap other types (like lists or numbers) in a{"value": ...}object and serialize it to a JSON string. The current code passes them through as-is, violating the function's contract thatargumentsis always a JSON string. - Missing name validation: It doesn't check if the tool
nameis empty. The previous implementation would correctly skip tool calls with no name.
Here's a suggestion to restore the more robust, original behavior.
| obj = json.loads(json_str) | |
| tc = { | |
| "id": f"call_{id_offset + len(tool_calls)}", | |
| "type": "function", | |
| "function": { | |
| "name": obj.get("name", ""), | |
| "arguments": obj.get("arguments", {}), | |
| }, | |
| } | |
| if isinstance(tc["function"]["arguments"], dict): | |
| tc["function"]["arguments"] = json.dumps( | |
| tc["function"]["arguments"] | |
| ) | |
| tool_calls.append(tc) | |
| obj = json.loads(json_str) | |
| name = obj.get("name", "") | |
| if not name: | |
| continue | |
| args = obj.get("arguments", {}) | |
| if isinstance(args, dict): | |
| args_str = json.dumps(args) | |
| elif isinstance(args, str): | |
| args_str = args | |
| else: | |
| args_str = json.dumps({"value": args}) | |
| tool_calls.append( | |
| { | |
| "id": f"call_{id_offset + len(tool_calls)}", | |
| "type": "function", | |
| "function": {"name": name, "arguments": args_str}, | |
| } | |
| ) |
|
Confirming the revert was correct. #5615 shipped five concrete parser regressions (Mistral nested-JSON truncation, |
…sors + MLX (unslothai#5615)" (unslothai#5619) Reverts PR unslothai#5615 to give the safetensors + MLX healing parity work more time to bake before re-merging. The reverted feature branch `studio-tools-multi-format` remains untouched, and the follow-up PR will layer the healing-parity commits on top.
Reverts #5615. Holding the multi-format parser back until we validate the full safetensors / MLX tool-call healing parity package end-to-end with real models.