mirror of
https://github.com/BerriAI/litellm.git
synced 2026-08-28 05:25:59 +00:00
fix(router): guard num_retries=None in async_function_with_retries (#30036)
When num_retries reaches async_function_with_retries as None - e.g. a caller
passes num_retries=None explicitly (dict.get() does not fall back on an
existing None value), an auto_router/complexity_router path does not propagate
it, or Router.update_settings(num_retries=None) is used - the comparison
`if num_retries > 0:` raised:
TypeError: '>' not supported between instances of 'NoneType' and 'int'
This only surfaced when the underlying call failed with a retryable error
(rate limit / connection / 5xx), so the real upstream error was masked by a
confusing TypeError.
Normalise an explicit num_retries=None to the router default in
_update_kwargs_before_fallbacks (falling back to 0 when the router default is
itself None, and preserving an explicit 0), and keep the matching guard at the
single pop site in async_function_with_retries as the safety net for paths that
bypass the setter. Adds regression tests to
test_router_per_deployment_num_retries.py.
Relates to #23316, #25889, #23699, #28126
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: yuneng-jiang <yuneng@berri.ai>
This commit is contained in:
parent
7020e1e5f7
commit
02f63d20bb
2 changed files with 149 additions and 3 deletions
|
|
@ -3134,7 +3134,18 @@ class Router:
|
|||
- litellm_trace_id
|
||||
- metadata
|
||||
"""
|
||||
kwargs["num_retries"] = kwargs.get("num_retries", self.num_retries)
|
||||
# Normalise an explicit num_retries=None to the router default here (dict.get()
|
||||
# only falls back when the key is absent, not when its value is None), then to 0
|
||||
# if the router default is itself None - mirroring the guard in
|
||||
# async_function_with_retries, which remains the safety net for paths that bypass
|
||||
# this setter.
|
||||
_req_num_retries = kwargs.get("num_retries")
|
||||
if _req_num_retries is not None:
|
||||
kwargs["num_retries"] = _req_num_retries
|
||||
else:
|
||||
kwargs["num_retries"] = (
|
||||
self.num_retries if self.num_retries is not None else 0
|
||||
)
|
||||
kwargs.setdefault("litellm_trace_id", str(uuid.uuid4()))
|
||||
model_group_alias: Optional[str] = None
|
||||
if self._get_model_from_alias(model=model):
|
||||
|
|
@ -6931,7 +6942,11 @@ class Router:
|
|||
"model_group_retry_policy", self.model_group_retry_policy
|
||||
)
|
||||
model_group: Optional[str] = kwargs.get("model")
|
||||
num_retries = kwargs.pop("num_retries")
|
||||
num_retries = kwargs.pop("num_retries", None)
|
||||
if num_retries is None:
|
||||
# Fall back to the router setting (then 0) so the comparisons below never
|
||||
# hit `None > int`, which would mask the real upstream error with a TypeError.
|
||||
num_retries = self.num_retries if self.num_retries is not None else 0
|
||||
|
||||
## ADD MODEL GROUP SIZE TO METADATA - used for model_group_rate_limit_error tracking
|
||||
_metadata: dict = kwargs.get("litellm_metadata", kwargs.get("metadata")) or {}
|
||||
|
|
|
|||
|
|
@ -4,8 +4,9 @@ GitHub Issue: #18968 - Per-deployment max_retries/num_retries in litellm_params
|
|||
"""
|
||||
|
||||
import pytest
|
||||
from unittest.mock import MagicMock, patch
|
||||
from unittest.mock import patch
|
||||
|
||||
import litellm
|
||||
from litellm import Router
|
||||
|
||||
|
||||
|
|
@ -188,3 +189,133 @@ class TestPerDeploymentNumRetries:
|
|||
|
||||
# Verify num_retries was converted from string to int
|
||||
assert exc.num_retries == 6
|
||||
|
||||
|
||||
class TestNumRetriesNoneGuard:
|
||||
"""
|
||||
Regression tests for the num_retries=None TypeError in async_function_with_retries.
|
||||
|
||||
When num_retries reaches async_function_with_retries as None - e.g. a caller passes
|
||||
num_retries=None explicitly (dict.get() does not fall back on an existing None value),
|
||||
an auto_router/complexity_router path does not propagate it, or
|
||||
Router.update_settings(num_retries=None) is used - AND the underlying call fails with a
|
||||
retryable error, the comparison `if num_retries > 0:` raised:
|
||||
|
||||
TypeError: '>' not supported between instances of 'NoneType' and 'int'
|
||||
|
||||
This masked the real upstream error (rate limit / connection / 5xx) behind a TypeError.
|
||||
Related issues: #23316, #25889, #23699, #28126.
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
def _mock_router(num_retries=2):
|
||||
return Router(
|
||||
model_list=[
|
||||
{
|
||||
"model_name": "mock-model",
|
||||
"litellm_params": {
|
||||
"model": "gpt-4o-mini",
|
||||
"mock_response": "ok",
|
||||
},
|
||||
}
|
||||
],
|
||||
num_retries=num_retries,
|
||||
)
|
||||
|
||||
def test_update_kwargs_normalises_explicit_none_to_router_default(self):
|
||||
"""
|
||||
_update_kwargs_before_fallbacks must normalise an explicit num_retries=None to
|
||||
the router default (not leave it as None), while preserving an explicit 0.
|
||||
"""
|
||||
router = self._mock_router(num_retries=4)
|
||||
|
||||
# explicit None -> router default
|
||||
kwargs = {"num_retries": None}
|
||||
router._update_kwargs_before_fallbacks(model="mock-model", kwargs=kwargs)
|
||||
assert kwargs["num_retries"] == 4
|
||||
|
||||
# explicit 0 is preserved (retries stay disabled)
|
||||
kwargs = {"num_retries": 0}
|
||||
router._update_kwargs_before_fallbacks(model="mock-model", kwargs=kwargs)
|
||||
assert kwargs["num_retries"] == 0
|
||||
|
||||
# absent -> router default (unchanged behaviour)
|
||||
kwargs = {}
|
||||
router._update_kwargs_before_fallbacks(model="mock-model", kwargs=kwargs)
|
||||
assert kwargs["num_retries"] == 4
|
||||
|
||||
# explicit None with router default also None -> 0 (mirrors the downstream guard)
|
||||
router.num_retries = None # simulate update_settings(num_retries=None) (#28126)
|
||||
kwargs = {"num_retries": None}
|
||||
router._update_kwargs_before_fallbacks(model="mock-model", kwargs=kwargs)
|
||||
assert kwargs["num_retries"] == 0
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_acompletion_num_retries_none_does_not_raise_typeerror(self):
|
||||
"""
|
||||
Per-request num_retries=None + a retryable error must NOT raise TypeError.
|
||||
The router falls back to its configured num_retries and retries the (transient)
|
||||
error, so the request succeeds.
|
||||
"""
|
||||
router = self._mock_router(num_retries=2)
|
||||
with patch("asyncio.sleep", return_value=None):
|
||||
response = await router.acompletion(
|
||||
model="mock-model",
|
||||
messages=[{"role": "user", "content": "hi"}],
|
||||
num_retries=None, # the trigger
|
||||
mock_testing_rate_limit_error=True, # retryable error path
|
||||
)
|
||||
assert response.choices[0].message.content == "ok"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_async_function_with_retries_none_falls_back_to_zero(self):
|
||||
"""
|
||||
When both the per-request value AND the router-level setting are None
|
||||
(e.g. after Router.update_settings(num_retries=None), #28126), num_retries must
|
||||
fall back to 0 and the real retryable error must surface - not a TypeError.
|
||||
"""
|
||||
router = self._mock_router(num_retries=0)
|
||||
router.num_retries = None # simulate update_settings(num_retries=None)
|
||||
|
||||
async def failing_fn(*args, **kwargs):
|
||||
raise litellm.RateLimitError(
|
||||
message="boom", model="mock-model", llm_provider="openai"
|
||||
)
|
||||
|
||||
with patch("asyncio.sleep", return_value=None):
|
||||
with pytest.raises(litellm.RateLimitError):
|
||||
await router.async_function_with_retries(
|
||||
original_function=failing_fn,
|
||||
model="mock-model",
|
||||
messages=[{"role": "user", "content": "hi"}],
|
||||
num_retries=None,
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_async_function_with_retries_none_falls_back_to_router_default(self):
|
||||
"""
|
||||
A None per-request num_retries falls back to the router-level setting, so retries
|
||||
still happen (original_function is invoked more than once) before the real error
|
||||
is raised - proving None did not silently disable retries or crash.
|
||||
"""
|
||||
router = self._mock_router(num_retries=3)
|
||||
calls = {"n": 0}
|
||||
|
||||
async def failing_fn(*args, **kwargs):
|
||||
calls["n"] += 1
|
||||
raise litellm.InternalServerError(
|
||||
message="boom", model="mock-model", llm_provider="openai"
|
||||
)
|
||||
|
||||
with patch("asyncio.sleep", return_value=None):
|
||||
with pytest.raises(litellm.InternalServerError):
|
||||
await router.async_function_with_retries(
|
||||
original_function=failing_fn,
|
||||
model="mock-model",
|
||||
messages=[{"role": "user", "content": "hi"}],
|
||||
metadata={}, # populated by acompletion in the real path; log_retry needs it
|
||||
num_retries=None,
|
||||
)
|
||||
|
||||
# 1 initial attempt + at least 1 retry -> proves None fell back to a positive int
|
||||
assert calls["n"] >= 2
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue