refactor(streaming): extract chunk_creator dispatch so basedpyright can analyze it - #30793
Conversation
…an analyze it CustomStreamWrapper.chunk_creator packed a ~20-branch provider if/elif dispatch plus heavy post-processing into one body, pushing it past basedpyright's code-flow complexity ceiling. The checker emitted "Code is too complex to analyze" and skipped the entire function, so every type error in one of the hottest streaming paths was invisible and unguarded. Extract the provider dispatch into _dispatch_provider_chunk, which returns a tagged union (_ProviderChunkParsed | _ProviderChunkEarlyReturn) so the original early returns and StopIteration semantics are preserved exactly. chunk_creator now sits well under the ceiling and basedpyright type-checks both functions. Restoring analysis surfaced pre-existing latent type noise in two legacy branches: dynamic proto attribute access in the vertex_ai path and Optional subscripting of completion_stream in petals/palm fake-streaming. Both are cast to Any at the point of dynamic access, matching the intent of the dead # type: ignore comments they already carried, so no basedpyright budget ceiling moves and no unrelated drift is absorbed. Behavior is unchanged. The mapped streaming_handler tests pass identically to main; the two vertex tests that fail also fail on main, a pre-existing test isolation issue where proto must be present in sys.modules.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR extracts the large provider dispatch block from
Confidence Score: 5/5Safe to merge — this is a pure structural refactor with no behavior changes to the streaming hot path. The extraction is mechanically equivalent to the original: No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/streaming_handler.py | Extracts provider dispatch into _dispatch_provider_chunk returning a typed tagged union; GChunk moved from a local import to the already-existing module-level import; StopIteration propagation and early-return semantics are preserved exactly. |
| tests/test_litellm/litellm_core_utils/test_streaming_handler.py | Adds 14 new unit tests driving _dispatch_provider_chunk directly across every legacy provider branch; all tests use mock objects with no real network calls, satisfying the no-network rule for this test directory. |
Reviews (3): Last reviewed commit: "test(streaming): cover _dispatch_provide..." | Re-trigger Greptile
…e builtin dict generics The chunk_creator refactor moved the ~20-branch provider dispatch into the new _dispatch_provider_chunk helper, but many provider paths had no direct test, so those moved lines showed up as uncovered and pushed patch coverage below target. Add focused tests that drive the extracted helper across the vllm, petals, palm, cached_response, legacy vertex_ai (text, no-candidate and function-call forms), registered custom-provider, text-completion-codestral, triton, ai21 and text-completion-openai branches, asserting the tagged-union contract (_ProviderChunkParsed vs _ProviderChunkEarlyReturn) along with the content, finish_reason, usage and fake-stream slicing side effects. This locks in the behavior-preserving thesis of the refactor: a mutation in any of those branches now fails a test Also switch the seven Dict[str, Any] annotations the refactor introduced to the builtin dict[str, Any] generics, keeping the UP006 strict-rule budget within its ceiling without moving any baseline
b95200d to
f6958e1
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f6958e1. Configure here.
The method was extracted from chunk_creator by upstream BerriAI#30793 (commit 6437b81 in our tree). The sync merge took upstream's version of the file for conflicting sections, dropping the 370-line method body while keeping the call site in chunk_creator. Also restores LlmProviders import that the body needs but was removed as 'unused' when the body was absent.
Relevant issues
Part of an effort to make basedpyright actually analyze the codebase's hottest functions. Across
litellm/, basedpyright bails out with "Code is too complex to analyze" on exactly three functions, leaving their entire bodies unchecked:completion(litellm/main.py, ~3,500 lines),exception_type(exception_mapping_utils.py, ~2,275 lines), andCustomStreamWrapper.chunk_creator(this PR). This is the first of three, scoped on its own.Linear ticket
N/A
Pre-Submission checklist
tests/test_litellm/litellm_core_utils/test_streaming_handler.pydrives the extracted_dispatch_provider_chunkdirectly across the legacy provider branches (vllm, petals, palm, cached_response, legacy vertex_ai, registered custom providers and text-completion-codestral) and asserts the tagged-union contract plus the content, finish_reason and fake-stream slicing side effects, on top of the existing mapped suite that already exerciseschunk_creatoracross providersmake test-unitchunk_creatorand its new helper in one fileWhat and why
CustomStreamWrapper.chunk_creatorpacked a ~20-branch providerif/elifdispatch plus heavy post-processing into a single body, which pushed it past basedpyright's code-flow complexity ceiling. basedpyright responded with "Code is too complex to analyze" and skipped the whole function, so every type error in one of the hottest streaming paths was invisible and unguarded.This extracts the provider dispatch into
_dispatch_provider_chunk, which returns a tagged union (_ProviderChunkParsed | _ProviderChunkEarlyReturn) so the original early-return andStopIterationsemantics are preserved exactly.chunk_creatornow sits well under the ceiling and basedpyright type-checks both functions.Restoring analysis surfaced pre-existing latent type noise in two legacy branches: dynamic
protoattribute access in thevertex_aipath, andOptionalsubscripting ofcompletion_streamin thepetals/palmfake-streaming paths. Both are cast toAnyat the point of dynamic access, matching the intent of the dead# type: ignorecomments they already carried, so no basedpyright budget ceiling moves and no unrelated drift is absorbed.Screenshots / Proof of Fix
Before, on
litellm_internal_staging, basedpyright refuses to analyze the function:After this PR, the bailout is gone and the per-rule budget gate passes on the unchanged budget (no ceilings moved):
Behavioral proof that streaming is unchanged, run against a live proxy hitting real provider APIs and spending real money. The refactored dispatch routes by provider, so this streams through three distinct branches (native Anthropic, Azure AI, OpenAI) and checks each one returns incremental
delta.content, a terminalfinish_reason, a usage chunk, thendata: [DONE].Repeat through the Azure AI and OpenAI branches (same request shape,
modelset toazure-haiku-4-5thengpt-5.5). Both return the same incremental-delta thenfinish_reasonthen usage then[DONE]structure.The proxy computes a real, non-zero cost for each stream and writes spend to the DB, which is what proves real money was billed (from
litellm.log):Type
🧹 Refactoring
Changes
Extracts the provider dispatch out of
CustomStreamWrapper.chunk_creatorinto a new typed helper_dispatch_provider_chunkreturning a tagged union, restoring basedpyright analysis of both functions, and casts two dynamic legacy branches toAnyso the newly-visible errors do not move any budget ceiling. No behavior change.A follow-up commit adds the test coverage described above and switches the seven
Dict[str, Any]annotations the refactor introduced to the builtindict[str, Any]generics, which keeps theUP006strict-rule budget within its ceiling without moving any baselineNote: two pre-existing tests in the mapped file (
test_gemini_legacy_vertex_stop_finish_reason_normalised,test_gemini_legacy_vertex_tool_calls_finish_reason_with_stop_enum) fail in deterministic order onlitellm_internal_stagingas well; they depend onprotobeing present insys.modulesfrom an earlier test. They are unrelated to this change and left for a separate fix.Note
Medium Risk
Touches core streaming dispatch for many LLM providers; behavior is intended to be identical but regressions would affect all streamed completions. Risk is mitigated by broad new unit tests and an explicit no-behavior-change refactor.
Overview
Pulls the large provider-specific
if/elifchain out ofCustomStreamWrapper.chunk_creatorinto_dispatch_provider_chunk, so basedpyright can type-check the hot streaming path instead of bailing with “too complex to analyze.”The helper returns a small tagged union (
_ProviderChunkParsedvs_ProviderChunkEarlyReturn) so existing behaviors stay the same: pass-through chunks for registered custom providers,Nonewhen only a finish reason arrives, andStopIterationafter the stream has finished.chunk_creatornow only runs dispatch and keeps the shared post-processing (tool calls,return_processed_chunk_logic, etc.).cast(Any, …)on legacy vertex_ai and petals/palm dynamic access addresses type noise the refactor surfaced, without changing runtime logic.Tests drive
_dispatch_provider_chunkdirectly across vllm, fake-stream providers, cached_response, legacy vertex, codestral, triton, ai21, and custom-provider early-return cases.Reviewed by Cursor Bugbot for commit f6958e1. Bugbot is set up for automated code reviews on this repo. Configure here.