-
Notifications
You must be signed in to change notification settings - Fork 227
fix(responses): accept per-request timeout on aresponses #1308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,11 +1,13 @@ | ||||||||||||
| from inspect import signature | ||||||||||||
| from typing import Any, cast | ||||||||||||
| from unittest.mock import AsyncMock, patch | ||||||||||||
|
|
||||||||||||
| import pytest | ||||||||||||
| from pydantic import ValidationError | ||||||||||||
|
|
||||||||||||
| from any_llm import AnyLLM | ||||||||||||
| from any_llm.api import aresponses | ||||||||||||
| from any_llm.api import aresponses, responses | ||||||||||||
| from any_llm.exceptions import UnsupportedParameterError | ||||||||||||
| from any_llm.types.responses import ResponsesParams | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
|
|
@@ -115,3 +117,64 @@ def add(a: int, b: int) -> int: | |||||||||||
| assert tools[1]["parameters"] == {} | ||||||||||||
| assert tools[1]["strict"] is True | ||||||||||||
| assert tools[3] == {"type": "web_search"} | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_responses_exposes_timeout_parameter() -> None: | ||||||||||||
| """The Responses entry points advertise timeout the same way the completion ones do.""" | ||||||||||||
| assert "timeout" in signature(responses).parameters | ||||||||||||
| assert "timeout" in signature(aresponses).parameters | ||||||||||||
| assert "timeout" in signature(AnyLLM.aresponses).parameters | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| @pytest.mark.asyncio | ||||||||||||
| @pytest.mark.parametrize(("requested_timeout", "expected"), [(60, 60), (None, None)]) | ||||||||||||
| async def test_timeout_forwarded_to_provider_not_params( | ||||||||||||
| requested_timeout: float | None, expected: float | None | ||||||||||||
| ) -> None: | ||||||||||||
| """timeout is an SDK request option: it must reach the provider call, never ResponsesParams.""" | ||||||||||||
| llm = AnyLLM.create("openai", api_key="test-key") | ||||||||||||
| with patch.object(type(llm), "_aresponses", new=AsyncMock(return_value=object())) as mock_aresponses: | ||||||||||||
| await llm.aresponses("gpt-4.1-mini", "hello", timeout=requested_timeout) | ||||||||||||
|
|
||||||||||||
| assert mock_aresponses.call_args.kwargs.get("timeout") == expected | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Assert that Line 139 uses Proposed test change- assert mock_aresponses.call_args.kwargs.get("timeout") == expected
+ if requested_timeout is None:
+ assert "timeout" not in mock_aresponses.call_args.kwargs
+ else:
+ assert mock_aresponses.call_args.kwargs["timeout"] == expectedAs per coding guidelines, “test every new branch, including error, raise, and edge paths”. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||
| params = mock_aresponses.call_args.args[0] | ||||||||||||
| assert "timeout" not in params.model_dump(exclude_none=True) | ||||||||||||
|
Comment on lines
+131
to
+141
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Cover both structured-output provider paths. The mock replaces As per coding guidelines, “Test both the dataclass/dict structured-output path ( 🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| @pytest.mark.asyncio | ||||||||||||
| async def test_aresponses_helper_forwards_timeout_to_provider() -> None: | ||||||||||||
| """The public aresponses() helper routes timeout through to the provider call.""" | ||||||||||||
| provider = AnyLLM.create("openai", api_key="test-key") | ||||||||||||
| with ( | ||||||||||||
| patch("any_llm.any_llm.AnyLLM.create", return_value=provider), | ||||||||||||
| patch.object(provider, "_aresponses", new=AsyncMock(return_value=object())) as mock_aresponses, | ||||||||||||
| ): | ||||||||||||
| await aresponses("gpt-4.1-mini", "hello", provider="openai", api_key="test-key", timeout=60) | ||||||||||||
|
|
||||||||||||
| assert mock_aresponses.call_args.kwargs["timeout"] == 60 | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_responses_helper_forwards_timeout_to_provider_sync() -> None: | ||||||||||||
| """The synchronous responses() helper routes timeout through the same path.""" | ||||||||||||
| provider = AnyLLM.create("openai", api_key="test-key") | ||||||||||||
| with ( | ||||||||||||
| patch("any_llm.any_llm.AnyLLM.create", return_value=provider), | ||||||||||||
| patch.object(provider, "_aresponses", new=AsyncMock(return_value=object())) as mock_aresponses, | ||||||||||||
| ): | ||||||||||||
| responses("gpt-4.1-mini", "hello", provider="openai", api_key="test-key", timeout=60) | ||||||||||||
|
|
||||||||||||
| assert mock_aresponses.call_args.kwargs["timeout"] == 60 | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| @pytest.mark.asyncio | ||||||||||||
| async def test_aresponses_rejects_timeout_for_unsupported_provider() -> None: | ||||||||||||
| """A provider declaring no per-request timeout support is rejected before the request is built.""" | ||||||||||||
| llm = AnyLLM.create("openai", api_key="test-key") | ||||||||||||
| llm.TIMEOUT_SUPPORT = "unsupported" | ||||||||||||
| with ( | ||||||||||||
| patch.object(llm, "_aresponses", new=AsyncMock()) as mock_aresponses, | ||||||||||||
| pytest.raises(UnsupportedParameterError, match="timeout"), | ||||||||||||
| ): | ||||||||||||
| await llm.aresponses("gpt-4.1-mini", "hello", timeout=60) | ||||||||||||
|
|
||||||||||||
| mock_aresponses.assert_not_called() | ||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe mapped timeout handling accurately.
The text says that
timeoutis passed through to the provider client or SDK. A provider withTIMEOUT_SUPPORT == "mapped"can translate the value in its conversion layer instead. Describe timeout as conditionally forwarded or mapped. Keep the synchronous and asynchronous descriptions identical.Also applies to: 514-518
🤖 Prompt for AI Agents