Update data plane SDK definition #11
Conversation
haruska
force-pushed
the
update_data_plane
branch
2 times, most recently
from
300e63d to
843ed3d
Compare
haruska
force-pushed
the
update_data_plane
branch
from
498f1bf to
948e9b7
Compare
There was a problem hiding this comment.
Nice work here getting this SDK back in business, overall the approach makes sense and feels clean. Caveat to my review being that I'm not that familiar with Go.
Left a lot of questions and nits but I don't necessarily consider any of that blocking. Again, nice job, looking forward to seeing this come together.
| } | ||
| } | ||
|
|
||
| func (idx *IndexConnection) akCtx(ctx context.Context) context.Context { |
There was a problem hiding this comment.
nit: What's the akCtx stand for here?
There was a problem hiding this comment.
"api key context" ... as in a new context with the api key added to the metadata.
Other thing of note, anything starting with a lowercase is not exported. So this is a private method on IndexConnection.
| namespaceSummaries := make(map[string]*NamespaceSummary) | ||
| for key, value := range res.Namespaces { | ||
| namespaceSummaries[key] = &NamespaceSummary{ | ||
| VectorCount: value.VectorCount, | ||
| } | ||
| } |
There was a problem hiding this comment.
question: We do some mapping before returning responses for FetchVectors, ListVectors, DescribeIndexStats, and query, and I suppose I'm mainly curious why that is.
There was a problem hiding this comment.
mainly to not expose the internal struct types from the generated code. This allows us to have a clean and well documented interface in the pinecone package without the user of the SDK having to dig into internal code.
| if err != nil { | ||
| return 0, err | ||
| } |
There was a problem hiding this comment.
thought: I'm not sure what the errors that are returned from the gRPC calls look like, but we may need to eventually think about centralized mapping / wrapping of things if that makes sense in Go.
There was a problem hiding this comment.
In Go, every error is an error. Unless we wanted different messages, these are generally easier to maintain as pass-through.
pinecone/index_connection.go
Outdated
| return err | ||
| } | ||
|
|
||
| type UpsertVectorsRequest struct { |
There was a problem hiding this comment.
question: Some structs are defined in-line in this file and some are in models.go, would it make sense to keep everything in one place or is it more reasonable to define these things near where they're used?
There was a problem hiding this comment.
It's sudo-arbitrary. The exported structs defined here are the Request and Response structs for exported actions. The data structs are in models.go. I guess because those have a higher probability of being reused outside of index connection?
This is something that can be re-organized in the future without breaking uses of the SDK. Everything is exported to the pinecone package regardless of filename.
| import ( | ||
| "context" | ||
| "github.com/google/uuid" | ||
| "github.com/stretchr/testify/assert" |
There was a problem hiding this comment.
Read up a bit on the testing framework in case anyone else is curious: https://github.com/stretchr/testify.
This was the one you mentioned was controversial? Seems fine to me, although I'm not sure if that's something we need to consider at this point.
Also less relevant now, but should we consider having a specific folder path for test files?
There was a problem hiding this comment.
Yeah, more that using anything not built in is controversial. This package allows for things like setup and tear-down on suite, sub-suite, test levels. It's just more like other testing frameworks folks are used to. Seemed like a good addition.
pinecone/index_connection_test.go
Outdated
| func (ts *IndexConnectionTests) TestFetchVectors() { | ||
| req := &FetchVectorsRequest{ | ||
| Ids: ts.vectorIds, | ||
| Namespace: ts.namespace, |
There was a problem hiding this comment.
My golang is rusty, but is there a way to make Namespace optional in these various request objects? On the one hand I like pushing people to be explicit, but on the other hand historically our other SDKs have treated namespace as optional and automatically applied the default "" when nothing is passed.
In Typescript we tried pulling the namespace up one level so it's more of a config property baked into the thing analgous to your IndexConnection. The advantage of that is the user only has to specify it once and doesn't have to remember to pass the correct namespace on every operation. The thinking was that seeing this param repeated across every method suggests it's really part of the connection config, and having to repeat it on every call feels unnecessarily repetitious and error prone. Food for thought.
There was a problem hiding this comment.
So, as-is these are optional. Golang sets any omitted value to the "zero value" of that type. In string type's case, that's "".
I do like the idea of capturing an optional Namespace in the IndexConnection struct and removing it from the individual calls. There aren't cases where folks would be changing the namespace per call, right?
There was a problem hiding this comment.
There aren't cases where folks would be changing the namespace per call, right?
It's possible they would want to work with multiple namespaces, but based on what I've seen in our docs and examples it's not common. If somebody wanted to work with multiple namespaces they could create two client instances and it wouldn't be a huge deal, certainly less error prone than having to remember to pass a namespace in every method call. Nobody has complained about this choice in the typescript client AFAIK. As an organizational concept, it's kind of a hangover from pre-serverless days when people needed access to a lighter weight form of multitenancy to save the cost of spinning up a completely separate (expensive) pod index.
There was a problem hiding this comment.
This cleaned up IndexConnection quite a bit! Note, IndexConnection.Namespace is exported and can be changed by the user. The new namespace would then be used in subsequent calls.
The Pinecone Golang SDK has not been actively developed in over two years. In that time, much has changed in PineconeDB. This PR:
internalto the module, reducing the SDK surface areaThere is follow-on work that still needs to happen including better testing (see GH Issue #12) and documentation.