diff --git a/docs/Tuning-Guide.md b/docs/Tuning-Guide.md index 8977f3f..1b33e0c 100644 --- a/docs/Tuning-Guide.md +++ b/docs/Tuning-Guide.md @@ -132,6 +132,21 @@ a local model can be held to less without affecting the frontier roles. executor. Frontier planning wants the large default. Leave this high, and set a local model's smaller ceiling as its `max_output_tokens` in LiteLLM. +### `llm.request_timeout_s` — default 600 + +How long one model request may take before it is abandoned. It applies to +every request, on both routes. + +- **Too low:** a slow model's long answer is cut off, and the executor + escalates work that was going well. At 120 s, the old fixed value, a dense + 27B model on the RTX 4090 timed out writing one module in one turn. +- **Too high:** a hung connection holds the milestone for that long before + anything notices. +- **Deciding:** this is a safety net, not a budget. `max_output_tokens` and + `max_turns` bound the work; set this above the slowest single answer you + expect, which is the model's output ceiling divided by its generation + speed. + ### `tuning.executor.max_turns` — default 24 How many model calls one milestone may make before the executor stops with diff --git a/nisse-dev.example.yaml b/nisse-dev.example.yaml index 7cf90e2..6395f9a 100644 --- a/nisse-dev.example.yaml +++ b/nisse-dev.example.yaml @@ -18,6 +18,9 @@ llm: # A5: LiteLLM is the only LLM destination # Ceiling on each response. The model's own max_output_tokens in LiteLLM # caps it further, which is where to hold a local model to less. max_output_tokens: 16384 + # How long one request may take before it is abandoned. A safety net for a + # hung connection; slow local models can take minutes over a long answer. + request_timeout_s: 600 roles: # A11: role -> LiteLLM model alias executor: local-fast # the local tier; the benchmark picks the model diff --git a/nisse-dev/src/nisse_dev/agent.py b/nisse-dev/src/nisse_dev/agent.py index 966f56d..2f5355f 100644 --- a/nisse-dev/src/nisse_dev/agent.py +++ b/nisse-dev/src/nisse_dev/agent.py @@ -144,7 +144,10 @@ def make_llm_clients( normalized = budgeted( LiteLLMClient( - config.llm_base_url, config.llm_token, max_tokens=config.max_output_tokens + config.llm_base_url, + config.llm_token, + max_tokens=config.max_output_tokens, + timeout_s=config.request_timeout_s, ) ) # Delegations prefer the vendor-native route; fidelity is cheap @@ -152,7 +155,10 @@ def make_llm_clients( # select_client, not by where the caller happens to sit. passthrough = budgeted( AnthropicPassthroughClient( - config.llm_base_url, config.llm_token, max_tokens=config.max_output_tokens + config.llm_base_url, + config.llm_token, + max_tokens=config.max_output_tokens, + timeout_s=config.request_timeout_s, ) ) return normalized, passthrough diff --git a/nisse-dev/src/nisse_dev/config.py b/nisse-dev/src/nisse_dev/config.py index 80d9071..29f261b 100644 --- a/nisse-dev/src/nisse_dev/config.py +++ b/nisse-dev/src/nisse_dev/config.py @@ -23,7 +23,7 @@ from typing import Any import yaml -from nisse_dev.llm import DEFAULT_MAX_OUTPUT_TOKENS, RoleMap +from nisse_dev.llm import DEFAULT_MAX_OUTPUT_TOKENS, DEFAULT_REQUEST_TIMEOUT_S, RoleMap from nisse_dev.matrix import MatrixConfig DEFAULT_CONFIG_FILE = "nisse-dev.yaml" @@ -48,7 +48,7 @@ REQUIRED_KEYS = ( "gitea.repo", "gitea.operators", ) -OPTIONAL_KEYS = ("llm.max_output_tokens", "gitea.base_branch") +OPTIONAL_KEYS = ("llm.max_output_tokens", "llm.request_timeout_s", "gitea.base_branch") _REFERENCE = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)\}") @@ -221,6 +221,7 @@ class AgentConfig: gitea_operators: frozenset[str] base_branch: str = "main" max_output_tokens: int = DEFAULT_MAX_OUTPUT_TOKENS + request_timeout_s: float = DEFAULT_REQUEST_TIMEOUT_S tuning: Tuning = field(default_factory=Tuning) config_file: Path | None = None @@ -291,6 +292,14 @@ class AgentConfig: raise ValueError except (TypeError, ValueError): errors.append("llm.max_output_tokens: must be a positive integer") + + request_timeout = values.get("llm.request_timeout_s", DEFAULT_REQUEST_TIMEOUT_S) + try: + request_timeout = float(request_timeout) + if request_timeout <= 0: + raise ValueError + except (TypeError, ValueError): + errors.append("llm.request_timeout_s: must be a positive number of seconds") if errors: raise ConfigError("cannot start: " + "; ".join(errors)) @@ -319,6 +328,7 @@ class AgentConfig: gitea_operators=_people(values["gitea.operators"]), base_branch=str(values.get("gitea.base_branch") or "main"), max_output_tokens=max_output, + request_timeout_s=request_timeout, tuning=tuning, config_file=config_file, ) diff --git a/nisse-dev/src/nisse_dev/llm.py b/nisse-dev/src/nisse_dev/llm.py index afbc1f1..84fffb1 100644 --- a/nisse-dev/src/nisse_dev/llm.py +++ b/nisse-dev/src/nisse_dev/llm.py @@ -46,6 +46,12 @@ RATE_LIMIT_RETRY_S = 20.0 # was observed at 4,064 against a 4,096 limit. DEFAULT_MAX_OUTPUT_TOKENS = 16384 +# How long one request may take before it is abandoned (llm.request_timeout_s). +# A safety net for a hung connection, not a budget: a dense local model +# writing a whole module in one turn was measured taking over 120 s, and +# timing it out failed work that was going well. +DEFAULT_REQUEST_TIMEOUT_S = 600.0 + class UpstreamUnavailable(LLMError): """The proxy or provider could not be reached, or refused for reasons @@ -124,7 +130,7 @@ class LiteLLMClient: base_url: str, token: str, *, - timeout_s: float = 120.0, + timeout_s: float = DEFAULT_REQUEST_TIMEOUT_S, transport: httpx.BaseTransport | None = None, sleep: Any = time.sleep, max_tokens: int | None = None, @@ -250,7 +256,7 @@ class AnthropicPassthroughClient: base_url: str, token: str, *, - timeout_s: float = 300.0, + timeout_s: float = DEFAULT_REQUEST_TIMEOUT_S, max_tokens: int | None = None, transport: httpx.BaseTransport | None = None, sleep: Any = time.sleep, diff --git a/nisse-dev/src/nisse_dev/render.py b/nisse-dev/src/nisse_dev/render.py index 63d4b2f..82df7a2 100644 --- a/nisse-dev/src/nisse_dev/render.py +++ b/nisse-dev/src/nisse_dev/render.py @@ -60,7 +60,7 @@ class OpenAIChatRenderer: kept = effective_events(events) answered = {e.event.call_id for e in kept if isinstance(e.event, ToolResult)} for stored in kept: - messages.append(self._message_for(stored.event)) + _append_merged(messages, self._message_for(stored.event)) # Providers reject a tool call with no result following it. One # can legitimately be missing: a crash between the call and its # result (A3), or a turn-ending gate tool. Render a placeholder so @@ -128,3 +128,28 @@ class OpenAIChatRenderer: } case _: raise ValueError(f"unrenderable event: {event!r}") + + +def _append_merged(messages: list[dict[str, Any]], message: dict[str, Any]) -> None: + """Append, folding into the previous message when both are the same user + or assistant role. + + One model turn is often several events (some text, then one or more tool + calls), and the OpenAI format carries a turn as one assistant message + with a tool_calls list. Strict chat templates enforce that: Devstral's + (Mistral) rejected a turn rendered as a text message then a tool-call + message with a 500, failing runs whenever the model spoke before acting.""" + previous = messages[-1] if messages else None + role = message["role"] + if ( + previous is None + or previous["role"] != role + or role not in ("user", "assistant") + ): + messages.append(message) + return + texts = [t for t in (previous.get("content"), message.get("content")) if t] + previous["content"] = "\n\n".join(texts) if texts else None + calls = previous.get("tool_calls", []) + message.get("tool_calls", []) + if calls: + previous["tool_calls"] = calls diff --git a/nisse-dev/tests/test_config.py b/nisse-dev/tests/test_config.py index 264ce63..a294696 100644 --- a/nisse-dev/tests/test_config.py +++ b/nisse-dev/tests/test_config.py @@ -5,6 +5,7 @@ from typing import Any import pytest from nisse_dev.config import AgentConfig, ConfigError +from nisse_dev.llm import DEFAULT_REQUEST_TIMEOUT_S # A complete config as the YAML file would give it, with secrets by reference. COMPLETE: dict[str, Any] = { @@ -122,6 +123,25 @@ def test_max_output_tokens_is_configurable_and_checked() -> None: build(llm__max_output_tokens=0) +def test_request_timeout_is_configurable_and_checked() -> None: + assert build().request_timeout_s == DEFAULT_REQUEST_TIMEOUT_S + assert build(llm__request_timeout_s=900).request_timeout_s == 900.0 + for bad in (0, -5, "soon"): + with pytest.raises(ConfigError, match="llm.request_timeout_s"): + build(llm__request_timeout_s=bad) + + +def test_the_request_timeout_reaches_both_clients() -> None: + from nisse_dev.agent import make_llm_clients + from nisse_dev.catalog import ModelCatalog + + normalized, passthrough = make_llm_clients( + build(llm__request_timeout_s=42), ModelCatalog({}) + ) + for client in (normalized, passthrough): + assert client._inner._http.timeout.read == 42.0 + + def test_redacted_summary_carries_no_secrets() -> None: summary = build().redacted() flat = " ".join(summary.values()) diff --git a/nisse-dev/tests/test_render.py b/nisse-dev/tests/test_render.py index 39b40a1..dc71534 100644 --- a/nisse-dev/tests/test_render.py +++ b/nisse-dev/tests/test_render.py @@ -1,4 +1,5 @@ import json +from itertools import pairwise from nisse_dev.canonical import ( AssistantText, @@ -68,7 +69,8 @@ def test_compaction_replaces_its_range_only() -> None: assert [e.id for e in remaining] == [3, 4] body = OpenAIChatRenderer().render(events, tools=None, model="q") contents = [m["content"] for m in body["messages"]] - assert contents == ["kept", "[summary of earlier conversation]\nthe old stuff"] + # Both render as user messages, so they share one: roles must alternate. + assert contents == ["kept\n\n[summary of earlier conversation]\nthe old stuff"] def test_tool_call_without_a_result_gets_a_placeholder() -> None: @@ -103,3 +105,47 @@ def test_gate_event_renders_as_context() -> None: body = OpenAIChatRenderer().render(events, tools=None, model="q") assert body["messages"][0]["role"] == "user" assert "arch" in body["messages"][0]["content"] + + +def _roles(body: dict) -> list[str]: + return [m["role"] for m in body["messages"]] + + +def test_one_turn_is_one_assistant_message() -> None: + # Text and two parallel tool calls from one model turn: the OpenAI format + # carries them as a single assistant message with a tool_calls list. + events = stored( + [ + UserMessage(text="go", source="m:1"), + AssistantText(text="Looking around first.", model="q"), + ToolCall(call_id="a", tool="list_dir", arguments={"path": "."}), + ToolCall(call_id="b", tool="read_file", arguments={"path": "README.md"}), + ToolResult(call_id="a", result="README.md", success=True), + ToolResult(call_id="b", result="hello", success=True), + ] + ) + body = OpenAIChatRenderer().render(events, tools=None, model="q") + assert _roles(body) == ["user", "assistant", "tool", "tool"] + turn = body["messages"][1] + assert turn["content"] == "Looking around first." + assert [c["id"] for c in turn["tool_calls"]] == ["a", "b"] + + +def test_roles_never_repeat_for_strict_templates() -> None: + """Mistral's chat template raises on two user or two assistant messages + in a row; every mix of events must render without one.""" + events = stored( + [ + UserMessage(text="one", source="m:1"), + UserMessage(text="two", source="m:2"), + AssistantText(text="thinking", model="q"), + ToolCall(call_id="a", tool="list_dir", arguments={}), + ToolResult(call_id="a", result="x", success=True), + AssistantText(text="more", model="q"), + AssistantText(text="and more", model="q"), + ToolCall(call_id="b", tool="finish", arguments={}), + ] + ) + roles = _roles(OpenAIChatRenderer().render(events, tools=None, model="q")) + for first, second in pairwise(roles): + assert not (first == second and first in ("user", "assistant")), roles