-
Notifications
You must be signed in to change notification settings - Fork 13
Update to AI SDK v6 #245
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 to AI SDK v6 #245
Conversation
|
Thanks @jtpio for working on this. When testing it, the approval tools is not called anymore on my side output.webm |
| .default(true) | ||
| .describe('Whether to record execution timing') | ||
| }), | ||
| needsApproval: true, |
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.
About the previous comment, it seems to be intentional.
Should we add a way to let user set the tools that need approval (similar to what we have for the commands) ?
Probably in a follow up PR.
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.
Maybe we can add it back like it was before, and reconsider after #219
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.
Done in 246dcf8
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 @jtpio, this is a great improvement.
I've tested it successfully so far!
| */ | ||
| export function createDiscoverCommandsTool(commands: CommandRegistry): ITool { | ||
| return tool({ | ||
| name: 'discover_commands', |
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 is a title attribute if we want to keep something equivalent.
No sure there is any use case to it, 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.
Added in 246dcf8
| ); | ||
| if (existingMessageIndex !== -1) { | ||
| const existingMessage = this.messages[existingMessageIndex]; | ||
| export function buildToolCallHtml(options: IToolCallHtmlOptions): string { |
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 like it, this is really nice to build it out of the model itself. 👍
| this._chatPanel = options.chatPanel; | ||
| this._chatModel = this._chatPanel.model as AIChatModel; | ||
| this._trans = options.trans; | ||
| this._agentManager = options.agentManager; |
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 have 2 opposite thoughts about this:
- we could retrieve it with
this.chatPanel.model.agentManagerto avoid adding a dependency - we could get rid of the
chatPaneldependency, and only depends on a node
The advantage of the second approach is that it makes the approval-buttons independent of the chat, and allows more easily to expose it as a standalone extension.
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 more I think about it, the more I am confused about the ApprovalButtons implementation, perhaps because of the history of the object (and what I think was not possible before), or (probably) I miss something.
The current behavior is:
- the chat panel receives an interruption, and create a DOM node with
APPROVAL_BUTTONS:text in there - the
ApprovalButtonslisten for any change in the chat node, looks forAPPROVAL_BUTTONS:text, and replace the corresponding node with a button
Why not creating an ApprovalButtons object in the chat itself, and build the buttons using it (something like approvalButtons.buildButtons(approvalId)), instead of building a temporary DOM node with APPROVAL_BUTTONS: text ?
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.
Right, I wasn't sure if it was worth looking too much into this now. The current approach of building HTML "by hand" is not great, but maybe it's something that should be investigated in @jupyter/chat or a sibling library, as part of jupyterlab/jupyter-chat#276?
Or we try to make use of the current message of the current message footer extension point for the approval buttons?
Another idea could be for @jupyter/chat to allow adding custom message renderers or something similar, that would allow passing custom reusable components (tool call box, approval UI, todo list).
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.
Another idea could be for @jupyter/chat to allow adding custom message renderers or something similar, that would allow passing custom reusable components (tool call box, approval UI, todo list).
This would probably be more generic than using the message footer extension point.
A sibling library providing markdown/rendered component could be a good idea, and may be used in other markdown renderer, not only in chat.
This is likely because there is a dedicated Line 644 in 948e1cd
|
brichet
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.
Thanks @jtpio, it looks good to me.
Fixes #146
v6 now provides built-in support for agents, so we don't need to depend on the OpenAI Agents SDK anymore.
Changes
@openai/agentsdependencies@ai-sdk/mcpinsteadjupyterlab-browser-ai✅