Add new CreateServerlessIndex method #94

aulorbe · 2024-04-05T00:12:42Z

Problem

It's currently cumbersome to force users to create CreateIndexRequest object, a ServerlessSpec object, and the required enums (IndexMetric and CloudEnum.

Solution

This PR is a start on the road towards removing the createIndex method.

In this PR, you will find a new method called CreateServerlessIndex. This is what users will use to create a serverless index. All they have to do is pass strings/ints for indexName, metric, dimension, cloud, and region. All validation of the inputs, transforms into enum objects, and construction of the Request objects takes place within the function itself.

Note:

From reading and asking ChatGPT, it seems that it's more idiomatic in Java to force users to construct the enums (necessary for metric and cloud), rather than pass in strings, since constructing enums directly is more type-safe and self-documenting. However, I think ease-of-use trumps those concerns, since we are building these SDKs primarily to increase developer experience.

LMK if you agree in the comments!

Asana ticket: https://app.asana.com/0/1203260648987893/1206510790411726/f

Follow-ups

Next up will be doing the same thing, but for a new CreatePodsIndex method (ticket)
After that, I will remove CreateIndex from the client altogether, updating unit tests and integration tests as necessary.
- Within this PR ^, I will also make the necessary documentation updates to Rohan's README, located here.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Infrastructure change (CI configs, etc)
Non-code change (docs, etc)
None of the above: (explain here)

Test Plan

Unit test & integration tests included in PR.

aulorbe · 2024-04-05T21:34:58Z

...va/io/pinecone/integration/controlPlane/serverless/CreateDescribeListAndDeleteIndexTest.java

    // Serverless currently has limited availability in specific regions, hard-code us-west-2 for now
    private static final String serverlessRegion = "us-west-2";
+    private static final Pinecone controlPlaneClient = new Pinecone.Builder(System.getenv("PINECONE_API_KEY")).build();


Just a lint; ignore

ssmith-pc

Looks good! I like the direct/action oriented/semantics.

Couple comments throughout, LMK if you have questions!

ssmith-pc · 2024-04-05T21:58:59Z

src/main/java/io/pinecone/clients/Pinecone.java

+    public <E extends Enum<E>> boolean validateEnums(String stringToValidate, List<E> validEnums) {
+        return validEnums.stream()
+                .anyMatch(validEnum -> Objects.equals(stringToValidate, validEnum.toString()));
+    }


Looks like there is a method in these enums called fromValue to convert a string into its equivalent enum, and I see you using that below post-validation. Is there a reason you can't use it here?

IndexMetric metric = IndexMetric.fromValue("cosine");

Ahh yes, thank you so much! Removing validation method now :)

ssmith-pc · 2024-04-05T22:01:52Z

src/main/java/io/pinecone/clients/Pinecone.java

+        }
+        List<IndexMetric> indexMetricEnums = Arrays.asList(IndexMetric.values());
+        if (!validateEnums(metric, indexMetricEnums)) {
+            throw new PineconeValidationException(String.format("Metric must be one of %s", IndexMetric.values()));


I'm not sure this will stringify the contents of the array properly. You might need to use Arrays.toString:

......... String.format("Metric must be one of %s", Arrays.toString(IndexMetric.values())) ...........

oh wow, incredible catch, thank you!

ssmith-pc · 2024-04-05T22:03:04Z

src/main/java/io/pinecone/clients/Pinecone.java

@@ -35,6 +40,64 @@ public IndexModel createIndex(CreateIndexRequest createIndexRequest) throws Pine
        return indexModel;
    }

+    public <E extends Enum<E>> boolean validateEnums(String stringToValidate, List<E> validEnums) {


This method doesn't need to be public, I don't think? Also, I'd suggest moving it down towards the bottom of the class.

I actually had it w/o any access modifier previously, but my tests wouldn't run because my IDE said it had to be public.

Is there something special I need to do? I'm using IntelliJ and I've marked the dir as a 'test source root'...

ssmith-pc · 2024-04-05T22:03:42Z

src/main/java/io/pinecone/clients/Pinecone.java

+        if (dimension < 1) {
+            throw new PineconeValidationException("Dimension must be greater than 0");
+        }


Do we have an upper bound on dimension? I am not sure what we have in our other SDKs.

Technically the limit is 4.2B haha (for dense vectors). I don't see in the Python or Node SDKs, at least any upper limit enacted through code.

I'll add a link to our limits section, though. That'll be helpful for users.

ssmith-pc · 2024-04-05T22:04:18Z

src/main/java/io/pinecone/clients/Pinecone.java

+        if (cloud == null || cloud.isEmpty()) {
+            throw new PineconeValidationException("Cloud cannot be null or empty.");
+        }


Would be helpful to include the possible values, like in the error message below

ssmith-pc · 2024-04-05T22:09:56Z

src/main/java/io/pinecone/clients/Pinecone.java

+    public IndexModel createServerlessIndex(String indexName, String metric, int dimension, String cloud,
+                                            String region) {


I like these as simple string types for simplicity. Can we add javadoc to this method? I bet copilot will write most of it for you.

I might leave this to @rohanshah18 since I think he's been working on that for this SDK in general, but I'll confirm in Slack :)

Good callout though, yeah we need to add javadoc comments all over.

ssmith-pc

Looks great!

austin-denoble

Nice work, looks great. 🚢

austin-denoble · 2024-04-06T19:17:25Z

src/main/java/io/pinecone/clients/Pinecone.java

+    public IndexModel createServerlessIndex(String indexName, String metric, int dimension, String cloud,
+                                            String region) {


Good callout though, yeah we need to add javadoc comments all over.

austin-denoble · 2024-04-06T19:55:25Z

src/main/java/io/pinecone/clients/Pinecone.java

+        }
+
+        if (region == null || region.isEmpty()) {
+            throw new PineconeValidationException("Region cannot be null or empty.");


Really small nit I'm sorry - let's drop the period since we don't use one for any of the other thrown errors.

Ha! Don't be sorry at all. I love grammatical consistency :)

austin-denoble · 2024-04-06T19:55:59Z

src/test/java/io/pinecone/PineconeIndexOperationsTest.java

+        OkHttpClient mockClient = mock(OkHttpClient.class);
+        when(mockClient.newCall(any(Request.class))).thenReturn(mockCall);
+
+        Pinecone client = new Pinecone.Builder("testAPiKey").withOkHttpClient(mockClient).build();


Thanks for covering all these test cases!

aulorbe · 2024-04-08T19:22:57Z

Merging this w/the integration test failure b/c the failure is a known flake, as documented here https://www.notion.so/Integration-Tests-Error-Log-a422cf031ca44271a76d899324ed385d?pvs=4

## Problem It's currently cumbersome to make the user create a bunch of objects from our generated code, e.g. `CreateIndexRequest`, etc. just to create an index. ## Solution Piggybacking off of [my last PR](#94), this PR creates a new method called `CreatePodsIndex`. Together with `CreateServerlessIndex`, we can now deprecate `createIndex` altogether (in a forthcoming PR). The overloaded method signatures for the `createPodsIndex` methods are as follows. (Please lmk if you having feelings about these options. It was a bit of a crapshoot figuring out which I thought would be most useful to users.) - The minimal required parameters (note: `podType` is required by the backend, not our OpenAPI spec, FWIW): - String indexName - Integer dimension - String environment - String podType - Minimal + metric: - String indexName - Integer dimension - String environment - String podType - String metric - Minimal + metric + metadata: - String indexName - Integer dimension - String environment - String podType - String metric - PodSpecMetadataConfig metadataConfig - Minimal + metric + source collection - String indexName - Integer dimension - String environment - String podType - String metric - String sourceCollection - Minimal + pods - String indexName - Integer dimension - String environment - String podType - Integer pods - Minimal + pods + metadata: - String indexName - Integer dimension - String environment - String podType - Integer pods - PodSpecMetadataConfig metadataConfig - Minimal + replicas and shards - String indexName - Integer dimension - String environment - String podType - Integer replicas - Integer shards - Minimal + replicas, shards + metadata: - String indexName - Integer dimension - String environment - String podType - Integer replicas - Integer shards - PodSpecMetadataConfig metadataConfig - Max: - String indexName - Integer dimension - String environment - String podType - String metric - Integer replicas - Integer shards - Integer pods - PodSpecMetadataConfig metadataConfig - String sourceCollection ## Callouts: - When a user provides `pods`, they should not provide `shards` or `replicas`, and vice versa (unless in the "max" method) - If `replicas * shards` do not equal `pods`, an error is thrown - Like with `createServerlessIndex` and `cloud`, the user passes `metric` as a string, and it's internally validated against the `IndexMetric.Enum` types - Besides checking for nulls and empties, there are no checks for the actual values in `environment`, `podType`, or `sourceCollection`. ## Type of Change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [x] This change requires a documentation update - [ ] Infrastructure change (CI configs, etc) - [ ] Non-code change (docs, etc) - [ ] None of the above: (explain here) ## Test Plan Unit and integration tests included. Note: in the next PR I will deprecate `createIndex` altogether. Within this PR, I will update the `TestIndexResourcesManager` singleton to work with the new methods. I will also update the README, as per this [ticket](https://app.asana.com/0/1203260648987893/1206994598304493/f).

aulorbe added 2 commits April 5, 2024 12:16

Add new CreateServerlessIndex method + unit tests

ae52253

Finalize CreateServerlessIndex

53da9c9

aulorbe force-pushed the audrey/createIndex branch from 14c2bc2 to 53da9c9 Compare April 5, 2024 20:46

aulorbe added 2 commits April 5, 2024 13:47

Remove comment

1d412bf

Add integration tests for CreateServerlessIndex

0636408

aulorbe changed the title ~~[DRAFT] Add new CreateServerlessIndex method + unit tests~~ Add new CreateServerlessIndex method Apr 5, 2024

aulorbe requested review from austin-denoble and ssmith-pc April 5, 2024 21:10

aulorbe marked this pull request as ready for review April 5, 2024 21:10

Eek sorry, forgot to re-enable other integration tests

4b3734a

aulorbe commented Apr 5, 2024

View reviewed changes

ssmith-pc reviewed Apr 5, 2024

View reviewed changes

aulorbe added 3 commits April 5, 2024 16:05

Add link to limits section in dimension ValidationException

bc59fc9

Fix messages on Enum throws

b3b6957

Remove Enums validation method

5b84747

ssmith-pc approved these changes Apr 5, 2024

View reviewed changes

Fix one msg in integration tests

d761f65

austin-denoble approved these changes Apr 6, 2024

View reviewed changes

aulorbe added 3 commits April 8, 2024 11:10

Remove period for error msg consistency

96ba65f

Merge branch 'main' into audrey/createIndex

6d97640

Oops, did not remove period from unit test msgs

d5b565f

aulorbe merged commit 9326c72 into main Apr 8, 2024

aulorbe deleted the audrey/createIndex branch April 8, 2024 19:23

aulorbe mentioned this pull request Apr 9, 2024

Add new createPodsIndex method #98

Merged

7 tasks

		public IndexModel createServerlessIndex(String indexName, String metric, int dimension, String cloud,
		String region) {

Add new CreateServerlessIndex method #94

Add new CreateServerlessIndex method #94

Uh oh!

Conversation

aulorbe commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Follow-ups

Type of Change

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssmith-pc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssmith-pc left a comment

Choose a reason for hiding this comment

Uh oh!

austin-denoble left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aulorbe commented Apr 8, 2024

Uh oh!

Uh oh!

aulorbe commented Apr 5, 2024 •

edited

Loading