-
Notifications
You must be signed in to change notification settings - Fork 18.3k
fix(core): properly handle empty or missing arguments for ToolCallChunk
#32017
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
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
CodSpeed WallTime Performance ReportMerging #32017 will not alter performanceComparing
|
CodSpeed Instrumentation Performance ReportMerging #32017 will not alter performanceComparing Summary
|
ToolCallChunk
Passing this off to @eyurtsev for final review/approval |
Co-authored-by: Christophe Bornet <[email protected]>
Co-authored-by: Christophe Bornet <[email protected]>
Co-authored-by: Christophe Bornet <[email protected]>
@@ -195,7 +195,7 @@ class ToolCall(TypedDict): | |||
|
|||
name: str | |||
"""The name of the tool to be called.""" | |||
args: dict[str, Any] | |||
args: Optional[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.
Why is this change necessary?
An empty dict represents no args currently. I'd rather not add a second way to represent the same information
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, an empty dict should represent a completed tool call that has no arguments.
None
is used exclusively to represent the in-progress state where arguments have not yet been received during streaming.
Using an empty dictionary for both the "no arguments" state and the "arguments are coming" state makes them indistinguishable. We wouldn't know how to interpret the message, e.g. two scenarios to illustrate this:
- Final State (No Arguments): The model finishes a tool call and intends for it to have zero args. It correctly sends
args={}
. We'd see this and execute the tool immediately. - In-Transit State (Streaming): The model starts a tool call and is about to stream the arguments. If it used
{}
as a temporary placeholder, we'd seeargs={}
If we went back to args_ = parse_partial_json(chunk["args"]) if chunk["args"] != "" else {}
, it doesn't handle the case where chunk["args"]
is literally None
. When a streaming chunk arrives with args=None
, chunk["args"] != ""
evaluates to True
(since None != ""
). The code then tries to execute parse_partial_json(None)
, which results in a TypeError
Even if it mapped None
to {}
we still lose the ability to distinguish between the two different scenarios.
Description: Streaming tool call chunks: if
chunk["args"] == None
, the tool call is incorrectly marked as anInvalidToolCall
.Fixes #32016