Repository navigation
Conversation
…ing chunks (BerriAI#26181) Replace the per-chunk `accumulated_json += chunk` + full json.loads with a list-based buffer plus a cheap brace/bracket completeness heuristic so the buffer is only joined and parsed when the latest shard looks like a closed envelope. The empty-string flush from __next__/__anext__ at StopIteration still forces a final attempt so partial-completion semantics are unchanged. Re-PR of BerriAI#26187 against the new litellm_internal_staging base (the original PR auto-closed when the daily-dated litellm_oss_staging_04_25_2026 base branch was retired). Scope intentionally narrowed to the Gemini hot path; the parallel fixes for Anthropic chat handler and SageMaker common_utils will follow once this lands so each can be reviewed independently. Signed-off-by: Tai An <antai12232931@outlook.com>
Greptile SummaryReplaces the
Confidence Score: 4/5The change is narrow (one file, one method) and the heuristic is safe for Gemini's object/array envelope format; the main gap is the absence of inline tests. The accumulation and heuristic logic is correct for Gemini's response format, and the StopIteration flush path still forces a final parse attempt. The flush path does introduce a minor behavioural divergence from the original (appending an empty string on each call rather than a no-op concatenation), and no automated tests ship with the PR to guard against regressions in the multi-shard assembly logic. litellm/llms/vertex_ai/gemini/vertex_and_google_ai_studio_gemini.py — specifically the flush path in handle_accumulated_json_chunk and the missing test coverage.
|
| Filename | Overview |
|---|---|
| litellm/llms/vertex_ai/gemini/vertex_and_google_ai_studio_gemini.py | Replaces O(n²) string concatenation + repeated json.loads with list-based accumulation and a closing-brace heuristic; logic is sound for Gemini's object/array response envelope, but no tests are included and the flush path introduces a minor list-growth divergence from the original no-op behaviour. |
Comments Outside Diff (1)
-
litellm/llms/vertex_ai/gemini/vertex_and_google_ai_studio_gemini.py, line 3488-3510 (link)The test plan in the PR description explicitly leaves the unit-test checkbox unchecked, with a promise to add them in a follow-up. The repository's review rules require any PR claiming to fix an issue to include evidence of resolution (passing tests or a detailed summary of changes). The summary is present, but the automated tests that would verify the O(n²) regression is fixed and that multi-shard JSON is assembled correctly are absent. Adding at least the "same-shape unit tests as perf(streaming): eliminate O(n²) json.loads in accumulated-JSON paths (Vertex, Anthropic, SageMaker) #26187" inline — covering chunked JSON arriving in multiple shards, exactly one
json.loadscall after the closing brace, and buffer cleared on success — would satisfy this requirement.Rule Used: What: Ensure that any PR claiming to fix an issue ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "perf(vertex/gemini): eliminate O(n²) jso..." | Re-trigger Greptile
| if self.chunk_type == "accumulated_json" and self.accumulated_json_chunks: | ||
| return self.handle_accumulated_json_chunk(chunk="") | ||
| raise StopIteration |
There was a problem hiding this comment.
Unbounded list growth on repeated flush calls
handle_accumulated_json_chunk("") appends an empty string to accumulated_json_chunks every time the flush path is taken. If json.loads fails on the incomplete/invalid accumulated JSON at stream end and the consumer's outer loop calls __next__ again, each call appends another "" — the list grows indefinitely. The original self.accumulated_json += "" was a no-op and did not change the buffer. Functionally the join result is identical (empty strings are transparent to "".join), but this is a silent behavioral divergence from the original: memory usage grows with every repeated call to the flush path. Guarding against repeated flush appends (e.g., by not appending when chunk is empty) would preserve the no-op semantics of the original.
| # parser only joins+json.loads when the trailing brace/bracket suggests | ||
| # a complete envelope, instead of re-parsing the growing buffer on every | ||
| # chunk and freezing the event loop. | ||
| self.accumulated_json_chunks: list = [] |
There was a problem hiding this comment.
The type annotation
list is valid but imprecise — the list holds str elements only. Using list[str] makes the contract explicit for static analysis and readers.
| self.accumulated_json_chunks: list = [] | |
| self.accumulated_json_chunks: list[str] = [] |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
list-accumulate + join-on-demand is the right shape, and you updated both next and anext flush guards to accumulated_json_chunks — easy spot to miss one and silently break the async final-flush, so good. one thing on the heuristic: |
|
Yep, exactly that. The whole point is to stop re- On the Happy to add the same-shape unit tests as #26187 inline here (multi-shard assembly, exactly-one-parse-after-close, buffer-cleared-on-success) to close greptile's P2 — just say the word. |
|
Superseded by #31297 (merged 2026-06-26), which resolved the same O(n^2) re-parse of the accumulated Gemini stream. Closing — thanks! |
Summary
Closes #26181 (re-PR of #26187 — that PR was auto-closed when the daily-dated
litellm_oss_staging_04_25_2026base branch was retired in favor oflitellm_internal_staging).When Vertex AI (Gemini) streams fragmented SSE chunks,
handle_accumulated_json_chunkpreviously:self.accumulated_json += chunk, O(n) copy each call → O(n²) total)json.loads()on the full accumulated buffer after every chunkFor multi-MB tool-call payloads or long code generations,
json.loads— a single CPython C call that holds the GIL — blocks for seconds at a time. In the default single-process uvicorn deployment this freezes the asyncio event loop, liveness probes time out, and kubelet restarts the pod.The reporter (@6matt) confirmed this with 7 consecutive
py-spythread dumps spanning ~70 seconds, all showing MainThread stuck injson.loads → json.decoder.decode → json.decoder.raw_decodefromhandle_accumulated_json_chunk.Fix
Two changes:
self.accumulated_json += chunkwithself.accumulated_json_chunks.append(chunk). O(n²) → O(n) total copy work."".join(...)+json.loads()when the latest shard's last non-whitespace character is}or]. Avoids repeatedly parsing the growing buffer on every chunk. The empty-string flush from__next__/__anext__atStopIterationstill forces one final parse attempt, so partial-completion semantics are unchanged.Scope
litellm/llms/vertex_ai/gemini/vertex_and_google_ai_studio_gemini.py).accumulated_json: str→accumulated_json_chunks: list).litellm/llms/anthropic/chat/handler.pyandlitellm/llms/sagemaker/common_utils.pywill follow in separate PRs after this lands, so each provider can be reviewed independently.Test Plan
json.loadscall after the closing brace, final state cleared.🤖 Generated with Claude Code (AI-assisted, human reviewed)