-
Notifications
You must be signed in to change notification settings - Fork 105
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
Slightly improve error handling for external providers #220
Slightly improve error handling for external providers #220
Conversation
Catch specific OpenAI errors and re-raise with a clear message
…pecific module In addition, the `transformers` module is very insisstent, so I added their dedicated mechanism for silencing warnings
Instead, individual RecordEncoders and LLMs would need to raise their own errors
To detect errors early
…ts - both LLM and RecordEncoder
Catch the error in RecordEncoder base class, then format for each inhertor differently
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.
add system test, minimal, overall i think this is pretty thorough as-is, but I understand the notion of this can be extended a lot
1. Needed to change after code was changed. 2. Added a few more test cases 3. Added more assertions on the naive create()
Some error types changed
The CLI shouldn't mention `delete_index()`
Sending just "hello" without any limitations can use many tokens
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
async def _aencode_documents_batch(self, | ||
documents: List[KBDocChunk] | ||
) -> List[KBEncodedDocChunk]: | ||
raise NotImplementedError | ||
|
||
async def _aencode_queries_batch(self, queries: List[Query]) -> List[KBQuery]: | ||
raise NotImplementedError | ||
|
||
def _format_error(self, err): |
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.
Maybe we should resuse this code in the LLM too? I think it's can improve the experience
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.
Yeah, that's the plan. We'll do it as part of the planned BaseLLM
refactor anyway
try: | ||
encoder = OpenAIEncoder(model_name, **kwargs) | ||
except OpenAIError as e: | ||
raise RuntimeError( |
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.
Probably for the future, but I guess it's reasonable to have our own auth and rate limit errors. For many services we are going to use this issue might occur, even pinecone by itself
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.
Yeah, that's definitely the plan!
Solution
This is a very initial and partial solution for #183.
We will need to improve error handling much further.
Type of Change