-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Test]: Hermes tool parser stream output error in Qwen3 case #25203
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
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 You ask your reviewers to trigger select CI tests on top of 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 If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
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 addresses a streaming output error in the Hermes tool parser, particularly for the Qwen3 model, by correcting the logic for handling partially streamed JSON arguments. The addition of a comprehensive test suite for the Hermes parser, including specific cases for Qwen tokenization, streaming, and non-streaming scenarios, is a great improvement and significantly enhances the robustness of the parser. While the main fix is sound, I've identified a pre-existing critical bug in the type checking logic that should be addressed.
d8ed627
to
e7e9587
Compare
@cedonley I saw that you changed lines 369-373 of hermes_tool_parser.py in #10979. The actual fix I am proposing here touches those lines as well by removing 2 string slice operations. Can you remember why you introduced them? Or do you have test cases at hand that I could add to the code base to make sure that they still run? |
Just found this question as well on this topic. The question and your answer seem to suggest that my fix might be applicable. |
The tests seem to pass and all looks fine; Do you have any idea why it used to be if (delta_text not in cur_arguments_json[:-2]): before? |
No, unfortunately not. Let's see if @cedonley has any insights. See also the links to a previous discussion in my previous comment. |
Hi @ahartel Could you retest based on the |
My newly added tests do indeed pass on main and used to fail previously. Seems to have been fixed. Thanks for pointing that out. @chaunceyjiang Would you mind merging the changes to file |
Of course, this is a new test case. We can move forward quickly. |
e7e9587
to
6a1d542
Compare
Signed-off-by: Andreas Hartel <[email protected]>
6a1d542
to
60cf77f
Compare
Thanks. I updated my PR to only contain my test additions (plus some very minor reformattings) |
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~
Thank you for your support @chaunceyjiang |
…oject#25203) Signed-off-by: Andreas Hartel <[email protected]>
…oject#25203) Signed-off-by: Andreas Hartel <[email protected]> Signed-off-by: charlifu <[email protected]>
Signed-off-by: Andreas Hartel <[email protected]> Signed-off-by: yewentao256 <[email protected]>
…oject#25203) Signed-off-by: Andreas Hartel <[email protected]> Signed-off-by: gaojc <[email protected]>
…oject#25203) Signed-off-by: Andreas Hartel <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
…oject#25203) Signed-off-by: Andreas Hartel <[email protected]>
Purpose
Fix: #19056
Test Plan
Added some tests in the PR.