fix(router): honor request-level num_retries over a deployment's litellm_params value - #35483
Conversation
|
|
Greptile SummaryThe PR corrects retry-count precedence so an explicit request value is retained over a deployment value while omitted values can still fall back through deployment and global settings
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains
|
| Filename | Overview |
|---|---|
| litellm/router.py | Adjusts shared retry resolution and affected entry points so request-level retry counts take precedence without suppressing deployment defaults |
| tests/test_litellm/test_router_per_deployment_num_retries.py | Adds focused regression coverage for retry precedence and preserves coverage for deployment retries on image generation |
Reviews (4): Last reviewed commit: "fix(router): honor request-level num_ret..." | Re-trigger Greptile
39144e0 to
9422ad5
Compare
|
@greptileai please review the current head 9422ad5 No logic changed since your 5/5. The branch was rebased onto current staging to pick up #35479, which removes a duplicate |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
9422ad5 to
f950382
Compare
|
@greptileai please review the current head f950382 Your 5/5 was against Removing the The seventh pre-fill, in the sync The live-proxy proof in the body was re-captured at this head, and the PR body now also flags that a key/team |
…llm_params value A failing deployment stamps its own litellm_params.num_retries onto the raised exception, and async_function_with_retries adopted that value unconditionally. So a model_list num_retries outranked both the x-litellm-num-retries header and the request body, inverting the documented precedence to model_list > header > body > litellm_settings. The router could not tell a request-level value from its own default because the entry points filled num_retries in with self.num_retries whenever the caller omitted it, collapsing "the request asked for N" and "nobody asked". Drop that pre-fill from _update_kwargs_before_fallbacks and from the six entry points that also did it a line above their own call to it (image generation sync and async, adapter completion, file create, batch create, batch cancel), all of which reach async_function_with_retries, where the router/global default is already resolved. Leaving them would have made the request value never None on those routes and permanently suppressed a deployment num_retries there. The sync text_completion pre-fill stays. That path resolves a deployment and calls litellm.text_completion directly, never entering the retry loop, so no request-versus- deployment ranking happens there and there is nothing to fix; removing the line would only change which value is forwarded to litellm.text_completion, a behaviour change this bug does not call for. async_function_with_retries then adopts the deployment's value only when the request carried none. Precedence is now header > body > model_list > litellm_settings, with the deployment value still beating litellm_settings when the request is silent, on every entry point that retries. Resolves LIT-4772
f950382 to
eaad030
Compare
|
@greptileai please review the current head eaad030 Superseding my previous re-trigger for |
…es is inert (#734) Spell out the four places num_retries can come from and how they rank (header > request body > model_list litellm_params > litellm_settings), and explain that max_retries is the provider SDK knob rather than a second spelling of num_retries, so it has no effect on a routed proxy request. Companion to BerriAI/litellm#35483 (LIT-4772)
TLDR
Problem this solves:
model_listnum_retriesoutranked the header and the bodyx-litellm-num-retries: 0could not disable retriesHow it solves it:
Relevant issues
Docs companion, which writes the precedence down and explains the
max_retriesanswer: BerriAI/litellm-docs#734. It carries no CI dependency on this PR and can merge in either orderLinear ticket
Resolves LIT-4772
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
The observable is the number of requests one proxy call sends upstream, which no real provider emits deterministically, so the deployment points at a tiny counting upstream that 500s every
POST /v1/chat/completionsand serves its tally onGET /count. One proxy call, then read the tally;1 + effective num_retriesis what the customer counts as "number of queries"Both runs use the same config, the same proxy launcher and the same driver script, on the same machine. Before was captured at
1e7b39d15c(the base branch tip this PR was cut from, i.e.litellm/router.pyin its pre-fix state) and after atf95038231f, which the current headeaad030fe6carries forward with an identical tree (git diff f95038231f eaad030fe6is empty; only the commit message changed)config.yaml, counting upstream, and the driver (click to expand)
Before, at
1e7b39d15cThe four
retry-testrows that carry a header or a body value all send 3 requests, which is the deployment'snum_retries: 2plus the initial attempt. The header and the body are ignored, includingx-litellm-num-retries: 0, which should have disabled retries entirely. Theretry-test-nomodelrows show the precedence is already correct as soon as no deployment value existsAfter, at
eaad030fe6Every row now matches
header > body > model_list > litellm_settings. The rows that were already correct are unchanged, so the deployment value keeps its place directly abovelitellm_settingswhen the request says nothing (retry-testwith no header and no body still sends 3), andx-litellm-num-retries: 0now disables retriesOne row verbatim, both sides
Same command, same rig, the only difference being which commit the proxy is running.
retry-testcarriesnum_retries: 2, and the request asks for 3 via the headerBefore, at
1e7b39d15cAfter, at
eaad030fe63 upstream requests is the deployment's
num_retries: 2plus the initial attempt, with the header discarded. 4 is the header's 3 plus the initial attemptThe other entry points keep their deployment retries
Dropping the pre-fill from
_update_kwargs_before_fallbacksalone would have left the six entry points that repeat it ranking their own default above the deployment, silently disabling amodel_listnum_retrieson image generation, adapter completion, and the file and batch routes. Measured throughrouter.aimage_generationagainst a counting upstream, deploymentnum_retries: 4with the router default at0, counting every upstream request (the provider SDK contributes its own attempts per router attempt on this route, hence the multiples of 3)1e7b39d15ceaad030fe615 is 5 router attempts, so the deployment's 4 retries are honoured before and after. The middle row is the regression this PR does not ship, and a test pins it: restoring the
aimage_generationpre-fill alone failstest_deployment_num_retries_applies_to_image_generationOn the second question in the report, request body
max_retriesnum_retriesandmax_retriesare different knobs:num_retriesis the router's own retry loop,max_retriesis the provider SDK's internal retry count. For a routed call the router is the sole retry owner, so the provider client is pinned tomax_retries: 0; that is deliberate, and it is what stops a deploymentnum_retries: Nfrom being applied twice and turning one call into(1 + N) ** 2upstream requests. A request bodymax_retriestherefore has no effect through the proxy, which this PR does not change. Same rig, after the fix, ateaad030fe6Both counts are exactly what the same request sends without
max_retries, so the value is inert rather than partially appliedType
🐛 Bug Fix
Changes
Router._update_kwargs_before_fallbacksused to fillkwargs["num_retries"]in withself.num_retrieswhenever the caller omitted it. That collapsed "the request asked for N retries" and "nobody asked, use the global" into one indistinguishable value, so by the timeasync_function_with_retriesran it had no way to rank a request value against a deployment one. It now leavesnum_retriesexactly as the caller passed it;async_function_with_retriesalready resolves the router default (and remains the safety net for an explicitnum_retries=None, so theNone > intTypeError guard is untouched)Six entry points repeated the same pre-fill a line above their own call to that helper:
image_generation,aimage_generation,aadapter_completion,acreate_file,acreate_batch,acancel_batch. All six reachasync_function_with_retries, so leaving them would have made the request value neverNonethere and permanently suppressed a deploymentnum_retrieson those routes, which is a regression rather than the fix. They are dropped too. The one remaining pre-fill, in the synctext_completion, stays deliberately: that path resolves a deployment and callslitellm.text_completiondirectly without entering the retry loop, so no request-versus-deployment ranking happens there and there is nothing for this fix to correct. Removing the line would only change which value is forwarded intolitellm.text_completion, which is a behaviour change this bug does not call for. Two pre-existing quirks of that path, neither introduced nor addressed here: the final spread puts**kwargsafter**data, so the router default already shadows the deployment's ownlitellm_params.num_retriesthere, and because the same function setsmetadata.model_group, the router-call check inlitellm/main.pyforcesmax_retries = 0on the chat funnel anywayasync_function_with_retrieskeeps the request-level value it popped and adopts the deployment's exception-stampednum_retriesonly when the request carried none. Nothing else about the deployment path changes: it still beatslitellm_settings, still accepts a string value from an env var, and still applies on themock_testing_rate_limit_errorpathTests extend the mapped file. Three new behavioural tests drive
router.acompletionthrough the real retry loop and count attempts via a callback: request beats deployment (and request0disables retries), deployment still beats global when the request is silent, and the request value also wins on the rate-limit mock path, which is the second place a deployment value is stamped onto an exception. All three fail on the unfixed tree and pass with the fix. Two existing tests asserted the old kwargs-filling contract directly and were rewritten against the new one; the invariants they protected (num_retries=Nonemust not raise, an explicit0must survive) stay covered by the behavioural tests in the same classTwo more cover the non-completion entry points through
aimage_generation, counting real upstream requests: a deploymentnum_retriesstill applies there when the request is silent, and a request value still wins. The provider SDK contributes a fixed number of its own attempts per router attempt on that route, so those two calibrate that factor from a single-attempt run rather than hard coding it. Restoring the pre-fill onaimage_generationalone fails the first of them, which is the regression the six deletions closeOne knock-on worth calling out: a key/team
router_settings_override.num_retriesis merged into the request data byroute_llm_request.pyonly when the request itself did not set one, so it reaches the router as the samenum_retrieskwarg and now also outranks a deployment value. That reads as the intent of a per-key/team override, which is described there as overriding the global router settings for that request, and it lands in the same slot a body value would. Distinguishing it from a real request value would need a second kwarg, which is not something this fix needs; flagging it so the ranking is a deliberate call rather than a surpriseFinal Attestation