mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-09 22:31:41 +00:00
4 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
ryan-crabbe-berri
|
680bcfd8aa
|
test(lint): ban blind pytest.raises(Exception) with ruff B017 (#37731)
* test(lint): ban blind pytest.raises(Exception) with ruff B017 A bare pytest.raises(Exception) accepts whatever the body throws. The TypeError a refactor introduces satisfies it exactly as well as the rejection the test was written for, so the crash reads as a pass and the test never goes red. All 111 existing sites are narrowed here. A runtime probe recorded the concrete exception each one actually catches, and each site now names that type. Where the code under test genuinely raises a bare Exception, the site pins a stable slice of the message with match= instead. Two sites tell on themselves. The shared responses-API cancel test raises "custom_llm_provider is required but passed as None" rather than talking to a provider at all, because cancel_responses takes a provider, not a model. And test_bedrock_guardrails_with_streaming was the only test in its file still passing without AWS credentials, because the NoCredentialsError boto3 raised long before the guardrail ran satisfied the blind raises. * fix(test): widen the openai batch-dispatch assertion to OpenAIError The narrowed NotFoundError only holds where OPENAI_API_KEY is set. Without one the SDK raises OpenAIError while building the client, long before any 404, so CI went red. OpenAIError covers both and still rejects a TypeError from a refactor. |
||
|
Deepanshu Lulla
|
05943b47a3
|
fix(router): cool down failed fallback deployments and correct cooldown TTL after Redis backfill (#35104)
* fix(router): cool down failed fallback deployments and correct cooldown TTL after Redis backfill A deployment that failed partway through a fallback chain (any attempt after the first) was silently exempt from cooldown, because the has_logged_async_failure dedup flag blocks the normal failure callback for every attempt past the first. _trigger_cooldown_for_failed_deployment now explicitly evaluates cooldown for that deployment when the dedup flag is set, using the same deployment-config > response-header > router-default precedence as the primary failure path, and skips advisor-orchestration failures. Deployment-ID resolution prefers the exception's stamped failed_deployment_id, now also set from the generic-API-call fallback path (rerank, embeddings, /v1/messages, etc.), falling back to metadata inspection for call paths that don't stamp it yet. CooldownCache also recomputes the remaining TTL when DualCache promotes a Redis entry into the in-memory layer: before this, a cooldown entry restored from Redis kept the in-memory layer's default 600s TTL regardless of the deployment's real cooldown_time, so a deployment could stay excluded from routing for up to 10 minutes after a much shorter cooldown had already expired. * fix(router): address Greptile review on the fallback-cooldown trigger Two P1 findings on PR #35104: - _trigger_cooldown_for_failed_deployment never incremented the deployment's per-minute failure counter before evaluating cooldown, so a fallback deployment's repeated retryable failures never accumulated toward the default percent-fail-rate threshold that _should_cooldown_deployment checks. - The metadata-bucket fallback (checking "metadata" before "litellm_metadata" for a deployment_model_name marker) could be fooled by a caller with permission to set metadata, since neither bucket's authorship can be determined without knowing the call's function_name. Removed it entirely; cooldown now requires the server-stamped failed_deployment_id, matching what the primary chat-completions path and the generic-API-call path (rerank, embeddings, /v1/messages, etc.) already set unconditionally. * fix(router): freeze the litellm_params fallback mapping to satisfy the type-discipline gate * fix(router): defer f-string interpolation in fallback-cooldown debug logs * fix(router): annotate cooldown-path locals with Final to satisfy the LIT010 budget * fix(router): don't cool down deployments for request-scoped 404s on generic API fallbacks * fix(router): stamp the dynamic client-side-credential deployment id, not the shared static one * fix(router): don't cool down deployments for a caller-supplied x-litellm-timeout * fix(router): stamp dynamic client-side-credential id in completion fallback paths too The generic-API-call helper already stamped the effective (dynamic-if-client-side-credential) deployment id on exceptions, but the regular _completion/_acompletion exception handlers still stamped the static shared deployment's id. A tenant using invalid forwarded credentials could generate repeated failures attributed to, and eventually cooling down, the shared deployment other tenants rely on. Extracted the stamping logic into one shared helper used by all three call sites (generic API, sync completion, async completion) so the fix and future changes to it stay in one place. * test(router): add direct-reference unit tests for the new stamping helper router_code_coverage.py's coverage gate flags _stamp_failed_deployment_id_with_effective_model_info as untested because it only sees the function invoked indirectly through _completion/_acompletion's exception handlers. Added two tests that call it directly, covering both the dynamic-id-present and static-fallback branches. * test(router): cover the timeout stamping branch and async active-cooldown append _acompletion's litellm.Timeout handler and async_get_active_cooldowns' happy path both lacked direct coverage despite their sibling branches (the generic Exception handler, the sync get_active_cooldowns) being tested. * test(router): remove duplicate cooldown-trigger and fallback-helper tests #34416 landed its own TestTriggerCooldownForFailedDeployment/ TestRunAsyncFallbackTriggersCooldown classes and test_ageneric_api_call_with_fallbacks_helper_stamps_failed_deployment_id covering the exact same scenarios as this branch's earlier flat-function tests, once its version of fallback_event_handlers.py was taken as-is during the last merge. Dropping the redundant copies. --------- Co-authored-by: Deepanshu <deepanshu.lulla@alpha-sense.com> |
||
|
Deepanshu Lulla
|
0580465384
|
feat(router): add per-deployment allowed_fails_policy and cooldown_time override support (#34416)
* feat(router): add per-deployment allowed_fails_policy and cooldown_time override support Three bugs fixed in the router cooldown system: (1) deployment-level allowed_fails and allowed_fails_policy in model_info now take precedence over router-level settings in _should_cooldown_deployment; (2) failed fallback deployments now get evaluated for cooldown via _trigger_cooldown_for_failed_deployment, bypassing the Logging dedup gate; (3) DualCache promotes Redis cooldown entries using default 600s TTL instead of true remaining cooldown time -- _corrected_active_cooldown now evicts expired entries and corrects stale in-memory TTLs on backfill. Adds ServiceUnavailableError, BadGatewayError, and NotFoundError fields to AllowedFailsPolicy and cooldown_time to LiteLLMParamsTypedDict. * fix(router): gate fallback cooldown trigger on has_logged_async_failure; use only litellm_metadata for deployment ID * fix(router): use X | Y union syntax to fix UP007 strict lint gate * test(router_utils): add coverage for _trigger_cooldown_for_failed_deployment and has_logged_async_failure gate * test(router_utils): cover deployment cooldown override and exception swallow paths * fix(router): add InternalServerError/ServiceUnavailableError/BadGatewayError/NotFoundError to router-level get_allowed_fails_from_policy * fix(router): format router.py and add router-level policy tests * test(router): add CI-visible coverage for per-deployment cooldown policy Tests for `_get_deployment_cooldown_policy`, `_resolve_allowed_fails_from_policy`, and `_should_cooldown_based_on_deployment_policy` (cooldown_handlers.py), the `_corrected_active_cooldown` branches in CooldownCache, and the four new exception-type branches in `Router.get_allowed_fails_from_policy` (router.py) -- all in `tests/test_litellm/` which the enterprise-routing CI job runs. * fix(router): use is not None guard for cooldown_time_override in should_cooldown_based_on_allowed_fails_policy A cooldown_time_override of 0 was previously treated as falsy and silently fell through to the router-level cooldown_time value. Switched to an explicit is not None check so that zero is honored as a valid override. Added a regression test covering the zero case. * fix(router): honor has_logged_async_failure and metadata for fallback cooldown; support both model_info and litellm_params locations Manual verification against a live proxy surfaced that the fallback-cooldown-gap trigger never actually fired: the has_logged_async_failure check read a plain attribute that Logging never sets (the real flag lives in model_call_details), and the deployment_id lookup only trusted litellm_metadata, which regular chat completions never populate (only batch/thread/file endpoints do). Router overwrites model_info on whichever key is present before every attempt, so metadata is equally authoritative there, not caller-controlled as previously assumed. Also let allowed_fails/allowed_fails_policy/cooldown_time be set under either model_info or litellm_params, each preferring its own canonical location. * fix(router): fix ContentPolicyViolationError policy shadowing and partial-policy zero-threshold Two bugs from Greptile review on PR #34416: - ContentPolicyViolationError subclasses BadRequestError, so listing BadRequestError first in _EXCEPTION_POLICY_FIELDS made the isinstance check always match BadRequestError for content-policy errors, using the wrong allowed_fails threshold. Reordered so the subclass is checked first. - A deployment with a partial allowed_fails_policy and no deployment-wide allowed_fails forced allowed_fails_override=0 for any exception type its policy didn't cover, cooling the deployment down on the first unrelated failure. Now defers to router-level behavior for uncovered exception types instead of forcing an immediate cooldown. * fix(router): only trust a metadata/litellm_metadata bucket the router itself wrote deployment info into veria-ai flagged that preferring litellm_metadata whenever present could pick up a caller-supplied litellm_metadata.model_info.id (preserved via allow_client_pricing_override) instead of the metadata bucket the router actually populated for a regular completion's fallback attempt, naming an arbitrary "victim" deployment for cooldown. Router._update_kwargs_with_deployment() always writes model_info and deployment_model_name into the same bucket together. Only trust a bucket that carries deployment_model_name alongside model_info, since that marker is only ever set by the router itself, not by request-body metadata. * test(router): add regression coverage for ContentPolicyViolationError policy shadowing The subclass-ordering fix in commit 38fe4e4490 had no regression test. Verified the new test fails on the pre-fix ordering (asserts 2, got 10) before restoring the fix, and confirmed the same behavior through the full _should_cooldown_deployment call path against a real Router instance. * fix(router): let explicit allowed_fails_policy entries override the generic 4XX cooldown exclusion _is_cooldown_required skips cooldown evaluation for any 4XX status outside {429, 401, 408, 404} by default, since a generic client error is usually not the deployment's fault. BadRequestError and ContentPolicyViolationError both carry status 400, so their AllowedFailsPolicy fields (BadRequestErrorAllowedFails, ContentPolicyViolationErrorAllowedFails, both router-level pre-existing and the new deployment-level ones) were silently unreachable: an operator could set them to any value with no effect, since _is_cooldown_required blocked cooldown evaluation before that policy was ever consulted. _should_run_cooldown_logic now also checks whether an explicit allowed_fails_policy entry (deployment-level or router-level) covers the exception's type, and if so, proceeds with cooldown evaluation regardless of the generic status-code exclusion. The exclusion remains the default for exception types with no explicit policy. Verified live against a mock-triggered ContentPolicyViolationError (config-level mock_response, azure/gpt-4.1-mini deployment) with BadRequestErrorAllowedFails=100 and ContentPolicyViolationErrorAllowedFails=0 on the same deployment: it now cools down after exactly one ContentPolicyViolationError instead of never cooling down. * fix(router): use the router-stamped failed_deployment_id for fallback cooldown targeting Greptile flagged a real gap in the metadata-bucket-based deployment lookup: for a generic-API-call fallback, the router writes the current attempt into litellm_metadata, but a stale "metadata" bucket carrying the same deployment_model_name marker (from an earlier point) would be picked first, cooling the wrong deployment. Router already has a more robust, pre-existing mechanism for this exact problem: _set_failed_deployment_id_on_exception stamps the failing deployment's id directly onto the exception at the point of failure, immune to metadata-bucket ambiguity since a caller can't influence it and it doesn't depend on which bucket the current call type happens to use. It just wasn't called from _ageneric_api_call_with_fallbacks_helper's except block, unlike _completion/_acompletion. Added the missing call there (matching the existing pattern exactly), and changed _trigger_cooldown_for_failed_deployment to prefer exception.failed_deployment_id when present, falling back to metadata-bucket inspection only for call paths that don't stamp it yet. Verified live: the standard fallback-cooldown-gap scenario (two bad-key deployments in a fallback chain) still correctly cools down both the originally-called and fallback deployment. * fix(router): address human review on per-deployment cooldown overrides Scope allowed_fails_policy override to deployment-level only (a router-level policy predates this feature and must keep its existing behavior), exempt advisor-orchestration failures from the fallback cooldown trigger, keep the single-deployment model group protection intact against a generic deployment-level allowed_fails, make cooldown_time precedence consistent across resolution paths, fix a falsy-zero swallowing bug in the router-level allowed_fails fallback, and make allowed_fails_policy resolution fall through to the next matching exception type instead of stopping at the first unset field. Also restrict allowed_fails/allowed_fails_policy/cooldown_time to model_info: litellm_params gets copied into the actual provider request, so a router-only setting placed there would leak into that request. * test(router): update test_cooldown_handlers.py for the deployment-policy signature change Surfaced by the rebase: this mirrored test file (tests/test_litellm/ mirrors litellm/) predates the router_unit_tests/ coverage added earlier in this PR and was still calling _should_cooldown_based_on_deployment_policy with its old 4-argument signature and asserting the now-removed litellm_params cooldown_time location. * test(router): update test_fallback_event_handlers.py for model_info-only cooldown_time Another mirrored test file surfaced by the rebase that still asserted the now-removed litellm_params.cooldown_time location. * fix(router): match cooldown-duration precedence in the fallback path to the primary path _trigger_cooldown_for_failed_deployment only checked deployment config before falling back to the router default, skipping the response Retry-After header step that Router.deployment_callback_on_failure applies on the primary path. * fix(router): restore litellm_params.cooldown_time as a pre-existing fallback cooldown_time already had litellm_params support on Router.deployment_callback_on_failure before this PR; the earlier model_info-only restriction (aimed at the leak concern for the genuinely new allowed_fails/allowed_fails_policy fields) incorrectly dropped that pre-existing capability too. model_info still takes priority when both are set. * fix(router): keep the fallback-cooldown trigger in sync with #35104's review fixes Applies the same two fixes landed on the split-out PR #35104 (which #34416 still duplicates until it's rebased onto the merged base): increment the deployment's per-minute failure counter before evaluating cooldown, and require the server-stamped failed_deployment_id instead of trusting a metadata bucket, since neither "metadata" nor "litellm_metadata" can be told apart from a caller-supplied one without knowing the call's function_name. * fix(router): freeze the model_info fallback mapping to satisfy the type-discipline gate * fix(router): defer f-string interpolation in fallback-cooldown debug logs * fix(router): annotate cooldown-path locals with Final to satisfy the LIT010 budget * fix(router): suppress reportPrivateUsage for cross-module cooldown helpers * fix(router): don't cool down deployments for request-scoped 404s on generic API fallbacks * fix(router): stamp the dynamic client-side-credential deployment id, not the shared static one * fix(router): keep up with upstream typing modernization and Final-annotation ratchet * fix(router): don't cool down deployments for a caller-supplied x-litellm-timeout * fix(router): stamp dynamic client-side-credential id in completion fallback paths too The generic-API-call helper already stamped the effective (dynamic-if-client-side-credential) deployment id on exceptions, but the regular _completion/_acompletion exception handlers still stamped the static shared deployment's id. A tenant using invalid forwarded credentials could generate repeated failures attributed to, and eventually cooling down, the shared deployment other tenants rely on. Extracted the stamping logic into one shared helper used by all three call sites (generic API, sync completion, async completion) so the fix and future changes to it stay in one place. * fix(proxy): recognize body-supplied timeout/request_timeout/stream_timeout as caller-controlled client_side_timeout was only set when the caller used the x-litellm-timeout header, but Router._get_timeout also resolves the effective timeout from kwargs["timeout"], kwargs["request_timeout"], and kwargs["stream_timeout"], all settable directly in the request body (and x-litellm-stream-timeout wasn't marked either). A caller could set any of those to a near-zero value, force a 408 on every deployment in a fallback chain, and cool down deployments other tenants rely on without the guard in _trigger_cooldown_for_failed_deployment recognizing it as caller-controlled. Also strip any client-forged client_side_timeout from the request body so the marker is always server-computed. --------- Co-authored-by: Deepanshu <deepanshu.lulla@alpha-sense.com> |
||
|
Sameer Kankute
|
50df072d95
|
feat: add weighted-routing failover (#27980)
* Feat: Add Weighted-Routing Failover
* test(router): cover weighted failover helper functions
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(router): align weighted failover deployment list type with mypy
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(router): address greptile review on weighted failover
- Narrow exception swallowing in `_maybe_run_weighted_failover` to
`openai.APIError` so model failures defer to the regular fallback
while programming bugs (AttributeError/KeyError/TypeError) surface.
- Note async-only limitation of `enable_weighted_failover` in the
Router constructor docstring.
- Make the weighted distribution test less flaky (1000 iterations,
looser bound) and make the non-simple-shuffle test deterministic by
failing both deployments instead of relying on the latency strategy's
first pick.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(router): ensure weighted failover metadata persists in kwargs
The previous `kwargs.setdefault(metadata_variable_name, {}) or {}` returned
a brand-new dict whenever the existing metadata was falsy (empty dict or
None), so writes to `_failover_excluded_ids` never made it back into
`kwargs`. Multi-hop weighted failover then re-selected previously failed
deployments and exhausted `max_fallbacks` prematurely.
Explicitly assign a fresh dict into kwargs when metadata is missing so
mutations are visible to subsequent failover hops.
Co-authored-by: Yassin Kortam <yassin@berri.ai>
* test(router): regression for weighted failover metadata persistence
Asserts kwargs["metadata"]["_failover_excluded_ids"] is populated after
_maybe_run_weighted_failover, proving the metadata dict written by the
helper is the same object that lives in kwargs (no disconnected copy).
Pairs with the prior fix that replaced `setdefault(..., {}) or {}` with
an explicit get/assign so writes survive across hops.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(router): harden weighted failover error/state handling
- Catch RouterRateLimitError (ValueError) alongside openai.APIError in
_maybe_run_weighted_failover so an exhausted intra-group retry falls
through to the regular cross-group fallback path instead of bubbling
out and bypassing configured fallbacks.
- Stop mutating the shared input_kwargs dict; build a local copy with
the weighted-failover keys so the entry (with _excluded_deployment_ids)
cannot leak into later fallback paths reading the same dict.
- _get_excluded_filtered_deployments now returns an empty list when the
exclusion filter removes every healthy deployment, instead of falling
back to the original list. The original-list behavior risked re-picking
the just-failed deployment; callers already handle the empty case by
raising their no-deployments error, which weighted failover now catches
and converts into a normal cross-group fallback.
Co-authored-by: Yassin Kortam <yassin@berri.ai>
* fix(router): fall through to rpm/tpm when total weight is zero
When the weight metric's total is zero (e.g. after weighted-failover
exclusion leaves only zero-weight backups), continue to the next metric
(rpm/tpm) instead of returning a uniform random pick immediately. This
lets rpm/tpm still drive routing when present, and only falls back to
the uniform random pick at the end if no metric provides a positive
total weight.
Co-authored-by: Yassin Kortam <yassin@berri.ai>
* fix(router): skip weighted failover when remaining deployments are all in cooldown
_maybe_run_weighted_failover was computing 'remaining' from all_deployments
(every deployment in the model group, including those in cooldown). This meant
that when all non-excluded deployments were in cooldown the method still invoked
run_async_fallback unnecessarily, which propagated into async_get_healthy_deployments,
found no eligible deployments, and raised RouterRateLimitError — only safely
caught thanks to the earlier exception-broadening fix.
The fix: before computing 'remaining', fetch the current cooldown set via
_async_get_cooldown_deployments and subtract it from all_ids. This allows
_maybe_run_weighted_failover to return None immediately (skipping the
run_async_fallback call entirely) when every non-failed deployment is in cooldown,
letting the caller fall through to the correct cross-group fallback path without
the wasteful extra round-trip.
Tests added:
- unit: _maybe_run_weighted_failover returns None without calling run_async_fallback
when all remaining deployments are in cooldown
- unit: _maybe_run_weighted_failover still calls run_async_fallback when at least
one healthy (non-cooldown) deployment is available
- integration: end-to-end fallthrough to cross-group fallback when remaining
deployments are in cooldown
Co-authored-by: Sameer Kankute <Sameerlite@users.noreply.github.com>
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Yassin Kortam <yassin@berri.ai>
Co-authored-by: Sameer Kankute <Sameerlite@users.noreply.github.com>
|