fix(google-genai): route streaming chunks to GeminiPassthroughLogging… - #24114
fix(google-genai): route streaming chunks to GeminiPassthroughLogging…#24114awais786 wants to merge 7 commits into
Conversation
…Handler so success_callbacks fire
streaming_iterator was tagging Google GenAI chunks as VERTEX_AI, causing
_route_streaming_logging_to_handler to use the wrong parser. Result was
None so async_complete_streaming_response was never set and all success
callbacks were silently skipped for /models/{model}:streamGenerateContent.
- Add EndpointType.GOOGLE_GENAI enum value
- Fix streaming_iterator to use GOOGLE_GENAI instead of VERTEX_AI
- Add GOOGLE_GENAI branch in streaming_handler routing to GeminiPassthroughLoggingHandler
- Add regression tests (7 tests, all passing)
Fixes BerriAI#24097
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes a silent callback regression for Key changes:
Remaining gaps:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| litellm/google_genai/streaming_iterator.py | Correctly switches endpoint_type to GOOGLE_GENAI and url_route to the real streaming path; now also passes model=self.model explicitly. However, the sync iterator (GoogleGenAIGenerateContentStreamingIterator.next) still never calls _handle_async_streaming_logging on StopIteration, meaning synchronous callers remain silently broken. |
| litellm/proxy/pass_through_endpoints/streaming_handler.py | Adds GOOGLE_GENAI branch routing to GeminiPassthroughLoggingHandler and correctly avoids pre-setting async_complete_streaming_response (which would have triggered an early-return guard). chunk_processor does not forward model for the new GOOGLE_GENAI endpoint type, leaving a latent fallback-to-URL-parsing gap. |
| litellm/types/passthrough_endpoints/pass_through_endpoints.py | Minimal, correct addition of GOOGLE_GENAI = "google-genai" to the EndpointType enum; no issues. |
| litellm/litellm_core_utils/litellm_logging.py | Formatting-only changes (parenthesis removal, line wrapping); no logic modifications. |
| litellm/proxy/management_endpoints/team_endpoints.py | Formatting-only change (wrapping a long type annotation); no logic modifications. |
| tests/test_litellm/proxy/pass_through_endpoints/llm_provider_handlers/test_google_genai_success_callbacks.py | The integration test (TestStreamingHandlerGoogleGenAIRouting) is well-designed — it uses a real LiteLLMLoggingObj and spy logger to verify that callbacks actually fire through the full async_success_handler path. Several test-quality issues have been flagged in previous review threads (dead-code assertion messages, fragile sys.path insertion, raw_bytes type mismatch, inspect-based source tests). |
Sequence Diagram
sequenceDiagram
participant SI as streaming_iterator.py<br/>(Async/SyncIterator)
participant SH as streaming_handler.py<br/>(_route_streaming_logging_to_handler)
participant GH as GeminiPassthroughLoggingHandler
participant LL as litellm_logging.py<br/>(async_success_handler)
participant CB as success_callbacks
Note over SI: StopAsyncIteration raised
SI->>SH: _route_streaming_logging_to_handler(<br/>endpoint_type=GOOGLE_GENAI,<br/>url_route=/models/{model}:streamGenerateContent,<br/>model=self.model)
SH->>GH: _handle_logging_gemini_collected_chunks(...)
GH-->>SH: {result: ModelResponse, kwargs: {response_cost, model}}
Note over SH: Does NOT pre-set<br/>async_complete_streaming_response
SH->>LL: async_success_handler(result, **kwargs)
Note over LL: Guard at line 2492:<br/>async_complete_streaming_response<br/>NOT in model_call_details → continues
Note over LL: call_type == "pass_through_endpoint"<br/>→ sets async_complete_streaming_response
LL->>CB: async_log_success_event(kwargs, response_obj, ...)
Note over SI,CB: ❌ Sync iterator (GoogleGenAIGenerateContentStreamingIterator)<br/>never reaches this flow — StopIteration raised without logging
Comments Outside Diff (2)
-
litellm/google_genai/streaming_iterator.py, line 96-104 (link)Sync iterator never fires logging callbacks
GoogleGenAIGenerateContentStreamingIterator.__next__raisesStopIterationwithout calling_handle_async_streaming_logging, so any synchronous caller of this iterator will silently skip all success callbacks — the same root bug that this PR fixes for the async path still exists in the sync path.The only fix applied is in
AsyncGoogleGenAIGenerateContentStreamingIterator.__anext__(line 158), which correctly awaits_handle_async_streaming_logging()onStopAsyncIteration. The sync__next__has no equivalent.Since
_handle_async_streaming_loggingisasync, it cannot beawait-ed directly from__next__. A possible approach is to schedule logging viaasyncio.get_event_loop().run_until_complete(...)if a running loop is available, or introduce a sync logging helper. At a minimum, this should be documented as a known limitation of the sync iterator.def __next__(self): try: chunk = next(self.stream_iterator) self.collected_chunks.append(chunk) return chunk except StopIteration: # TODO: _handle_async_streaming_logging is async and cannot be # awaited here — sync callers do NOT fire success callbacks. raise StopIteration
-
litellm/proxy/pass_through_endpoints/streaming_handler.py, line 83-94 (link)chunk_processornever passesmodelfor GOOGLE_GENAIchunk_processorcreates the logging task without forwardingmodelto_route_streaming_logging_to_handler. For the newGOOGLE_GENAIbranch inside_route_streaming_logging_to_handler, the handler falls back toextract_model_from_url(url_route).If
endpoint_type == EndpointType.GOOGLE_GENAIis ever routed throughchunk_processor(rather than thestreaming_iterator.pypath), model extraction silently falls back to URL parsing and will return"unknown"for any URL that doesn't contain/models/{model}. This could silently compute an incorrect cost.The
VERTEX_AIbranch inside_route_streaming_logging_to_handleralready receivesmodelbecauseVertexPassthroughLoggingHandler._handle_logging_vertex_collected_chunksaccepts it; parity should be maintained for the new GOOGLE_GENAI branch.
Last reviewed commit: "fix(google-genai): r..."
| mock_gemini.assert_called_once(), ( | ||
| "GeminiPassthroughLoggingHandler was NOT called for GOOGLE_GENAI endpoint — " | ||
| "callbacks would be silently skipped (issue #24097 not fixed)" | ||
| ) |
There was a problem hiding this comment.
Custom assertion messages are dead code
The pattern mock_gemini.assert_called_once(), ("message string") creates a tuple expression — it does not attach the string as the failure message to the assertion.
If assert_called_once() fails, it raises AssertionError with the default mock message (e.g., "Expected 'mock' to be called once. Called 0 times."), not the custom string. The string after the comma is evaluated and discarded.
The same bug appears on line 168 (assert_not_called()).
To attach a custom failure message, use pytest's assert statement:
| mock_gemini.assert_called_once(), ( | |
| "GeminiPassthroughLoggingHandler was NOT called for GOOGLE_GENAI endpoint — " | |
| "callbacks would be silently skipped (issue #24097 not fixed)" | |
| ) | |
| assert mock_gemini.call_count == 1, ( | |
| "GeminiPassthroughLoggingHandler was NOT called for GOOGLE_GENAI endpoint — " | |
| "callbacks would be silently skipped (issue #24097 not fixed)" | |
| ) |
| cd /Users/awais.qureshi/Documents/devstack/forks/litellm | ||
| pyenv activate wagtail-chat-env | ||
| python -m pytest tests/test_litellm/proxy/pass_through_endpoints/llm_provider_handlers/test_google_genai_success_callbacks.py -v |
There was a problem hiding this comment.
Developer local machine path should be removed
The docstring contains a personal filesystem path and environment activation command specific to the author's machine. This will be confusing and misleading to other contributors.
| cd /Users/awais.qureshi/Documents/devstack/forks/litellm | |
| pyenv activate wagtail-chat-env | |
| python -m pytest tests/test_litellm/proxy/pass_through_endpoints/llm_provider_handlers/test_google_genai_success_callbacks.py -v | |
| Run: | |
| python -m pytest tests/test_litellm/proxy/pass_through_endpoints/llm_provider_handlers/test_google_genai_success_callbacks.py -v |
| endpoint_type=EndpointType.GOOGLE_GENAI, | ||
| start_time=datetime.now(), | ||
| end_time=datetime.now(), | ||
| raw_bytes=b"data: {}\n", |
There was a problem hiding this comment.
_route_streaming_logging_to_handler expects raw_bytes: List[bytes], but b"data: {}\n" is a plain bytes object, not a list. This doesn't cause a test failure here because _convert_raw_bytes_to_str_lines is mocked, but it misrepresents the correct call signature and could confuse future readers.
| raw_bytes=b"data: {}\n", | |
| raw_bytes=[b"data: {}\n"], |
| start_time=datetime.now(), | ||
| end_time=datetime.now(), | ||
| raw_bytes=b"data: {}\n", | ||
| model="gemini-1.5-pro", |
| mock_gemini.assert_not_called(), ( | ||
| "GeminiPassthroughLoggingHandler was called for VERTEX_AI endpoint — " | ||
| "routing regression detected" | ||
| ) |
There was a problem hiding this comment.
Same dead-code assertion message pattern
mock_gemini.assert_not_called(), ("message") has the same issue as line 125-128 — the custom message string is unreachable. Use:
| mock_gemini.assert_not_called(), ( | |
| "GeminiPassthroughLoggingHandler was called for VERTEX_AI endpoint — " | |
| "routing regression detected" | |
| ) | |
| assert mock_gemini.call_count == 0, ( | |
| "GeminiPassthroughLoggingHandler was called for VERTEX_AI endpoint — " | |
| "routing regression detected" | |
| ) |
…Handler so success_callbacks fire
streaming_iterator was tagging Google GenAI chunks as VERTEX_AI, causing
_route_streaming_logging_to_handler to use the wrong parser. Result was
None so async_complete_streaming_response was never set and all success
callbacks were silently skipped for /models/{model}:streamGenerateContent.
- Add EndpointType.GOOGLE_GENAI enum value
- Fix streaming_iterator to use GOOGLE_GENAI instead of VERTEX_AI
- Add GOOGLE_GENAI branch in streaming_handler routing to GeminiPassthroughLoggingHandler
- Add regression tests (7 tests, all passing)
Fixes BerriAI#24097
| # Set async_complete_streaming_response so function-based success_callbacks | ||
| # are not silently skipped when self.stream is True (see async_success_handler). | ||
| litellm_logging_obj.model_call_details[ | ||
| "async_complete_streaming_response" | ||
| ] = standard_logging_response_object |
There was a problem hiding this comment.
async_complete_streaming_response now fires for ALL passthrough endpoint types
The placement of this new block — after the if/elif chain — means async_complete_streaming_response is now set for every endpoint type: Anthropic, Vertex AI, OpenAI, and Google GenAI. Before this PR, it was never set, so function-based success_callbacks were silently skipped for all passthrough streaming endpoints. Now they will suddenly fire for Anthropic, Vertex AI, and OpenAI passthrough callers that previously never received them.
This is a broader behavioral change than the PR description suggests and could surprise existing users of those other passthrough endpoints. Per the project's backwards-compatibility policy, unintended behaviour changes should be gated behind a feature flag unless they are clearly a universally correct fix.
If this is intentional (fixing the same silent-skip bug for all providers), it should be called out explicitly in the PR description and ideally covered by tests for each of the other endpoint types. If it's only meant to fix Google GenAI, the block should be placed inside the elif endpoint_type == EndpointType.GOOGLE_GENAI: branch.
Rule Used: What: avoid backwards-incompatible changes without... (source)
| @@ -51,7 +51,7 @@ async def _handle_async_streaming_logging( | |||
| passthrough_success_handler_obj=GLOBAL_PASS_THROUGH_SUCCESS_HANDLER_OBJ, | |||
| url_route="/v1/generateContent", | |||
There was a problem hiding this comment.
Hardcoded
url_route won't match streamGenerateContent logging checks
The url_route is hardcoded as "/v1/generateContent", but the actual endpoint being called for streaming is /models/{model}:streamGenerateContent. This discrepancy flows into _build_complete_streaming_response, which checks:
if "generateContent" in url_route or "streamGenerateContent" in url_route:Because "generateContent" is a substring of "streamGenerateContent", the current hardcoded value happens to pass the check. However, it also means extract_model_from_url("/v1/generateContent") would return "unknown" if the model kwarg were ever missing — the URL lacks the /models/{model} segment that the regex expects. The bug is latent today because model=self.model is passed explicitly, but the hardcoded URL does not reflect reality and could silently break model-extraction if the call signature changes.
Consider either passing the real URL or using a constant that includes the /models/{model} segment to make the intent clear.
|
|
||
| import pytest | ||
|
|
||
| sys.path.insert(0, os.path.abspath("../../..")) |
There was a problem hiding this comment.
Fragile
sys.path insertion uses a working-directory-relative path
os.path.abspath("../../..") is resolved relative to the current working directory at the time the test runs, not relative to the test file's location. When pytest is run from the repository root (the typical CI invocation), this expands to three levels above the repo root — an entirely wrong path that adds nothing useful.
If a path insertion is genuinely needed, use the file's own location as the anchor:
| sys.path.insert(0, os.path.abspath("../../..")) | |
| sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "../../../.."))) |
That said, the project's standard test infrastructure (pytest.ini / pyproject.toml with pythonpath or an installed package) should already make litellm importable without manual sys.path manipulation. Removing this line entirely may be the cleanest fix.
…Handler so success_callbacks fire
streaming_iterator was tagging Google GenAI chunks as VERTEX_AI, causing
_route_streaming_logging_to_handler to use the wrong parser. Result was
None so async_complete_streaming_response was never set and all success
callbacks were silently skipped for /models/{model}:streamGenerateContent.
- Add EndpointType.GOOGLE_GENAI enum value
- Fix streaming_iterator to use GOOGLE_GENAI instead of VERTEX_AI
- Add GOOGLE_GENAI branch in streaming_handler routing to GeminiPassthroughLoggingHandler
- Add regression tests (7 tests, all passing)
Fixes BerriAI#24097
| def test_uses_google_genai_not_vertex_ai(self): | ||
| """streaming_iterator must tag chunks as GOOGLE_GENAI.""" | ||
| import litellm.google_genai.streaming_iterator as si | ||
| src = inspect.getsource(si.BaseGoogleGenAIGenerateContentStreamingIterator) | ||
| assert "EndpointType.GOOGLE_GENAI" in src, ( | ||
| "streaming_iterator still references VERTEX_AI — fix not applied" | ||
| ) | ||
| assert "EndpointType.VERTEX_AI" not in src, ( | ||
| "streaming_iterator still uses VERTEX_AI — must use GOOGLE_GENAI" | ||
| ) |
There was a problem hiding this comment.
Source-inspection tests are fragile
inspect.getsource parses the raw source text of the class and checks for string literals. This means the assertion:
assert "EndpointType.VERTEX_AI" not in srcwould falsely fail if anyone adds a docstring or comment to BaseGoogleGenAIGenerateContentStreamingIterator that mentions the old type — e.g. # Previously tagged as EndpointType.VERTEX_AI. Comments and docstrings are included in getsource() output.
A more robust approach is to directly test the runtime behaviour rather than parsing source text:
def test_uses_google_genai_not_vertex_ai(self):
"""streaming_iterator must tag chunks as GOOGLE_GENAI, not VERTEX_AI."""
from litellm.types.passthrough_endpoints.pass_through_endpoints import EndpointType
from unittest.mock import MagicMock, patch, AsyncMock
captured = {}
async def capture_call(**kwargs):
captured.update(kwargs)
with patch(
"litellm.proxy.pass_through_endpoints.streaming_handler"
".PassThroughStreamingHandler._route_streaming_logging_to_handler",
side_effect=capture_call,
):
import asyncio, litellm.google_genai.streaming_iterator as si
# ... drive one iteration through an AsyncIterator
# then assert captured["endpoint_type"] == EndpointType.GOOGLE_GENAIThis tests the actual runtime value that ends up in the call, making it immune to comment or docstring changes.
…Handler so success_callbacks fire
streaming_iterator was tagging Google GenAI chunks as VERTEX_AI, causing
_route_streaming_logging_to_handler to use the wrong parser. Result was
None so async_complete_streaming_response was never set and all success
callbacks were silently skipped for /models/{model}:streamGenerateContent.
- Add EndpointType.GOOGLE_GENAI enum value
- Fix streaming_iterator to use GOOGLE_GENAI instead of VERTEX_AI
- Add GOOGLE_GENAI branch in streaming_handler routing to GeminiPassthroughLoggingHandler
- Add regression tests (7 tests, all passing)
Fixes BerriAI#24097
| PassThroughStreamingHandler._route_streaming_logging_to_handler( | ||
| litellm_logging_obj=self.litellm_logging_obj, | ||
| passthrough_success_handler_obj=GLOBAL_PASS_THROUGH_SUCCESS_HANDLER_OBJ, | ||
| url_route="/v1/generateContent", | ||
| url_route=f"/models/{self.model}:streamGenerateContent", | ||
| request_body=self.request_body or {}, | ||
| endpoint_type=EndpointType.VERTEX_AI, | ||
| endpoint_type=EndpointType.GOOGLE_GENAI, | ||
| start_time=self.start_time, | ||
| raw_bytes=self.collected_chunks, | ||
| end_time=end_time, |
There was a problem hiding this comment.
Missing
model kwarg leaves extraction to URL parsing
_route_streaming_logging_to_handler accepts an optional model parameter that is forwarded directly to _handle_logging_gemini_collected_chunks. The iterator already has self.model available, but it is not passed here — instead the handler falls back to parsing the model name out of the URL string via extract_model_from_url.
If self.model is ever None, the formatted URL becomes /models/None:streamGenerateContent, and extract_model_from_url will return the literal string "None". That string is then fed into litellm.completion_cost(model="None"), which will silently return an incorrect cost (or raise).
Passing model=self.model explicitly would be more direct and avoids this edge case.
| PassThroughStreamingHandler._route_streaming_logging_to_handler( | |
| litellm_logging_obj=self.litellm_logging_obj, | |
| passthrough_success_handler_obj=GLOBAL_PASS_THROUGH_SUCCESS_HANDLER_OBJ, | |
| url_route="/v1/generateContent", | |
| url_route=f"/models/{self.model}:streamGenerateContent", | |
| request_body=self.request_body or {}, | |
| endpoint_type=EndpointType.VERTEX_AI, | |
| endpoint_type=EndpointType.GOOGLE_GENAI, | |
| start_time=self.start_time, | |
| raw_bytes=self.collected_chunks, | |
| end_time=end_time, | |
| PassThroughStreamingHandler._route_streaming_logging_to_handler( | |
| litellm_logging_obj=self.litellm_logging_obj, | |
| passthrough_success_handler_obj=GLOBAL_PASS_THROUGH_SUCCESS_HANDLER_OBJ, | |
| url_route=f"/models/{self.model}:streamGenerateContent", | |
| request_body=self.request_body or {}, | |
| endpoint_type=EndpointType.GOOGLE_GENAI, | |
| start_time=self.start_time, | |
| raw_bytes=self.collected_chunks, | |
| end_time=end_time, | |
| model=self.model, |
…Handler so success_callbacks fire
streaming_iterator was tagging Google GenAI chunks as VERTEX_AI, causing
_route_streaming_logging_to_handler to use the wrong parser. Result was
None so async_complete_streaming_response was never set and all success
callbacks were silently skipped for /models/{model}:streamGenerateContent.
- Add EndpointType.GOOGLE_GENAI enum value
- Fix streaming_iterator to use GOOGLE_GENAI instead of VERTEX_AI
- Add GOOGLE_GENAI branch in streaming_handler routing to GeminiPassthroughLoggingHandler
- Add regression tests (7 tests, all passing)
Fixes BerriAI#24097
| # Set async_complete_streaming_response so function-based success_callbacks | ||
| # are not silently skipped when self.stream is True (see async_success_handler). | ||
| # This intentionally applies to ALL passthrough endpoint types: the guard in | ||
| # async_success_handler skips function callbacks for any streaming response | ||
| # that lacks this key, so all providers require it. | ||
| litellm_logging_obj.model_call_details[ | ||
| "async_complete_streaming_response" | ||
| ] = standard_logging_response_object |
There was a problem hiding this comment.
Pre-setting
async_complete_streaming_response triggers unconditional early return, silently skipping all callbacks
async_success_handler has an early-exit guard at line 2486–2487 of litellm_logging.py:
if "async_complete_streaming_response" in self.model_call_details:
return # break out of this.By setting async_complete_streaming_response on model_call_details before calling async_success_handler, this block causes the handler to return immediately — before the callback loops at lines 2571+ (CustomLogger, function-based, DynamoDB, OpenMeter, etc.) are ever reached.
Effect: All callbacks are silently skipped for every passthrough provider — Anthropic, Vertex AI, OpenAI, and Google GenAI alike. This is worse than the original bug (#24097), which only affected Google GenAI.
The prior working path for pass-through endpoints was: async_success_handler set async_complete_streaming_response internally (within the elif self.call_type == "pass_through_endpoint" branch at line 2543), and then execution continued to the callback loop below. By pre-setting the key before the call, that path is bypassed entirely.
The fix should set the key inside async_success_handler (or remove the pre-set), not before calling it.
…Handler so success_callbacks fire
streaming_iterator was tagging Google GenAI chunks as VERTEX_AI, causing
_route_streaming_logging_to_handler to use the wrong parser. Result was
None so async_complete_streaming_response was never set and all success
callbacks were silently skipped for /models/{model}:streamGenerateContent.
- Add EndpointType.GOOGLE_GENAI enum value
- Fix streaming_iterator to use GOOGLE_GENAI instead of VERTEX_AI
- Add GOOGLE_GENAI branch in streaming_handler routing to GeminiPassthroughLoggingHandler
- Add regression tests (7 tests, all passing)
Fixes BerriAI#24097
| mock_logging = MagicMock() | ||
| mock_logging.model_call_details = {} | ||
| mock_logging.async_success_handler = AsyncMock() | ||
|
|
||
| await PassThroughStreamingHandler._route_streaming_logging_to_handler( | ||
| litellm_logging_obj=mock_logging, | ||
| passthrough_success_handler_obj=MagicMock(), | ||
| url_route="/v1/models/gemini-1.5-flash:streamGenerateContent", | ||
| request_body={}, | ||
| endpoint_type=EndpointType.GOOGLE_GENAI, | ||
| start_time=datetime.now(), | ||
| end_time=datetime.now(), | ||
| raw_bytes=[b"data: {}\n"], | ||
| model="gemini-1.5-flash", | ||
| ) | ||
|
|
||
| assert mock_gemini.call_count == 1, ( | ||
| "GeminiPassthroughLoggingHandler was NOT called for GOOGLE_GENAI endpoint — " | ||
| "callbacks would be silently skipped (issue #24097 not fixed)" | ||
| ) | ||
| assert ( | ||
| "async_complete_streaming_response" in mock_logging.model_call_details | ||
| ), ( | ||
| "async_complete_streaming_response not set on model_call_details — " | ||
| "function-based success_callbacks will be silently skipped (self.stream=True guard)" | ||
| ) |
There was a problem hiding this comment.
Mock hides the regression this test is designed to catch
async_success_handler is replaced with a bare AsyncMock, so the test never exercises the real guard in litellm_logging.py:
# litellm_logging.py ~line 2492
if "async_complete_streaming_response" in self.model_call_details:
return # break out of this.Because streaming_handler.py pre-sets async_complete_streaming_response on model_call_details before calling async_success_handler, the real handler hits this guard and returns immediately — skipping every callback loop. The test then asserts the pre-set key as a positive assertion (lines 128–133), effectively validating the broken behaviour as correct.
A test that truly verifies "success callbacks fire" must either:
- Not mock
async_success_handler(let the real one run), or - Use
side_effectto verify that the callbacks list is iterated.
As written, the test passes even when no callback ever executes, defeating its stated purpose.
Rule Used: # Code Review Rule: Mock Test Integrity
What:... (source)
…Handler so success_callbacks fire
streaming_iterator was tagging Google GenAI chunks as VERTEX_AI, causing
_route_streaming_logging_to_handler to use the wrong parser. Result was
None so async_complete_streaming_response was never set and all success
callbacks were silently skipped for /models/{model}:streamGenerateContent.
- Add EndpointType.GOOGLE_GENAI enum value
- Fix streaming_iterator to use GOOGLE_GENAI instead of VERTEX_AI
- Add GOOGLE_GENAI branch in streaming_handler routing to GeminiPassthroughLoggingHandler
- Add regression tests (7 tests, all passing)
Fixes BerriAI#24097
…Handler so success_callbacks fire Fixes BerriAI#24097. Supersedes BerriAI#24114 (closed by author without merging). This revival preserves the structural fix and incorporates Greptile's review feedback that was outstanding when the original was closed. ## What was broken For the Google-native streaming endpoints `/models/{model}:streamGenerateContent` and `/v1beta/models/{model}:streamGenerateContent`, the streaming iterator was tagging collected chunks as `EndpointType.VERTEX_AI`. The downstream `_route_streaming_logging_to_handler` had no `VERTEX_AI` branch that knew how to parse Google GenAI native chunks, so `async_complete_streaming_response` was never set and every function-based and CustomLogger success callback was silently skipped on stream end. Sync callers were doubly broken: `__next__` re-raised `StopIteration` without ever invoking the logging route at all. ## The fix 1. Add `EndpointType.GOOGLE_GENAI = "google-genai"` to the enum. 2. In the async iterator, tag chunks as `GOOGLE_GENAI`, pass the real `/models/{model}:streamGenerateContent` URL, and forward the explicit `model` kwarg so downstream handlers do not have to fall back to URL parsing. 3. In the sync iterator, mirror the async path on `StopIteration` so sync callers also receive callbacks. 4. Add a `GOOGLE_GENAI` routing branch in `_route_streaming_logging_to_handler` that dispatches to `GeminiPassthroughLoggingHandler._handle_logging_gemini_collected_chunks`. ## Testing `tests/test_litellm/proxy/pass_through_endpoints/llm_provider_handlers/test_google_genai_streaming_callbacks.py` adds 7 regression tests, all runtime-behavior based (no `inspect.getsource` source-text checks): - `TestEndpointTypeEnum` — enum membership and regression guard for existing values. - `test_async_iterator_routes_with_google_genai_endpoint_type` — drives the async iterator end-to-end and asserts the captured routing kwargs. - `test_sync_iterator_routes_with_google_genai_endpoint_type` — same for the sync iterator (the gap PR BerriAI#24114 left open). - `test_streaming_handler_routes_google_genai_to_gemini_handler` — asserts the new branch wires to `GeminiPassthroughLoggingHandler` with the explicit `model` kwarg. - `test_streaming_handler_does_not_route_vertex_ai_to_gemini_handler` — regression guard that VERTEX_AI still uses `VertexPassthroughLoggingHandler`. - `test_callbacks_actually_fire_for_google_genai_endpoint` — end-to-end test using a real `Logging` instance and a real `CustomLogger` subclass, asserting that `async_log_success_event` actually fires. (PR BerriAI#24114 mocked `async_success_handler` itself, so it would have passed even when callbacks were never invoked.) Verified locally: 6 of 7 fail on `main`, all 7 pass with this commit. ## Differences from PR BerriAI#24114 - Sync iterator gap fixed (was unaddressed). - Tests rewritten to runtime behavior; removed `inspect.getsource` fragility, removed `sys.path` manipulation hack, replaced dead-code assertion-message-as-tuple pattern, removed `bytes` vs `List[bytes]` type mismatch in stub args. - End-to-end callback-fires test no longer mocks `async_success_handler`, so it actually exercises the dispatch. - Dropped unrelated formatting changes that were carried in the original PR (`team_endpoints.py`, `litellm_logging.py`). Co-Authored-By: Awais Qureshi <awais786> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Hi @awais786 — heads up that I've opened #25960 as a revival of this PR's approach, since the underlying issue (#24097) is still affecting users. The new PR preserves your structural fix (the EndpointType.GOOGLE_GENAI enum, streaming_iterator routing change, and streaming_handler branch) and addresses the outstanding Greptile feedback that was open when this PR was closed:
Also closed one functional gap I noticed while reviewing: the sync iterator's next never invoked the logging route on StopIteration, so sync callers were silently broken even with your async-side fix. The new PR mirrors the async path on the sync side. You're credited as the original author via Co-Authored-By trailer. Happy to defer to you if you'd rather pick this back up — just let me know. |
…ept it Resolves the silent strip of Anthropic Structured Outputs across the Vertex AI Claude transformation paths and the Anthropic-adapter re-merge. Consolidates and supersedes four stalled community PRs addressing overlapping aspects of the same root bug: - #23475 (Vertex AI Claude blanket-strip removal) - #23396 (Vertex AI Claude conditional passthrough) - #23706 (Anthropic adapter exclude output_config from non-Anthropic backends) - #22727 (Anthropic adapter strip output_config for non-Anthropic backends) Closes / addresses: #23380 (Vertex AI Claude output_config drop), related: #26423, #25079, #24549, #25971, #25957, #26163, #24856. What was broken --------------- * Vertex AI Claude paths called ``data.pop("output_config")`` and ``data.pop("output_format")`` unconditionally even when Vertex accepted those fields. Callers asking for Structured Outputs got a 200 with prose and never knew the schema constraints had been silently dropped (often masked for months by permissive fallback parsers). * The ``/v1/messages`` -> ``/chat/completions`` adapter (``LiteLLMMessagesToCompletionTransformationHandler``) re-merged the raw Anthropic-shaped ``output_config`` into ``completion_kwargs`` AFTER the translator already mapped its meaningful parts to ``response_format`` / ``reasoning_effort``. Non-Anthropic backends (Azure OpenAI, Fireworks, Bedrock Nova, etc.) then 400'd with "Extra inputs are not permitted". Approach -------- Vertex AI Claude (chat-completion + experimental_pass_through paths): Replace the unconditional pop with a sanitizer ``_sanitize_vertex_anthropic_output_params`` that strips only the Vertex-unsupported keys (today: ``effort``) from ``output_config`` while forwarding ``format`` and the legacy top-level ``output_format``. Defensive: non-dict ``output_config`` values are dropped to avoid sending malformed payloads downstream. Greptile P1 from PR #23396 addressed: when ``output_config`` carries both ``format`` and ``effort``, the prior conditional pass-through forwarded ``effort`` and reproduced the 400. The new helper filters per-key. Anthropic ``/v1/messages`` adapter: Add ``output_config`` to a named module-level constant ``ANTHROPIC_ONLY_REQUEST_KEYS`` and wire it into ``excluded_keys`` so the post-translation re-merge skips re-adding the raw key. This fixes the 400 on non-Anthropic backends and avoids the conflicting duplicate (``response_format`` + raw ``output_config``) on Anthropic-family backends. Greptile P2 from PR #23706 addressed: the constant gives reviewers one grep target instead of an inline literal that silently grows. Greptile P2 from PR #22727 addressed: ``extra_kwargs or {}`` is replaced with explicit ``is None`` checks so empty-dict callers no longer skip the fallback path. Tests ----- * tests/test_litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/ test_vertex_ai_partner_models_anthropic_transformation.py: - 5 new/updated cases plus a direct unit test for ``_sanitize_vertex_anthropic_output_params``. - Updated ``test_vertex_ai_claude_sonnet_4_5_structured_output_fix`` so its mock-injected ``output_format`` is asserted to FLOW THROUGH (the original test asserted the now-buggy strip behavior). * tests/test_litellm/llms/anthropic/experimental_pass_through/ adapters/test_handler_output_config_passthrough.py (new): - Constant export sanity, output_config strip with ``effort`` only, output_config strip with ``format`` only, regression guard that unrelated extras still flow, explicit-empty-dict path, and the ``extra_kwargs=None`` no-crash path. Test-quality fixes incorporated from Greptile review on the superseded PRs: * No ``inspect.getsource`` source-text assertions (PR #24114 / #23475). * ``sys.path`` insertion is anchored to ``__file__`` (PR #23706). * Assertion messages are positional, not tuple (PR #24114-class bug). * No ``or {}`` masking explicit empty dicts in helper signatures (PR #22727). Verified locally: 26/26 pass with this commit. The new tests fail (or fail to import) on ``main`` without it. Out of scope ------------ * The ``max_tokens`` capping logic from PR #22727 — independent concern, deserves its own PR with a focused test plan. * Architectural rework of the ``excluded_keys`` mechanism (Greptile P2 on PR #23706 noted point-fix growth). The named constant gives maintainers a clear place to extend; a registry-based approach would be a follow-up. Co-Authored-By: netbrah <netbrah> Co-Authored-By: s-zx <s-zx> Co-Authored-By: invoicepulse <invoicepulse> Co-Authored-By: cfdude <cfdude> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ept it Resolves the silent strip of Anthropic Structured Outputs across the Vertex AI Claude transformation paths and the Anthropic-adapter re-merge. Consolidates and supersedes four stalled community PRs addressing overlapping aspects of the same root bug: - BerriAI#23475 (Vertex AI Claude blanket-strip removal) - BerriAI#23396 (Vertex AI Claude conditional passthrough) - BerriAI#23706 (Anthropic adapter exclude output_config from non-Anthropic backends) - BerriAI#22727 (Anthropic adapter strip output_config for non-Anthropic backends) Closes / addresses: BerriAI#23380 (Vertex AI Claude output_config drop), related: BerriAI#26423, BerriAI#25079, BerriAI#24549, BerriAI#25971, BerriAI#25957, BerriAI#26163, BerriAI#24856. What was broken --------------- * Vertex AI Claude paths called ``data.pop("output_config")`` and ``data.pop("output_format")`` unconditionally even when Vertex accepted those fields. Callers asking for Structured Outputs got a 200 with prose and never knew the schema constraints had been silently dropped (often masked for months by permissive fallback parsers). * The ``/v1/messages`` -> ``/chat/completions`` adapter (``LiteLLMMessagesToCompletionTransformationHandler``) re-merged the raw Anthropic-shaped ``output_config`` into ``completion_kwargs`` AFTER the translator already mapped its meaningful parts to ``response_format`` / ``reasoning_effort``. Non-Anthropic backends (Azure OpenAI, Fireworks, Bedrock Nova, etc.) then 400'd with "Extra inputs are not permitted". Approach -------- Vertex AI Claude (chat-completion + experimental_pass_through paths): Replace the unconditional pop with a sanitizer ``_sanitize_vertex_anthropic_output_params`` that strips only the Vertex-unsupported keys (today: ``effort``) from ``output_config`` while forwarding ``format`` and the legacy top-level ``output_format``. Defensive: non-dict ``output_config`` values are dropped to avoid sending malformed payloads downstream. Greptile P1 from PR BerriAI#23396 addressed: when ``output_config`` carries both ``format`` and ``effort``, the prior conditional pass-through forwarded ``effort`` and reproduced the 400. The new helper filters per-key. Anthropic ``/v1/messages`` adapter: Add ``output_config`` to a named module-level constant ``ANTHROPIC_ONLY_REQUEST_KEYS`` and wire it into ``excluded_keys`` so the post-translation re-merge skips re-adding the raw key. This fixes the 400 on non-Anthropic backends and avoids the conflicting duplicate (``response_format`` + raw ``output_config``) on Anthropic-family backends. Greptile P2 from PR BerriAI#23706 addressed: the constant gives reviewers one grep target instead of an inline literal that silently grows. Greptile P2 from PR BerriAI#22727 addressed: ``extra_kwargs or {}`` is replaced with explicit ``is None`` checks so empty-dict callers no longer skip the fallback path. Tests ----- * tests/test_litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/ test_vertex_ai_partner_models_anthropic_transformation.py: - 5 new/updated cases plus a direct unit test for ``_sanitize_vertex_anthropic_output_params``. - Updated ``test_vertex_ai_claude_sonnet_4_5_structured_output_fix`` so its mock-injected ``output_format`` is asserted to FLOW THROUGH (the original test asserted the now-buggy strip behavior). * tests/test_litellm/llms/anthropic/experimental_pass_through/ adapters/test_handler_output_config_passthrough.py (new): - Constant export sanity, output_config strip with ``effort`` only, output_config strip with ``format`` only, regression guard that unrelated extras still flow, explicit-empty-dict path, and the ``extra_kwargs=None`` no-crash path. Test-quality fixes incorporated from Greptile review on the superseded PRs: * No ``inspect.getsource`` source-text assertions (PR BerriAI#24114 / BerriAI#23475). * ``sys.path`` insertion is anchored to ``__file__`` (PR BerriAI#23706). * Assertion messages are positional, not tuple (PR BerriAI#24114-class bug). * No ``or {}`` masking explicit empty dicts in helper signatures (PR BerriAI#22727). Verified locally: 26/26 pass with this commit. The new tests fail (or fail to import) on ``main`` without it. Out of scope ------------ * The ``max_tokens`` capping logic from PR BerriAI#22727 — independent concern, deserves its own PR with a focused test plan. * Architectural rework of the ``excluded_keys`` mechanism (Greptile P2 on PR BerriAI#23706 noted point-fix growth). The named constant gives maintainers a clear place to extend; a registry-based approach would be a follow-up. Co-Authored-By: netbrah <netbrah> Co-Authored-By: s-zx <s-zx> Co-Authored-By: invoicepulse <invoicepulse> Co-Authored-By: cfdude <cfdude> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ept it Resolves the silent strip of Anthropic Structured Outputs across the Vertex AI Claude transformation paths and the Anthropic-adapter re-merge. Consolidates and supersedes four stalled community PRs addressing overlapping aspects of the same root bug: - BerriAI#23475 (Vertex AI Claude blanket-strip removal) - BerriAI#23396 (Vertex AI Claude conditional passthrough) - BerriAI#23706 (Anthropic adapter exclude output_config from non-Anthropic backends) - BerriAI#22727 (Anthropic adapter strip output_config for non-Anthropic backends) Closes / addresses: BerriAI#23380 (Vertex AI Claude output_config drop), related: BerriAI#26423, BerriAI#25079, BerriAI#24549, BerriAI#25971, BerriAI#25957, BerriAI#26163, BerriAI#24856. What was broken --------------- * Vertex AI Claude paths called ``data.pop("output_config")`` and ``data.pop("output_format")`` unconditionally even when Vertex accepted those fields. Callers asking for Structured Outputs got a 200 with prose and never knew the schema constraints had been silently dropped (often masked for months by permissive fallback parsers). * The ``/v1/messages`` -> ``/chat/completions`` adapter (``LiteLLMMessagesToCompletionTransformationHandler``) re-merged the raw Anthropic-shaped ``output_config`` into ``completion_kwargs`` AFTER the translator already mapped its meaningful parts to ``response_format`` / ``reasoning_effort``. Non-Anthropic backends (Azure OpenAI, Fireworks, Bedrock Nova, etc.) then 400'd with "Extra inputs are not permitted". Approach -------- Vertex AI Claude (chat-completion + experimental_pass_through paths): Replace the unconditional pop with a sanitizer ``_sanitize_vertex_anthropic_output_params`` that strips only the Vertex-unsupported keys (today: ``effort``) from ``output_config`` while forwarding ``format`` and the legacy top-level ``output_format``. Defensive: non-dict ``output_config`` values are dropped to avoid sending malformed payloads downstream. Greptile P1 from PR BerriAI#23396 addressed: when ``output_config`` carries both ``format`` and ``effort``, the prior conditional pass-through forwarded ``effort`` and reproduced the 400. The new helper filters per-key. Anthropic ``/v1/messages`` adapter: Add ``output_config`` to a named module-level constant ``ANTHROPIC_ONLY_REQUEST_KEYS`` and wire it into ``excluded_keys`` so the post-translation re-merge skips re-adding the raw key. This fixes the 400 on non-Anthropic backends and avoids the conflicting duplicate (``response_format`` + raw ``output_config``) on Anthropic-family backends. Greptile P2 from PR BerriAI#23706 addressed: the constant gives reviewers one grep target instead of an inline literal that silently grows. Greptile P2 from PR BerriAI#22727 addressed: ``extra_kwargs or {}`` is replaced with explicit ``is None`` checks so empty-dict callers no longer skip the fallback path. Tests ----- * tests/test_litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/ test_vertex_ai_partner_models_anthropic_transformation.py: - 5 new/updated cases plus a direct unit test for ``_sanitize_vertex_anthropic_output_params``. - Updated ``test_vertex_ai_claude_sonnet_4_5_structured_output_fix`` so its mock-injected ``output_format`` is asserted to FLOW THROUGH (the original test asserted the now-buggy strip behavior). * tests/test_litellm/llms/anthropic/experimental_pass_through/ adapters/test_handler_output_config_passthrough.py (new): - Constant export sanity, output_config strip with ``effort`` only, output_config strip with ``format`` only, regression guard that unrelated extras still flow, explicit-empty-dict path, and the ``extra_kwargs=None`` no-crash path. Test-quality fixes incorporated from Greptile review on the superseded PRs: * No ``inspect.getsource`` source-text assertions (PR BerriAI#24114 / BerriAI#23475). * ``sys.path`` insertion is anchored to ``__file__`` (PR BerriAI#23706). * Assertion messages are positional, not tuple (PR BerriAI#24114-class bug). * No ``or {}`` masking explicit empty dicts in helper signatures (PR BerriAI#22727). Verified locally: 26/26 pass with this commit. The new tests fail (or fail to import) on ``main`` without it. Out of scope ------------ * The ``max_tokens`` capping logic from PR BerriAI#22727 — independent concern, deserves its own PR with a focused test plan. * Architectural rework of the ``excluded_keys`` mechanism (Greptile P2 on PR BerriAI#23706 noted point-fix growth). The named constant gives maintainers a clear place to extend; a registry-based approach would be a follow-up. Co-Authored-By: netbrah <netbrah> Co-Authored-By: s-zx <s-zx> Co-Authored-By: invoicepulse <invoicepulse> Co-Authored-By: cfdude <cfdude>
…Handler so success_callbacks fire
streaming_iterator was tagging Google GenAI chunks as VERTEX_AI, causing
_route_streaming_logging_to_handler to use the wrong parser. Result was
None so async_complete_streaming_response was never set and all success
callbacks were silently skipped for /models/{model}:streamGenerateContent.
Fixes #24097
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes