Conversation
…streaming Signed-off-by: Lifan Sun <lifansun1412@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request optimizes memory usage in the router by only accumulating the response body when it is actually needed, such as for non-streaming requests or when a post-request callback is configured. It also introduces a new test suite to verify response buffering behavior. The feedback recommends replacing direct calls to the __anext__() dunder method with the built-in anext() function in the test file for cleaner, more idiomatic Python code.
| gen = process_request( | ||
| request, body, URL, "req-1", "/v1/chat/completions", background_tasks | ||
| ) | ||
| await gen.__anext__() # headers and status |
There was a problem hiding this comment.
There was a problem hiding this comment.
taken. will fix in next revision
| tracemalloc.start() | ||
| try: | ||
| gen = process_request(request, body, URL, "req-1", "/v1/chat/completions", None) | ||
| await gen.__anext__() |
Signed-off-by: Lifan Sun <lifansun1412@gmail.com>
chrikrah
left a comment
There was a problem hiding this comment.
@lfsun02 I would merge this once the anext() revision you mentioned is in, since that touches only the tests. At 2374d04 I reverted request.py to 691dffa with your tests kept, and mutated each half of the new allocation condition in turn. All three break a test:
$ PYTHONPATH=src python -m pytest src/tests/test_response_buffering.py -q
3 passed # at 2374d04
1 failed, 2 passed # request.py reverted: retained 4621438 bytes, limit 1048576
1 failed, 2 passed # condition cut to `not is_streaming`
1 failed, 2 passed # condition cut to `body_consumed_by_callback`
# not run: the rest of src/tests, and no live vLLM backend
The three red checks are the self-hosted runner's minikube (Kubernetes cluster unreachable, No such container: minikube), not this change, so your next push should re-run them.
When that revision is up, I will re-run the three mutations on the new head.
thanks for your review. the revision is already in 2374d04 |
|
@ruizhang0101 gently bumping up. the failing check is due to timeout, not relevant to the change |
|
looks like there are some infra issues. in the logs it shows the vLLM backend pods were repeatedly OOMKilled on the self-hosted runner, so every routing test failed at model listing before any inference request reached the router. |
Summary
process_requestaccumulated every proxied response body into abytearray, including streaming responses where nothing on the default path reads it. The router therefore held a full copy of every in-flight stream until the stream ended, so router memory grew with (concurrent streams × response size).The loop already guarded the append with
if full_response is not None:but the buffer was initialized tobytearray()unconditionally and was neverNone.Change
full_responseis now allocated only when a consumer exists:post_requestcustom callbackSo the body is buffered for every non-streaming response, and for streaming responses only when a
post_requestcallback is configured andbackground_tasksis available to run it.Tests
-swhen doinggit commit[Bugfix],[Feat], and[CI].Detailed Checklist (Click to Expand)
Thank you for your contribution to production-stack! Before submitting the pull request, please ensure the PR meets the following criteria. This helps us maintain the code quality and improve the efficiency of the review process.
PR Title and Classification
Please try to classify PRs for easy understanding of the type of changes. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:
[Bugfix]for bug fixes.[CI/Build]for build or continuous integration improvements.[Doc]for documentation fixes and improvements.[Feat]for new features in the cluster (e.g., autoscaling, disaggregated prefill, etc.).[Router]for changes to thevllm_router(e.g., routing algorithm, router observability, etc.).[Misc]for PRs that do not fit the above categories. Please use this sparingly.Note: If the PR spans more than one category, please include all relevant prefixes.
Code Quality
The PR need to meet the following code quality standards:
pre-committo format your code. SeeREADME.mdfor installation.DCO and Signed-off-by
When contributing changes to this project, you must agree to the DCO. Commits must include a
Signed-off-by:header which certifies agreement with the terms of the DCO.Using
-swithgit commitwill automatically add this header.What to Expect for the Reviews
We aim to address all PRs in a timely manner. If no one reviews your PR within 5 days, please @-mention one of YuhanLiu11
, Shaoting-Feng or ApostaC.