From 7f39872073c034fb3fedfcee5a25699bdcc47fc1 Mon Sep 17 00:00:00 2001 From: Aakash Kattelu Date: Tue, 4 Aug 2026 11:36:25 -0400 Subject: [PATCH] refactor(llm): address review nits on timeout plumbing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply eisene's review feedback: - Rename PROVIDER_TIMEOUT_ERROR → PROVIDER_TIMEOUT_ERROR_TEXT - Move request_timeout_from_extra_params from backend.py (pure dataclasses) to request_builder.py (request assembly) - Add comment explaining Gemini's ms timeout conversion - Generalize _normalize_extra_params with _strip_none_params helper Co-Authored-By: Claude Opus 4.6 --- src/config.py | 14 ++++++----- src/llm/backend.py | 21 ----------------- src/llm/backends/anthropic.py | 8 +++---- src/llm/backends/gemini.py | 13 +++++------ src/llm/backends/openai.py | 8 +++---- src/llm/request_builder.py | 44 +++++++++++++++++++++++++++++------ 6 files changed, 57 insertions(+), 51 deletions(-) diff --git a/src/config.py b/src/config.py index 82fa5716..177bef05 100644 --- a/src/config.py +++ b/src/config.py @@ -67,32 +67,34 @@ ThinkingEffortLevel = Literal[ StructuredOutputMode = Literal["json_schema", "json_object"] -PROVIDER_TIMEOUT_ERROR = "provider_params.timeout must be a positive number of seconds" +PROVIDER_TIMEOUT_ERROR_TEXT = ( + "provider_params.timeout must be a positive number of seconds" +) def coerce_provider_timeout(value: Any) -> float: """Coerce a `provider_params.timeout` value to positive, finite seconds. Canonical implementation shared by config-load validation (here) and - per-request validation (`src.llm.backend.request_timeout_from_extra_params`, + per-request validation (`src.llm.request_builder.request_timeout_from_extra_params`, which translates the ValueError into a ValidationException). Lives in config.py because src.exceptions imports src.config, so config validators cannot raise Honcho exception types. """ if isinstance(value, bool): - raise ValueError(PROVIDER_TIMEOUT_ERROR) + raise ValueError(PROVIDER_TIMEOUT_ERROR_TEXT) if isinstance(value, int | float): timeout = float(value) elif isinstance(value, str): try: timeout = float(value.strip()) except ValueError as exc: - raise ValueError(PROVIDER_TIMEOUT_ERROR) from exc + raise ValueError(PROVIDER_TIMEOUT_ERROR_TEXT) from exc else: - raise ValueError(PROVIDER_TIMEOUT_ERROR) + raise ValueError(PROVIDER_TIMEOUT_ERROR_TEXT) if not math.isfinite(timeout) or timeout <= 0: - raise ValueError(PROVIDER_TIMEOUT_ERROR) + raise ValueError(PROVIDER_TIMEOUT_ERROR_TEXT) return timeout diff --git a/src/llm/backend.py b/src/llm/backend.py index 0eca1fc9..380911d2 100644 --- a/src/llm/backend.py +++ b/src/llm/backend.py @@ -6,9 +6,6 @@ from typing import Any, Protocol, runtime_checkable from pydantic import BaseModel -from src.config import coerce_provider_timeout -from src.exceptions import ValidationException - @dataclass(slots=True) class ToolCallResult: @@ -48,24 +45,6 @@ class StreamChunk: output_tokens: int | None = None -def request_timeout_from_extra_params( - extra_params: dict[str, Any] | None, -) -> float | None: - """Return a validated per-request provider timeout from extra params. - - Config-sourced timeouts are already validated and normalized at config - load (`coerce_provider_timeout` in src.config); this guards extra_params - passed programmatically at call time. - """ - if not extra_params or "timeout" not in extra_params: - return None - - try: - return coerce_provider_timeout(extra_params["timeout"]) - except ValueError as exc: - raise ValidationException(str(exc)) from exc - - @runtime_checkable class ProviderBackend(Protocol): """Transport-agnostic interface for LLM providers. diff --git a/src/llm/backends/anthropic.py b/src/llm/backends/anthropic.py index c37adf1e..9f58af07 100644 --- a/src/llm/backends/anthropic.py +++ b/src/llm/backends/anthropic.py @@ -8,13 +8,11 @@ from typing import Any from anthropic.types import TextBlock, ThinkingBlock, ToolUseBlock from pydantic import BaseModel, ValidationError -from src.llm.backend import ( - CompletionResult, - StreamChunk, - ToolCallResult, +from src.llm.backend import CompletionResult, StreamChunk, ToolCallResult +from src.llm.request_builder import ( + apply_sdk_passthroughs, request_timeout_from_extra_params, ) -from src.llm.request_builder import apply_sdk_passthroughs from src.llm.structured_output import repair_response_model_json, schema_instruction diff --git a/src/llm/backends/gemini.py b/src/llm/backends/gemini.py index ae9b6b02..3f6ade64 100644 --- a/src/llm/backends/gemini.py +++ b/src/llm/backends/gemini.py @@ -8,19 +8,17 @@ from google.genai import types as genai_types from pydantic import BaseModel from src.exceptions import LLMError, ValidationException -from src.llm.backend import ( - CompletionResult, - StreamChunk, - ToolCallResult, - request_timeout_from_extra_params, -) +from src.llm.backend import CompletionResult, StreamChunk, ToolCallResult from src.llm.caching import ( GeminiCacheHandle, PromptCachePolicy, build_cache_key, gemini_cache_store, ) -from src.llm.request_builder import coerce_passthrough_mapping +from src.llm.request_builder import ( + coerce_passthrough_mapping, + request_timeout_from_extra_params, +) from src.llm.structured_output import repair_response_model_json, schema_instruction GEMINI_BLOCKED_FINISH_REASONS = { @@ -323,6 +321,7 @@ class GeminiBackend: if timeout is not None: if http_options is None: http_options = genai_types.HttpOptions() + # Gemini has no native timeout kwarg; set the httpx-level value in ms. http_options.timeout = int(timeout * 1000) if http_options is not None: config["http_options"] = http_options diff --git a/src/llm/backends/openai.py b/src/llm/backends/openai.py index 29d91971..672709ff 100644 --- a/src/llm/backends/openai.py +++ b/src/llm/backends/openai.py @@ -10,13 +10,11 @@ from openai import BadRequestError, LengthFinishReasonError from pydantic import BaseModel, ValidationError from src.exceptions import ValidationException -from src.llm.backend import ( - CompletionResult, - StreamChunk, - ToolCallResult, +from src.llm.backend import CompletionResult, StreamChunk, ToolCallResult +from src.llm.request_builder import ( + apply_sdk_passthroughs, request_timeout_from_extra_params, ) -from src.llm.request_builder import apply_sdk_passthroughs from src.llm.structured_output import ( StructuredOutputError, empty_structured_output, diff --git a/src/llm/request_builder.py b/src/llm/request_builder.py index 20c710aa..7eadab71 100644 --- a/src/llm/request_builder.py +++ b/src/llm/request_builder.py @@ -11,14 +11,13 @@ from typing import Any, cast from pydantic import BaseModel -from src.config import ModelConfig, PromptCachePolicy +from src.config import ModelConfig, PromptCachePolicy, coerce_provider_timeout from src.exceptions import ValidationException from .backend import ( CompletionResult, ProviderBackend, StreamChunk, - request_timeout_from_extra_params, ) # Operator escape-hatch keys recognized inside ModelConfig.provider_params. @@ -104,12 +103,43 @@ def build_config_extra_params(config: ModelConfig) -> dict[str, Any]: return extra_params +def request_timeout_from_extra_params( + extra_params: dict[str, Any] | None, +) -> float | None: + """Return a validated per-request provider timeout from extra params. + + Config-sourced timeouts are already validated and normalized at config + load (`coerce_provider_timeout` in src.config); this guards extra_params + passed programmatically at call time. + """ + if not extra_params or "timeout" not in extra_params: + return None + + try: + return coerce_provider_timeout(extra_params["timeout"]) + except ValueError as exc: + raise ValidationException(str(exc)) from exc + + +def _strip_none_params( + params: dict[str, Any], + keys: tuple[str, ...], +) -> dict[str, Any]: + """Remove specified keys from extra params when their values are None.""" + return {k: v for k, v in params.items() if not (k in keys and v is None)} + + def _normalize_extra_params(extra_params: dict[str, Any]) -> dict[str, Any]: - """Normalize shared extra params before they reach provider backends.""" - timeout = request_timeout_from_extra_params(extra_params) - if timeout is None: - return extra_params - return {**extra_params, "timeout": timeout} + """Normalize and clean shared extra params before they reach backends. + + Centralizes per-key coercion and null-stripping so new keys are added + here rather than spawning one-off normalizers. + """ + result = dict(extra_params) + timeout = request_timeout_from_extra_params(result) + if timeout is not None: + result["timeout"] = timeout + return _strip_none_params(result, ("timeout",)) async def execute_completion(