This repository has been archived by the owner on Nov 13, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add Cohere Reranker #269
Add Cohere Reranker #269
Changes from 3 commits
9674051
4cd8142
837fa0b
14e4328
c583a71
04b6d42
11060a7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Also needs to be wrapped in
try
clause.Transient errors like rate limits etc should be retried (if the Cohere client itself doesn't do that for us already).
Errors that are caused by wrong configuration (like wrong model name or bad API key) need to be re-raised with an actionable error message
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.
Cohere retries internally. Since Cohere does not return different error types it is hard to understand what the message is. For now I am raising a RuntimeError from the actual error.
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.
We try to eliminate underlying service's errors like
CohereAPIError
orOpenAIError
, and replace them with actionable error message (like something the user needs to change in the Canopy config, or the explicit env var to set).In the future we will have our own error types like
EncoderError
,AuthenticationError
etc. In the meantime simply re-raiseRuntimeError
for all of these cases (the CLI catchesRuntimeError
and prints them nicely)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 checked the client, client does not return a specific error for different errors we always get a CohereAPIError. For now I am raising RuntimeError from that error, if they improve the client we can write actionable messages.
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.
Missing more negative tests - wrong model name, bad input (e.g. not strings) etc.
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.
Added wrong model name, bad input is not possible since we validate our data with pydantic.