-
Notifications
You must be signed in to change notification settings - Fork 142
Integration with AI SDK #1792
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: main
Are you sure you want to change the base?
Integration with AI SDK #1792
Conversation
packages/ai-sdk/src/activities.ts
Outdated
| * | ||
| * @experimental The AI SDK integration is an experimental feature; APIs may change without notice. | ||
| */ | ||
| export const createActivities = (provider: ProviderV2, mcpClientFactory?: (_: any) => Promise<MCPClient>): 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.
A few things:
- Prefer the function syntax rather than the const lambda form unless there's a reason for the later;
- Make a named type definition for
McpClientFactory. - Make the arg's type
unknownrather thanany.
| export const createActivities = (provider: ProviderV2, mcpClientFactory?: (_: any) => Promise<MCPClient>): object => { | |
| // In mcp.ts | |
| type McpClientFactory = (args: unknown) => Promise<MCPClient>; | |
| export function createActivities(provider: ProviderV2, mcpClientFactory?: McpClientFactory): object { |
packages/ai-sdk/src/activities.ts
Outdated
| */ | ||
| export const createActivities = (provider: ProviderV2, mcpClientFactory?: (_: any) => Promise<MCPClient>): object => { | ||
| const activities = { | ||
| async invokeModel( |
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.
Should we not namespace those activities somehow, to avoid eventual conflicts with user defined activities, or those from other plugins? Could be something like e.g. __ai_temporal_invokeModel?
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.
We didn't in Python, and it does come with the downside of being UI clutter since the name is shown there.
packages/ai-sdk/src/mcp.ts
Outdated
| ) {} | ||
|
|
||
| async tools(): Promise<ToolSet> { | ||
| const tools: Record<string, ListToolResult> = await workflow |
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.
Is it the way you did this in Python? I mean, listTools being an activity call, which is call everytime tools() is called?
A few things that can be considered to make this more efficient:
- Cache
toolsafter the first call - Make
listToolsa local activity call instead of a regular activity; - Extract the list of tools at bundle time and inject it as a static value into the workflow bundle (so no activity call needed to list tools).
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 is one of the implementations we provided. Caching is a potential addition, but tools() is generally called once.
Extract the list of tools at bundle time and inject it as a static value into the workflow bundle (so no activity call needed to list tools).
I don't believe this is valid as the tool list is not known to be static.
mjameswh
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.
See my previous comments.
What was changed
Added a new
AiSDKPluginto support running AI SDK in workflows. The plugin registers new activities for model invocation and optionallyMCPtools. Workflows can then usetemporalProviderto create models in workflows.Why?
Checklist
Closes
How was this tested:
Added a new test suite with local model coverage.
AI_SDK_REMOTE_TESTSenv variable is available to run against a real endpoint.Any docs updates needed?