diff --git a/backend/app/services/chat_model.py b/backend/app/services/chat_model.py index a0fc8e7..feb882c 100644 --- a/backend/app/services/chat_model.py +++ b/backend/app/services/chat_model.py @@ -33,7 +33,7 @@ from app.config.config_models import RAGConfig from app.config.llm_settings import GUARDRAIL_ROLE, LLMSettings -from app.services.llm_providers import PROVIDERS +from app.services.llm_providers import PROVIDERS, apply_timeout logger = logging.getLogger(__name__) @@ -43,11 +43,6 @@ # Cap on prior turns included as context, to avoid overflowing the model's context window. MAX_HISTORY_MESSAGES = 10 -# ChatOllama has no top-level `timeout` field — a bare `timeout=` kwarg is silently -# dropped. Its request timeout instead goes through `client_kwargs`, forwarded verbatim -# to the underlying httpx client. Every other provider accepts `timeout` directly. -_OLLAMA_PROVIDER = "ollama" - # Answer generation wants some variety in wording; safety classification wants the same # message to get the same verdict every time (#207), so the guardrail stage samples greedily. GENERATION_TEMPERATURE: Final = 0.7 @@ -104,10 +99,7 @@ def build_chat_model( max_tokens_kwarg: config.max_tokens, } if timeout is not None: - if provider_name == _OLLAMA_PROVIDER: - kwargs["client_kwargs"] = {"timeout": timeout} - else: - kwargs["timeout"] = timeout + apply_timeout(provider, kwargs, timeout) return init_chat_model(model_name, **kwargs) diff --git a/backend/app/services/llm_providers.py b/backend/app/services/llm_providers.py index ccf6723..17c721c 100644 --- a/backend/app/services/llm_providers.py +++ b/backend/app/services/llm_providers.py @@ -4,11 +4,12 @@ files: the max-tokens kwarg, base-URL fallback env vars, and credential env var in ``app.services.chat_model``, plus the infra-error exception types in ``app.guardrails.llm_classifier``. This module is the single per-provider table both -read from instead. +read from instead — including the ways a provider's constructor kwargs differ from +the common case, such as how the request timeout is wired (#164). """ from dataclasses import dataclass -from typing import Final +from typing import Any, Final import anthropic import ollama @@ -18,6 +19,10 @@ from langchain_ollama import ChatOllama from langchain_openai import ChatOpenAI +# The constructor kwarg carrying the request timeout for a provider that takes it +# top-level, which is what LangChain's own provider integrations expect. +DEFAULT_TIMEOUT_KWARG: Final = "timeout" + @dataclass(frozen=True, slots=True) class ProviderConfig: @@ -26,6 +31,9 @@ class ProviderConfig: Args: model_type: The LangChain ``BaseChatModel`` subclass this provider constructs. max_tokens_kwarg: The constructor kwarg name for the output-length cap. + timeout_kwarg: The constructor kwarg name carrying the request timeout, or + ``None`` for a provider whose chat model has no top-level timeout field + and takes the timeout nested under ``client_kwargs`` instead. base_url_env_vars: Env vars this provider's own SDK falls back to for its API base URL, in fallback order. credential_env_var: The env var this provider's own SDK reads its credential @@ -40,6 +48,12 @@ class ProviderConfig: # field isn't named max_tokens (e.g. ChatOllama's num_predict) has to be mapped. max_tokens_kwarg: str = "max_tokens" + # The constructor kwarg carrying the request timeout, or None for a provider whose + # chat model has no top-level timeout field: ChatOllama's request timeout belongs + # to its underlying httpx client, so it goes through `client_kwargs`, forwarded + # verbatim — a bare `timeout=` kwarg is silently dropped. + timeout_kwarg: str | None = DEFAULT_TIMEOUT_KWARG + # Each provider's own SDK falls back to these env vars for its API base URL when # neither an explicit kwarg nor TAPIO_LLM_API_BASE is set. The cleartext-transport # guard in chat_model.py has to check the URL that will actually be used, not just @@ -64,6 +78,9 @@ class ProviderConfig: "ollama": ProviderConfig( model_type=ChatOllama, max_tokens_kwarg="num_predict", + # ChatOllama has no top-level timeout field — its request timeout goes to the + # underlying httpx client through client_kwargs. + timeout_kwarg=None, # ollama.Client reads OLLAMA_HOST as its API base fallback. base_url_env_vars=("OLLAMA_HOST",), infra_error_types=(ollama.ResponseError,), @@ -88,3 +105,25 @@ class ProviderConfig: infra_error_types=(anthropic.APIConnectionError, anthropic.APIStatusError), ), } + + +def apply_timeout(provider: ProviderConfig | None, kwargs: dict[str, Any], timeout: float) -> None: + """Apply a request timeout to constructor kwargs the way the provider takes it. + + A provider whose chat model has no top-level timeout field has its timeout nested + under ``client_kwargs`` instead, which LangChain forwards verbatim to the + provider's underlying HTTP client. Which of the two applies is a per-provider + table entry, so no caller needs a provider-specific branch (#164). + + Args: + provider: The provider's table entry, or ``None`` for a provider LangChain + supports but Tapio has no entry for — those take the plain top-level + ``timeout`` kwarg. + kwargs: The constructor kwargs to add the timeout to, in place. + timeout: The request timeout in seconds. + """ + timeout_kwarg = provider.timeout_kwarg if provider else DEFAULT_TIMEOUT_KWARG + if timeout_kwarg is None: + kwargs["client_kwargs"] = {"timeout": timeout} + else: + kwargs[timeout_kwarg] = timeout diff --git a/backend/tests/services/test_llm_providers.py b/backend/tests/services/test_llm_providers.py new file mode 100644 index 0000000..b22207f --- /dev/null +++ b/backend/tests/services/test_llm_providers.py @@ -0,0 +1,51 @@ +"""Tests for the per-provider configuration table (#146, #164).""" + +from typing import Any + +import pytest + +from app.services.llm_providers import PROVIDERS, ProviderConfig, apply_timeout + + +class TestApplyTimeout: + """Tests for wiring a request timeout into a provider's constructor kwargs.""" + + def test_a_provider_with_a_top_level_timeout_kwarg_takes_it_directly(self) -> None: + kwargs: dict[str, Any] = {} + + apply_timeout(PROVIDERS["openai"], kwargs, 30.0) + + assert kwargs == {"timeout": 30.0} + + def test_a_provider_without_one_nests_it_under_client_kwargs(self) -> None: + """ChatOllama has no top-level timeout field — its httpx client takes one.""" + kwargs: dict[str, Any] = {} + + apply_timeout(PROVIDERS["ollama"], kwargs, 30.0) + + assert kwargs == {"client_kwargs": {"timeout": 30.0}} + + def test_a_provider_without_a_table_entry_takes_a_top_level_timeout(self) -> None: + """A provider LangChain supports but Tapio doesn't configure gets the common case.""" + kwargs: dict[str, Any] = {} + + apply_timeout(None, kwargs, 30.0) + + assert kwargs == {"timeout": 30.0} + + def test_the_wiring_comes_from_the_table_entry(self) -> None: + """The nesting is the provider's own quirk, not a hardcoded provider name.""" + provider = ProviderConfig(model_type=PROVIDERS["ollama"].model_type, timeout_kwarg=None) + kwargs: dict[str, Any] = {} + + apply_timeout(provider, kwargs, 30.0) + + assert kwargs == {"client_kwargs": {"timeout": 30.0}} + + @pytest.mark.parametrize("provider_name", sorted(PROVIDERS)) + def test_every_configured_provider_wires_the_timeout_one_way(self, provider_name: str) -> None: + kwargs: dict[str, Any] = {} + + apply_timeout(PROVIDERS[provider_name], kwargs, 30.0) + + assert kwargs in ({"timeout": 30.0}, {"client_kwargs": {"timeout": 30.0}})