Skip to content

VS-197: Enable wrangler to interact with Vectorize V2 indexes#6252

Merged
RamIdeas merged 1 commit intocloudflare:mainfrom
garvit-gupta:garvit/VS-197/vectorize_v2
Aug 12, 2024
Merged

VS-197: Enable wrangler to interact with Vectorize V2 indexes#6252
RamIdeas merged 1 commit intocloudflare:mainfrom
garvit-gupta:garvit/VS-197/vectorize_v2

Conversation

@garvit-gupta
Copy link
Copy Markdown
Contributor

@garvit-gupta garvit-gupta commented Jul 11, 2024

What this PR solves / how to test

Enables users to operate on Vectorize V2 operations via wrangler.

Fixes #[VS-197].

Author has addressed the following

  • Tests
    • TODO (before merge)
    • Included
    • Not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required / Maybe required
    • Not required because:
  • Changeset (Changeset guidelines)
    • TODO (before merge)
    • Included
    • Not necessary because:
  • Public documentation

@garvit-gupta garvit-gupta requested a review from a team as a code owner July 11, 2024 18:45
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jul 11, 2024

🦋 Changeset detected

Latest commit: a7a4f39

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

Copy link
Copy Markdown
Contributor

@ndisidore ndisidore left a comment

Choose a reason for hiding this comment

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

Looks great!

@garvit-gupta garvit-gupta force-pushed the garvit/VS-197/vectorize_v2 branch from 56dbe68 to 66bfefa Compare July 15, 2024 15:51
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jul 15, 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/10209363038/npm-package-wrangler-6252

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

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

Or you can use npx with this latest build directly:

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

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


wrangler@3.68.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240725.0
workerd 1.20240725.0 1.20240725.0
workerd --version 1.20240725.0 2024-07-25

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

Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

I was a bit confused by the use of v2 at first.
I think what you are saying is that the v2 flag effectively provides a brand new product from v1, right?
For example it seems that you can create v1 and v2 indexes with the same name and then interact with them separately?
If not then what would it mean to create a v1 index and then use v2 commands to interact with it?

Can you clarify this in the command docs?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"wrangler": patch
"wrangler": minor

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Vectorize V1 is an open-beta product, and Vectorize V2 is a re-architectured system that will soon go beta and eventually GA (at which point we will deprecate Vectorize v1).

Vectorize V2 is essentially a different product. V2 operations can only work on a v2 index (created using the use-v2 flag) and the same applies for V1. However, we cannot create a v2 index if a v1 index of the same name exists and vice versa.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the changeset patch to minor, and made the v2 option description in the commands more descriptive. @petebacondarwin I hope that would be sufficient.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does v2 mean when it comes to indexes?
What would happen if test-index was not v2?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we try a v2 operation (say get index) on a v1 index, the wrangler client would receive a 403 response from Vectorize. Same goes if we try a v1 operation on a v2 index.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What would be the error message. Will it be clear to the user what went wrong and what to do to fix it?
Also what is the roll off plan for deprecating and removing v1?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For example, for how long will you support use-v2 as a command line flag?
In general we are moving towards feature flagging things in Wrangler with the experimental/x prefix. So in this case it would be

wrangler vectorize get test-index --experimental-v2

or

wrangler vectorize get test-index --x-v2

And for stuff you are removing prefixing with legacy/deprecated. For example:

wrangler vectorize get test-index --legacy-v1

or

wrangler vectorize get test-index --deprecated-v1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The error message in the apiv4 response format would be like Index: "${index_name}" with version "${index_version}" called with api version "${operation_version}" along with a 403 response.

I would let Yevgen address the deprecation plan.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For example, for how long will you support use-v2 as a command line flag? In general we are moving towards feature flagging things in Wrangler with the experimental/x prefix. So in this case it would be

wrangler vectorize get test-index --experimental-v2

or

wrangler vectorize get test-index --x-v2

And for stuff you are removing prefixing with legacy/deprecated. For example:

wrangler vectorize get test-index --legacy-v1

or

wrangler vectorize get test-index --deprecated-v1

@petebacondarwin the plan to start with private beta this month, once we will be confident to open it up for public beta sometime in August we will stop allowing V1 index creation and V2 only will be used for new indexes.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like the idea of using --deprecated-v1 for dealing with existing V1 indexes once V2 goes to public beta.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We discussed this within our team, and decided to with an --experimental-v2 flag with a default value of false until we put v1 indexes on the deprecation path. At that time we will switch to --deprecated-v1.

Made the change to update the commands and the related docs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We discussed this within our team, and decided to with an --experimental-v2 flag with a default value of false until we put v1 indexes on the deprecation path. At that time we will switch to --deprecated-v1.

Made the change to update the commands and the related docs.

This is not what I see in the current code for this PR.
I see that there is no --experimental-v2 flag here only the --deprecated-v1 flag, which defaults to false.

I would assume that for the private beta the default needs to switch to true and the --experimental-v2 flag needs to be implemented?

Also, could I call out that it would be much nicer for the user if these commands inferred the version of the index from its type (via a REST request) rather than requiring the user to keep track of which version they are using. The only place one really needs the v1/v2 flag is at index creation time, no?

@garvit-gupta garvit-gupta force-pushed the garvit/VS-197/vectorize_v2 branch from 66bfefa to 6921707 Compare July 16, 2024 05:55
@threepointone
Copy link
Copy Markdown
Contributor

Is there a command that converts a v1 index to a v2 index?

@sejoker
Copy link
Copy Markdown

sejoker commented Jul 16, 2024

Is there a command that converts a v1 index to a v2 index?

This will be worked on later in the quarter, definitely something we will provide before GA.

@garvit-gupta garvit-gupta force-pushed the garvit/VS-197/vectorize_v2 branch from 6921707 to 8997c93 Compare July 16, 2024 23:21
@CarmenPopoviciu CarmenPopoviciu added the awaiting Cloudflare response Awaiting response from workers-sdk maintainer team label Jul 18, 2024
@CarmenPopoviciu
Copy link
Copy Markdown
Contributor

CarmenPopoviciu commented Jul 19, 2024

@garvit-gupta is my understanding correct that some backend work needs to happen before we can merge & release this PR? Are any further wrangler changes required or is the PR in its current state ready to go? Can you please let us know when we should revisit this PR to merge it 🙏 ?

@CarmenPopoviciu CarmenPopoviciu added awaiting reporter response Needs clarification or followup from OP and removed awaiting Cloudflare response Awaiting response from workers-sdk maintainer team labels Jul 19, 2024
@garvit-gupta garvit-gupta force-pushed the garvit/VS-197/vectorize_v2 branch from 8997c93 to 7fbe3e7 Compare July 19, 2024 20:00
@CarmenPopoviciu
Copy link
Copy Markdown
Contributor

spoke async with the team, and this is now ready for re-review

@CarmenPopoviciu CarmenPopoviciu removed the awaiting reporter response Needs clarification or followup from OP label Jul 22, 2024
@garvit-gupta garvit-gupta force-pushed the garvit/VS-197/vectorize_v2 branch 2 times, most recently from 88ce511 to 719cfad Compare July 23, 2024 06:34
@petebacondarwin petebacondarwin added awaiting reporter response Needs clarification or followup from OP vectorize Relating to vectorize blocked Blocked on other work and removed awaiting reporter response Needs clarification or followup from OP labels Jul 24, 2024
@petebacondarwin
Copy link
Copy Markdown
Contributor

As I understand it, this PR should only land when we V2 goes into public beta.

@garvit-gupta garvit-gupta force-pushed the garvit/VS-197/vectorize_v2 branch from 719cfad to 587abd8 Compare July 26, 2024 13:49
@garvit-gupta garvit-gupta requested review from netgusto and sejoker July 26, 2024 14:13
@garvit-gupta garvit-gupta requested a review from ndisidore July 26, 2024 14:13
Copy link
Copy Markdown
Contributor

@ndisidore ndisidore left a comment

Choose a reason for hiding this comment

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

This is looking great! I know there's some work you want to do around the query parse function. Happy to approve once we look at that

@garvit-gupta garvit-gupta force-pushed the garvit/VS-197/vectorize_v2 branch from 587abd8 to 04eb095 Compare July 26, 2024 19:29
@garvit-gupta
Copy link
Copy Markdown
Contributor Author

This is looking great! I know there's some work you want to do around the query parse function. Happy to approve once we look at that

The query filter parse and validate logic looks cleaner now. Ready for review!

@sejoker
Copy link
Copy Markdown

sejoker commented Jul 31, 2024

@petebacondarwin @CarmenPopoviciu could you please confirm this PR is ready to be merged from your perspective? Confirmed, we want to release this change once Vectorize V2 goes into public beta.

Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

What a massive PR! From what I can see this looks great.
Happy for this to be merged when it is unblocked for product release.
cc @RamIdeas - you might be interested in this contribution.

@petebacondarwin petebacondarwin changed the title VS-197: Enable wrangler to interact with Vectorize V2 indexes DO NOT MERGE YET: VS-197: Enable wrangler to interact with Vectorize V2 indexes Aug 1, 2024
@garvit-gupta garvit-gupta force-pushed the garvit/VS-197/vectorize_v2 branch from 04eb095 to 7a6a5d9 Compare August 2, 2024 02:55
@garvit-gupta garvit-gupta force-pushed the garvit/VS-197/vectorize_v2 branch from 7a6a5d9 to a7a4f39 Compare August 2, 2024 03:11
@sejoker
Copy link
Copy Markdown

sejoker commented Aug 9, 2024

@RamIdeas @petebacondarwin we are happy to merge the change and release it next week.

@RamIdeas RamIdeas changed the title DO NOT MERGE YET: VS-197: Enable wrangler to interact with Vectorize V2 indexes VS-197: Enable wrangler to interact with Vectorize V2 indexes Aug 9, 2024
@petebacondarwin petebacondarwin removed the blocked Blocked on other work label Aug 9, 2024
@RamIdeas RamIdeas merged commit a2a144c into cloudflare:main Aug 12, 2024
@workers-devprod workers-devprod added the contribution [Holopin] Recognizes an open-source contribution, big or small label Aug 12, 2024
@holopin-bot
Copy link
Copy Markdown

holopin-bot bot commented Aug 12, 2024

Congratulations @garvit-gupta, the maintainer of this repository has issued you a holobyte! Here it is: https://holopin.io/holobyte/clzr77nwc143120cleznufaycs

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution [Holopin] Recognizes an open-source contribution, big or small vectorize Relating to vectorize

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

9 participants