fix(sagemaker): forward stream events as they arrive to cut TTFT - #34338
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR fixes SageMaker streaming latency by dropping the
Confidence Score: 5/5Safe to merge — the production changes are four one-line argument deletions, each with dedicated regression tests that fail on the old code and pass after the fix. The change is minimal: dropping a No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/llms/sagemaker/chat/transformation.py | Drops chunk_size=1024 from both sync iter_bytes() and async aiter_bytes() calls — minimal, correct change that lets httpx yield transport bytes as they arrive |
| litellm/llms/sagemaker/completion/handler.py | Extracts a new make_sync_call() method mirroring make_async_call(), removes chunk_size=1024 from async path; logging_obj is accepted but unused in make_sync_call, consistent with the original inline code which also never passed it to the raw httpx client |
| tests/test_litellm/llms/sagemaker/test_sagemaker_chat_transformation.py | New regression tests: mock-only (no network calls), verify per-frame incremental delivery for both sync and async paths; test helper code is duplicated verbatim with the completion handler test file |
| tests/test_litellm/llms/sagemaker/test_sagemaker_completion_handler.py | New regression tests for native sagemaker/ completion path; covers sync, async, and 500-error paths; helper classes are identical copies of those in the chat transformation test file |
Reviews (3): Last reviewed commit: "test(sagemaker): assert make_sync_call m..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…k failure Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptile review again |
… TTFT Mirror the sagemaker_chat fix on the native sagemaker/ streaming path: the sync and async completion handlers read the invocations-response-stream body with iter_bytes(chunk_size=1024) / aiter_bytes(chunk_size=1024), so httpx withholds bytes until 1024 accumulate and tokens arrive in gap-then-burst waves. Drop the fixed chunk size so each decoded event is forwarded as its bytes arrive. Also add a boundary-agnostic decoder test proving frames reassemble correctly regardless of where transport reads split the stream. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…_sync_call Extract the inline sync streaming post/decode into make_sync_call so it can be exercised with an injected client, mirroring make_async_call, and add a sync regression test that each token is forwarded after exactly one pulled frame. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
TLDR
Problem this solves:
How it solves it:
Relevant issues
Linear ticket
Resolves LIT-4313
Pre-Submission checklist
Screenshots / Proof of Fix
Live e2e against real SageMaker TGI endpoints (us-west-2), comparing a litellm proxy built from the merge-base (
d25bac5a41, stillchunk_size=1024) against this branch (5bc1df5e47), same endpoint and prompt, 3 runs each. A streaming client timestamps every SSE content delta as it arrives: "burst gaps" counts deltas that landed <1ms after the previous one (flushed together from the buffer), "arrival cadence" is the median gap between distinct arrivalssagemaker_chatthrough proxy/v1/chat/completions(TinyLlama-1.1B-Chat on TGI 1.4.0 withMESSAGES_API_ENABLED=true, 80 tokens):Native
sagemaker/through proxy (MPT-7B-Instruct on TGI 1.4.0, 25 tokens):The sync SDK paths (
litellm.completion(stream=True)) show the same collapse: nativesagemaker/goes from 19/24 burst gaps at ~126 ms cadence to 0/24 at ~33 ms, andsagemaker_chatfrom 13-16/24 to 2-3/24 at the model's ~8 ms token cadence. Together this exercises all four changed reads (sagemaker_chatsync/async, completion handler sync/async): with the fix every delta arrives at the provider's real token cadence instead of in 1024-byte flush waves. Native-path TTFT moves less because MPT-7B's own first token dominates it; the chat endpoint's TTFT halves. The temporary Messages API endpoint was deleted after the runEarlier adapter-path proof with simulated arrival timing (pre-provisioning)
This drives real AWS event-stream frames through exactly the adapter path (
httpx.Response.iter_bytes(...)->AWSEventStreamDecoder.iter_bytes(...)) with a stream that emits one frame every 40ms, so only the network arrival timing is simulated; the buffering code under test is the real thing. Comparison captured at commit 0ff3ade by toggling the chunk_size the code used before vs after:Before (
chunk_size=1024), provider emits a frame every 40ms:After (no fixed threshold):
Type
🐛 Bug Fix
Changes
Both SageMaker streaming surfaces read the
/invocations-response-streambody withresponse.iter_bytes(chunk_size=1024)/response.aiter_bytes(chunk_size=1024). httpx'sByteChunkerwithholds bytes untilchunk_sizeaccumulates before yielding, so when each AWS event frame is small (a single-token SSE delta is ~150-270 bytes) the client cannot see delta N until enough later frames arrive to cross 1024 bytes;AWSEventStreamDecoderthen drains every complete frame from the released block in one pass, producing the ticket's wait-then-drain-a-burst pattern and inflating client-observed time-to-first-tokenThe fix drops the
chunk_sizeargument on both reads so httpx yields transport bytes as they arrive and each decoded event is forwarded immediately. This matches the bedrock invoke path, whosestream_chunk_sizedefaults toNonesagemaker_chat(litellm/llms/sagemaker/chat/transformation.py):The native
sagemakercompletion path had the identical buffering on its real event-stream reads, so it is fixed the same way for consistency (litellm/llms/sagemaker/completion/handler.py, synccompletion+ asyncmake_async_call):On the ticket's second proposed root cause (
supports_stream_param_in_request_bodyreturningFalseso"stream": trueis missing from the signed body): that does not reproduce.streamalready reaches the signed body today because it flows throughoptional_paramsinto the transformed request ({**optional_params}inOpenAIGPTConfig.transform_request) and is serialized by_sign_requestbefore SigV4 signing. Flipping the property is a no-op for the request that actually goes out (the signed path sendssigned_json_body, and_add_stream_param_to_request_bodyonly mutates the post-signdatadict thatsagemaker_chatnever sends), so I left the property alone rather than make a change with no runtime effect that also breaks an existing backwards-compat assertion. A regression test pins thatstreamsurvives signingTests in
tests/test_litellm/llms/sagemaker/test_sagemaker_chat_transformation.pyfeed real AWS event-stream frames (one small SSE delta each) through the actualget_sync_custom_stream_wrapper/get_async_custom_stream_wrapperwiring using a stream that counts how many source frames have been pulled. They prove the first delta is emitted after exactly one frame is pulled (not held until a 1024-byte buffer fills), that delta i is emitted after exactly i+1 frames (steady, not held-and-replayed as a burst), and thatstream: truesurvives SigV4 signing. A parametrized decoder test also re-chunks the concatenated stream at boundaries that deliberately ignore frame edges (1, 3, 7, 64, 4096 bytes) and asserts every delta still decodes in order exactly once, since removing the fixed chunk size lets httpx hand the decoder arbitrary transport-sized reads that can straddle or split framestests/test_litellm/llms/sagemaker/test_sagemaker_completion_handler.pycovers the nativesagemakerasync path throughmake_async_callwith an injected client, asserting each token is emitted after exactly one newly-pulled frame. The buffering tests fail on the pre-fix code and pass after, so they lock in the regressionDocs are updated separately in litellm-docs to distinguish native
sagemaker_chatstreaming from the legacysagemaker/fake-streaming pathFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/aef0886b17894acd972bab04f108708e
Requested by: @mateo-berri