-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add new tool call streaming system #1713
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
zanllan23-dev
left a comment
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.
Ok
src/helpers/beta/zod.ts
Outdated
| import type { infer as zodInfer, ZodType } from 'zod'; | ||
| import * as z from 'zod'; |
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.
nit: these should use v4 imports https://zod.dev/library-authors#how-to-support-zod-3-and-zod-4-simultaneously
src/resources/beta/beta.ts
Outdated
| chatkit: ChatKitAPI.ChatKit = new ChatKitAPI.ChatKit(this._client); | ||
| assistants: AssistantsAPI.Assistants = new AssistantsAPI.Assistants(this._client); | ||
| threads: ThreadsAPI.Threads = new ThreadsAPI.Threads(this._client); | ||
| chat: ChatAPI.Chat = new ChatAPI.Chat(this._client); |
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.
you should define a sibling chat.ts file in this directory and define a new class there instead of adding toolRunner() to the existing resource class, as now it'll be accessible from client.chat.completions.toolRunner() which isn't what we want yet
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 extended and created a BetaChat, I'm not entirely sure if this is what you had in mind, but it limits it to .beta.chat and not just .chat like we want
update types start setting up tests get non streaming working begin work on modifying anthropic tests to openai update models
src/resources/beta/chat.ts
Outdated
| } from '../../lib/beta/BetaToolRunner'; | ||
| import { Chat as ChatResource } from '../../resources'; | ||
|
|
||
| export class BetaCompletions extends ChatResource.Completions { |
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 should just extend the base APIResource as there's no benefit to exposing the other methods on Completions here as well
| export class BetaCompletions extends ChatResource.Completions { | |
| export class BetaCompletions extends APIResource { |
RobertCraigie
left a comment
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.
Looking good.
Once we've ironed out some of the naming questions, could you also add docs for this?
src/helpers/beta/zod.ts
Outdated
| */ | ||
| export function betaZodTool<InputSchema extends ZodType>(options: { | ||
| name: string; | ||
| inputSchema: InputSchema; |
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.
nit: this should be parameters to match the API shape
src/helpers/beta/zod.ts
Outdated
| if (jsonSchema.type !== 'object') { | ||
| throw new Error(`Zod schema for tool "${options.name}" must be an object, but got ${jsonSchema.type}`); | ||
| } | ||
|
|
||
| // TypeScript doesn't narrow the type after the runtime check, so we need to assert it | ||
| const objectSchema = jsonSchema as typeof jsonSchema & { type: 'object' }; |
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.
nit: I don't think we need this check? I think we should be able to just do parameters: objectSchema below?
examples/tool-helpers-advanced.ts
Outdated
| }, | ||
| ], | ||
| tools: [ | ||
| betaZodTool({ |
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.
thinking through these again, I wonder if we should call these helpers betaZodFunction()? because that feels more accurate with respect to the API.
thoughts?
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.
It's a big mix --
They use "tool_calls" in the actual messages for the list of tool calls, "function" as the type of the tool (or at least the ones we are using), and title their docs page "function calling."
Because we're talking about a type of tool call called a function call, for full transparency I'm actually starting to think we should call it "betaZodFunctionTool" (on their page they also refer user supplied functions as "function tool")
src/lib/beta/BetaToolRunner.ts
Outdated
| /** | ||
| * A ToolRunner handles the automatic conversation loop between the assistant and tools. | ||
| * | ||
| * A ToolRunner is an async iterable that yields either BetaMessage or BetaMessageStream objects |
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.
some incorrect references here 😅
src/lib/beta/BetaToolRunner.ts
Outdated
| tools: params.tools, | ||
| messages: params.messages, | ||
| model: params.model, |
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.
q: why are these explicitly listed?
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.
Because typescript can't infer that stream: false even though we're in the else. tools/messages/model are not necessary
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.
In completions.ts I saw we always explicitly provide stream (we don't leave it as undefined) so I think it's okay to override it here to get nicer inference
src/lib/beta/BetaToolRunner.ts
Outdated
| // TODO: what if it is empty? | ||
| if (prevMessage) { |
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 seems redundant given we check prevMessage above?
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'll remove both checks -- both are no longer relevant under the assumption that the API always will return at least one choice
| ); | ||
|
|
||
| this.#message = this.#chatCompletion.then((resp) => resp.choices.at(0)!.message); | ||
| this.#message.catch(() => {}); |
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.
q: should we be logging the error? or is this safe to just ignore if the error is already reported somewhere else?
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.
When the caller does an "await" (i.e. for await const of), if it was rejected they will get to see the error since they'll be awaiting a rejected promise (of which we ignored the error previously -- here)
If i'm understanding right, the footgun is when they do iterator.next() without an await or looking at the result then there'll be an uncaught exception that is caught and ignored. In the way people use this I think that that is okay
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.
Also, in the "handles api errors" test we show where you can allow an api error to happen but keep on iterating anyway, which is an artifact of how the exception doesn't throw in the actual iterator
50dbc89 to
d5d3818
Compare
d5d3818 to
c1dd29b
Compare
b870a1f to
d18bbfb
Compare
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.
Thinking through naming more again (sorry), we'll likely want to add a similar helper for the Responses API as well in the future, so we should namespace these helpers in some fashion to make it clear they're for Chat Completions instead of Responses, e.g. betaChatFunction() etc.
Could you review the current naming and suggest some alternatives taking this into account?
|
|
||
| ## Tool Helpers | ||
|
|
||
| The SDK makes it easy to create and run [function tools with the chats API](https://platform.openai.com/docs/guides/function-calling). You can use Zod schemas or direct JSON schemas to describe the shape of tool input, and then you can run the tools using the `client.beta.messages.toolRunner` method. This method will automatically handle passing the inputs generated by the model into your tools and providing the results back to the model. |
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.
FYI looks like the primary examples in those linked docs are for the Responses API, not Chat Completions.
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.
It's where they explain what a function tool call actually is, but yes it focuses on the responses API. Should we re-explain it here instead and not link to it, since it could cause confusion?
Sure, some thoughts (in order of preference):
|
2db4f0a to
949d8a0
Compare
2f06c89 to
ec31a58
Compare
Changes being requested
Update the tool call pattern to use a new async iterable style pattern that makes more easily accessible tool calls as they are fired, and API responses.
Additional context & links