-
Notifications
You must be signed in to change notification settings - Fork 9
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
Rebase & Implement Rerank #80
Rebase & Implement Rerank #80
Conversation
…nch, refactor code to use generated inference module, correct a few issues with Documents argument shape and rerank test
4806323
to
d76c4aa
Compare
This is exciting to read. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -1261,7 +1261,7 @@ type InferenceService struct { | |||
// 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: A pointer to an EmbedRequest object that contains the model t4o use for embedding generation, the | |||
// - in: A pointer to an EmbedRequest object that contains the model to use for embedding generation, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
pinecone/client.go
Outdated
// - Model: "The [model] to use for reranking. | ||
// - Query: (Required) The query to compare with documents. | ||
// - Documents: (Required) A list of Document objects to be reranked. | ||
// - RankFields: (Optional) A list of document fields to use for ranking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is SUCH a nit, but I think it might make it even more clear if you said "A list of fields in each Document" instead of "A list of document fields"
pinecone/client.go
Outdated
// - Query: (Required) The query to compare with documents. | ||
// - Documents: (Required) A list of Document objects to be reranked. | ||
// - RankFields: (Optional) A list of document fields to use for ranking. | ||
// - ReturnDocuments: (Optional) Whether to include documents in the response. Defaults to true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oo this is nice! I don't think this is an optional field in TS. Maybe I'll incorporate...
pinecone/client.go
Outdated
// - Documents: (Required) A list of Document objects to be reranked. | ||
// - RankFields: (Optional) A list of document fields to use for ranking. | ||
// - ReturnDocuments: (Optional) Whether to include documents in the response. Defaults to true. | ||
// - TopN: (Optional) How many documents to return. Defaults to the length of input Documents. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd uppercase Documents, just to reinforce that we're talking about Document objects here
pinecone/client.go
Outdated
// - Data: A list of documents which have been reranked. The documents are sorted in order of relevance, | ||
// with the first being the most relevant. | ||
// - Model: The model used to rerank documents. | ||
// - Usage: Usage statistics for the reranking operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe link out to our page about read and write units? Maybe this guy? https://docs.pinecone.io/guides/organizations/manage-cost/monitor-your-usage Might want to check w/Silas and Jesse re: which pg would be best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had linked to it elsewhere, but added a link here.
// - TopN: (Optional) How many documents to return. Defaults to the length of input Documents. | ||
// - Parameters: (Optional) Additional model-specific parameters for the reranker | ||
// | ||
// [model]: https://docs.pinecone.io/guides/inference/understanding-inference#models |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
pinecone/client.go
Outdated
// topN := 2 | ||
// retunDocuments := true | ||
// documents := []pinecone.Document{ | ||
// {"id": "vec1", "text": "Apple is a popular fruit known for its sweetness and crisp texture."}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I actually wouldn't use the term vec[whatever]
here. I think it's a little confusing, since what's in the "Document" objs we're passing in is text, not an embedding. Maybe just stick with "abc1" or whatever for ID
?
pinecone/client.go
Outdated
// | ||
// ranking, err := pc.Inference.Rerank(ctx, &pinecone.RerankRequest{ | ||
// Model: rerankModel, | ||
// Query: "i love cats", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd make the query relevant to the docs. As a reader, I'm like, why would I ask a q about cats when my docs are about apples :)
// - Index: The index position of the document from the original request. This can be used | ||
// to locate the position of the document relative to others described in the request. | ||
// - Score: The relevance score of the document indicating how closely it matches the query. | ||
type RankedDocument struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd specifiy in the docstring here (or somewhere) that the Docs w/2+ fields in them will only be ranked on 1 field -- either text
if they do not pass in a custom field to rank or, or their customField
, which has to be present in the Doc objs.
Here's how I did it in TS: https://github.com/pinecone-io/pinecone-ts-client/pull/303/files#diff-77967717b18045071d22a72f646f1c4f3cbddc394751cda28af4dfa8732cbd7fR114 (the backend throws the error, so we just have to catch it)
ReturnDocuments: &retunDocuments, | ||
TopN: &topN, | ||
RankFields: &[]string{"text"}, | ||
Documents: []Document{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this is the situation in which the doc will only be ranked on text
, in case the user isn't clear on that re: my comment here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! A few nits:
- I'd capitalize things like
Document
when you're referring to the objs (or the array of objs) themselves - I'd add tests for every situation outlined by Silas here: https://github.com/pinecone-io/python-plugin-inference/blob/main/tests/integration/inference/test_rerank_scenarios.py. Before I did that, I thought everything was 100% good, but it definitely wasn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
## Problem The `InferenceService` and `Embed` operation were implemented in a previous PR: #67 As a part of the `2024-10` release we are also adding `Rerank` to the client, which will live under `InferenceService` with `Embed`. ## Solution This work builds on top of external contributions from @Stosan (#73) Thank you for you diligence in getting this started! - Update `InferenceService` to expose the new `Rerank` method. - Add various request and response types to represent the interface. - Add doc comments and an integration test for calling the reranker. ## Type of Change - [ ] Bug fix (non-breaking change which fixes an issue) - [X] 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 `just test` If you want to use the code yourself, here's an example: ```go ctx := context.Background() clientParams := pinecone.NewClientParams{ ApiKey: "YOUR_API_KEY", } pc, err := pinecone.NewClient(clientParams) if err != nil { log.Fatalf("Failed to create Client: %v", err) } rerankModel := "bge-reranker-v2-m3" topN := 2 retunDocuments := true documents := []pinecone.Document{ { "id": "vec1", "text": "Apple is a popular fruit known for its sweetness and crisp texture."}, { "id": "vec2", "text": "Many people enjoy eating apples as a healthy snack."}, { "id": "vec3", "text": "Apple Inc. has revolutionized the tech industry with its sleek designs and user-friendly interfaces."}, { "id": "vec4", "text": "An apple a day keeps the doctor away, as the saying goes."}, } ranking, err := pc.Inference.Rerank(ctx, &pinecone.RerankRequest{ Model: rerankModel, Query: "i love cats", ReturnDocuments: &retunDocuments, TopN: &topN, RankFields: &[]string{"text"}, Documents: documents, }) if err != nil { log.Fatalf("Failed to rerank: %v", err) } fmt.Printf("Rerank result: %+v\n", ranking) ``` --------- Co-authored-by: Sam Ayo <[email protected]>
Problem
The
InferenceService
andEmbed
operation were implemented in a previous PR: #67As a part of the
2024-10
release we are also addingRerank
to the client, which will live underInferenceService
withEmbed
.Solution
This work builds on top of external contributions from @Stosan (#73) Thank you for you diligence in getting this started!
InferenceService
to expose the newRerank
method.Type of Change
Test Plan
just test
If you want to use the code yourself, here's an example: