Fix streaming endpoint failure handling#314
Conversation
| hooks: | ||
| - id: mypy | ||
| name: mypy-clients-python | ||
| files: clients/python/.* |
There was a problem hiding this comment.
shouldn't be using clients/python mypy checking server files
| model_endpoint_service=external_interfaces.model_endpoint_service, | ||
| llm_model_endpoint_service=external_interfaces.llm_model_endpoint_service, | ||
| ) | ||
| response = use_case.execute(user=auth, model_endpoint_name=model_endpoint_name, request=request) |
There was a problem hiding this comment.
here use_case.execute doesn't actually execute until L237
| assert response_1.status_code == 404 | ||
|
|
||
|
|
||
| @pytest.mark.skip(reason="Need to figure out FastAPI test client asyncio funkiness") |
There was a problem hiding this comment.
do we still want to skip this test?
There was a problem hiding this comment.
yes, unfortunately i still haven't figured out how to run two streaming tests in a row. there's something wrong about how test client uses event loop that i wasn't able to fix: more context in Kludex/starlette#1315
| except (ObjectNotFoundException, ObjectNotAuthorizedException) as exc: | ||
| yield {"data": {"error": {"status_code": 404, "detail": str(exc)}}} | ||
| except ObjectHasInvalidValueException as exc: | ||
| yield {"data": {"error": {"status_code": 400, "detail": str(exc)}}} |
There was a problem hiding this comment.
Should we have a helper function to generate these payloads?
There was a problem hiding this comment.
i think this snippet is short enough so okay to not have a helper function
yixu34
left a comment
yixu34
left a comment
There was a problem hiding this comment.
Should also have @song-william take a look
| { | ||
| "request_id": str(request_id), | ||
| "error": { | ||
| "status_code": code, |
There was a problem hiding this comment.
Maybe this is a bit pedantic (pydantic? lol), but since you ended up creating the DTOS anyway, wonder if it makes sense to instantiate the DTOs and convert them to JSON? That way you get the typing.
Alternatively, I think we want a unit test to enforce the API contract behind the error return type.
There was a problem hiding this comment.
will do. for test i added test_completion_stream_endpoint_not_found_returns_404 but unfortunately i still haven't figured out how to run it (it works individually)
| async for message in response: | ||
| yield {"data": message.json()} | ||
| except (InvalidRequestException, ObjectHasInvalidValueException) as exc: | ||
| yield handle_streaming_exception(exc, 400, str(exc)) |
There was a problem hiding this comment.
Just for my edification, was the primary fix for urllib3.exceptions.ProtocolError to push the exception handling inside the generator with yield?
There was a problem hiding this comment.
exception in use_case.execute won't throw until async for message in response:, and since originally we don't capture exceptions other than InvalidRequestException, my understanding is 0 bytes would get returned and client side throws error about not able to decode
4 participants
before the fix, getting error:
after the fix, getting response: