Repository navigation
fix(streaming): backport text-completion usage fix and e2e provider-flake tolerance to rc/1.103.0 #43400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(streaming): backport text-completion usage fix and e2e provider-flake tolerance to rc/1.103.0 #43400
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,16 +11,28 @@ | |
| fails when no proxy answers and also fails once a request reaches it: the proxy | ||
| fetches each provider's referenced secrets, so a | ||
| missing credential surfaces as a live provider error rather than silent green. | ||
|
|
||
| The provider keys are shared with other pipelines, so a provider's rate limit can | ||
| hold across the bounded retries. The case then accepts the gateway's faithful relay | ||
| of that 429 (throttling_error, code 429) as its second expected outcome; any other | ||
| non-success still fails at once. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from dataclasses import dataclass | ||
| from typing import Protocol | ||
| from typing import Final, Protocol | ||
|
|
||
| import pytest | ||
| from e2e_config import unique_marker | ||
| from e2e_http import assert_client_error, unwrap | ||
| from e2e_http import ( | ||
| PROVIDER_RATE_LIMIT_ATTEMPTS, | ||
| RateLimitedError, | ||
| Success, | ||
| assert_client_error, | ||
| relayed_provider_rate_limit, | ||
| tolerate_provider_rate_limit, | ||
| ) | ||
| from endpoints_client import EndpointsClient | ||
| from lifecycle import ResourceManager | ||
| from models import LiteLLMParamsBody, OcrBody, OcrDocument, OcrResponse | ||
|
|
@@ -146,6 +158,17 @@ def _assert_ocr_document(response: OcrResponse) -> None: | |
| assert response.pages[0].markdown is not None, "first page has no markdown" | ||
|
|
||
|
|
||
| def _assert_provider_rate_limit_relayed(model: str, outcome: RateLimitedError) -> None: | ||
| detail: Final = relayed_provider_rate_limit(outcome) | ||
| assert detail is not None, f"{model}: the 429 is the gateway's own, not the provider's: {outcome.body}" | ||
| assert (detail.type, detail.code) == ("throttling_error", "429"), f"{model}: provider 429 relayed as {detail!r}" | ||
| print( | ||
| f"{model}: the provider's rate limit held across {PROVIDER_RATE_LIMIT_ATTEMPTS} attempts; " | ||
| f"the gateway relayed it as {detail.type} {detail.code}", | ||
| flush=True, | ||
| ) | ||
|
|
||
|
|
||
| class TestRustOcrGateway: | ||
| @pytest.mark.parametrize("case", RUST_OCR_CASES, ids=_CASE_IDS) | ||
| def test_rust_ocr_response( | ||
|
|
@@ -156,8 +179,15 @@ def test_rust_ocr_response( | |
| resources.defer(lambda: endpoints_client.delete_model(model_id)) | ||
| key = resources.key() | ||
|
|
||
| response = unwrap(endpoints_client.proxy.ocr(key, OcrBody(model=model, document=case.document))) | ||
| _assert_ocr_document(response) | ||
| match tolerate_provider_rate_limit( | ||
| lambda: endpoints_client.proxy.ocr(key, OcrBody(model=model, document=case.document)) | ||
| ): | ||
| case Success(data=response): | ||
| _assert_ocr_document(response) | ||
| case RateLimitedError() as outcome: | ||
| _assert_provider_rate_limit_relayed(model, outcome) | ||
|
Comment on lines
+187
to
+188
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Rule Used: What: Flag any modifications to existing tests and verify they don't weaken test coverage or mask regressions. Why: Developers may alter tests to make failing code pass rather than fix the actual bug, hiding regressions. Good: ``` // Test updated t... (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! |
||
| case outcome: | ||
| pytest.fail(f"{model}: {outcome!r}") | ||
|
|
||
| @pytest.mark.skip(reason="stage red: product gap, /v1/ocr 500s (aocr TypeError) on missing document instead of 400") | ||
| @pytest.mark.covers("llm.ocr.openai.input_validation.nonstream.works") | ||
|
|
@@ -174,4 +204,3 @@ def test_missing_document_returns_error( | |
| json=_OptionalOcrBody(model=model), | ||
| ) | ||
| assert_client_error(result, "ocr missing document") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delaylocal lacks: Final, as docandidateandrespelsewhere in this change. The repository requires every variable to have that annotation; satisfy this requirement before merging.Context Used: AGENTS.md (source)