[Model] Add Qwen3CoderToolParser #21396
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
![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 introduces a new tool parser for Qwen3 Coder's XML format. The implementation is comprehensive, covering both streaming and non-streaming modes. However, I've identified a critical security vulnerability with the use of eval() on model output, which must be addressed. I've also pointed out several high-severity issues related to exception handling and unsafe string operations that could lead to crashes or hide bugs. Addressing these points will significantly improve the robustness and security of the new parser.
| try: | ||
| param_value = eval(param_value) | ||
| except: | ||
| logger.warning( | ||
| f"Parsed value '{param_value}' of parameter '{param_name}' cannot be converted via Python `eval()` in tool '{func_name}', degenerating to string." | ||
| ) |
There was a problem hiding this comment.
The use of eval() on model-generated output is a critical security vulnerability as it can lead to arbitrary code execution. The model's output can be influenced by user input, creating a potential attack vector. Please remove eval() and rely on safer parsing methods. The existing logic will correctly fall back to returning the parameter as a string if other conversions fail.
| ): | ||
| try: | ||
| param_value = int(param_value) | ||
| except: |
There was a problem hiding this comment.
Avoid using bare except: clauses. They catch all exceptions, including SystemExit and KeyboardInterrupt, which can hide bugs and make the program difficult to terminate. Please catch a more specific exception, like ValueError in this case, which is raised by int() on conversion failure.
| except: | |
| except ValueError: |
| try: | ||
| float_param_value = float(param_value) | ||
| param_value = float_param_value if float_param_value - int(float_param_value) != 0 else int(float_param_value) | ||
| except: |
There was a problem hiding this comment.
Avoid using bare except: clauses. They catch all exceptions, including SystemExit and KeyboardInterrupt, which can hide bugs and make the program difficult to terminate. Please catch a more specific exception, like ValueError in this case, which is raised by float() on conversion failure.
| except: | |
| except ValueError: |
| try: | ||
| param_value = json.loads(param_value) | ||
| return param_value | ||
| except: |
There was a problem hiding this comment.
Avoid using bare except: clauses. They catch all exceptions, including SystemExit and KeyboardInterrupt, which can hide bugs and make the program difficult to terminate. Please catch a more specific exception, like json.JSONDecodeError in this case.
| except: | |
| except json.JSONDecodeError: |
| return param_value | ||
|
|
||
| # Extract function name | ||
| end_index = function_call_str.index(">") |
There was a problem hiding this comment.
Using str.index() will raise a ValueError if the substring '>' is not found. This could cause an unhandled exception for malformed model output. It's safer to use str.find(), which returns -1 on failure. Please add a check for end_index == -1 to handle malformed input gracefully.
| end_index = function_call_str.index(">") | |
| end_index = function_call_str.find(">") |
| param_dict = {} | ||
| for match in self.tool_call_parameter_regex.findall(parameters): | ||
| match_text = match[0] if match[0] else match[1] | ||
| idx = match_text.index(">") |
There was a problem hiding this comment.
Using str.index() will raise a ValueError if the substring '>' is not found. This could cause an unhandled exception for malformed model output. It's safer to use str.find(), which returns -1 on failure. Please add a check for idx == -1 to handle malformed input gracefully.
| idx = match_text.index(">") | |
| idx = match_text.find(">") |
| content=content if content else None, | ||
| ) | ||
|
|
||
| except Exception: |
There was a problem hiding this comment.
Catching a broad Exception can hide bugs and make debugging difficult. It's better to catch more specific exceptions that you expect to occur in the try block. Based on the code, ValueError from parsing errors is a likely candidate.
| except Exception: | |
| except ValueError: |
Signed-off-by: simon-mo <[email protected]>
Signed-off-by: simon-mo <[email protected]>
|
I'm force merging this to unblock model usage, after lint. |
Signed-off-by: simon-mo <[email protected]>
Signed-off-by: simon-mo <[email protected]>
Signed-off-by: simon-mo <[email protected]> Co-authored-by: simon-mo <[email protected]> Signed-off-by: qizixi <[email protected]>
Signed-off-by: simon-mo <[email protected]> Co-authored-by: simon-mo <[email protected]> Signed-off-by: x22x22 <[email protected]>
Signed-off-by: simon-mo <[email protected]> Co-authored-by: simon-mo <[email protected]>
Signed-off-by: simon-mo <[email protected]> Co-authored-by: simon-mo <[email protected]>
Signed-off-by: simon-mo <[email protected]> Co-authored-by: simon-mo <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
Signed-off-by: simon-mo <[email protected]> Co-authored-by: simon-mo <[email protected]> Signed-off-by: Paul Pak <[email protected]>
Signed-off-by: simon-mo <[email protected]> Co-authored-by: simon-mo <[email protected]> Signed-off-by: Diego-Castan <[email protected]>
Signed-off-by: simon-mo <[email protected]> Co-authored-by: simon-mo <[email protected]>
2 participants
Edit from @simon-mo
Tested locally for both tool use example and unit test.