Validate chatcompletion to avoid unexpected bugs #551
Conversation
Signed-off-by: Sambhav Kothari <[email protected]>
There was a problem hiding this comment.
Hi @sambhav,
Thanks for your contribution.
A few things:
- We can probably implement a fix both in
_process_responseand_process_streamed_response. - To be more consistent with other models (like gemini), let's just do something like the following instead of raising an error
if response.created:
timestamp = datetime.fromtimestamp(response.created, tz=timezone.utc)
else:
timestamp = _utils.now_utc()
|
@sydney-runkle that might be a sane default for the timestamp, but as mentioned in the issue, the bug arises from the fact that it's not a chat completion but instead an error response. As a result, the other fields will be undefined. |
We should then handle this differently, designing a code path that makes the error explicit rather than hiding this in a timestamp related error. |
There was a problem hiding this comment.
Just my two cents. The second use of response in _map_usage(response) is not explicitly validated. The validation check (response.created) occurs only inside _process_response. This means that _map_usage(response) operates on an unvalidated response, which could lead to unexpected issues if the response is invalid. (in the async def request).
There was a problem hiding this comment.
O, heck, I overlooked that the exception raised in _process_response ensures _map_usage cannot receive the response if it’s invalid. This guarantees the response is always validated before being passed further.
Fixes #527
This PR addresses the incorrect handling of error responses from the API when a rate limit is exceeded. The changes include:
These changes ensure that the appropriate error message is surfaced when a rate limit is exceeded, instead of throwing a date-time parsing exception.