-
Notifications
You must be signed in to change notification settings - Fork 15k
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
core[minor], integrations...[patch]: Support ToolCall as Tool input and ToolMessage as Tool output #24038
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
corresponding ToolNode update langchain-ai/langgraph#977 |
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.
partial review
type: NotRequired[Literal["tool_call_chunk"]] | ||
|
||
|
||
def tool_call_chunk( |
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.
why is this needed?
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.
convenience / to make sure ppl don't mess up the type
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.
How is that easier than using ToolCallChunk? Is it validating that there are no extra arguments or typos in the arguments?
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.
type is hard coded
self, | ||
tool_input: Union[str, Dict], | ||
) -> Union[str, Dict[str, Any]]: | ||
def _parse_input(self, tool_input: Union[str, Dict]) -> Union[str, Dict[str, Any]]: |
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.
What can tool input be here? (if you know of the top of you head and have time could be good to update the doc-string)
…chain-ai/langchain into bagatur/rfc_sep_method_for_tool_res
Integration tests look good: https://github.com/langchain-ai/langchain/actions/runs/9893304257 |
run_id=run_id, | ||
# Inputs by definition should always be dicts. |
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.
why do we say they're by definition dicts (and can't be str)?
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.
copy/pasted from elsewhere, cc @eyurtsev
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.
Comment is originally from observing this:
@tool
def foo(x: str):
return x
foo.invoke({'x': 'hello'})
It's possible that str is still supported on its own. I don't know what we want to do in that case -- since we have pretty odd behavior when it comes to langsmith API requiring auto upgrades to dicts
"id": rtc.get("id"), | ||
"index": rtc.get("index"), | ||
} | ||
create_tool_call_chunk( |
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.
There are places we've duplicated logic (e.g., in astream
) that may also need to be updated.
What will the behavior be if a tool call is missing a type
? Possible there will be issues operating on old serialized tool calls (ok if so-- helps to know in case bugs pop up)? And do we need to update other integrations (e.g., community and other repos)?
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.
only thing that depends on type atm is Tools when they're taking a ToolCall (which is a new feature, you couldn't pass in ToolCalls at all before)
Relies on #24038 --------- Co-authored-by: Erick Friis <erick@langchain.dev>
Changes:
Example: