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

Add native run binding for Workers AI #5371

Merged
merged 2 commits into from
Apr 1, 2024

Conversation

G4brym
Copy link
Member

@G4brym G4brym commented Mar 25, 2024

What this PR solves / how to test

Adds native binding for workers ai.
CR-852557

Author has addressed the following

  • Tests
    • TODO (before merge)
    • Included
    • Not necessary because:
  • Changeset (Changeset guidelines)
    • TODO (before merge)
    • Included
    • Not necessary because:
  • Public documentation

Copy link

changeset-bot bot commented Mar 25, 2024

🦋 Changeset detected

Latest commit: c49bf87

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
wrangler Minor
@cloudflare/vitest-pool-workers Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@G4brym G4brym force-pushed the improve-workers-ai-binding branch 3 times, most recently from a38364d to 146916d Compare March 25, 2024 18:55
@G4brym G4brym marked this pull request as ready for review March 25, 2024 18:56
@G4brym G4brym requested a review from a team as a code owner March 25, 2024 18:56
@G4brym G4brym force-pushed the improve-workers-ai-binding branch from 81a6392 to 9a3b827 Compare March 25, 2024 19:07
@G4brym G4brym changed the title WIP: Improve workers ai binding using wrappedBindings Improve workers ai binding using wrappedBindings Mar 25, 2024
@G4brym G4brym changed the title Improve workers ai binding using wrappedBindings Add native run binding for Workers AI Mar 25, 2024
Copy link
Contributor

github-actions bot commented Mar 25, 2024

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/8498132672/npm-package-wrangler-5371

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5371/npm-package-wrangler-5371

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8498132672/npm-package-wrangler-5371 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8498132672/npm-package-create-cloudflare-5371 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8498132672/npm-package-cloudflare-kv-asset-handler-5371
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8498132672/npm-package-miniflare-5371
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8498132672/npm-package-cloudflare-pages-shared-5371
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8498132672/npm-package-cloudflare-vitest-pool-workers-5371

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.41.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240320.1
workerd 1.20240329.0 1.20240329.0
workerd --version 1.20240329.0 2024-03-29

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@G4brym G4brym force-pushed the improve-workers-ai-binding branch 4 times, most recently from 1937246 to 61ab497 Compare March 25, 2024 20:02
@admah
Copy link
Contributor

admah commented Mar 26, 2024

Will this change the user experience for local dev?

@G4brym
Copy link
Member Author

G4brym commented Mar 26, 2024

@admah yes, user will be able to use Workers ai without installing the @cloudflare/ai npm package
but i've kept the .fetch() method, so anyone using the "soon deprecated" npm packge will still be able to run models without changing anything in ther code

@irvinebroque
Copy link
Contributor

@G4brym — assume this need to go out in wrangler for next week? ready for review? Think @craigsdennis trying to build something using new binding

@G4brym G4brym force-pushed the improve-workers-ai-binding branch 7 times, most recently from 258b887 to fe1b31d Compare March 30, 2024 14:32
@G4brym
Copy link
Member Author

G4brym commented Mar 30, 2024

@irvinebroque only now i could finish the pr, because workerd was only released this morning
this pr is ready, but with the new workerd release, some queue's tests are now failing

@G4brym
Copy link
Member Author

G4brym commented Mar 30, 2024

I'm not sure if miniflare needs to be released first, as this wrangler changes depends on a miniflare release

@irvinebroque
Copy link
Contributor

Think PR description would help here as took a sec to wrap my head around why this needed to refactor bit of Miniflare.

Tests fail only with your changes or also with latest workerd? Have you dug in? Bumped here to test: #5451

  [mf:wrn] Dropped message "1df5e791450c89d5864f9204b6094d8e" on queue "queue" after 3 failed attempts!
   ❯ |queues| test/queue-producer-unit.test.ts  (2 tests | 1 failed) 1101ms
     ❯ test/queue-producer-unit.test.ts > produces queue message with mocked consumer
       → Timed out in waitUntil!
   ❯ |queues| test/queue-consumer-integration-self.test.ts  (1 test | 1 failed) 78ms
     ❯ test/queue-consumer-integration-self.test.ts > consumes queue messages
       → The value cannot be converted because it is not an integer.
   ✓ |queues| test/queue-consumer-unit.test.ts  (1 test) 68ms
    index › Miniflare: getWorker() allows dispatching events directly
  
    test/index.spec.ts:1177
  
     1176:   // Check `Fetcher#queue()`                          
     1177:   let queueResult = await fetcher.queue("needy", [    
     1178:     { id: "a", timestamp: new Date(1000), body: "a" },
  
    Rejected promise returned by test. Reason:
  
    TypeError {
      message: 'The value cannot be converted because it is not an integer.',
    }

https://github.com/cloudflare/workers-sdk/blob/main/packages/miniflare/test/index.spec.ts#L1177

https://github.com/cloudflare/workers-sdk/blob/main/packages/miniflare/test/index.spec.ts#L19

https://github.com/cloudflare/workers-sdk/blob/main/packages/miniflare/test/index.spec.ts#L1156

@irvinebroque
Copy link
Contributor

Tests fail without the Miniflare changes, just when bumping workerd to latest.

@irvinebroque
Copy link
Contributor

Believe cloudflare/workerd@5a4beaa is the only Queues related commit that went out in latest workerd release.

@sesteves — not totally sure if cause, but wanted to flag

@mrbbot
Copy link
Contributor

mrbbot commented Mar 31, 2024

#5455 should bump workerd and address the failures we're seeing here. Once that's merged, we can rebase this PR on top of that. 👍

.changeset/hot-geese-deliver.md Outdated Show resolved Hide resolved
@@ -117,6 +121,7 @@ export interface ConfigBundle {
upstreamProtocol: "http" | "https";
inspect: boolean;
serviceBindings: Record<string, (_request: Request) => Promise<Response>>;
wrappedBindings?: Record<string, object>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wrappedBindings?: Record<string, object>;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -262,6 +267,7 @@ type MiniflareBindingsConfig = Pick<
| "queueConsumers"
| "name"
| "serviceBindings"
| "wrappedBindings"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| "wrappedBindings"

...but add this to the Pick for WorkerOptionsBindings above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 380 to 382
if (!config.wrappedBindings) {
config.wrappedBindings = {};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we shouldn't be mutating the config object passed in here. Instead, I would construct an empty wrappedBindings object above this if block with the type WorkerOptions["wrappedBindings"], then modify that here if AI is enabled:

const wrappedBindings: WorkerOptions["wrappedBindings"] = {};
if (bindings.ai?.binding) {
  // ...
  wrappedBindings[bindings.ai.binding] = {
	  scriptName: EXTERNAL_AI_WORKER_NAME,
	};
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed and done

@G4brym G4brym force-pushed the improve-workers-ai-binding branch from fe1b31d to 60b392c Compare March 31, 2024 11:22
@G4brym G4brym force-pushed the improve-workers-ai-binding branch from 60b392c to a774a97 Compare March 31, 2024 11:34
Copy link

codecov bot commented Mar 31, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 72.27%. Comparing base (d994066) to head (c49bf87).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5371      +/-   ##
==========================================
+ Coverage   72.15%   72.27%   +0.12%     
==========================================
  Files         326      326              
  Lines       16848    16853       +5     
  Branches     4311     4311              
==========================================
+ Hits        12156    12180      +24     
+ Misses       4692     4673      -19     
Files Coverage Δ
packages/wrangler/src/ai/fetcher.ts 35.71% <66.66%> (+10.71%) ⬆️
...es/wrangler/src/api/integrations/platform/index.ts 16.66% <0.00%> (ø)
packages/wrangler/src/dev/miniflare.ts 63.43% <66.66%> (+1.82%) ⬆️

... and 8 files with indirect coverage changes

@G4brym
Copy link
Member Author

G4brym commented Mar 31, 2024

Thanks, i've rebased the pr, implemented the suggestions and tests are now passing 👍

Co-authored-by: MrBBot <me@mrbbot.dev>
@G4brym G4brym force-pushed the improve-workers-ai-binding branch from 301eb02 to c49bf87 Compare March 31, 2024 12:23
@G4brym
Copy link
Member Author

G4brym commented Mar 31, 2024

Hey @mrbbot i don't have permissions to merge this pr, can you merge it please?

@mrbbot mrbbot merged commit 77152f3 into cloudflare:main Apr 1, 2024
17 checks passed
@mrbbot mrbbot added the ldw-exempt PRs marked with this label can be included in releases during LDW. label Apr 1, 2024
@workers-devprod workers-devprod mentioned this pull request Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ldw-exempt PRs marked with this label can be included in releases during LDW.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants