-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[Frontend] Pythonic tool parser #9859
[Frontend] Pythonic tool parser #9859
Conversation
Signed-off-by: Mike Depinet <[email protected]>
👋 Hi! Thank you for contributing to the vLLM project. 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 do one of these:
🚀 |
cc @K-Mistele |
thanks, taking a look! |
Hi @mdepinet, the parser looks good to me! Just a couple notes:
|
Signed-off-by: Mike Depinet <[email protected]>
0bcad39
to
683eb27
Compare
Sure, done.
Thanks, I didn't see these. I'm happy to add a config so these tests exercise the new tool parser also, but I think it's worthwhile to keep the tests I added also because:
Put differently, I view the existing tests more as integration tests and the tests I added more as unit tests. I think it'd be a mistake to entirely do away with either. Does that seem reasonable to you? |
Signed-off-by: Mike Depinet <[email protected]>
c18bdf4
to
b64ca59
Compare
Signed-off-by: Mike Depinet <[email protected]>
For sure, having extra ones is great! I just wanted to make sure that we didn't skip adding this into the existing integration tests for tool use. Checking it out now for testing :) cc @maxdebayser it sounds like this might handle the llama 3.2 1B and 3B tool calling format that we were having issues with , using the Possible closes #9991 ? will test. |
Not an issue with this PR, but Couple options here:
I'll provide a tool call parser here in a bit if you're willing to add it to |
using The first one looks like it could be a parser error, but I'm not sure. |
Just tried running tests with pytest - everything looks good, the model just fails the tests with some frequency ( @mdepinet can you see if it's possible to get them passing with some additional prompting? I think |
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Mike Depinet <[email protected]>
Signed-off-by: Mike Depinet <[email protected]>
@K-Mistele I think this is ready for you now. The smaller Llama models aren't especially reliable. The tests are passing for me, but I'd be inclined to remove the Llama3.2 entry in favor of ToolACE if it's still flaky for you. (I'm actually most interested in fixie-ai/ultravox-v0_4-ToolACE-8B anyway.) |
Thanks for the heads-up, I will give it another pass! |
Yeah so overall, ToolACE implementation is definitely good to go. The llama 3.2 models I think need the
or like this: For what it's worth, some parsers like I think there are a couple ways to move forwards here:
What do you think @mdepinet @DarkLight1337 @mgoin ? |
Unless the author of #9991 @SinanAkkoyun is ok with the first option, I'd go with the second option, and perhaps also log a warning in the code for Llama 3.2 models. |
Hi! However, I don't really care if llama3.2 tool calling is supported here as long as I can verify that for example the hermes tool calling format works, but I can't even verify that sadly. Maybe I am missing configs? |
Signed-off-by: Mike Depinet <[email protected]>
Related #10164 |
Signed-off-by: Mike Depinet <[email protected]>
Signed-off-by: Mike Depinet <[email protected]>
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 for adding this!
Signed-off-by: Mike Depinet <[email protected]> Signed-off-by: OmerD <[email protected]>
Signed-off-by: Mike Depinet <[email protected]> Signed-off-by: Sumit Dubey <[email protected]>
This PR adds a tool parser for models that output tools formatted as Python function calls, such as Llama 3.2 and ToolACE-8B.
FIX #9991