Repository navigation
fix(litellm): make the handler the only retry layer - #4425
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
⚙️ Agent run details
|
PR Code Suggestions ✨Explore these optional code suggestions:
⚙️ Agent run details
|
|
|
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks @dodoshell for taking the whole retry path in one PR. The behaviour matches the spec in #4407: the call counts for 429, 5xx, timeout and 400 reproduce, and odd Retry-After values fall back to the jitter. Keeping the jitter above 30 s is fine. One ask: the autouse fixture in conftest.py replaces retry.wait, so deleting wait=_same_model_retry_wait from the decorator still passes every test. Patching sleep instead and recording it in one test covers the wiring.
| import sys | ||
|
|
||
| import pytest | ||
| from tenacity import wait_none |
There was a problem hiding this comment.
| from tenacity import wait_none |
| """ | ||
| handler_module = sys.modules.get("pr_agent.algo.ai_handlers.litellm_ai_handler") | ||
| if handler_module is not None: | ||
| monkeypatch.setattr(handler_module.LiteLLMAIHandler._chat_completion_with_retry.retry, "wait", wait_none()) |
There was a problem hiding this comment.
| monkeypatch.setattr(handler_module.LiteLLMAIHandler._chat_completion_with_retry.retry, "wait", wait_none()) | |
| async def no_sleep(seconds): | |
| pass | |
| monkeypatch.setattr(handler_module.LiteLLMAIHandler._chat_completion_with_retry.retry, "sleep", no_sleep) |
|
|
||
| assert "num_retries" not in mock_call.call_args.kwargs | ||
| assert "max_retries" not in mock_call.call_args.kwargs | ||
| assert mock_call.call_count == expected_attempts |
There was a problem hiding this comment.
| assert mock_call.call_count == expected_attempts | |
| assert mock_call.call_count == expected_attempts | |
| @pytest.mark.asyncio | |
| async def test_chat_completion_waits_retry_after_between_rate_limit_retries(monkeypatch): | |
| monkeypatch.setattr(litellm_handler, "get_settings", FakeSettings) | |
| sleeps = [] | |
| async def record_sleep(seconds): | |
| sleeps.append(seconds) | |
| monkeypatch.setattr(litellm_handler.LiteLLMAIHandler._chat_completion_with_retry.retry, "sleep", record_sleep) | |
| error = _answered(litellm.RateLimitError("throttled", llm_provider="bedrock", model="m")) | |
| error.litellm_response_headers = httpx.Headers({"retry-after": "7"}) | |
| with patch("pr_agent.algo.ai_handlers.litellm_ai_handler.acompletion", new_callable=AsyncMock) as mock_call: | |
| mock_call.side_effect = error | |
| handler = litellm_handler.LiteLLMAIHandler() | |
| with pytest.raises(litellm.RateLimitError): | |
| await handler.chat_completion(model="gpt-4o", system="sys", user="usr") | |
| assert sleeps == [7.0] * litellm_handler.DEFAULT_NUM_RETRIES |
|
Thanks @IsmaelMartinez, good catch. Applied both suggestions in c06098d: the fixture now patches |
A model call could be retried by LiteLLM's client, the OpenAI SDK and the handler's own tenacity retry, and nothing waited between attempts in the handler. A Bedrock throttle went straight to the fallback models and a 503 burned its attempts in a few milliseconds. The completion call now always gets num_retries=0 and max_retries=0. config.num_retries (default 4, 0 disables) is the number of same-model retries of a rejected request: a 429, a 5xx answer or a connection error. Each retry waits for a finite Retry-After of up to 30 s, or with exponential backoff and jitter. Timeouts, MidStreamFallbackError, empty responses and 200 responses that fail to parse keep MODEL_RETRIES; timeouts and empty responses keep their existing knobs. Fixes The-PR-Agent#4407
The autouse fixture replaced retry.wait, so dropping wait= from the decorator still passed every test. It now replaces the retry's sleep, and a rate-limit test records the Retry-After it sleeps between attempts.
c06098d to
a05a51d
Compare
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks @dodoshell for applying both suggestions so quickly; the new test now fails without the backoff wiring, approving and merging.
User description
Fixes #4407.
The completion call now always gets
num_retries=0andmax_retries=0, so the handler's retry is the only layer.config.num_retries(default 4,0disables) is the number of same-model retries of a rejected request: a 429, a 5xx answer including 529, or a connection error. Each retry waits for a finite, positiveRetry-Afterof up to 30 s, otherwisewait_random_exponential(multiplier=1, max=20). I read "capped at 30 s" together with "huge values must fall back to the jitter", so aRetry-Afterabove 30 s falls back to the jitter instead of being clamped; happy to clamp instead if you prefer. Timeouts,MidStreamFallbackError, empty responses and 200s that fail to parse keepMODEL_RETRIESand are replayed at once, as before.LiteLLM reports a 200 that fails to parse as a 5xx or
APIConnectionError, the same types as a real failure. A real 5xx carrieslitellm_response_headersand a connection error has anhttpx.TransportErrorin its cause chain, and in my checks on the OpenAI and Bedrock paths an unparsed 200 had neither.Measured with an httpx-level mock, primary plus one fallback model:
num_retries = 3The extra Bedrock connection-error calls come from LiteLLM's HTTP client, which re-sends once on
httpx.ConnectError;num_retriesdoes not reach it.Unit tests cover each error class, the bad
Retry-Aftervalues and the stop and wait per class, and an autouse fixture keeps the backoff out of the test run.num_retries = 4inconfiguration.tomlis a new active key, soconfiguration_reference.mdis regenerated and the key count goes to 296. The unit suite has no new failures; Ruff and pre-commit passed.PR Type
Bug fix, Tests, Documentation
Description
Centralize LiteLLM retries in the handler.
Back off rejected requests before model fallback.
Preserve separate timeout and empty-response retry controls.
Update retry defaults, documentation, and regression tests.
Diagram Walkthrough
File Walkthrough
1 files
Centralize rejected-request retries with bounded backoff5 files
Disable handler retry waits throughout unit testsUpdate active configuration count for retry defaultVerify retry classification and disabled client retriesVerify throttling retries preserve ambient AWS credentialsCover retry budgets, error classification, and backoff2 files
Explain same-model retries before fallback model selectionDocument retry default and rejected-request backoff behavior1 files
Enable four handler retries for rejected requests⚙️ Agent run details