workers-sdk icon indicating copy to clipboard operation
workers-sdk copied to clipboard

VS-286 Support mixed-mode local bindings for Vectorize in Miniflare

Open garrettgu10 opened this issue 1 year ago • 11 comments

What this PR solves / how to test

Fixes VS-286.

Author has addressed the following

  • Tests
    • [ ] TODO (before merge)
    • [x] Tests included
    • [ ] Tests not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • [ ] I don't know
    • [x] Required
    • [ ] Not required because:
  • Changeset (Changeset guidelines)
    • [ ] TODO (before merge)
    • [x] Changeset included
    • [ ] Changeset not necessary because:
  • Public documentation
    • [ ] TODO (before merge)
    • [ ] Cloudflare docs PR(s):
    • [x] Documentation not necessary because: I don't think this limitation is mentioned anywhere in docs and it's generally expected that bindings work in local dev mode.

garrettgu10 avatar Oct 07 '24 19:10 garrettgu10

🦋 Changeset detected

Latest commit: 2559ae64cc68897f027809f2d2bcf35fcd7fc358

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

changeset-bot[bot] avatar Oct 07 '24 19:10 changeset-bot[bot]

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/11503159617/npm-package-wrangler-6916

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

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

Or you can use npx with this latest build directly:

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

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


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241022.0
workerd 1.20241022.0 1.20241022.0
workerd --version 1.20241022.0 2024-10-22

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

github-actions[bot] avatar Oct 07 '24 19:10 github-actions[bot]

What’s the reasoning behind disabling write in local mode? Doesn’t seem very useful without ability to write

cloudkite avatar Oct 08 '24 22:10 cloudkite

@cloudkite Both D1 and R2 operate on local instances that are separate from prod. Since we're directly connecting to the prod Vectorize instance instead, we definitely don't want to catch people off guard and make them cause changes in prod while testing "locally".

We're considering re-enabling write operations behind an optional flag, and I think that's probably the best option.

garrettgu10 avatar Oct 08 '24 22:10 garrettgu10

@garrettgu10 makes sense in terms of accidentally writing to a live db instance. However having trouble imaging how a readonly "local" db will be useful for development/testing purposes?

cloudkite avatar Oct 08 '24 22:10 cloudkite

@cloudkite we are not (yet) launching a local simulation database for vectorize. All requests are sent to the prod database in local dev mode.

garrettgu10 avatar Oct 08 '24 22:10 garrettgu10

@garrettgu10 yup I understood that part. I just meant if I can’t write I’ll still need to use —remote

How about when wrangler is in local mode it refuses to bind unless the index name has suffix of “-local-dev” or similar, that would make it very unlikely for someone to accidentally bind to their live/prod vectorize index

cloudkite avatar Oct 08 '24 23:10 cloudkite

@garrettgu10 yup I understood that part. I just meant if I can’t write I’ll still need to use —remote

How about when wrangler is in local mode it refuses to bind unless the index name has suffix of “-local-dev” or similar, that would make it very unlikely for someone to accidentally bind to their live/prod vectorize index

This is not the direction we want to go at the moment.

sejoker avatar Oct 09 '24 11:10 sejoker

Hey all! 👋 I'm excited to see support for Vectorize in a local mode, but I'm a little sad to see this isn't a fully-local implementation.

Whilst there's precedent for proxying to remote services with Workers AI, Vectorize is a stateful service. As @garrettgu10 mentioned, this encourages developers to reuse their production indices in development, which is nice in some scenarios, but contrasts to every other Cloudflare data store. This will undoubtedly cause confusion for users, and should ideally be part of a larger mixed-mode story. This confusion would be compounded if a fully-local implementation of Vectorize was built in the future, as you'd likely want to enable this by default to match other data stores.

Arguably the main reason local support for Workers AI is provided by a remote proxy is the large hardware requirements to run the models, both in terms of storage and compute. These requirements don't apply to Vectorize.

When building local simulators, the primary concern is correctly implementing the production semantics, not performance. The size of datasets is significantly smaller in local mode. This enables you to build much simpler implementations of the services. It should be possible to build a Vectorize simulator with brute force vector similarity on top of the SQLite API provided by Durable Objects, similar to the other simulators. You could explore enabling an SQLite vector index extension in workerd only to improve performance if needed.

A fully-local simulator would also allow Vectorize to be used with the Workers Vitest pool.

I know I don't work at Cloudflare anymore and I'm sure there's discussions about this internally, but I hope you consider these points. Again, I'm very excited to see local mode development, I just want to make sure the user experience is consistent. 🙂

mrbbot avatar Oct 09 '24 19:10 mrbbot

@mrbbot Thanks for your comments, they've been super helpful internally and we've discussed them at length. I agree that a local simulator is the superior solution re. DX and seems fairly simple to implement as a first-pass, but the team doesn't have the bandwidth for maintaining such a solution at the moment, esp. wrt. feature-parity with updates to the service.

With that in mind, I think the best compromise is to allow users to bind to production indexes through an optional flag such as --experimental-vectorize-bind-to-production. That way, users must explicitly opt-in, minimizing possibility for confusion. Then, sessions launched without the flag will have no Vectorize bindings until we're ready to launch a local simulator. That's the approach we're going with for now.

garrettgu10 avatar Oct 10 '24 18:10 garrettgu10

With that in mind, I think the best compromise is to allow users to bind to production indexes through an optional flag such as --experimental-vectorize-bind-to-production. That way, users must explicitly opt-in, minimizing possibility for confusion. Then, sessions launched without the flag will have no Vectorize bindings until we're ready to launch a local simulator. That's the approach we're going with for now.

@garrettgu10 This feels like a pretty big break from how other bindings are implemented, and I'd be concerned about adding an --experimental-vectorize-bind-to-production flag without a larger discussion of how that fits in to other bindings. This PR looks relatively reasonable as a mixed mode implementation for Vectorize (other than the write limitations, which I touched on in another comment), but I'd been assuming that there were fundamental limitations that required mixed mode for Vectorize (similar to Workers AI & Browser bindings, both of which require large downloads/specific hardware).

If there aren't any fundamental limits on getting Vectorize working locally, would it be possible to revisit that discussion? Do you have a link to a decision doc or anything around this?

penalosa avatar Oct 14 '24 14:10 penalosa