-
Notifications
You must be signed in to change notification settings - Fork 192
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
Update Cohere client library to V2 with Structured Outputs #370
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Mark Sze <[email protected]>
@Anirudh31415926535, would love your review of this PR with V2 |
Signed-off-by: Mark Sze <[email protected]>
Signed-off-by: Mark Sze <[email protected]>
# Strip out name | ||
for message in cohere_params["messages"]: | ||
if "name" in message: | ||
del message["name"] |
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.
f"{message.get('name', '')}:{message.get('content')}" Could we have the message content look like this so that we're still capturing the agent names properly?
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, that's definitely possible - I wanted to check, though, it is likely that the models will respond in the same format? I've seen before when we prefix content the model seems to want to prefix it themselves, too?
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.
That's a good question... I suppose that might end up happening... Let me check and get back on this :)
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 @Anirudh31415926535!
cohere_finish = "tool_calls" | ||
tool_calls = [] | ||
for tool_call in response.tool_calls: | ||
for tool_call in response.message.tool_calls: |
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.
Would the message.tool_calls be available in the chat history as well?
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, they should be
cohere_params["message"] = final_message | ||
cohere_params["preamble"] = preamble | ||
# Strip out name | ||
for message in cohere_params["messages"]: |
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.
If the messages contain previous tool calls and tool results, then could we convert it to a chatbot message if the current agent doesn't have access to those tools? This seems to happen quite often in multi-agent settings, and doing this seems to minimize tool-call hallucinations for us. Similar to this line here: https://github.com/ag2ai/ag2/pull/338/files#diff-abb1d050329eb20452af099582df3efa9bd5ccb69b02a0a57daf26e911159d31R440
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.
That sounds like a good idea! Happy for you to make the change here if you'd like, otherwise let me know and I can.
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.
Hey @marklysze Would appreciate if you would like to take the first stab at it? I think I'll be able to get to this maybe in the end of next week. Lmk, happy to proceed either way :)
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, let me see how I go - hopefully I'll find some time, too :)
Why are these changes needed?
Updates the Cohere client to use V2 of the API, includes Structured Output support.
Related issue number
N/A
Checks