-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Bugfix] Fix hermes tool parser handling of non-string argument types #22002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bugfix] Fix hermes tool parser handling of non-string argument types #22002
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request fixes a bug in the Hermes tool parser where it failed to handle non-string argument types during streaming. The change correctly identifies whether the last argument is a string by inspecting the JSON string representation and adjusts how the string is processed. My review includes one suggestion to enhance the readability and maintainability of this critical but complex logic.
stripped_cur_arguments_json = cur_arguments_json[:-2] \ | ||
if (cur_arguments_json[-2] == '"' | ||
or cur_arguments_json[-2] == "'") else \ | ||
cur_arguments_json[:-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this logic correctly fixes the issue with non-string arguments, the ternary expression with a backslash for line continuation is a bit dense and can be hard to read and maintain.
For better clarity and maintainability, I suggest refactoring this into a standard if/else
block. This makes the logic more explicit and easier to understand at a glance, which is valuable for complex string manipulations like this.
if cur_arguments_json[-2] in ('"', "'"):
# Last argument is a string, so remove the closing quote and brace.
stripped_cur_arguments_json = cur_arguments_json[:-2]
else:
# Last argument is not a string, so remove the closing brace only.
stripped_cur_arguments_json = cur_arguments_json[:-1]
👋 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 🚀 |
@DarkLight1337 @chaunceyjiang please review, thanks |
@aarnphm please review, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some unit tests?
I used the test script you provided and the latest code from the main branch, but I couldn’t reproduce the issue you described.
|
I think this is exactly the result described in the issue. The correct result should be |
@BruceW-07 My question is: in your PR description, you mentioned that the Hermes tool parser cannot handle non-string argument types, but based on my tests, |
Do you mean that when the argument type is non-string, the Hermes tool parser unexpectedly removes some characters? |
Yes, and according to my observation, the original code did not take into account the case where the argument is non-string, which resulted in some characters being unexpectedly missing in the result. |
stripped_cur_arguments_json = cur_arguments_json[:-2] | ||
else: | ||
# last argument is not a string, | ||
# so remove the closing brace only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an example in the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I added a simple example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks~
tests/tool_use/test_tool_calls.py
Outdated
# validate arguments | ||
streamed_args = json.loads(function_args_str) | ||
assert isinstance(streamed_args, dict) | ||
assert isinstance(streamed_args.get("product_id"), int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write another test with a bool parameter?
I believe bool types may also encounter the truncation issue with the tool parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, I add a new Boolean argument to the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openai_api_key = "EMPTY"
openai_api_base = "http://localhost:8000/v1"
client = OpenAI(
api_key=openai_api_key,
base_url=openai_api_base,
)
messages = [
{
"role":
"system",
"content":
"You are an artificial intelligence assistant who will call tools everytime when responding.",
},
{
"role":
"user",
"content":
"Hi! Do you have any detailed information about the product id 7355608 and inserted true?",
},
]
tools = [
{
"type": "function",
"function": {
"name": "get_product_info",
"description":
"Get detailed information of a product based on its product ID.",
"parameters": {
"type": "object",
"properties": {
"inserted": {
"type": "boolean",
"description": "inserted.",
},
"product_id": {
"type": "integer",
"description": "The product ID of the product.",
},
},
"required": ["product_id", "inserted"],
},
},
},
]
use_stream = True
model = client.models.list().data[0].id
chat_completion = client.chat.completions.create(
stream=use_stream,
messages=messages,
top_p=0.95,
temperature=0.66,
presence_penalty=0,
frequency_penalty=0.04,
model=model,
tools=tools,
extra_body={
"top_k": 20,
"repetition_penalty": 1.05,
"chat_template_kwargs": {
"enable_thinking": False
},
},
)
I can reproduce the issue of the boolean value being truncated using this example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've added it to the unit test
Here's another example which might be good to test for: {
"type": "function",
"function": {
"name": "search",
"parameters": {
"type": "object",
"properties": {
"search_request": {
"type": "object",
"properties": {
"query": {
"type": "string"
},
"retrieval_method": {
"enum": ["keyword", "neural", "rrf"],
"type": "string"
}
},
"required": ["query", "retrieval_method"]
}
},
"required": ["search_request"]
}
}
} Expected: |
Hi! Is there an ETA for this bugfix? |
Thanks for your example, I've fixed the issue and now I can get the correct result with Qwen3-4B. I haven't added it to the unit test yet, because the tool_chat_template_hermes.jinja used in the test seems to have problem dealing with the example you provided. |
@aarnphm @chaunceyjiang please review, thanks! |
3c870a2
to
b7ddfec
Compare
Signed-off-by: wangzi <[email protected]>
Signed-off-by: wangzi <[email protected]>
Signed-off-by: wangzi <[email protected]>
Signed-off-by: wangzi <[email protected]>
Signed-off-by: wangzi <[email protected]>
Signed-off-by: wangzi <[email protected]>
6b5b690
to
1a37f4f
Compare
Signed-off-by: David Chen <[email protected]>
Signed-off-by: David Chen <[email protected]>
r'\{"name":\s*"' + | ||
re.escape(function_name) + r'"\s*,\s*"arguments":\s*(.*)', | ||
tool_call_portion.strip(), re.DOTALL) | ||
cur_arguments_json = match.group(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might still be a need to check if there is a match, as the match could be None
for string
arguments:
(APIServer pid=1) DEBUG 09-19 07:20:26 [entrypoints/.../tool_parsers/hermes_tool_parser.py:344] diffing old arguments: {}
(APIServer pid=1) DEBUG 09-19 07:20:26 [entrypoints/.../tool_parsers/hermes_tool_parser.py:345] against new ones: {'name': '263012.pdf'}
(APIServer pid=1) ERROR 09-19 07:20:26 [entrypoints/.../tool_parsers/hermes_tool_parser.py:441] Error trying to handle streaming tool call.
(APIServer pid=1) ERROR 09-19 07:20:26 [entrypoints/.../tool_parsers/hermes_tool_parser.py:441] Traceback (most recent call last):
(APIServer pid=1) ERROR 09-19 07:20:26 [entrypoints/.../tool_parsers/hermes_tool_parser.py:441] File "/usr/local/lib/python3.12/dist-packages/vllm/entrypoints/openai/tool_parsers/hermes_tool_parser.py", line 375, in extract_tool_calls_streaming
(APIServer pid=1) ERROR 09-19 07:20:26 [entrypoints/.../tool_parsers/hermes_tool_parser.py:441] cur_arguments_json = match.group(1)
(APIServer pid=1) ERROR 09-19 07:20:26 [entrypoints/.../tool_parsers/hermes_tool_parser.py:441] ^^^^^^^^^^^
(APIServer pid=1) ERROR 09-19 07:20:26 [entrypoints/.../tool_parsers/hermes_tool_parser.py:441] AttributeError: 'NoneType' object has no attribute 'group'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding this change fixed it in my case:
if match:
cur_arguments_json = match.group(1)
else:
cur_arguments_json = json.dumps(cur_arguments,
ensure_ascii=False)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thanks for your suggestion
Signed-off-by: David Chen <[email protected]>
Signed-off-by: David Chen <[email protected]>
Signed-off-by: David Chen <[email protected]>
…vllm-project#22002) Signed-off-by: wangzi <[email protected]> Signed-off-by: David Chen <[email protected]> Co-authored-by: wangzi <[email protected]> Co-authored-by: Chauncey <[email protected]>
…vllm-project#22002) Signed-off-by: wangzi <[email protected]> Signed-off-by: David Chen <[email protected]> Co-authored-by: wangzi <[email protected]> Co-authored-by: Chauncey <[email protected]>
…vllm-project#22002) Signed-off-by: wangzi <[email protected]> Signed-off-by: David Chen <[email protected]> Co-authored-by: wangzi <[email protected]> Co-authored-by: Chauncey <[email protected]> Signed-off-by: charlifu <[email protected]>
…#22002) Signed-off-by: wangzi <[email protected]> Signed-off-by: David Chen <[email protected]> Co-authored-by: wangzi <[email protected]> Co-authored-by: Chauncey <[email protected]> Signed-off-by: yewentao256 <[email protected]>
…vllm-project#22002) Signed-off-by: wangzi <[email protected]> Signed-off-by: David Chen <[email protected]> Co-authored-by: wangzi <[email protected]> Co-authored-by: Chauncey <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
…vllm-project#22002) Signed-off-by: wangzi <[email protected]> Signed-off-by: David Chen <[email protected]> Co-authored-by: wangzi <[email protected]> Co-authored-by: Chauncey <[email protected]>
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.Purpose
Fix hermes tool parser handling of non-string argument types.
One example of the situation when argument type is integer is described in #21372.
Test Plan
Test the issue described in #21372.
Serving command:
Code:
Test Result
use_stream = True
use_stream = False