[Fix] Request Timeout needs to be also fetched from litellm_settings.request_timeout - #25591
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
add fetch ``request_timeout`` from litellm_settings
Greptile SummaryThis PR introduces a The Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/improvement suggestions that do not block functionality. Previous thread concerns (print statement, sentinel applied to per-call timeouts, constants-change widening scope) are all resolved. The core fix — reading litellm.request_timeout via getattr and coercing the package default sentinel to 600 s — is correct and well-tested for non-6000 values. Remaining P2 items are minor and do not block merge. litellm/litellm_core_utils/completion_timeout.py — sentinel edge case for global_timeout==6000; litellm/llms/custom_httpx/http_handler.py — _DEFAULT_TIMEOUT widened from 5s to 600s
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/completion_timeout.py | New helper class correctly resolves timeout from model params → kwargs → global litellm.request_timeout fallback; sentinel logic is confined to the fallback path, not applied to explicit per-call values. |
| litellm/constants.py | Adds COMPLETION_HTTP_FALLBACK_SECONDS (600s) and HTTP_HANDLER_CONNECT_TIMEOUT_SECONDS (5s) constants; DEFAULT_REQUEST_TIMEOUT_SECONDS remains 6000s to avoid widening scope to Router/TTS paths. |
| litellm/main.py | Wires CompletionTimeout.resolve() into the completion() dispatch path; global_timeout reads litellm.request_timeout so proxy config changes propagate correctly. |
| litellm/llms/custom_httpx/http_handler.py | _DEFAULT_TIMEOUT changed from httpx.Timeout(5.0, connect=5.0) to httpx.Timeout(600.0, connect=5.0) — factory functions were already using 600s explicitly, but direct instantiation of HTTPHandler()/AsyncHTTPHandler() with no timeout now gets 600s instead of 5s. |
| tests/test_litellm/test_completion_timeout_resolution.py | Good unit coverage for CompletionTimeout.resolve() — explicit, kwargs, global sentinel, httpx coercion paths all tested; no real network calls. |
| tests/test_litellm/llms/azure_ai/claude/test_main_azure_anthropic_timeout.py | Tests that main.py correctly forwards explicit timeout=42.5 to the Azure Anthropic handler; no real network calls. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["completion(timeout=T, **kwargs)"] --> B{T is not None?}
B -- Yes --> E[resolved = T]
B -- No --> C{kwargs timeout?}
C -- kwargs.timeout --> E
C -- kwargs.request_timeout --> E
C -- Neither --> D["_fallback_when_no_explicit_timeout(litellm.request_timeout)"]
D --> F{global_timeout is None or == 6000.0?}
F -- Yes --> G["resolved = 600.0 (COMPLETION_HTTP_FALLBACK_SECONDS)"]
F -- No --> H["resolved = float(global_timeout)"]
E --> I{isinstance httpx.Timeout and provider supports it?}
G --> I
H --> I
I -- Yes --> J[Return httpx.Timeout as-is]
I -- No, httpx.Timeout --> K["resolved = read_timeout or 600.0"]
I -- float/str --> L["resolved = float(resolved)"]
K --> M[Return float]
L --> M
Reviews (9): Last reviewed commit: "modify default timeout values, replacing..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…efaults handled in the proxy
harish876
changed the title
harish876
changed the title
litellm_settings.request_timeout
| @@ -393,7 +393,7 @@ | |||
| ) | |||
There was a problem hiding this comment.
request_timeout default change widens scope beyond completion()
Changing the default from 6000 to 600 fixes the completion() regression flagged in the previous thread, but litellm.request_timeout is also the default for Router.__init__ (self.timeout = timeout or litellm.request_timeout, router.py:530), speech() (main.py:6760), and the Anthropic / Azure-Anthropic / OpenAI count-token handlers. All of these would silently drop from a 6000 s ceiling to 600 s for users who have not set an explicit timeout. Long-running router calls or TTS jobs that complete in 600–6000 s will now time out.
Per the "avoid backwards-incompatible changes without user-controlled flags" rule, consider keeping the constant at 6000 (or introducing a separate COMPLETION_REQUEST_TIMEOUT constant) and only using the explicit-600 fallback inside _resolve_completion_timeout() itself, where you control the scope.
Rule Used: What: avoid backwards-incompatible changes without... (source)
ishaan-berri
left a comment
ishaan-berri
left a comment
There was a problem hiding this comment.
what problem does this solve ?
| return entry | ||
|
|
||
|
|
||
| def _resolve_completion_timeout( |
There was a problem hiding this comment.
this should be it's own file + class
There was a problem hiding this comment.
Currently, we have 2 issues related to timeout. The timeout parameters can be defined either globally under litellm_settings.request_timeout or at a per model level. In the completion route this litellm_settings.request_timeout config is never read, and hence defaults to a 600s timeout. This doesnt solve the root cause, but a potential fix.
ishaan-berri
left a comment
ishaan-berri
left a comment
There was a problem hiding this comment.
- this just solves it for /chat/completions what about /responses, /messages do they have the same bug ?
- IS there a simpler way to fix this across call types
| resolved_from_litellm_request_timeout_attr = True | ||
| if timeout is None: | ||
| timeout = 600 | ||
| elif ( |
There was a problem hiding this comment.
this section looks super complicated, can you write cleaner code here ?
it's hard to follow how this logic is set
There was a problem hiding this comment.
Agreed. The reason for this logic is due to the fact that the fallback/default value for litellm.request_timeout is different than the 600s. To avoid this, this logic is mangled, but agreed to the point that this code looks complex. Working on this to make it readable.
…anthropic-timeout-bug
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 29203053 | Triggered | Generic Password | fdccbcb | .circleci/config.yml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
| if not isinstance(resolved, httpx.Timeout) and float(resolved) == float( | ||
| DEFAULT_REQUEST_TIMEOUT_SECONDS | ||
| ): | ||
| resolved = 600.0 |
There was a problem hiding this comment.
timeout=6000 from the caller
The sentinel float(resolved) == float(DEFAULT_REQUEST_TIMEOUT_SECONDS) runs over all resolved values, including those taken from model_timeout (the value the caller passed directly to completion()). A user who calls completion(..., timeout=6000) or sets request_timeout: 6000 in their model config will silently receive 600 s instead — a 10× reduction. The old or-based chain did not apply any such cap, so this is a backwards-incompatible regression for anyone who deliberately configured a long timeout.
The guard should only trigger when the value originated from the package default, not from an explicit caller-supplied timeout:
# Only coerce the sentinel when no per-call or per-model timeout was set
def _is_global_sentinel(value: float) -> bool:
return float(value) == float(DEFAULT_REQUEST_TIMEOUT_SECONDS)
if model_timeout is not None:
resolved = model_timeout
elif kwargs.get("timeout") is not None:
resolved = kwargs["timeout"]
elif kwargs.get("request_timeout") is not None:
resolved = kwargs["request_timeout"]
else:
# Only here can we safely coerce the sentinel
raw = global_timeout if global_timeout is not None else 600
resolved = 600.0 if (not isinstance(raw, httpx.Timeout) and _is_global_sentinel(float(raw))) else rawWith this structure the coercion never reaches values the caller set explicitly.
Rule Used: What: avoid backwards-incompatible changes without... (source)
| elif kwargs.get("request_timeout") is not None: | ||
| resolved = kwargs["request_timeout"] | ||
| else: | ||
| resolved = global_timeout if global_timeout is not None else 600 |
There was a problem hiding this comment.
this looks odd, can you write this cleanly
There was a problem hiding this comment.
sure, will look into this again
There was a problem hiding this comment.
Logic for this is cleaned up
| elif kwargs.get("request_timeout") is not None: | ||
| resolved = kwargs["request_timeout"] | ||
| else: | ||
| resolved = global_timeout if global_timeout is not None else 600 |
There was a problem hiding this comment.
600, should be a constant
There was a problem hiding this comment.
why is it not DEFAULT_REQUEST_TIMEOUT_SECONDS ?
There was a problem hiding this comment.
if litellm_settings.request_timeout is unset, then it falls back to DEFAULT_REQUEST_TIMEOUT_SECONDS. The default for chat completion is 600, since thats the default for openai as well. So specifically the default for chat completion is 600.
Agreed, will make it into a const. We did not define default for chat completion in constants before, we can do it now. Will fix
…eslve function readable by moving fallback logic to a seperate function
2147d07
into
BerriAI:litellm_ishaan_april15_2
3 participants
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Screenshots / Proof of Fix
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes
litellm/main.pyAdded
_resolve_completion_timeout()with an explicitNonechain (avoidsor/truthiness bugs):timeoutargumentkwargs["timeout"]kwargs["request_timeout"](model / deployment alias)getattr(litellm, "request_timeout", None)— picks uplitellm_settings.request_timeoutafter proxy config load600secondsPreserved
httpx.Timeoutbehavior: unchanged for providers wheresupports_httpx_timeoutis true; otherwise coerce using read timeout (or600.0).completion()setstimeout = _resolve_completion_timeout(...)so all provider branches (includingazure_ai→azure_anthropic_chat_completions.completion) see the same resolved value.Docstring on the helper describes sources: deployment/model config vs
litellm_settings.request_timeout.