feat: add preview vector index client #710

pgautier404 · 2023-08-11T17:44:55Z

This commit adds a preview vector index client to the Web SDK. The client currently only implements data plane operations including create, list, and delete index.

examples/web/vector-index/doc-example-files/doc-examples-web-apis.ts

cprice404

overall looking good. exciting that the tests pass. I had one question/nit about the Configuration object, and also would like Michael to weigh in with a quick API review, especially comparing/contrasting the response namespacing pattern that he did in python vs. what we should do here.

packages/client-sdk-web/src/vector-client-props.ts

cprice404 · 2023-08-24T21:10:29Z

packages/client-sdk-web/src/index.ts

+// VectorClient Response Types
+import * as CreateIndex from '@gomomento/sdk-core/dist/src/messages/responses/create-index';
+import * as ListIndexes from '@gomomento/sdk-core/dist/src/messages/responses/list-indexes';
+import * as DeleteIndex from '@gomomento/sdk-core/dist/src/messages/responses/delete-index';


Michael I believe that you decided we wanted to start namespacing these a bit more in python. Do you have thoughts on what we should do here?

cprice404 · 2023-08-24T22:40:48Z

packages/client-sdk-web/src/config/vector-configuration.ts

+  transportStrategy: TransportStrategy;
+}
+
+export class VectorConfiguration implements Configuration {


will we need these in nodejs SDK too? or does your PR not wire up node.js SDK yet?

I'm not doing anything on the node side in this PR. This is specifically for Web SDK and console.

do you have a sense of how much extra work it would be? I presumed it would be pretty small, and that we would knock them both out together (in fact I'd kind of prefer that we establish a pattern of always adding stuff to both at the same time and not letting them drift from one another).

not a blocker for this PR but if it seems easy and you have bandwidth maybe you could file a follow-up PR before you split.

cprice404

still would really like @malandis to skim this if possible but otherwise I'm +1

malandis

Looks good just one inconsistency vs the protos

packages/core/src/messages/responses/list-indexes.ts

Co-authored-by: Michael Landis <[email protected]>

cprice404 · 2023-08-25T19:27:38Z

@malandis what about the namespacing for the response objects? Do you have thoughts on how we should handle that here for symmetry with what you did in Python?

@pgautier404 you might want to look at the python PR to see what they are doing there. I think the goal is to stop dumping all the response classes into one big namespace and instead have a separate namespace for Vector vs Cache etc.

pgautier404 · 2023-08-25T22:34:55Z

@malandis what about the namespacing for the response objects? Do you have thoughts on how we should handle that here for symmetry with what you did in Python?

@pgautier404 you might want to look at the python PR to see what they are doing there. I think the goal is to stop dumping all the response classes into one big namespace and instead have a separate namespace for Vector vs Cache etc.

In the Javascript SDKs, there is basically no namespacing whatsoever of classes we export. Every symbol that is exposed to developers is exposed through the root index.ts of each SDK. All clients, configs, responses, loggers, and so on are imported from the root. For example, the imports in examples/nodejs/cache/advanced.ts are:

import {
  CacheGet,
  ListCaches,
  CreateCache,
  CacheSet,
  CacheDelete,
  CacheClient,
  Configurations,
  MomentoLoggerFactory,
  DefaultMomentoLoggerFactory,
  DefaultMomentoLoggerLevel,
  ExperimentalMetricsCsvMiddleware,
  ExperimentalRequestLoggingMiddleware,
  CredentialProvider,
} from '@gomomento/sdk';

I don't really see how we could introduce a namespace for the vector index responses that wasn't inherently confusing to users who are importing every other class from the top level. Not to say there isn't a way to introduce namespaced responses in a UX-friendly way. I am not strong enough with TS to have any bright ideas here though.

cprice404 · 2023-08-29T16:20:04Z

discussed this with Michael; there are a few changes I want to get in here before this gets merged.

…nto a namespace This commit attempts to put a little more intentional organizational structure in place for the vector response types. This diverges a bit from the way the other response types are organized, but we should be able to move the other response types into a structure like this in the future, while still supporting the old import paths for backward compat. I'm not quite satisfied with this yet because I'd like to have a way to optionally import the types without the `vector.` prefix, so I will follow up with another commit to address that.

In the previous commit I had grouped the vector response types into a `vector` namespace, hoping that there would be an easy way to import the classes directly from within that. However, I couldn't find one, so this commit also exports them at the top level of the index so that users can choose whether they want the `vector.` prefix or not.

cprice404 · 2023-08-30T17:01:13Z

@malandis I'm done with this and ready to merge it if you are okay with my changes.

I tried a few things for namespacing the vector responses. I was able to separate them a little more cleanly in the file tree, and simplify the export statements in the index files, but I wasn't able to do what I really wanted to do, which was this:

#784

I think that's too risky for a 1.x. So my current thinking is:

The control plane responses in this PR now have Vector in their names
Users who want better namespacing can opt to import vector from the main index, and then reference these as vector.*, but
The types are also available from the main index for consistency / simplicity.
The data plane response types (coming in the next few PRs) will all be prefixed with Vector*, like the current Cache ones are prefixed with Cache*.

LMK if you are okay with that.

malandis · 2023-08-30T17:07:21Z

That's fine with me. My only concern was some of the names can get unwieldy/long, eg VectorAddItemBatchResponse, VectorAddItemBatch.Success. If that's actually not bad then works for me.

cprice404 · 2023-08-30T18:19:04Z

That's fine with me. My only concern was some of the names can get unwieldy/long, eg VectorAddItemBatchResponse, VectorAddItemBatch.Success. If that's actually not bad then works for me.

I don't see a better option at the moment and I'm hoping that with autocomplete those won't be too bad. I don't think they are meaningfully longer than the existing names for the Cache response classes.

malandis

* feat: adding preview vector index client Co-authored-by: Michael Landis <[email protected]> Co-authored-by: Chris Price <[email protected]>

pgautier404 force-pushed the add-vector-index-client branch from 234a141 to 846712f Compare August 14, 2023 20:40

pgautier404 added 10 commits August 23, 2023 16:01

feat: adding preview vector index client

0542259

chore: adding tests

381bc5e

fix: fix incorrect log message:

07ba769

fix: cleanup

eedcff7

fix: remove staged test function that is not actually used

678c035

fix: stop casting str to str

4dec932

doc: adding API examples for docs

e67aab2

chore: update wire types version

e40cde7

chore: update method name

5889b61

chore: regen package locks

d1eb54b

pgautier404 force-pushed the add-vector-index-client branch from 99756cb to d1eb54b Compare August 23, 2023 23:13

pgautier404 added 2 commits August 23, 2023 16:26

chore: try downgrading wire types

af57eff

chore: enable vector index control plane tests

5f20de4

pgautier404 requested review from cprice404, bruuuuuuuce, rishtigupta and anitarua August 24, 2023 18:41

pgautier404 marked this pull request as ready for review August 24, 2023 18:42

anitarua reviewed Aug 24, 2023

View reviewed changes

examples/web/vector-index/doc-example-files/doc-examples-web-apis.ts Outdated Show resolved Hide resolved

fix: fix incorrect log message

e6d4e4a

pgautier404 requested a review from anitarua August 24, 2023 20:48

cprice404 reviewed Aug 24, 2023

View reviewed changes

chore: split out vector-specific configuration classes

0cc099d

pgautier404 requested review from cprice404 and malandis August 24, 2023 22:33

cprice404 reviewed Aug 24, 2023

View reviewed changes

pgautier404 requested a review from cprice404 August 24, 2023 22:43

cprice404 previously approved these changes Aug 24, 2023

View reviewed changes

malandis reviewed Aug 25, 2023

View reviewed changes

packages/core/src/messages/responses/list-indexes.ts Outdated Show resolved Hide resolved

pgautier404 dismissed cprice404’s stale review via 73e7c99 August 25, 2023 16:55

Update accessor for listIndexes to agree with protos

0a9db39

Co-authored-by: Michael Landis <[email protected]>

pgautier404 force-pushed the add-vector-index-client branch from 73e7c99 to 0a9db39 Compare August 25, 2023 17:31

pgautier404 requested review from malandis and cprice404 August 25, 2023 19:28

chore: fix accessor method name

fb4853c

pgautier404 force-pushed the add-vector-index-client branch from a0b7152 to fb4853c Compare August 25, 2023 19:33

cprice404 added 3 commits August 30, 2023 05:53

chore: export vector response types as top level exports from web sdk

ff00106

malandis approved these changes Aug 30, 2023

View reviewed changes

cprice404 merged commit f1721cc into main Aug 30, 2023

cprice404 deleted the add-vector-index-client branch August 30, 2023 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add preview vector index client #710

feat: add preview vector index client #710

pgautier404 commented Aug 11, 2023 •

edited

Loading

cprice404 left a comment

cprice404 Aug 24, 2023

cprice404 Aug 24, 2023

pgautier404 Aug 24, 2023

cprice404 Aug 24, 2023

cprice404 left a comment

malandis left a comment

cprice404 commented Aug 25, 2023

pgautier404 commented Aug 25, 2023 •

edited

Loading

cprice404 commented Aug 29, 2023

cprice404 commented Aug 30, 2023

malandis commented Aug 30, 2023 •

edited

Loading

cprice404 commented Aug 30, 2023

malandis left a comment

feat: add preview vector index client #710

feat: add preview vector index client #710

Conversation

pgautier404 commented Aug 11, 2023 • edited Loading

cprice404 left a comment

Choose a reason for hiding this comment

cprice404 Aug 24, 2023

Choose a reason for hiding this comment

cprice404 Aug 24, 2023

Choose a reason for hiding this comment

pgautier404 Aug 24, 2023

Choose a reason for hiding this comment

cprice404 Aug 24, 2023

Choose a reason for hiding this comment

cprice404 left a comment

Choose a reason for hiding this comment

malandis left a comment

Choose a reason for hiding this comment

cprice404 commented Aug 25, 2023

pgautier404 commented Aug 25, 2023 • edited Loading

cprice404 commented Aug 29, 2023

cprice404 commented Aug 30, 2023

malandis commented Aug 30, 2023 • edited Loading

cprice404 commented Aug 30, 2023

malandis left a comment

Choose a reason for hiding this comment

pgautier404 commented Aug 11, 2023 •

edited

Loading

pgautier404 commented Aug 25, 2023 •

edited

Loading

malandis commented Aug 30, 2023 •

edited

Loading