-
Notifications
You must be signed in to change notification settings - Fork 132
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
refactor: Migrate Cohere to V2 #1321
Conversation
@davidsbatista please review ranker changes. This is for the next week @anakin87 @davidsbatista |
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 left some comments, but in general I agree with the direction of this PR.
Just one question: should we use the opportunity of this refactoring to move the client definition to __init__
?
integrations/cohere/src/haystack_integrations/components/embedders/cohere/document_embedder.py
Outdated
Show resolved
Hide resolved
integrations/cohere/src/haystack_integrations/components/embedders/cohere/text_embedder.py
Outdated
Show resolved
Hide resolved
integrations/cohere/src/haystack_integrations/components/embedders/cohere/utils.py
Show resolved
Hide resolved
integrations/cohere/src/haystack_integrations/components/rankers/cohere/ranker.py
Outdated
Show resolved
Hide resolved
integrations/cohere/src/haystack_integrations/components/rankers/cohere/ranker.py
Outdated
Show resolved
Hide resolved
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, I would just suggest to replace the None
by 4096 as this is the default value from Chohere API V2 (https://docs.cohere.com/v2/reference/rerank)
…ders/cohere/document_embedder.py Co-authored-by: Stefano Fiorucci <[email protected]>
…rs/cohere/ranker.py Co-authored-by: David S. Batista <[email protected]>
…rs/cohere/ranker.py Co-authored-by: David S. Batista <[email protected]>
Co-authored-by: David S. Batista <[email protected]>
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 from my side
Looks good, but please answer this question:
|
@anakin87 so that we can then simply do, for instance:
or any other component? I would vote for this simplification in importing |
@davidsbatista no no, sorry if I did not explain myself better (we have discussed this with Vladimir in the past). Currently, the Cohere client is recreated every time the |
Ah right, @anakin87 apologies - too many context switches. Yes, let's fix that one in a separate PR to ease the cognitive load - as IIRC we agreed privately? |
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.
Yes, let's fix that one in a separate PR to ease the cognitive load - as IIRC we agreed privately?
OK
Why:
embedding_types
to choose the size of the embeddings, providing more control over memory usage and performance.What:
EmbeddingTypes
enum to define and manage supported embedding types:float
: Default float embeddings (valid for all models).int8
: Signed int8 embeddings (valid for v3 models only).uint8
: Unsigned int8 embeddings (valid for v3 models only).binary
: Signed binary embeddings (valid for v3 models only).ubinary
: Unsigned binary embeddings (valid for v3 models only).CohereDocumentEmbedder
andCohereTextEmbedder
to handle multiple embedding types.CohereRanker
to use Cohere V2 API, addedmax_tokens_per_doc
for token-level control, and deprecated the use ofmax_chunks_per_doc
(now ignored).How can it be used:
Users can specify the desired embedding type and initialize the embedders or ranker:
How did you test it:
CohereDocumentEmbedder
andCohereTextEmbedder
to validate functionality with multiple embedding types.int8
,binary
).CohereRanker
testsNotes for the reviewer:
embedding_types
in the embedders to ensure that only valid types are accepted for v3 models.max_tokens_per_doc
inCohereRanker
works as intended and aligns with the Cohere V2 API.