Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ConfigureIndex method #34

Merged
merged 20 commits into from
Jul 12, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ jobs:
TEST_POD_INDEX_NAME: ${{ secrets.TEST_POD_INDEX_NAME }}
TEST_SERVERLESS_INDEX_NAME: ${{ secrets.TEST_SERVERLESS_INDEX_NAME }}
PINECONE_API_KEY: ${{ secrets.API_KEY }}
TEST_CONFIGURE_INDEX_NAME: ${{ secrets.TEST_CONFIGURE_INDEX_NAME }}
96 changes: 96 additions & 0 deletions pinecone/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,102 @@ func (c *Client) DeleteIndex(ctx context.Context, idxName string) error {
return nil
}

// ConfigureIndex is used to [scale a pods-based index] up or down by changing the size of pods or the number of
//replicas.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a space is needed between // and replicas.

//
// Parameters:
// - name: The name of the index to configure.
// - pods: The size of pods to scale the index to (e.g. for a "p1" pod type, you could pass "p1.x2", "p1.x4",
// "p1.x6", etc. Optional.
// - replicas: The number of replicas to scale the index to. Optional.
// This is capped by the maximum number of replicas allowed in your Pinecone project. To configure this number,
// go to app.pinecone.io, select your project, and configure the maximum number of pods.
//
// Note: You can only scale an index up, not down. If you want to scale an index down,
// you must create a new index with the desired configuration.
//
// It returns a ConfigureIndexResponse object, which contains the new configuration of the index, or an error.
//
// Example for a pods-based index originally configured with 1 "p1" pod of size "x2" and 1 replica:
// // To scale the size of your pods from "x2" to "x4":
// _, err := pc.ConfigureIndex(ctx, "my-index", "p1.x4", nil)
// if err != nil {
// fmt.Printf("Failed to configure index: %v\n", err)
// }
//
// // To scale the number of replicas:
// _, err := pc.ConfigureIndex(ctx, "my-index", nil, 4)
// if err != nil {
// fmt.Printf("Failed to configure index: %v\n", err)
// }
//
// // To scale both the size of your pods and the number of replicas:
// _, err := pc.ConfigureIndex(ctx, "my-index", "p1.x4", 4)
// if err != nil {
// fmt.Printf("Failed to configure index: %v\n", err)
// }
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you happen to run gofmt on this file? Just wondering as the formatting seems a bit off. There should be a blank empty line between the "Example for a pods-based index..." line and the first comment of the code block.

I think gofmt should also indent the code block further, but I'm not positive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I didn't -- this was pushed before before our Slack convo about gofmt. I'll run it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay done :)

// [scale a pods-based index]: https://docs.pinecone.io/guides/indexes/configure-pod-based-indexes
func (c *Client) ConfigureIndex(ctx context.Context, name string, pods *string,
Copy link
Contributor

Choose a reason for hiding this comment

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

pods may be a little confusing given that fields means something else when creating an index. Although I'm realizing now we don't provide a pods value in CreatePodIndexRequest.

Still, we use PodType there and I think we should do the same here. I know it's a positional arg, but just for clarity in the code here. I know we then assign things to podType further down, but initially when I was reading the function I was a bit confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, np, I'll change to podType! (I hate how we call it type b/c technically it's size, but yeah best not to confuse ppl who're used to our other SDKs)

replicas *int32) (*control.ConfigureIndexResponse,
error) {

var podType *control.PodSpecPodType
var replicasAmt *control.PodSpecReplicas

if pods == nil && replicas == nil {
return nil, fmt.Errorf("Must specify either pods or replicas")
}

if pods != nil {
podTypeVal := control.PodSpecPodType(*pods)
podType = &podTypeVal
}
if replicas != nil {
replicasVal := control.PodSpecReplicas(*replicas)
replicasAmt = &replicasVal
}

request := control.ConfigureIndexRequest{
Spec: struct {
Pod struct {
PodType *control.PodSpecPodType `json:"pod_type,omitempty"`
Replicas *control.PodSpecReplicas `json:"replicas,omitempty"`
} `json:"pod"`
}{
Pod: struct {
PodType *control.PodSpecPodType `json:"pod_type,omitempty"`
Replicas *control.PodSpecReplicas `json:"replicas,omitempty"`
}{
PodType: podType,
Replicas: replicasAmt,
},
},
}

req, err := c.restClient.ConfigureIndex(ctx, name, request)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename req to res to make it clearer it's the http.Response from the function call here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

if err != nil {
log.Fatalf("Failed to configure index %s. Error: %v", name, err)
}
if req.StatusCode != http.StatusOK {
err := handleErrorResponseBody(req, "failed to configure index: ")
if err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think here you can just do:

Suggested change
err := handleErrorResponseBody(req, "failed to configure index: ")
if err != nil {
return nil, err
}
return nil, handleErrorResponseBody(req, "failed to configure index: ")

We can assume if the StatusCode is not http.StatusOK we're returning an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oo nice!

}


response, err := control.ParseConfigureIndexResponse(req) // TODO: need this?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be using the internals, I don't think we're doing that anywhere else. I'd maybe look at how we're handling things in something like DescribeIndex. Could you juse use the decodeIndex function instead?

Suggested change
response, err := control.ParseConfigureIndexResponse(req) // TODO: need this?
res, err := c.restClient.ConfigureIndex(ctx, name, request)
if err != nil {
log.Fatalf("Failed to configure index %s. Error: %v", name, err)
}
defer res.Body.Close()
if req.StatusCode != http.StatusOK {
return nil, handleErrorResponseBody(req, "failed to configure index: ")
}
return decodeIndex(res.Body)

Ultimately this just returns an IndexModel, there's not really anything else in ConfigureIndexResponse, so you'd need to update the return type and maybe doc string here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure yeah good call, I'll do that!

if err != nil {
log.Fatalf("Failed to configure index %s. Error: %v", name, err)
return nil, err
}

defer req.Body.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: In other functions we put this line right after the function call so it's a bit more clear we're actually closing it, so I'd maybe move it up to after the initial function call and error check:

req, err := c.restClient.ConfigureIndex(ctx, name, request)
if err != nil {
		log.Fatalf("Failed to configure index %s. Error: %v", name, err)
}
defer req.Body.Close()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay will do!


return response, nil
}

func (c *Client) ListCollections(ctx context.Context) ([]*Collection, error) {
res, err := c.restClient.ListCollections(ctx)
if err != nil {
Expand Down
155 changes: 149 additions & 6 deletions pinecone/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,18 @@ package pinecone
import (
"context"
"fmt"
"github.com/google/uuid"
"github.com/pinecone-io/go-pinecone/internal/mocks"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"io"
"log"
"net/http"
"os"
"reflect"
"strings"
"testing"

"github.com/google/uuid"
"github.com/pinecone-io/go-pinecone/internal/mocks"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think goimports should split these apart automatically right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thinkkk so. This was also pre-our formatting convo, so I'll push up some changes wrt that soon!

)

type ClientTests struct {
Expand All @@ -24,6 +24,7 @@ type ClientTests struct {
sourceTag string
podIndex string
serverlessIndex string
configureIndex string
}

func TestClient(t *testing.T) {
Expand Down Expand Up @@ -81,6 +82,9 @@ func (ts *ClientTests) SetupSuite() {
ts.serverlessIndex = os.Getenv("TEST_SERVERLESS_INDEX_NAME")
require.NotEmpty(ts.T(), ts.serverlessIndex, "TEST_SERVERLESS_INDEX_NAME env variable not set")

ts.configureIndex = os.Getenv("TEST_CONFIGURE_INDEX_NAME")
require.NotEmpty(ts.T(), ts.configureIndex, "TEST_CONFIGURE_INDEX_NAME env variable not set")

client, err := NewClient(NewClientParams{})
require.NoError(ts.T(), err)

Expand Down Expand Up @@ -496,6 +500,145 @@ func (ts *ClientTests) TestDeleteCollection() {
require.NoError(ts.T(), err)
}

func (ts *ClientTests) TestConfigureIndexIllegalScaleDown() {
deleteErr := ts.client.DeleteIndex(context.Background(), ts.configureIndex)
if deleteErr != nil {
//log.Fatalf("Error deleting index %s: %v", ts.configureIndex, deleteErr)
fmt.Println("Found a delete error: ", deleteErr)

}
_, erdr := ts.client.CreatePodIndex(context.Background(), &CreatePodIndexRequest{
Name: ts.configureIndex,
Dimension: 10,
Metric: Cosine,
Environment: "us-east1-gcp",
PodType: "p1.x2",
},)
if erdr != nil {
log.Fatalf("Error creating index %s: %v", ts.configureIndex, erdr)
}

pods := "p1.x1" // test index originally created with "p1.x2" pods
replicas := int32(1) // could be nil, but do not want to test nil case here
_, err := ts.client.ConfigureIndex(context.Background(), ts.configureIndex, &pods, &replicas)
require.ErrorContainsf(ts.T(), err, "Cannot scale down", err.Error())

deleteErr2 := ts.client.DeleteIndex(context.Background(), ts.configureIndex)
if deleteErr != nil {
aulorbe marked this conversation as resolved.
Show resolved Hide resolved
log.Fatalf("Error deleting index %s: %v", ts.configureIndex, deleteErr2)
}
}

func (ts *ClientTests) TestConfigureIndexScaleUpNoPods() {
deleteErr := ts.client.DeleteIndex(context.Background(), ts.configureIndex)
if deleteErr != nil {
//log.Fatalf("Error deleting index %s: %v", ts.configureIndex, deleteErr)
fmt.Println("Found a delete error: ", deleteErr)

}
_, erdr := ts.client.CreatePodIndex(context.Background(), &CreatePodIndexRequest{
Name: ts.configureIndex,
Dimension: 10,
Metric: Cosine,
Environment: "us-east1-gcp",
PodType: "p1.x2",
},)
if erdr != nil {
log.Fatalf("Error creating index %s: %v", ts.configureIndex, erdr)
}

replicas := int32(2)
_, err := ts.client.ConfigureIndex(context.Background(), ts.configureIndex, nil, &replicas)
require.NoError(ts.T(), err)

deleteErr2 := ts.client.DeleteIndex(context.Background(), ts.configureIndex)
if deleteErr != nil {
log.Fatalf("Error deleting index %s: %v", ts.configureIndex, deleteErr2)
}
}

func (ts *ClientTests) TestConfigureIndexScaleUpNoReplicas() {
deleteErr := ts.client.DeleteIndex(context.Background(), ts.configureIndex)
if deleteErr != nil {
//log.Fatalf("Error deleting index %s: %v", ts.configureIndex, deleteErr)
fmt.Println("Found a delete error: ", deleteErr)

}
_, erdr := ts.client.CreatePodIndex(context.Background(), &CreatePodIndexRequest{
Name: ts.configureIndex,
Dimension: 10,
Metric: Cosine,
Environment: "us-east1-gcp",
PodType: "p1.x2",
},)
if erdr != nil {
log.Fatalf("Error creating index %s: %v", ts.configureIndex, erdr)
}

pods := "p1.x4"
_, err := ts.client.ConfigureIndex(context.Background(), ts.configureIndex, &pods, nil)
require.NoError(ts.T(), err)

deleteErr2 := ts.client.DeleteIndex(context.Background(), ts.configureIndex)
if deleteErr != nil {
log.Fatalf("Error deleting index %s: %v", ts.configureIndex, deleteErr2)
}
}

func (ts *ClientTests) TestConfigureIndexIllegalNoPodsOrReplicas() {
deleteErr := ts.client.DeleteIndex(context.Background(), ts.configureIndex)
if deleteErr != nil {
fmt.Println("Found a delete error: ", deleteErr)
//log.Fatalf("Error deleting index %s: %v", ts.configureIndex, deleteErr)
}
_, erdr := ts.client.CreatePodIndex(context.Background(), &CreatePodIndexRequest{
Name: ts.configureIndex,
Dimension: 10,
Metric: Cosine,
Environment: "us-east1-gcp",
PodType: "p1.x2",
},)
if erdr != nil {
log.Fatalf("Error creating index %s: %v", ts.configureIndex, erdr)
}

_, err := ts.client.ConfigureIndex(context.Background(), ts.configureIndex, nil, nil)
require.ErrorContainsf(ts.T(), err, "Must specify either pods or replicas", err.Error())

deleteErr2 := ts.client.DeleteIndex(context.Background(), ts.configureIndex)
if deleteErr != nil {
log.Fatalf("Error deleting index %s: %v", ts.configureIndex, deleteErr2)
}
}

func (ts *ClientTests) TestConfigureIndexHitPodLimit() {
deleteErr := ts.client.DeleteIndex(context.Background(), ts.configureIndex)
if deleteErr != nil {
//log.Fatalf("Error deleting index %s: %v", ts.configureIndex, deleteErr)
fmt.Println("Found a delete error: ", deleteErr)

}
_, erdr := ts.client.CreatePodIndex(context.Background(), &CreatePodIndexRequest{
Name: ts.configureIndex,
Dimension: 10,
Metric: Cosine,
Environment: "us-east1-gcp",
PodType: "p1.x2",
},)
if erdr != nil {
log.Fatalf("Error creating index %s: %v", ts.configureIndex, erdr)
}

replicas := int32(30000)
_, err := ts.client.ConfigureIndex(context.Background(), ts.configureIndex, nil, &replicas)
require.ErrorContainsf(ts.T(), err, "You've reached the max pods allowed", err.Error())

deleteErr2 := ts.client.DeleteIndex(context.Background(), ts.configureIndex)
if deleteErr != nil {
log.Fatalf("Error deleting index %s: %v", ts.configureIndex, deleteErr2)
}
}

func (ts *ClientTests) deleteIndex(name string) error {
return ts.client.DeleteIndex(context.Background(), name)
}
Expand Down
Loading