[v1] Implement collection operations #70

jhamon · 2023-06-25T23:57:13Z

Problem

Similar to recent PRs #66 and #67, this PR is adding new functionality to the revamped node client. Specifically:

createCollection
describeCollection
listCollections
deleteCollection

Solution

More or less followed the approach used in #66 and #67 to provide simpler arguments, runtime argument validation, and improved error handling.

Type of Change

New feature (non-breaking change which adds functionality)

Test Plan

Added new unit tests covering these commands.
Manual testing with npm run repl

Creating a collection

Validating arguments

> await client.createCollection()
Uncaught:
PineconeArgumentError: Argument to createCollection has a problem. The argument must be object.
    at /Users/jhamon/workspace/pinecone-ts-client/dist/validator.js:45:15
    at /Users/jhamon/workspace/pinecone-ts-client/dist/validator.js:36:13
    at /Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:56:21
    at step (/Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:33:23)
    at Object.next (/Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:14:53)
    at /Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:8:71
    at new Promise (<anonymous>)
    at __awaiter (/Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:4:12)
    at Client.createCollection (/Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:51:40)
> await client.createCollection({})
Uncaught:
PineconeArgumentError: Argument to createCollection has a problem. The argument must have required property 'name'. The argument must have required property 'source'.
    at /Users/jhamon/workspace/pinecone-ts-client/dist/validator.js:45:15
    at /Users/jhamon/workspace/pinecone-ts-client/dist/validator.js:36:13
    at /Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:56:21
    at step (/Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:33:23)
    at Object.next (/Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:14:53)
    at /Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:8:71
    at new Promise (<anonymous>)
    at __awaiter (/Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:4:12)
    at Client.createCollection (/Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:51:40)
> await client.createCollection({ name: 'baz-collection', source: 'baz' })
Uncaught PineconeBadRequestError: source database baz does not exist
    at mapHttpStatusError (/Users/jhamon/workspace/pinecone-ts-client/dist/errors/http.js:127:20)
    at /Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:88:63
    at step (/Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:33:23)

bad request

> await client.createCollection({ name: 'my-collection', source: 'asdf' })
Uncaught PineconeBadRequestError: source database asdf does not exist
    at mapHttpStatusError (/Users/jhamon/workspace/pinecone-ts-client/dist/errors/http.js:127:20)
    at /Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:88:63
    at step (/Users/jhamon/workspace/pinecone-ts-client/dist/control/createCollection.js:33:23)

happy path

> await client.listIndexes()
[ 'bar', 'foofoo', 'jen-numpy-test' ]
> await client.createCollection({ name: 'bar-collection', source: 'bar' })
undefined

describeCollection

Validations

> await client.describeCollection()
Uncaught:
PineconeArgumentError: Argument to describeCollection has a problem. The argument must be string.
    at /Users/jhamon/workspace/pinecone-ts-client/dist/validator.js:45:15
    at /Users/jhamon/workspace/pinecone-ts-client/dist/validator.js:36:13
    at /Users/jhamon/workspace/pinecone-ts-client/dist/control/describeCollection.js:78:21
    at step (/Users/jhamon/workspace/pinecone-ts-client/dist/control/describeCollection.js:44:23)
    at Object.next (/Users/jhamon/workspace/pinecone-ts-client/dist/control/describeCollection.js:25:53)
    at /Users/jhamon/workspace/pinecone-ts-client/dist/control/describeCollection.js:19:71
    at new Promise (<anonymous>)
    at __awaiter (/Users/jhamon/workspace/pinecone-ts-client/dist/control/describeCollection.js:15:12)
    at Client.describeCollection (/Users/jhamon/workspace/pinecone-ts-client/dist/control/describeCollection.js:73:37)

bad request

> await client.describeCollection('asdf')
Uncaught:
PineconeNotFoundError: Collection 'asdf' does not exist. Valid collection names: []
    at mapHttpStatusError (/Users/jhamon/workspace/pinecone-ts-client/dist/errors/http.js:131:20)
    at /Users/jhamon/workspace/pinecone-ts-client/dist/control/describeCollection.js:98:63
    at step (/Users/jhamon/workspace/pinecone-ts-client/dist/control/describeCollection.js:44:23)

happy path

> await client.describeCollection('bar-collection')
{ name: 'bar-collection', size: undefined, status: 'Initializing' }
> await client.describeCollection('bar-collection')
{ name: 'bar-collection', size: 3088261, status: 'Ready' }

listCollection

> await client.listCollections()
[ 'bar-collection' ]

deleteCollection

> await client.deleteCollection('foo-collection')
Uncaught:
PineconeNotFoundError: Collection 'foo-collection' does not exist. Valid collection names: ['bar-collection']
    at mapHttpStatusError (/Users/jhamon/workspace/pinecone-ts-client/dist/errors/http.js:131:20)
    at /Users/jhamon/workspace/pinecone-ts-client/dist/control/deleteCollection.js:82:63
    at step (/Users/jhamon/workspace/pinecone-ts-client/dist/control/deleteCollection.js:44:23)
> await client.deleteCollection('bar-collection')
undefined
> await client.listCollections()
[]
>

micmmakarov

Like to all these beautiful tests

iamannamai

Nice work! Big fan of the attention to DevX. Still using PRs as a way of getting up to speed, nothing majorly blocking 🚀

iamannamai · 2023-06-27T13:32:25Z

src/control/listCollections.ts

+      const listCollectionsError = e as ResponseError;
+      throw mapHttpStatusError({
+        status: listCollectionsError.response.status,
+        url: listCollectionsError.response.url,
+        message: await listCollectionsError.response.text(),
+      });


I'd conveniently forgotten that JS error catching is... so rough.

nit Would it help to be a bit defensive here and do an instanceof ResponseError check in case this isn't for whatever reason a ResponseError?

That's a good thought. I will add a follow-up task to my todo list.

iamannamai · 2023-06-27T13:33:20Z

src/control/listCollections.ts

+export const listCollections = (api: IndexOperationsApi) => {
+  return async (): Promise<CollectionList> => {
+    try {
+      return await api.listCollections();


nit I think you can just return api.listCollections() if the intent is to return a promise, since you'd have to await on the other end anyway, right? Same for anywhere you return await.

iamannamai · 2023-06-27T13:34:27Z

src/control/index.ts

+export { listCollections } from './listCollections';
+export { deleteCollection } from './deleteCollection';
+
+export type { CollectionName, CollectionList } from './listCollections';


iamannamai · 2023-06-27T13:35:10Z

src/control/describeCollection.ts

+// If user passes the empty string for collection name, the generated
+// OpenAPI client will call /databases/ which is the list
+// collection endpoint. This returns 200 instead of 404, but obviously
+// no descriptive information is returned for an collection named empty
+// string. To avoid this confusing case, we require lenth > 1.


iamannamai · 2023-06-27T13:37:56Z

src/control/describeCollection.ts

+const DescribeCollectionSchema = Type.String({ minLength: 1 });
+
+export type CollectionName = Static<typeof DescribeCollectionSchema>;


Very cool! I know @zaeemadamjee mentioned he was looking at zod for something on the console-side. Probably not too important to do now, but think it might be nice to align on a single tool at some point

iamannamai · 2023-06-27T13:41:00Z

src/control/describeCollection.ts

+  const removeDeprecatedFields = (result: any) => {
+    if (result.database) {
+      for (const key of Object.keys(result.database)) {
+        if (result.database[key] === undefined) {


question Is it always going to be true that an undefined value is always deprecated? I'm wondering if it's safer to allow/deny-list fields and build a new object.

Yeah, it's on my todo list to dig into this question more. Probably for the long term we want some way to distinguish deprecated from undefined.

iamannamai · 2023-06-27T13:46:05Z

src/control/deleteCollection.ts

+
+    try {
+      await api.deleteCollection({ collectionName: collectionName });
+      return;


thought I think I'm elixir-braining this a bit so you can ignore if you want--Do we want to pass along some kind of indicator of success or is undefined standard practice?

The overall return value here is Promise<void> which I think makes sense. I considered this question when I was implementing and couldn't think of anything very meaningful to return; on success the underlying generated openapi client just returns '' when deleting which I don't think adds any information and can actually be misleading because '' is a falsy value even though it succeeded.

If the request fails, this will throw and the promise will be rejected. Some APIs echo back the name/id of the thing that was deleted or a delete count when deleting multiple records, but our API doesn't return those today and to me it doesn't seem that useful anyway. As a caller, if I ask an api to return collection 'foo' and it says it succeeded, I already know everything I need to know about the state of the world.

iamannamai · 2023-06-27T13:49:38Z

src/control/createCollection.ts

+  source: nonemptyString,
+});
+
+export type CreateCollectionOptions = Static<


LOL, line length limits.

Yeah, the line length formatting has honestly been a big pain because // @ts-ignore doesn't work if things break across two lines and a lot of my test cases are specifically passing params that violate the typescript expectations to test method validation. Thinking about disabling that formatting rule.

iamannamai · 2023-06-27T13:58:02Z

src/control/__tests__/createCollection.test.ts

+import { IndexOperationsApi } from '../../pinecone-generated-ts-fetch';
+import type { CreateCollectionOperationRequest as CCOR } from '../../pinecone-generated-ts-fetch';
+
+const setOpenAPIResponse = (fakeCreateCollectionResponse) => {


suggestion For the future, I think potentially a better way to do this would mock of IOA could be to use jest manual mocks but I think typing this could be hellish too...

jhamon marked this pull request as ready for review June 26, 2023 01:01

jhamon requested review from micmmakarov and iamannamai June 26, 2023 18:59

micmmakarov approved these changes Jun 26, 2023

View reviewed changes

iamannamai approved these changes Jun 27, 2023

View reviewed changes

jhamon added 4 commits June 27, 2023 19:58

Update client.ts with docstrings for collection actions

f3e8358

Implement create, delete, describe collection

6db062c

List collections

ee2e7be

Lint fixes

ace0c53

jhamon force-pushed the jhamon/collections branch from d511d07 to ace0c53 Compare June 27, 2023 23:58

Lint fixes

8b715d9

jhamon merged commit ad9f894 into main Jun 28, 2023

jhamon deleted the jhamon/collections branch June 28, 2023 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v1] Implement collection operations #70

[v1] Implement collection operations #70

jhamon commented Jun 25, 2023 •

edited

Loading

micmmakarov left a comment

iamannamai left a comment

iamannamai Jun 27, 2023

jhamon Jun 28, 2023

iamannamai Jun 27, 2023

iamannamai Jun 27, 2023

iamannamai Jun 27, 2023

iamannamai Jun 27, 2023

iamannamai Jun 27, 2023

jhamon Jun 28, 2023

iamannamai Jun 27, 2023

jhamon Jun 28, 2023

iamannamai Jun 27, 2023

jhamon Jun 28, 2023

iamannamai Jun 27, 2023

		const DescribeCollectionSchema = Type.String({ minLength: 1 });

		export type CollectionName = Static<typeof DescribeCollectionSchema>;

[v1] Implement collection operations #70

[v1] Implement collection operations #70

Conversation

jhamon commented Jun 25, 2023 • edited Loading

Problem

Solution

Type of Change

Test Plan

Creating a collection

Validating arguments

bad request

happy path

describeCollection

Validations

bad request

happy path

listCollection

deleteCollection

micmmakarov left a comment

Choose a reason for hiding this comment

iamannamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhamon commented Jun 25, 2023 •

edited

Loading