mistral[minor]: Added Retrying Mechanism in case of Request Rate Limit Error for MistralAIEmbeddings
#27818
Conversation
keenborder786
commented
Nov 1, 2024
keenborder786
commented
- Description:: In the event of a Rate Limit Error from the MistralAI server, the response JSON raises a KeyError. To address this, a simple retry mechanism has been implemented to handle cases where the request limit is exceeded.
- Issue: Cannot create MistralAI embeddings from pdf or urls #27790
dosubot
bot
added
the
size:S
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
dosubot
bot
added
Ɑ: embeddings
| batch_responses = [] | ||
|
|
||
| @retry( | ||
| retry=retry_if_exception_type(Exception), |
There was a problem hiding this comment.
Generally, retries should never be implemented on 4xx errors (except for 408 and 429). e.g., 403 should not be retried by default
- What do we do in other parts of the code? Perhaps there's a better example that can be adopted?
- What do other models do in the code base in terms of exposing the retry parameters so users can adjust? (e.g.,what if someone wants to have the first retry after 1 second rather than 30 seconds?)
There was a problem hiding this comment.
- @eyurtsev it is a 429 error therefore retrying makes sense.
- I was not able to find any related example.
- Yes I can make the wait and stop seconds as parameters.
| for batch in self._get_batches(texts) | ||
| ) | ||
| if response.status_code == 429: | ||
| raise Exception("Requests rate limit exceeded") |
There was a problem hiding this comment.
This code takes an exception that was good and informative and turns it into one that's a broad Exception of type Exception -- this is usually not a good pattern for exception handling. Stack trace will be partially lost, the exception type is less specific etc.
There was a problem hiding this comment.
- Their were no specific exception being raised to being with. But I can change it from a general Exception.
|
@eyurtsev please check now |
|
@eyurtsev this is really important, please give feedback if needed |
1 similar comment
| url="/embeddings", | ||
| json=dict( | ||
| model=self.model, | ||
| input=batch, | ||
| ), | ||
| ) | ||
| for batch in self._get_batches(texts) | ||
| ) | ||
| if response.status_code == 429: |
There was a problem hiding this comment.
any reason not to use raise_for_status? We're trying not to drop the original exception which might have useful information inside it
| batch_responses = [] | ||
|
|
||
| @retry( | ||
| retry=retry_if_exception_type(httpx.TimeoutException), |
There was a problem hiding this comment.
(no need to change if you don't want) This is OK b/c it's probably the dominant failure mode
But it's very common to retry 5xx errors as well. And 408 (request timeout)
dosubot
bot
added
the
lgtm
|
@keenborder786 sorry for how long it took. Pushed a minor change to raise on error, so originally information isn't lost |
eyurtsev
changed the title
MistralAIEmbeddingsMistralAIEmbeddings
