Conversation
…sponses The goal is to avoid parsing json data multiple times if we want to do any post-processing on the data returned from the view.
Note: MCP views still return JsonResponse, because they won't work with the raw one; the exceptions are error responses (just like in other views).
We have rewritten the tests and don't use it any more.
For now, chat completions, completions, and embeddings streaming works. All the operations processing the streaming content are gathered in the RawStreamingResponse's `transforms` attribute, and applied to individual chunks in the middleware, at the end of the response processing. This is necessary, because otherwise the streaming content (an async iterator) gets exhausted after one transform is applied. Only after applying the transformations are the chunks dumped into SSE json format. This also allows us to avoid repeated json-loading and json-dumping of the response content.
Without the `--prefer-offline` flag, npx always tried to access internet, and in case there was no internet connection, the server wouldn't start, even if it was installed locally.
…il!) The end-to-end test in test_http_response_middleware.py passes, but the some of the tests in the test_mcp.py module fail inconsistently with `httpx.ReadTimeout`.
natkam
changed the title
5f055b1 to
a6980ce
Compare
…onse Pydantic models are now passed in the response as they are, and the middleware takes care of dumping them to dict.
|
@JaeYeonLee0621 Since I'm away until 23.09, please feel free to do whatever you need with this PR, especially if it blocks you from working on other tasks! Otherwise, if you have any comments, I'll apply them when I'm back :) |
|
|
||
| data = resp.model_dump(exclude_unset=True) | ||
| request_log.token_usage = _get_token_usage(data) | ||
| # TODO # data = resp.model_dump(exclude_unset=True) |
There was a problem hiding this comment.
Do we still need this comment? It looks like it might be left over from before the change.
| @@ -153,7 +153,7 @@ async def vector_stores( | |||
| # Return upstream response directly (ID already matches) | |||
| response_data = remote_vs.model_dump(mode="json") | |||
There was a problem hiding this comment.
Could we pass remote_vs here instead? If you agree, the logic below would need the same change.
| if ( | ||
| not self._registered | ||
| and isinstance(chunk, ResponseCreatedEvent) | ||
| and chunk.type == "response.created" |
There was a problem hiding this comment.
Since ResponseCreatedEvent already narrows chunk.type to response.created, is this condition still needed?
type: Literal["response.created"]| response.content = response.content.model_dump( | ||
| exclude_none=True, exclude_unset=True, mode="json" | ||
| ) | ||
| else: |
There was a problem hiding this comment.
Could we change model_dump() to model_dump(mode='json') here for consistency? Same for the logic below, if that works for you.
mode="python"(default) : keeps native Python typesmode="json": converts to JSON-compatible types
meffmadd
left a comment
meffmadd
left a comment
There was a problem hiding this comment.
Can you check the feasibility of the sub-classing approach? Maybe I missed something but subclassing seams simpler to me...
| def _get_token_usage(content: bytes | dict[str, Any]) -> Usage: | ||
| """Retrieves token usage information from the response content. | ||
|
|
||
| class RawJsonResponse: |
There was a problem hiding this comment.
RawJsonResponse is intended to replace JsonResponse so why not sub-class it?
I had a quick look at the implementation and it seems like Django probably uses the content property to determine what data to send. The only problem with the current JsonResponse is that it immediately json.dumps the content. We could just rewrite the class to dump the JSON content dynamically as far as I understand it. I assume Django just accepts a sub-class as response type.
This would make the middleware obsolete and would be cleaner I think.
3 participants
Main goal
The gateway views no longer return Django
StreamingHttpResponse/JsonResponseobjects directly. Instead they return lightweight "raw" response objects, containing a Pydantic model or a dict as data, and a newly addedHttpResponseMiddlewaremiddleware converts those raw objects into real HTTP responses. (Exceptions: the two cases when there is no json in the response content: the speech endpoint, which streams bytes in aStreamingHttpResponse, and transcriptions with text format, which returns anHttpResponsewith text content type.)For streaming responses, in order to avoid repeatedly iterating over the content to apply any post-processing and then creating a new async generator, the post-processing transformations are gathered in a list (
RawStreamingResponse.transforms) and then applied to individual chunks in the middleware. Example:normalize_reasoning_fields()decorator.Note: MCP streaming responses have to yield a slightly different format of data then other streaming responses, and they do not have a
request_logobject attached, so the middleware handles them separately from the others.The idea here is that any post-processing of the response can be now executed on a Python object, without repeatedly converting the response's content to JSON and back. The conversion happens only once, at the very end of the response handling - in the middleware.
Other unrelated changes
--prefer-offlineflag).