Add docstrings for index_connection.go #31

aulorbe · 2024-06-24T23:20:53Z

Problem

We do not have our publicly exported funcs, methods, or structs documented in godoc comments.

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

Solution

Add 'em!

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

CI passes.

jseldess

This is very helpful, @aulorbe! Just a few requests. And let me know if/when you want a closer copy review.

jseldess · 2024-06-25T14:40:44Z

pinecone/index_connection.go

 func (idx *IndexConnection) Close() error {
 	err := idx.grpcConn.Close()
 	return err
 }

+// UpsertVectors indexes vectors into a Pinecone index.


Do you think you could add metadata and sparse values to the example?

jseldess · 2024-06-25T14:45:02Z

pinecone/index_connection.go

+//	  fmt.Println("Error:", err)
+//  }
+//
+//  queryVector := []float32{1.0, 2.0}


Can you include a metadata filter? A hybrid query would be nice as well.

Thanks you!

This reverts commit da99ab8.

austin-denoble

Thank you so much for getting all of these added! Took an initial pass through it, but I should probably do another.

Overall looking good, I left some similar feedback to the Client PR. Mainly, I think we need to remove all the usage of getEnvVars in these examples, and we could probably look at using DescribeIndex over ListIndexes in some of these code examples for targeting the index host.

Let me know if you'd like to sync to discuss the feedback holistically for these PRs.

austin-denoble · 2024-06-25T21:44:59Z

pinecone/index_connection.go

+// IndexConnection holds the parameters for Pinecone IndexConnection object.
+//
+// Fields:
+// 	- Namespace: The namespace of the index.


This Namespace line looks too indented. I think for wording we could be a bit more explicit, like the TypeScript client here: https://github.com/pinecone-io/pinecone-ts-client/blob/4fe3e04e05c4c8748262e5a00446bcb656d379eb/src/data/index.ts#L131

"The namespace where index operations will be performed." Just something to make the targeting aspect for the connection a bit more explicit.

Re: indentation -- I doubled checked and without an extra whitespace in the front, they don't turn into bullet pts in the godoc rendering, so I think they gotta stay as-is.

Changed the namespace wording, thank you (and the add'l metadata wording)!

austin-denoble · 2024-06-25T21:48:11Z

pinecone/index_connection.go

+//  - additionalMetadata: Additional metadata to be sent with each request.
+//  - dataClient: The gRPC client for the index.
+//  - grpcConn: The gRPC connection.
 type IndexConnection struct {


This struct is public but these fields are technically private in that they're not exported. I think we can still add doc comments for them.

nitpick'y wording:
"Additional metadata to be sent with each RPC request."

austin-denoble · 2024-06-25T21:59:33Z

pinecone/index_connection.go

+//  ctx := context.Background()
+//
+//  clientParams := pinecone.NewClientParams{
+//	  ApiKey:    getEnvVars("PINECONE_API_KEY"),


Gave this feedback on the Client PR: we should not be using getEnvVars() for this.

I'd either use a string directly or use os.Getenv():

ApiKey: "YOUR_API_KEY",

or

ApiKey: os.Getenv("PINECONE_API_KEY")

We try and pull PINECONE_API_KEY from the environment inside the SDK though, so it's a little redundant maybe to show it here as well.

Sounds good! I'll just use

ApiKey: "YOUR_API_KEY"

austin-denoble · 2024-06-25T22:04:31Z

pinecone/index_connection.go

@@ -49,11 +56,88 @@ func newIndexConnection(in newIndexParameters) (*IndexConnection, error) {
 	return &idx, nil
 }

+// Close closes the connection to a Pinecone index.
+//
+// Returns a pointer to an IndexConnection object and an error if the connection cannot be closed.


This is incorrect, the Close() only returns an error if there is one, otherwise it returns nil:

go-pinecone/pinecone/index_connection.go

Line 54 in 01670f8

return err

I would also update this to explicitly refer to this closing the grpc.ClientConn.

austin-denoble · 2024-06-25T22:09:51Z

pinecone/index_connection.go

+// Parameters:
+//   idx: The IndexConnection object.


idx: IndexConnection isn't a parameter here, it's the receiver for the function, meaning it's part of the IndexConnection struct. I don't think we need to refer to the receiver in doc comments at all.

ah thank you so much!

austin-denoble · 2024-06-25T22:17:15Z

pinecone/index_connection.go

+//  clientParams := pinecone.NewClientParams{
+//	  ApiKey:    getEnvVars("PINECONE_API_KEY"),
+//	  SourceTag: "your_source_identifier", // optional
+//	}


nit: Check indentations.

austin-denoble · 2024-06-25T22:20:46Z

pinecone/index_connection.go

+//
+//  count, err := idx.UpsertVectors(ctx, vectors)
+//  if err != nil {
+//	  fmt.Println("Error:", err)


nit: Similar to previous feedback - anywhere we're doing an "Error:" let's look at adding a little more context around what failed so grepping the code examples is a bit quicker.

Noticed this issue across the comments in this file.

austin-denoble · 2024-06-25T22:22:05Z

pinecone/index_connection.go

+//  allowing for the request to be canceled or to timeout according to the context's deadline.
+//  - in: The vectors to index.
+//
+// Returns the number of vectors upserted and an error if the request fails.


nit: or an error

austin-denoble · 2024-06-25T22:24:10Z

pinecone/index_connection.go

+//  idxs, err := pc.ListIndexes(ctx)
+//
+//  idx, err := pc.Index(idxs[0].Host)
+//  if err != nil {
+//	  fmt.Println("Error:", err)
+//  }


There's a missing err check after the ListIndexes call.

Also, I think we could use DescribeIndex to just pull a single index and then that makes the example code a bit clearer for these, let me know what you think.

austin-denoble · 2024-06-25T22:25:19Z

pinecone/index_connection.go

+//
+//	res, err := idx.QueryByVectorValues(ctx, &pinecone.QueryByVectorValuesRequest{
+//	  Vector:        queryVector,
+//	  TopK:          topK,  // number of vectors (nearest neighbors) you want returned


Should we be referencing "nearest neighbors"? I don't think we do in other SDKs, and it may be superfluous here. Isn't it based on the similarity metric anyways?

It's in a few places in the comments in this file, I'm thinking we should maybe just drop it.

Yeah can do!

austin-denoble

Awesome work! These look great, left a few comments and nits which you can take or leave. I think we could always iterate on these as we move forwards anyways, and you've spent plenty of time getting these looking nice.

austin-denoble · 2024-06-26T21:54:08Z

pinecone/index_connection.go

+// Parameters:
+//  - ctx: A context.Context object controls the request's lifetime,
+//  allowing for the request to be canceled or to timeout according to the context's deadline.
+//  - in: The vectors to index.


This is still "index".

austin-denoble · 2024-06-26T22:38:50Z

pinecone/index_connection.go

+//    Values: []float32{1.0, 2.0},
+//     },


nit: Indentation here is actually a bit off (checked locally). I think just indent the Values line, and move the bracket back.

austin-denoble · 2024-06-26T22:45:25Z

pinecone/index_connection.go

+//  - Prefix: The prefix by which to filter.
+//  - Limit: The maximum number of vectors to return.


These two could use a little more clarity due to how the default behavior works, what do you think?

Suggested change

// - Prefix: The prefix by which to filter.

// - Limit: The maximum number of vectors to return.

// - Prefix: The prefix by which to filter. If unspecified, an empty string will be used which will list all vector ids in // the namespace.

// - Limit: The maximum number of vectors to return. If unspecified, the server will use a default value.

Referencing what I wrote 😅: https://github.com/pinecone-io/pinecone-ts-client/blob/4fe3e04e05c4c8748262e5a00446bcb656d379eb/src/data/list.ts#L10

oh great! I didn't know that was the default behavior, actually. Thanks!

austin-denoble · 2024-06-26T22:49:34Z

pinecone/index_connection.go

+// which is passed into the QueryByVectorValues method.
+//
+// Fields:
+//  - Vector: The ID of the vector for which you want to find similar vectors.


This should be talking about the vector itself, not the ID:

Suggested change

// - Vector: The ID of the vector for which you want to find similar vectors.

// - Vector: The query vector for which you want to find similar vectors.

Or however you want to word it.

austin-denoble · 2024-06-26T22:52:26Z

pinecone/index_connection.go

+// which is passed into the QueryByVectorId method.
+//
+// Fields:
+//  - VectorId: The ID of the vector for which you want to find similar vectors.


nit: We mention the id being "unique" in some of our other doc strings.

austin-denoble · 2024-06-26T22:54:45Z

pinecone/index_connection.go

+// Returns an error if the request fails,
+// otherwise returns nil. This method will also return nil if the passed vector ID does not exist in the index or
+// namespace.


nit: Line breaks on this are a little wonky.

yeah unfortunately this is just the auto-wrapping on my linter. I think I should keep it just so it's consistent throughout this PR. WDYT?

austin-denoble · 2024-06-26T22:54:57Z

pinecone/index_connection.go

+// Note: You must instantiate an IndexWithNamespace connection in order to delete vectors by ID in namespaces other
+// than the default.


austin-denoble · 2024-06-26T22:56:46Z

pinecone/index_connection.go

+//
+// Returns an error if the request fails, otherwise returns nil.
+//
+// Parameters:


Can we also put the note about needing to instantiate an index with a namespace, etc here?

// Note: You must instantiate an IndexWithNamespace connection in order to delete vectors by ID in namespaces other // than the default.

austin-denoble · 2024-06-26T23:00:27Z

pinecone/index_connection.go

+//
+// Fields:
+//  - Dimension: The dimension of the index.
+//  - IndexFullness: The fullness level of the index. Note: only available on pods-based indexes.


nit: Does "pod-based" feel better than "pods-based"? Might just be me.

Also I didn't know this was the case, is it just always 0 for serverless?

I like pods-based b/c it's more technically correct but 🤷 .

If you try to run DescribeIndexStatsFiltered on a Serverless index, you get this error:

Failed to describe index "serverless-test". Error: rpc error: code =
InvalidArgument desc = Serverless and Starter indexes do not support describing index stats with metadata filtering

jseldess

LGTM

jseldess · 2024-06-27T17:41:45Z

pinecone/index_connection.go

+//	  fmt.Println("Error:", err)
+//  }
+//
+//  queryVector := []float32{1.0, 2.0}


Thanks you!

austin-denoble

Thanks for implementing all the changes!

aulorbe added 2 commits June 24, 2024 16:20

Add godoc comments

b5f54e7

Save work

47a2410

jseldess reviewed Jun 25, 2024

View reviewed changes

Remove docstring for method we will probs deprecate

da99ab8

aulorbe requested a review from austin-denoble June 25, 2024 17:46

aulorbe marked this pull request as ready for review June 25, 2024 17:46

Revert "Remove docstring for method we will probs deprecate"

8c98729

This reverts commit da99ab8.

austin-denoble reviewed Jun 25, 2024

View reviewed changes

aulorbe changed the title ~~Add godoc comments to index_connection.go~~ Add docstrings for index_connection.go Jun 25, 2024

aulorbe added 11 commits June 25, 2024 20:11

Address Austin comments, round 1

9218dc6

Change wording of nearest neighbors replacement

0d64ed2

Change ListIndexes to DescribeIndex call

fbd7fcb

Got DescribeIndexStatsFiltere to work + clean up examples

701270d

Clean up FetchVectors example code

a56beb6

Finalize QueryByVectorValues example code

a4fc153

Finish QueryByVectorId

4a80530

Finish DeleteVectorsById

4f33fa3

Finish ListVectors

2919e02

Finalizing comments

1a69b18

Merge branch 'main' into Audrey/IndexConnection-docstrings

5fa56e2

austin-denoble approved these changes Jun 26, 2024

View reviewed changes

aulorbe added 3 commits June 27, 2024 10:16

Finalize all docstrings

4351850

Add metadata and sparse values to UpsertVectors docstring

ab0572d

Add metadata and sparse values to QueryByVectorValues docstring

f76e544

aulorbe requested a review from jseldess June 27, 2024 17:37

jseldess approved these changes Jun 27, 2024

View reviewed changes

aulorbe added 4 commits June 27, 2024 11:59

Oops missed some errors

5b22aae

Update errors to be log.Fatalf + fix some if/else stmts

9b7b4d5

Weird idk why this change was made; revert

a56297b

Fix indentation

22c38dc

austin-denoble approved these changes Jun 27, 2024

View reviewed changes

aulorbe merged commit 81bd572 into main Jun 28, 2024
3 checks passed

aulorbe deleted the Audrey/IndexConnection-docstrings branch June 28, 2024 18:07

		// - Prefix: The prefix by which to filter.
		// - Limit: The maximum number of vectors to return.

	// - Vector: The ID of the vector for which you want to find similar vectors.
	// - Vector: The query vector for which you want to find similar vectors.

		// Note: You must instantiate an IndexWithNamespace connection in order to delete vectors by ID in namespaces other
		// than the default.

Add docstrings for index_connection.go #31

Add docstrings for index_connection.go #31

Conversation

aulorbe commented Jun 24, 2024 • edited Loading

Problem

Solution

Type of Change

Test Plan

jseldess 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

austin-denoble 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

Choose a reason for hiding this comment

austin-denoble 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

aulorbe Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

aulorbe Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

jseldess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble left a comment

Choose a reason for hiding this comment

aulorbe commented Jun 24, 2024 •

edited

Loading

aulorbe Jun 27, 2024 •

edited

Loading

aulorbe Jun 27, 2024 •

edited

Loading