-
Notifications
You must be signed in to change notification settings - Fork 16.3k
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
cohere[patch]: Add cohere tools agent #19602
cohere[patch]: Add cohere tools agent #19602
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
"token_count": response.token_count, | ||
"is_search_required": response.is_search_required, | ||
"generation_id": response.generation_id, | ||
"tool_calls": response.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.
let's factor this into the same format as other models
tool_calls = [
{
"id": "some id string",
"function": {
"name": ...,
"arguments": "json serialized dict or args {}"
}
}
]
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.
Without doing this, this will also mess with serialization
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.
This is implemented, thank you!
) | ||
|
||
|
||
class CohereToolsAgentOutputParser( |
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.
Matching the format above will allow ChatCohere to be used with OpenAIToolsAgentOutputParser
!
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.
so we can delete 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.
That sounds good, however, we'd like the flexibility (and naming) in the partner package. We're only 'exporting' create_cohere_tools_agent
, and if there are plans to refactor some of the tools interfaces in langchain
we could later update this. Would that work for you?
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.
Sounds good. Added an integration test to confirm interface matches at least, and it's failing :(
Could you get that passing, then it looks good
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 thing!
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.
I think I fixed a couple of bugs, but I also wanted to double check that the behaviour is correct as I changed the tests because they looked wrong to me:
- In the non-stream test I updated it to use
invoke
instead ofstream
- What's the expected kwargs.tool_calls value when tools are bound to the request but the model indicates to not call any tool? The tests previously had this as "tool_calls is not present", but I think it should be "tool_calls is an empty sequence". Happy to change this though!
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.
@harry-cohere these are the tests that should work on the chat model
@@ -121,7 +121,7 @@ class Person(BaseModel): | |||
} | |||
|
|||
# where it doesn't call the tool | |||
strm = tool_llm.stream("What is 2+2?") | |||
strm = llm.stream("What is 2+2?") |
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.
This was right before! Does cohere's tool calling always force a tool call, even if the query isn't relevant to the provided tool?
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.
The default behaviour is to indicate that the provided tools are irrelevant. In these cases an empty tool_calls is returned. I can fix the implementation so that it doesn't populate kwargs.tool_calls in this case!
@@ -80,7 +80,7 @@ class Person(BaseModel): | |||
tool_llm = llm.bind_tools([Person]) | |||
|
|||
# where it calls the tool | |||
result = tool_llm.stream("Erick, 27 years old") | |||
result = tool_llm.invoke("Erick, 27 years old") |
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.
good catch
def test_streaming_tool_call_no_tool_calls() -> None: | ||
llm = ChatCohere(temperature=0) | ||
|
||
class Person(BaseModel): | ||
name: str | ||
age: int | ||
|
||
tool_llm = llm.bind_tools([Person]) | ||
|
||
# where it doesn't call the tool | ||
strm = tool_llm.stream("What is 2+2?") | ||
acc: Any = None | ||
for chunk in strm: | ||
assert isinstance(chunk, AIMessageChunk) | ||
acc = chunk if acc is None else acc + chunk | ||
assert acc.content != "" | ||
assert "tool_calls" not in acc.additional_kwargs |
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.
@harry-cohere this is the test that is failing currently.
If it's a model-level problem, I'm happy to skip it.
But currently, this call returns an AIMessage with
- content = ""
- tool calls = []
So basically an empty response :(
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.
DMd you! In short, "don't call any tools because they're all irrelevant" is expressed as empty content and an empty tool_calls array. Happy to change to whatever suits the framework! :)
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.
Sounds good. I'll mark this test as "skip" for now.
As a heads up, this will make cohere models perform poorly in agents. There's lots of cases, where e.g. you might give an agent a tool like calculator
, but still want the agent to be able to respond to queries like "what is the capital of france"
**Description**: Adds a cohere tools agent and related notebook. --------- Co-authored-by: BeatrixCohere <[email protected]> Co-authored-by: Erick Friis <[email protected]>
**Description**: Adds a cohere tools agent and related notebook. --------- Co-authored-by: BeatrixCohere <[email protected]> Co-authored-by: Erick Friis <[email protected]>
Description: Adds a cohere tools agent and related notebook.