async-openai icon indicating copy to clipboard operation
async-openai copied to clipboard

Fix Error Propagation During Chat Streaming

Open gilljon opened this issue 1 year ago • 6 comments

If stream = True, we are noticing that a 400 returns (in the server logs):

StreamError("Invalid status code: 400 Bad Request")

On the client side (Python consumption):

httpx.RemoteProtocolError: peer closed connection without sending complete message body (incomplete chunked read)

If stream = False, then we still get the 400, but also more rich information:

Traceback (most recent call last):
  File "test.py", line 5, in <module>
    out = client.chat.completions.create(
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "python3.12/site-packages/openai/_utils/_utils.py", line 275, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "python3.12/site-packages/openai/resources/chat/completions.py", line 829, in create
    return self._post(
           ^^^^^^^^^^^
  File "python3.12/site-packages/openai/_base_client.py", line 1277, in post
    return cast(ResponseT, self.request(cast_to, opts, stream=stream, stream_cls=stream_cls))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "python3.12/site-packages/openai/_base_client.py", line 954, in request
    return self._request(
           ^^^^^^^^^^^^^^
  File "python3.12/site-packages/openai/_base_client.py", line 1058, in _request
    raise self._make_status_error_from_response(err.response) from None
openai.BadRequestError: After the optional system message, conversation roles must alternate user/assistant/user/assistant/...

What would be great is that we are able to retrieve the enriched error information even if streaming is true. That is the current behavior with the Python client.

gilljon avatar Nov 06 '24 19:11 gilljon

Chasing it down a bit, the actual response is contained in the error, so a fix shouldn't be that hard.

Basically, in the file error.rs in the reqwest-eventsource-0.6.0, the error type that is later used is defined. It is on line 43: InvalidStatusCode and contains the StatusCode and the Response. However, it is annotated with the error macro from the thiserror crate (See line 42) and thus only outputs the StatusCode, the Response is discarded.

That is, if the error code defined by the macro is used.

In src/client.rs on line 441, async-openai (0.25.0, sorry) simply calls e.to_string() on the error it might have gotten from the Eventsource, discarding all other information contained in e, such as the Response.

Changing this to something that properly captures the Response or just adds it to the Error String will drastically improve stream debugging capabilities, as was talked about in this issue.

I might work on it later.

SeseMueller avatar Feb 01 '25 00:02 SeseMueller

I added a simple block to, if an invalid status code or content type is encountered, a custom error String is used instead that also contains the full Response text.

I didn't understand the testing suite or whether this is covered by the tests; sorry about that.

I'll quickly test it in my local environment and report back.

SeseMueller avatar Feb 02 '25 20:02 SeseMueller

It does work, I got it to output a more useful error when calling o3-mini. Comparison:

Before:

StreamError("Invalid status code: 400 Bad Request")

Now:

StreamError("Invalid status code: 400 Bad Request\n{\n  \"error\": {\n    \"message\": \"Unsupported parameter: 'parallel_tool_calls' is not supported with this model.\",\n    \"type\": \"invalid_request_error\",\n    \"param\": \"parallel_tool_calls\",\n    \"code\": \"unsupported_parameter\"\n  }\n}")

Note that the error is quite ugly, but is much more informative. I'm not sure how the errors should reasonably be printed in a pretty way that still contains all useful information.

Edit: This actually helped a lot in working with the new reasoning models, as they have a few parameters they don't want set. The error messages just directly contained the information which parameters I needed to turn off.

SeseMueller avatar Feb 02 '25 20:02 SeseMueller

Thanks for debugging and sharing, it seems like underlying response.text().await can be deserialized into WrappedError and eventually to OpenAIError::ApiError variant to return. When error deserialization of response.text().await to WrapedError fails - fallback to OpenAIError::StreamError(response.text().await.<unwrap-with-default>).

64bit avatar Feb 02 '25 23:02 64bit

Could someone share a code snippet of what exactly needs to change in order to support better error handling?

gilljon avatar Feb 05 '25 17:02 gilljon

Could someone share a code snippet of what exactly needs to change in order to support better error handling?

I did a minimal solution on my fork: https://github.com/SeseMueller/async-openai/commit/4b82e9af90a0f3c91a70ade0b9d39f00148e6fdc

SeseMueller avatar Feb 05 '25 18:02 SeseMueller

I just tested it again with version 0.30.0 and the error looks good.

Fixed as per #445

SeseMueller avatar Oct 20 '25 12:10 SeseMueller

Yup the v0.30.0 handles it better - thanks for confirming!

64bit avatar Oct 20 '25 18:10 64bit