-
Notifications
You must be signed in to change notification settings - Fork 778
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
Fix workers AI usage with @cloudflare/vite-plugin #8016
base: main
Are you sure you want to change the base?
Fix workers AI usage with @cloudflare/vite-plugin #8016
Conversation
## Context Hi there o/ At RedwoodJS, we're trying out @cloudflare/vite-plugin alongside Workers AI, but we've run into an issue where the two don't seem to work together. This is mainly to highlight the issue (its easier to explain with code). Feel free to use or disregard the solution as needed. ## Problem When using `@cloudflare/vite-plugin` for a worker with an `[ai]` binding in the `worker.toml`, we end up getting this error: ``` workerd/server/workerd-api.c++:753: error: wrapped binding module can't be resolved (internal modules only); moduleName = miniflare-internal:wrapped:__WRANGLER_EXTERNAL_AI_WORKER ``` This seems to be because the miniflare options include the wrapped bindings mapping to an internal AI worker script, but the corresponding worker options for this worker are not included in the miniflare options. ## Solution * In `wrangler`: Include `externalWorkers` in the results of [`unstable_getMiniflareWorkerOptions`](https://github.com/cloudflare/workers-sdk/blob/a7163b3a21f56c9bd839e34c9d5f31c3099a585a/packages/wrangler/src/api/integrations/platform/index.ts#L273) - it contains the worker options for the AI worker * In `@cloudflare/vite-plugin-cloudflare`: For the bindings in `wrappedBindings`, if their corresponding scripts are in `externalWorkers`, add these worker options to the miniflare options
|
? { | ||
[workerConfig.assets.binding]: ASSET_WORKER_NAME, | ||
externalWorkers, | ||
worker: { |
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 diff here is misleading - this was the only change for this part of code
externalWorkersMap.set(scriptName, externalWorker); | ||
} | ||
} | ||
} |
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 idea is:
- only include the workers that map to script names referenced in
wrappedBindings
- only include them once - in case, for e.g. there were multiple workers each with their own AI binding, I think we'd end up with more than once reference to the internal AI worker script
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 this actually necessary?
In Wrangler's MultiWorkerRuntimeController
it appears that any external workers are all just concatenated together. It doesn't do this kind of filtering and deduping.
I am pretty sure that Miniflare only adds external workers that are relevant to the bindings in the config. So if there is no AI binding there would be no equivalent external worker. See
workers-sdk/packages/wrangler/src/dev/miniflare.ts
Lines 589 to 608 in 444a630
const wrappedBindings: WorkerOptions["wrappedBindings"] = {}; | |
if (bindings.ai?.binding) { | |
externalWorkers.push({ | |
name: EXTERNAL_AI_WORKER_NAME, | |
modules: [ | |
{ | |
type: "ESModule", | |
path: "index.mjs", | |
contents: EXTERNAL_AI_WORKER_SCRIPT, | |
}, | |
], | |
serviceBindings: { | |
FETCHER: AIFetcher, | |
}, | |
}); | |
wrappedBindings[bindings.ai.binding] = { | |
scriptName: EXTERNAL_AI_WORKER_NAME, | |
}; | |
} |
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.
Actually, while I think the filtering is not needed, the deduping probably is. The fact that the MultiWorkerRuntimeController
doesn't do this might be a bug! cc @penalosa
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.
@petebacondarwin if we remove this for
block so that we're no longer filtering to only the workers that have corresponding wrappedBindings
, then I get this when I have workers referencing Durable Objects:
service core:user:__WRANGLER_EXTERNAL_DURABLE_OBJECTS_WORKER: Worker "core:user:__WRANGLER_EXTERNAL_DURABLE_OBJECTS_WORKER"'s binding "SessionDO" refers to a service "core:user:worker", but no such service is defined.
|
||
test("basic hello-world functionality", async () => { | ||
expect(JSON.parse(await getTextResponse())).toContain("PONG"); | ||
}); |
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 E2E test causes a CLI prompt to appear for the user to choose a CF account to use for the Workers AI API - I'm not sure if there's a way to avoid that interactivity in tests, and how to programmatically set which account to use for CI.
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.
Normally, Wrangler, we provide a CLOUDFLARE_ACCOUNT_ID env variable to avoid such login popups, but so far I don't think we've needed that in the Vite plugin work, since it has all been local only.
I think this might be one of the first wrappedBindings that has a test?
I need to check how one can set the account ID for our plugin (if at all) right now.
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 at the built types for options the plugin takes in, I think there might not be a way to set an account id dynamically.
type PersistState = boolean | {
path: string;
};
interface BaseWorkerConfig {
viteEnvironment?: {
name?: string;
};
}
interface EntryWorkerConfig extends BaseWorkerConfig {
configPath?: string;
}
interface AuxiliaryWorkerConfig extends BaseWorkerConfig {
configPath: string;
}
interface PluginConfig extends EntryWorkerConfig {
auxiliaryWorkers?: AuxiliaryWorkerConfig[];
persistState?: PersistState;
}
declare function cloudflare(pluginConfig?: PluginConfig): vite.Plugin;
Considering that, to avoid scope creep in the PR, I skipped the test for now with a comment: b4d16b5
Happy to go a different route though, or even just remove the test or example :)
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 think this is generally the right idea. Thanks for the investigation and suggested solution. Let's iterate and get it in.
…e avoided for auth for workers AI API
@petebacondarwin thank you for taking a look! I think this is ready for another look now. |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-wrangler-8016 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/8016/npm-package-wrangler-8016 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-wrangler-8016 dev path/to/script.js Additional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-cloudflare-workers-bindings-extension-8016 -O ./cloudflare-workers-bindings-extension.0.0.0-v37b23c02c.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v37b23c02c.vsix create-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-create-cloudflare-8016 --no-auto-update @cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-cloudflare-kv-asset-handler-8016 miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-miniflare-8016 @cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-cloudflare-pages-shared-8016 @cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-cloudflare-unenv-preset-8016 @cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-cloudflare-vite-plugin-8016 @cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-cloudflare-vitest-pool-workers-8016 @cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-cloudflare-workers-editor-shared-8016 @cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-cloudflare-workers-shared-8016 @cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13137587239/npm-package-cloudflare-workflows-shared-8016 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
I'm also encountering this issue with |
Context
Hi there o/
At RedwoodJS, we're trying out
@cloudflare/vite-plugin
alongside Workers AI, but we've run into an issue where the two don't seem to work together.I've made a PR mainly to illustrate where the issue is - feel free to use or disregard the solution as needed. For this same reason I'm marking this as a draft, and will bring it up with Cloudflare folks in Redwood's slack.
Problem
When using
@cloudflare/vite-plugin
for a worker with an[ai]
binding in theworker.toml
, we end up getting this error:This seems to be because the miniflare options include the wrapped bindings mapping to an internal AI worker script, yet the corresponding worker options for this worker are not included in the miniflare options.
Solution
wrangler
: IncludeexternalWorkers
in the results ofunstable_getMiniflareWorkerOptions
- it contains the worker options for the AI worker@cloudflare/vite-plugin-cloudflare
: For the bindings inwrappedBindings
, if their corresponding scripts are inexternalWorkers
, add these worker options to the miniflare optionsunstable_getMiniflareWorkerOptions
- the returned object now hasexternalWorkers
added to it.