Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { expect, test } from "vitest";
import { getTextResponse } from "../../__test-utils__";

// todo(justinvdm, 2025-02-04): Since this example uses Workers AI, there is CLI interactivity required to choose and authenticate for the account
// that should be used when talking to the Workers AI API, making it difficult to run this test automatically. Once the plugin accepts an account id
// this test might be able to be unskipped.
test.skip("basic hello-world functionality", async () => {
expect(JSON.parse(await getTextResponse())).toContain("PONG");
});
Copy link
Author

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.

Copy link
Contributor

@petebacondarwin petebacondarwin Feb 3, 2025

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.

Copy link
Author

@justinvdm justinvdm Feb 4, 2025

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 :)

19 changes: 19 additions & 0 deletions packages/vite-plugin-cloudflare/playground/ai/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"name": "@playground/ai",
"private": true,
"type": "module",
"scripts": {
"build": "vite build --app",
"check:types": "tsc --build",
"dev": "vite dev",
"preview": "vite preview"
},
"devDependencies": {
"@cloudflare/vite-plugin": "workspace:*",
"@cloudflare/workers-tsconfig": "workspace:*",
"@cloudflare/workers-types": "^4.20250129.0",
"typescript": "catalog:default",
"vite": "catalog:vite-plugin",
"wrangler": "catalog:vite-plugin"
}
}
15 changes: 15 additions & 0 deletions packages/vite-plugin-cloudflare/playground/ai/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export interface Env {
AI: Ai;
}

export default {
async fetch(_request: Request, env: Env): Promise<Response> {
const response = await env.AI.run("@cf/meta/llama-3.1-8b-instruct", {
prompt: "When I say PING, you say PONG. PING",
});

return new Response(
JSON.stringify((response as { response: string }).response.toUpperCase())
);
},
} satisfies ExportedHandler<Env>;
7 changes: 7 additions & 0 deletions packages/vite-plugin-cloudflare/playground/ai/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"files": [],
"references": [
{ "path": "./tsconfig.node.json" },
{ "path": "./tsconfig.worker.json" }
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"extends": ["@cloudflare/workers-tsconfig/base.json"],
"include": ["vite.config.ts", "__tests__"]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"extends": ["@cloudflare/workers-tsconfig/worker.json"],
"include": ["src"]
}
9 changes: 9 additions & 0 deletions packages/vite-plugin-cloudflare/playground/ai/turbo.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"$schema": "http://turbo.build/schema.json",
"extends": ["//"],
"tasks": {
"build": {
"outputs": ["dist/**"]
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { cloudflare } from "@cloudflare/vite-plugin";
import { defineConfig } from "vite";

export default defineConfig({
plugins: [cloudflare({ persistState: false })],
});
6 changes: 6 additions & 0 deletions packages/vite-plugin-cloudflare/playground/ai/wrangler.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
name = "worker"
main = "./src/index.ts"
compatibility_date = "2024-12-30"

[ai]
binding = "AI"
139 changes: 85 additions & 54 deletions packages/vite-plugin-cloudflare/src/miniflare-options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ export function getDevMiniflareOptions(
},
];

const userWorkers =
const workersFromConfig =
resolvedPluginConfig.type === "workers"
? Object.entries(resolvedPluginConfig.workers).map(
([environmentName, workerConfig]) => {
Expand All @@ -288,69 +288,99 @@ export function getDevMiniflareOptions(
resolvedPluginConfig.cloudflareEnv
);

const { externalWorkers } = miniflareWorkerOptions;

const { ratelimits, ...workerOptions } =
miniflareWorkerOptions.workerOptions;

return {
...workerOptions,
// We have to add the name again because `unstable_getMiniflareWorkerOptions` sets it to `undefined`
name: workerConfig.name,
modulesRoot: miniflareModulesRoot,
unsafeEvalBinding: "__VITE_UNSAFE_EVAL__",
bindings: {
...workerOptions.bindings,
__VITE_ROOT__: resolvedViteConfig.root,
__VITE_ENTRY_PATH__: workerConfig.main,
},
serviceBindings: {
...workerOptions.serviceBindings,
...(environmentName ===
resolvedPluginConfig.entryWorkerEnvironmentName &&
workerConfig.assets?.binding
? {
[workerConfig.assets.binding]: ASSET_WORKER_NAME,
externalWorkers,
worker: {
Copy link
Author

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

...workerOptions,
// We have to add the name again because `unstable_getMiniflareWorkerOptions` sets it to `undefined`
name: workerConfig.name,
modulesRoot: miniflareModulesRoot,
unsafeEvalBinding: "__VITE_UNSAFE_EVAL__",
bindings: {
...workerOptions.bindings,
__VITE_ROOT__: resolvedViteConfig.root,
__VITE_ENTRY_PATH__: workerConfig.main,
},
serviceBindings: {
...workerOptions.serviceBindings,
...(environmentName ===
resolvedPluginConfig.entryWorkerEnvironmentName &&
workerConfig.assets?.binding
? {
[workerConfig.assets.binding]: ASSET_WORKER_NAME,
}
: {}),
__VITE_INVOKE_MODULE__: async (request) => {
const payload =
(await request.json()) as vite.CustomPayload;
const invokePayloadData = payload.data as {
id: string;
name: string;
data: [string, string, FetchFunctionOptions];
};

assert(
invokePayloadData.name === "fetchModule",
`Invalid invoke event: ${invokePayloadData.name}`
);

const [moduleId] = invokePayloadData.data;

// For some reason we need this here for cloudflare built-ins (e.g. `cloudflare:workers`) but not for node built-ins (e.g. `node:path`)
// See https://github.com/flarelabs-net/vite-plugin-cloudflare/issues/46
if (moduleId.startsWith("cloudflare:")) {
const result = {
externalize: moduleId,
type: "builtin",
} satisfies vite.FetchResult;

return new MiniflareResponse(JSON.stringify({ result }));
}
: {}),
__VITE_INVOKE_MODULE__: async (request) => {
const payload = (await request.json()) as vite.CustomPayload;
const invokePayloadData = payload.data as {
id: string;
name: string;
data: [string, string, FetchFunctionOptions];
};

assert(
invokePayloadData.name === "fetchModule",
`Invalid invoke event: ${invokePayloadData.name}`
);

const [moduleId] = invokePayloadData.data;

// For some reason we need this here for cloudflare built-ins (e.g. `cloudflare:workers`) but not for node built-ins (e.g. `node:path`)
// See https://github.com/flarelabs-net/vite-plugin-cloudflare/issues/46
if (moduleId.startsWith("cloudflare:")) {
const result = {
externalize: moduleId,
type: "builtin",
} satisfies vite.FetchResult;

return new MiniflareResponse(JSON.stringify({ result }));
}

const devEnvironment = viteDevServer.environments[
environmentName
] as CloudflareDevEnvironment;

const result = await devEnvironment.hot.handleInvoke(payload);

return new MiniflareResponse(JSON.stringify(result));

const devEnvironment = viteDevServer.environments[
environmentName
] as CloudflareDevEnvironment;

const result =
await devEnvironment.hot.handleInvoke(payload);

return new MiniflareResponse(JSON.stringify(result));
},
},
},
} satisfies Partial<WorkerOptions>;
} satisfies Partial<WorkerOptions>,
};
}
)
: [];

const userWorkers = workersFromConfig.map((worker) => worker.worker);

const resolvedExternalWorkersMap = new Map(
workersFromConfig
.flatMap((worker) => worker.externalWorkers)
.map((worker) => [worker.name, worker])
);

const externalWorkersMap = new Map<string, WorkerOptions>();

for (const worker of userWorkers) {
for (const binding of Object.values(worker.wrappedBindings ?? {})) {
const scriptName =
typeof binding === "string" ? binding : binding.scriptName;

const externalWorker = resolvedExternalWorkersMap.get(scriptName);

if (externalWorker) {
externalWorkersMap.set(scriptName, externalWorker);
}
}
}
Copy link
Author

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

Copy link
Contributor

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

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,
};
}

Copy link
Contributor

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

Copy link
Author

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.


const workerToWorkerEntrypointNamesMap =
getWorkerToWorkerEntrypointNamesMap(userWorkers);
const workerToDurableObjectClassNamesMap =
Expand All @@ -375,6 +405,7 @@ export function getDevMiniflareOptions(
),
workers: [
...assetWorkers,
...externalWorkersMap.values(),
...userWorkers.map((workerOptions) => {
const wrappers = [
`import { createWorkerEntrypointWrapper, createDurableObjectWrapper, createWorkflowEntrypointWrapper } from '${RUNNER_PATH}';`,
Expand Down
10 changes: 8 additions & 2 deletions packages/wrangler/src/api/integrations/platform/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ export interface Unstable_MiniflareWorkerOptions {
workerOptions: SourcelessWorkerOptions;
define: Record<string, string>;
main?: string;
externalWorkers: WorkerOptions[];
}

export function unstable_getMiniflareWorkerOptions(
Expand Down Expand Up @@ -264,7 +265,7 @@ export function unstable_getMiniflareWorkerOptions(
}));

const bindings = getBindings(config, env, true, {});
const { bindingOptions } = buildMiniflareBindingOptions({
const { bindingOptions, externalWorkers } = buildMiniflareBindingOptions({
name: undefined,
bindings,
workerDefinitions: undefined,
Expand Down Expand Up @@ -330,5 +331,10 @@ export function unstable_getMiniflareWorkerOptions(
...assetOptions,
};

return { workerOptions, define: config.define, main: config.main };
return {
workerOptions,
define: config.define,
main: config.main,
externalWorkers,
};
}
21 changes: 21 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading