-
Notifications
You must be signed in to change notification settings - Fork 5k
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
fix: tool calling cohere #3355
fix: tool calling cohere #3355
Conversation
Hi @Anirudh31415926535, thanks for your contribution - can you provide a code example that isn't working with the current code so we can test it? |
Hi @marklysze Thanks for looking through! I think its not so much that an example doesn't work because it throws an error... |
Hey @Anirudh31415926535, thanks for the note - I'll give it a test. Would you be able to provide a little bit of a summary as to what may have been incomplete? |
Well, the basic idea is that from our api, we parse the different bits of the request body and have it formatted into a raw prompt that is seen by the model. Since the autogen integration was missing some parameters, it results in an incorrect/incomplete raw prompt being generated which is sent to our model, which might cause some regressions. An example of this is that we need the tool_calls field in the chat history Chatbot messages, that is parsed and contributes to a section of the raw prompt that enabels the model to know that so and so tool call was performed as part of this conversation so that it can continue to hold context of it. If this were absent, then it would lead to that part of the raw prompt being empty, which would be unfortunate. Similarly, for the other changes added too! Hope that is a clearer picture of what I was referring to! |
That's a great explanation and definitely a worthy change to incorporate :) Thanks for coming and helping align AutoGen's client class with your API. |
@Anirudh31415926535, is it possible to move the code for the It would also be good to update the documentation, I think just the "API parameters" section is okay: |
Done! Just pushed the fixes! |
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.
@Anirudh31415926535 has been great in making all the adjustments - changes look good to go. Happy to merge!
@thinkall is that something you can help with?
@microsoft-github-policy-service agree [company="Cohere"] |
@microsoft-github-policy-service agree company="Cohere" |
* Add support for tool calling cohere * update tool calling code * make client name configurable with default * formatting nits * update docs --------- Co-authored-by: Mark Sze <[email protected]> Co-authored-by: Li Jiang <[email protected]>
Why are these changes needed?
The tool calling in the current integration with Cohere has some subtle fallacies in them. These changes ensure that the tool calling happens in the apt mechanism for the Cohere models.
Checks