fix(anthropic): clear error for non-streaming timeout guard; add first-class timeout param - #1255
Merged
Merged
Conversation
peteski22
temporarily deployed
to
integration-tests
August 10, 2026 08:17 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe completion APIs now accept optional request timeouts and forward set values to providers. Providers adapt timeout values to their SDKs. Anthropic translates matching non-streaming timeout guard errors into Completion timeout support
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/any_llm/any_llm.py (1)
580-598: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward
timeouton the non-streaming synchronous path.Line 598 forwards
timeoutonly whenstreamis true. The call at lines 604-612 omits it. Therefore,AnyLLM.completion(..., timeout=600)silently uses the provider default for the normal non-streaming path.Add
timeout=timeoutto the secondself.acompletion(...)call. Add a regression test for synchronous non-streaming completion.Proposed fix
response = run_async_in_sync( self.acompletion( model=model, messages=messages, response_format=response_format, stream=stream, prompt_cache_key=prompt_cache_key, + timeout=timeout, **kwargs, ),As per coding guidelines, test every new branch, including error, raise, and edge paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/any_llm/any_llm.py` around lines 580 - 598, Forward the caller’s timeout through the non-streaming self.acompletion call in AnyLLM.completion, matching the existing streaming path so completion(..., timeout=...) reaches the provider. Add a regression test covering synchronous non-streaming completion and verifying the timeout is propagated.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/any_llm/any_llm.py`:
- Line 580: Add timeout: float | None = ... explicitly to the overload
declarations for both completion and acompletion, while retaining the existing
**kwargs: Any and implementation behavior.
In `@tests/unit/providers/test_anthropic_provider.py`:
- Around line 125-147: Add a focused async test for
AnthropicProvider._acompletion where messages.create raises an unrelated
ValueError, then assert the exact same exception is propagated unchanged. Keep
the existing timeout-guard test intact and mock the client call so no network
access occurs.
- Around line 162-164: Combine the nested context managers in the _acompletion
test into a single with statement containing both patch.object and pytest.raises
contexts, while preserving the existing RuntimeError assertion and call
behavior.
---
Outside diff comments:
In `@src/any_llm/any_llm.py`:
- Around line 580-598: Forward the caller’s timeout through the non-streaming
self.acompletion call in AnyLLM.completion, matching the existing streaming path
so completion(..., timeout=...) reaches the provider. Add a regression test
covering synchronous non-streaming completion and verifying the timeout is
propagated.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da1488f1-9e0d-48e9-9f74-ed0533a67431
📒 Files selected for processing (5)
src/any_llm/any_llm.pysrc/any_llm/api.pysrc/any_llm/providers/anthropic/base.pytests/unit/providers/test_anthropic_provider.pytests/unit/test_completion.py
peteski22
force-pushed
the
fix/anthropic-timeout-handling
branch
from
August 10, 2026 09:13
9d06f13 to
ed8bedd
Compare
peteski22
temporarily deployed
to
integration-tests
August 10, 2026 09:13 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/test_completion.py`:
- Around line 42-46: Expand tests/unit/test_completion.py lines 42-46 by
patching AnyLLM.create and exercising top-level completion() and acompletion()
with both configured and None timeout values, asserting each provider receives
the expected timeout semantics. At tests/unit/test_completion.py lines 79-90,
add a synchronous stream=True test that consumes the returned iterator and
verifies _acompletion() receives the configured timeout; cover every changed
branch.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa70face-8165-41d4-b2a8-7791bba0a954
📒 Files selected for processing (3)
src/any_llm/any_llm.pytests/unit/providers/test_anthropic_provider.pytests/unit/test_completion.py
peteski22
force-pushed
the
fix/anthropic-timeout-handling
branch
from
August 10, 2026 09:25
ed8bedd to
df07c12
Compare
peteski22
temporarily deployed
to
integration-tests
August 10, 2026 09:25 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the Anthropic provider experience by introducing a first-class per-request timeout parameter across the public completion entry points and by translating Anthropic SDK’s opaque non-streaming timeout-guard ValueError into an actionable InvalidRequestError.
Changes:
- Added an optional
timeoutparameter tocompletion()/acompletion()andAnyLLM.completion/AnyLLM.acompletion, forwarding it to providers only when set. - Updated the Anthropic provider to catch the SDK’s non-streaming timeout-guard
ValueErrorand re-raise a clearerInvalidRequestErrorthat explains how to fix it. - Added unit tests covering
timeoutexposure/forwarding and the Anthropic non-streaming guard translation behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/any_llm/api.py |
Exposes timeout on top-level completion and acompletion and documents it. |
src/any_llm/any_llm.py |
Adds timeout to AnyLLM completion APIs and forwards it via kwargs only when set. |
src/any_llm/providers/anthropic/base.py |
Translates Anthropic SDK non-streaming guard ValueError into InvalidRequestError with guidance. |
tests/unit/test_completion.py |
Verifies timeout is exposed on all public completion entry points and forwarded correctly (including dropping explicit None). |
tests/unit/providers/test_anthropic_provider.py |
Adds regression tests for the non-streaming guard and confirms timeout bypasses it. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
peteski22
force-pushed
the
fix/anthropic-timeout-handling
branch
from
August 10, 2026 19:16
df07c12 to
e11f166
Compare
peteski22
temporarily deployed
to
integration-tests
August 10, 2026 19:16 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
August 10, 2026 20:27 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
reviewed
Aug 10, 2026
njbrake
left a comment
njbrake
left a comment
Contributor
There was a problem hiding this comment.
Note: this comment was drafted by Claude via back-and-forth with @njbrake. The reasoning and decisions are his; the prose is Claude's.
Heads up that I pushed 77ef72d to this branch. Drop it if you'd rather own the change.
The Anthropic half is solid, and the bot findings all look addressed. Marker-matching is the right call given the SDK exposes no error code or subtype for that guard, and driving it through a real AsyncAnthropic client means a future reword fails in CI rather than slipping through.
One correction to the description: "every other provider are unaffected, no cross-provider regression" does not hold. timeout is forwarded through **kwargs to every provider, and three SDKs have no such keyword, so they raise before anything is sent:
mistral -> TypeError: Chat.complete_async() got an unexpected keyword argument 'timeout'
cohere -> TypeError: AsyncV2Client.chat() got an unexpected keyword argument 'timeout'
xai -> TypeError: BaseClient.create() got an unexpected keyword argument 'timeout'
Reproduced against the real SDKs. Not a regression in the strict sense, since timeout in kwargs did the same on main. But a typed, documented parameter on the universal API is a promise, and prompt_cache_key sets the precedent for handling partial support deliberately instead of leaking a vendor TypeError.
77ef72d maps it per provider: timeout_ms for mistral, request_options for cohere, and a warning pointing at client_args for xai, whose SDK only sets timeouts on the gRPC client. That follows Bedrock #1240. All three now reach transport instead of failing on the keyword.
Three left for you:
-
watsonxpasses the converted kwargs asparams=tomodel_inference.achat, sotimeoutwould land as a generation parameter. I read that rather than ran it, so worth confirming.sagemakermay share the shape. -
"An explicit
Noneis dropped, leaving the default path unchanged" holds for the default but not for an explicitNone. On main,timeout=Nonereaches the SDK, whereis_given(None)is true, so it lifts the guard. Now it is indistinguishable from unset and the same call raisesInvalidRequestError. Worth a docstring line, since the typed signature makes "no limit" unreachable. -
amessages()still surfaces the bareValueError; the translation only wraps the_acompletioncall site. Same guard, same file.
tbille
force-pushed
the
fix/anthropic-timeout-handling
branch
from
August 11, 2026 06:35
77ef72d to
8690e9a
Compare
tbille
temporarily deployed
to
integration-tests
August 11, 2026 06:35 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
peteski22
temporarily deployed
to
integration-tests
August 11, 2026 10:39 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
@njbrake kept Full scope found:
Pushed on top of your commit:
Filed #1262 for the better-engineered central approach (an opt-in support declaration like |
Expose `timeout` (in seconds) as a typed, documented parameter on the public completion()/acompletion() functions and on AnyLLM.completion/acompletion, mirroring how prompt_cache_key is surfaced. It is forwarded to the provider through kwargs only when set, rather than added to CompletionParams, so providers that already read `timeout` from kwargs (Gemini, Bedrock) and every other provider are unaffected. An explicit None is dropped, leaving the default path and its provider behavior unchanged. Relates to #1251.
…uard The Anthropic SDK runs a client-side pre-flight check that rejects a non-streaming completion whose max_tokens could exceed its time limit for a single response, raising a bare ValueError before any request is sent. Large max_tokens requests therefore failed with an opaque message even though the actual response might be tiny. Catch that specific guard and raise an InvalidRequestError that names the levers a caller has (pass a timeout, or stream), while re-raising any unrelated ValueError untouched. The one point of coupling to the SDK's wording is isolated in a named constant. Fixes #1251.
The new first-class `timeout` parameter is forwarded through kwargs to every provider, but three SDKs have no such keyword and raised `TypeError: ... got an unexpected keyword argument 'timeout'` before any request was sent: - mistral names it `timeout_ms`, so translate seconds to milliseconds. - cohere carries it inside `request_options`, so fold it in there without overriding an explicit one. - xai sets timeouts on the gRPC client only, so drop it with a warning pointing at `client_args`, matching the bedrock precedent. Verified against the real SDKs: all three now reach the transport layer instead of failing on an unexpected keyword. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sonx, sagemaker, lmstudio The first-class `timeout` parameter is forwarded through kwargs to every provider, but four more SDKs cannot honor a per-request timeout and would otherwise mishandle it before any request is sent: - ollama's chat() has no timeout keyword, so it raised TypeError. - watsonx forwards these kwargs as generation parameters, so timeout was sent as an invalid model field. - SageMaker serializes these kwargs into the request body, so timeout was sent as a payload field. - LM Studio folds these kwargs into the prediction config, which has no per-request timeout. Each now pops `timeout` and warns, pointing at client_args, matching the xai precedent; huggingface already stripped it. A central, enforced design is tracked in #1262. Relates to #1251.
…; document timeout=None The pre-flight guard translation only wrapped the completion path, so the native messages API (_amessages) still surfaced the bare SDK ValueError for a large-max_tokens non-streaming request. Extract the translation into a shared helper and apply it to both paths. Also document that an explicit `timeout=None` is treated the same as omitting it: the typed signature makes an unbounded timeout unreachable via the parameter. Relates to #1251.
tbille
force-pushed
the
fix/anthropic-timeout-handling
branch
from
August 11, 2026 11:20
10a992d to
ee23627
Compare
tbille
temporarily deployed
to
integration-tests
August 11, 2026 11:20 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
tbille
approved these changes
Aug 11, 2026
11 tasks
11 tasks
pull Bot
pushed a commit
to pepe57/any-llm
that referenced
this pull request
Aug 12, 2026
…rs (mozilla-ai#1263) ## Description Follow-up to mozilla-ai#1255. That PR added a first-class `timeout` parameter and forwarded it to every provider through `**kwargs`, handling each SDK shape individually (map where supported, warn+drop where not). As mozilla-ai#1262 notes, that per-provider approach is whack-a-mole: a new provider that forwards kwargs to its SDK would silently `TypeError` or mis-send `timeout` again, with nothing to catch it. The two silent mis-sends (`sagemaker`, `watsonx`) are the dangerous class. This PR centralizes the contract the way `prompt_cache_key` already does with `PROMPT_CACHE_KEY_SUPPORT`: - A per-provider `TIMEOUT_SUPPORT` capability: `Literal["unsupported", "native", "mapped"]`, defaulting to `unsupported`. - A single guard in the base `acompletion` (`_validate_and_forward_timeout`) that routes `timeout`: - `native`/`mapped` → forwarded via `kwargs` (mapped providers translate it in their own conversion); - `unsupported` → raises a clear `UnsupportedParameterError` (hinting to set it on the client via `client_args`) instead of leaking a vendor `TypeError` or silently mis-sending the value. - `timeout=None` is treated as unset in one place, so an unbounded timeout is not expressible. A new provider now inherits the safe `unsupported` default and must opt in explicitly, so the failure mode is a loud, testable default rather than a silent per-SDK surprise. ### Provider classification - **native** (SDK accepts `timeout` directly): OpenAI base (covers all OpenAI-compatible and config-registry providers), Anthropic base, `cerebras`, `groq`, `together`. - **mapped** (translated): Google base / `gemini`+`vertexai` (`http_options` ms), `bedrock` (per-timeout boto3 client), `cohere` (`request_options`), `mistral` (`timeout_ms`). - **unsupported** (no per-request timeout): `huggingface`, `lmstudio`, `ollama`, `sagemaker`, `watsonx`, `xai`, `azure`. The now-dead per-provider warn+drop code is removed; each records *why* it is unsupported in a comment (e.g. `watsonx`/`sagemaker` would otherwise mis-send `timeout` into the request body). ### Behavior change For `unsupported` providers, passing a per-request `timeout` now **raises `UnsupportedParameterError`** rather than logging a warning and dropping it (the mozilla-ai#1255 behavior). This matches the `prompt_cache_key` precedent and avoids silently proceeding without a requested bound. Client-level timeouts via `client_args` are unaffected. Since the typed `timeout` shipped only in mozilla-ai#1255, regression exposure is minimal. ### Note on `azure` `azure` (Azure AI Inference SDK) is deliberately left at the conservative `unsupported` default: it was untouched by mozilla-ai#1255, is not verified to honor a per-request timeout, and its SDK rejects unknown transport kwargs. It is flagged in-code for a maintainer to promote to `native`/`mapped` with a verifying test if appropriate. ### Notes on tests - Added `test_acompletion_rejects_timeout_for_unsupported_provider` (central rejection path). The existing forward and `None`-drop tests plus the mapped-translation tests (`cohere`/`mistral`/`gemini`) continue to cover the supported paths; the obsolete per-provider warn+drop tests were removed. - `pre-commit` (ruff + ruff-format + mypy strict) is clean for the changed code (no new mypy errors versus the `main` baseline). - The only failing tests in a local `tests/unit` run are pre-existing and identical to `main`: the `mistralai` `ThinkChunk.signature` SDK drift and the `lmstudio` optional SDK not being installed. Neither is touched by this change. - Integration tests were not run locally (they need real keys or the `run-integration-tests` label). The change is provider-SDK-agnostic at the boundary; a real `cohere`/`mistral`/`gemini`/`bedrock` run would confirm the mapped translation end to end. ## PR Type - 🆕 New Feature - 💅 Refactor ## Relevant issues Fixes mozilla-ai#1262 Follow-up to mozilla-ai#1255 (per-provider timeout handling) ## Checklist - [x] I understand the code I am submitting. - [x] I have added unit tests that prove my fix/feature works - [x] I have run this code locally and verified it fixes the issue. - [x] New and existing tests pass locally - [x] Documentation was updated where necessary - [x] I have read and followed the [contribution guidelines](https://github.com/mozilla-ai/any-llm/blob/main/CONTRIBUTING.md) - [x] **AI Usage:** - [ ] No AI was used. - [x] AI was used for drafting/refactoring. - [ ] This is fully AI-generated. ## AI Usage Information - AI Model used: Claude Opus 4.8 - AI Developer Tool used: Claude Code - Any other info you'd like to share: Claude was used collaboratively. It assisted with investigation, design, drafting, and iteration under human review and direction, including the decision to raise `UnsupportedParameterError` (over a centralized warn+drop) and restoring the per-provider rationale for the `unsupported` classifications. - [x] I am an AI Agent filling out this form (check box if true) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added per-request timeout support to synchronous and asynchronous completion and messages requests. * Timeout values are automatically forwarded or converted for compatible providers. * Omitting a timeout preserves provider defaults. * Client-level timeout configuration remains available where per-request timeouts are unsupported. * **Bug Fixes** * Unsupported per-request timeouts are now rejected clearly before requests are sent. * Improved timeout forwarding and validation across synchronous and asynchronous APIs. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: njbrake <njbrake@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 17, 2026
This was referenced Sep 9, 2026
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes the opaque failure reported in #1251: a non-streaming Anthropic completion with a large
max_tokens(roughly> 128000/6 ≈ 21,333) raises a bareValueError: Streaming is required for operations that may take longer than 10 minutes.from the SDK's client-side pre-flight check, before any request is sent, even when the actual response is tiny.While investigating I confirmed that a
timeoutalready reaches the Anthropic SDK today (via completionkwargs→messages.create(timeout=...)and viaclient_args={"timeout": ...}→AsyncAnthropic(timeout=...)), and either one already lifts the guard. So the real gaps were (a)timeoutwas not a first-class, documented, typed parameter, and (b) when no timeout is set the caller hits the SDK's opaque error with no hint at the fix. This PR closes both:timeoutparameter oncompletion()/acompletion()andAnyLLM.completion/acompletion, mirroring howprompt_cache_keyis surfaced. It is forwarded to the provider throughkwargsonly when set, rather than being added toCompletionParams. Cross-provider handling: because it rideskwargsto every provider, those whose SDK accepts atimeoutkeyword (OpenAI-based, groq, cerebras, together, anthropic) honor it directly; Gemini (timeout kwarg rejected by Gemini provider — should be a first-class parameter #901)/Bedrock (Bedrock: BYO credentials aren't forwarded to boto3, no bearer-token support, and timeout kwarg breaks completions #1240) already map it; and providers whose SDK has no per-request timeout are handled explicitly so they neitherTypeErrornor silently mis-send it — mistral (timeout_ms), cohere (request_options), and warn+drop for xai, ollama, watsonx, sagemaker, lmstudio (huggingface already stripped it). A central, enforced version of this is tracked in Centralize per-request timeout handling across providers (opt-in support, like prompt_cache_key) #1262. An explicittimeout=Noneis treated the same as omitting it, so an unbounded timeout is not expressible via the typed parameter.InvalidRequestErrorthat names the levers (Pass a timeout (in seconds) or use stream=True), while re-raising any unrelatedValueErroruntouched. The single point of coupling to the SDK's wording is isolated in a named constant; the SDK exposes only a bareValueError(no error code/subtype) for this condition, so matching the (version-stable) message is the only available signal, and a real-client test would surface any future reword in CI.PR Type
Relevant issues
Fixes #1251
Related: #901 (Gemini timeout), #1240 (Bedrock timeout)
Checklist
Notes on tests
New tests are regression-verified (they fail on
main, pass with the change):tests/unit/test_completion.py:timeoutis exposed on all four entry points; forwarded to the provider only when set; explicitNoneis dropped.tests/unit/providers/test_anthropic_provider.py: without a timeout the large-max_tokensnon-streaming path now raises a clearInvalidRequestError(driven through a realAsyncAnthropicclient so the SDK guard actually runs); with a timeout it bypasses the guard and reaches transport.pre-commit(ruff + format + mypy) is clean. The only failing tests in the full localtests/unitrun are pre-existing, unrelated optional-SDK import errors (lmstudionot installed locally), identical to themainbaseline.AI Usage Information
AI Model used: Claude Opus 4.8
AI Developer Tool used: Claude Code
Any other info you'd like to share: Used collaboratively — AI assisted with investigation, drafting, and iteration under human review and direction (including auditing and fixing cross-provider timeout handling surfaced in review).
I am an AI Agent filling out this form (check box if true)
Summary by CodeRabbit
New Features
Bug Fixes