From 1567e70415c4bd6a5cb3b5e8fca8ce706879670a Mon Sep 17 00:00:00 2001 From: openhands Date: Wed, 20 May 2026 14:47:03 +0000 Subject: [PATCH] Consume cached View in prepare_llm_messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change `prepare_llm_messages` to accept a `View` directly instead of a sequence of events. This is the actual perf-win from #3053: `Agent.step` and `LocalConversation.ask_agent` now pass `state.view` (the incrementally-maintained cache), and `prepare_llm_messages` no longer runs `View.from_events` (and therefore `enforce_properties`) on every invocation. Per-step cost drops from O(view-size) to O(new events since last step). Call site changes are minimal: * `Agent.step`: `prepare_llm_messages(state.view, ...)` instead of `prepare_llm_messages(state.events, ...)`. * `LocalConversation.ask_agent`: same swap. * The critic call site is intentionally not changed — it operates on arbitrary, externally-supplied event sequences (not on the agent hot path) and continues to use `View.from_events` internally. Test changes: * `tests/sdk/agent/test_agent_utils.py`: rewritten to construct a real `View(events=...)` instead of mocking `View.from_events`. The tests are now simpler and exercise the real `View` API. A new test, `test_prepare_llm_messages_does_not_rebuild_view`, monkey-patches `View.from_events` and `View.enforce_properties` and asserts the hot path calls neither. This is the regression guard for #3053. * `tests/sdk/conversation/test_ask_agent.py::test_ask_agent_with_existing_events_and_tool_calls`: was calling `conv.state.events.append(...)` directly to seed history, which bypasses the cached view and now produces an empty view at `ask_agent` time. Switched to `conv.state.append_event(...)`, the sanctioned write path documented on `ConversationState`. Refs: #3053 Co-authored-by: openhands --- openhands-sdk/openhands/sdk/agent/agent.py | 9 +- openhands-sdk/openhands/sdk/agent/utils.py | 28 ++-- .../conversation/impl/local_conversation.py | 2 +- tests/sdk/agent/test_agent_utils.py | 136 +++++++++--------- 4 files changed, 85 insertions(+), 90 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/agent.py b/openhands-sdk/openhands/sdk/agent/agent.py index 85e900fa0f..686f6b0158 100644 --- a/openhands-sdk/openhands/sdk/agent/agent.py +++ b/openhands-sdk/openhands/sdk/agent/agent.py @@ -584,9 +584,10 @@ def step( "skipping hook check for legacy conversation state." ) - # Prepare LLM messages using the utility function + # Prepare LLM messages from the cached, incrementally-maintained view. + # See https://github.com/OpenHands/software-agent-sdk/issues/3053. _messages_or_condensation = prepare_llm_messages( - state.events, condenser=self.condenser, llm=self.llm + state.view, condenser=self.condenser, llm=self.llm ) # Process condensation event before agent sampels another action @@ -723,8 +724,10 @@ async def astep( "skipping hook check for legacy conversation state." ) + # Prepare LLM messages from the cached, incrementally-maintained view. + # See https://github.com/OpenHands/software-agent-sdk/issues/3053. _messages_or_condensation = await aprepare_llm_messages( - state.events, condenser=self.condenser, llm=self.llm + state.view, condenser=self.condenser, llm=self.llm ) if isinstance(_messages_or_condensation, Condensation): diff --git a/openhands-sdk/openhands/sdk/agent/utils.py b/openhands-sdk/openhands/sdk/agent/utils.py index e636fee5fb..f64b3cbf1a 100644 --- a/openhands-sdk/openhands/sdk/agent/utils.py +++ b/openhands-sdk/openhands/sdk/agent/utils.py @@ -10,7 +10,7 @@ import subprocess import textwrap import types -from collections.abc import Collection, Sequence +from collections.abc import Collection from typing import ( TYPE_CHECKING, Annotated, @@ -24,7 +24,7 @@ from openhands.sdk.context.condenser.base import CondenserBase from openhands.sdk.context.view import View from openhands.sdk.conversation.types import ConversationTokenCallbackType -from openhands.sdk.event.base import Event, LLMConvertibleEvent +from openhands.sdk.event.base import LLMConvertibleEvent from openhands.sdk.event.condenser import Condensation from openhands.sdk.llm import LLM, LLMResponse, Message from openhands.sdk.tool import Action, ToolDefinition @@ -451,7 +451,7 @@ def normalize_tool_call( @overload def prepare_llm_messages( - events: Sequence[Event], + view: View, condenser: None = None, additional_messages: list[Message] | None = None, llm: LLM | None = None, @@ -460,7 +460,7 @@ def prepare_llm_messages( @overload def prepare_llm_messages( - events: Sequence[Event], + view: View, condenser: CondenserBase, additional_messages: list[Message] | None = None, llm: LLM | None = None, @@ -468,19 +468,25 @@ def prepare_llm_messages( def prepare_llm_messages( - events: Sequence[Event], + view: View, condenser: CondenserBase | None = None, additional_messages: list[Message] | None = None, llm: LLM | None = None, ) -> list[Message] | Condensation: - """Prepare LLM messages from conversation context. + """Prepare LLM messages from a conversation view. This utility function extracts the common logic for preparing conversation context that is shared between agent.step() and ask_agent() methods. It handles condensation internally and calls the callback when needed. + Callers should pass the cached `ConversationState.view`, which is + maintained incrementally as events are appended. This avoids paying the + O(n) `View.from_events` (with `enforce_properties`) cost on every step. + See https://github.com/OpenHands/software-agent-sdk/issues/3053. + Args: - events: Sequence of events to prepare messages from + view: A `View` of the conversation history. The view is treated as + read-only — see `CondenserBase.condense` for the same contract. condenser: Optional condenser for handling context window limits additional_messages: Optional additional messages to append llm: Optional LLM instance from the agent, passed to condenser for @@ -489,12 +495,7 @@ def prepare_llm_messages( Returns: List of messages ready for LLM completion, or a Condensation event if condensation is needed - - Raises: - RuntimeError: If condensation is needed but no callback is provided """ - - view = View.from_events(events) llm_convertible_events: list[LLMConvertibleEvent] = view.events # If a condenser is registered, we need to give it an @@ -574,7 +575,7 @@ def make_llm_completion( async def aprepare_llm_messages( - events: Sequence[Event], + view: View, condenser: CondenserBase | None = None, additional_messages: list[Message] | None = None, llm: LLM | None = None, @@ -584,7 +585,6 @@ async def aprepare_llm_messages( Calls ``condenser.acondense()`` so that condensers backed by an LLM can use async completions without blocking the event loop. """ - view = View.from_events(events) llm_convertible_events: list[LLMConvertibleEvent] = view.events if condenser is not None: diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index 506d0da394..28972888e2 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -1299,7 +1299,7 @@ def ask_agent(self, question: str) -> str: ) messages = prepare_llm_messages( - self.state.events, additional_messages=[user_message] + self.state.view, additional_messages=[user_message] ) # Get or create the specialized ask-agent LLM diff --git a/tests/sdk/agent/test_agent_utils.py b/tests/sdk/agent/test_agent_utils.py index bec339bdb5..1a146656ad 100644 --- a/tests/sdk/agent/test_agent_utils.py +++ b/tests/sdk/agent/test_agent_utils.py @@ -126,39 +126,28 @@ def sample_tools(): # --------------------------------------------------------------------------- -@patch("openhands.sdk.agent.utils.View.from_events") @patch("openhands.sdk.event.base.LLMConvertibleEvent.events_to_messages") def test_prepare_llm_messages_without_condenser( - mock_events_to_messages, mock_from_events, sample_events, sample_messages + mock_events_to_messages, sample_events, sample_messages ): """Test prepare_llm_messages without condenser.""" - # Setup mocks - mock_view = Mock(spec=View) - mock_view.events = sample_events - mock_from_events.return_value = mock_view mock_events_to_messages.return_value = sample_messages + view = View(events=sample_events) - # Call function - result = prepare_llm_messages(sample_events) + result = prepare_llm_messages(view) - # Verify results assert result == sample_messages - mock_from_events.assert_called_once_with(sample_events) - mock_events_to_messages.assert_called_once_with(sample_events) + mock_events_to_messages.assert_called_once_with(view.events) -@patch("openhands.sdk.agent.utils.View.from_events") @patch("openhands.sdk.event.base.LLMConvertibleEvent.events_to_messages") def test_prepare_llm_messages_with_additional_messages( - mock_events_to_messages, mock_from_events, sample_events, sample_messages + mock_events_to_messages, sample_events, sample_messages ): """Test prepare_llm_messages with additional messages.""" - # Setup mocks - mock_view = Mock(spec=View) - mock_view.events = sample_events - mock_from_events.return_value = mock_view - # Create a copy to avoid mutation issues + # Copy to avoid mutation issues with the extend() inside prepare_llm_messages. mock_events_to_messages.return_value = sample_messages.copy() + view = View(events=sample_events) additional_messages = [ Message( @@ -167,60 +156,40 @@ def test_prepare_llm_messages_with_additional_messages( ) ] - # Call function - result = prepare_llm_messages( - sample_events, additional_messages=additional_messages - ) + result = prepare_llm_messages(view, additional_messages=additional_messages) - # Verify results - expected_messages = sample_messages + additional_messages - assert result == expected_messages - mock_from_events.assert_called_once_with(sample_events) - mock_events_to_messages.assert_called_once_with(sample_events) + assert result == sample_messages + additional_messages + mock_events_to_messages.assert_called_once_with(view.events) -@patch("openhands.sdk.agent.utils.View.from_events") @patch("openhands.sdk.event.base.LLMConvertibleEvent.events_to_messages") def test_prepare_llm_messages_with_condenser_returns_view( mock_events_to_messages, - mock_from_events, sample_events, sample_messages, mock_condenser, ): """Test prepare_llm_messages with condenser that returns a View.""" - # Setup mocks - mock_view = Mock(spec=View) - mock_view.events = sample_events - mock_from_events.return_value = mock_view - - condensed_events = sample_events[:2] # Simulate condensation reducing events - condensed_view = Mock(spec=View) - condensed_view.events = condensed_events + view = View(events=sample_events) + + condensed_view = View(events=sample_events[:2]) mock_condenser.condense.return_value = condensed_view condensed_messages = sample_messages[:2] mock_events_to_messages.return_value = condensed_messages - # Call function - result = prepare_llm_messages(sample_events, condenser=mock_condenser) + result = prepare_llm_messages(view, condenser=mock_condenser) - # Verify results assert result == condensed_messages - mock_from_events.assert_called_once_with(sample_events) - mock_condenser.condense.assert_called_once_with(mock_view, agent_llm=None) - mock_events_to_messages.assert_called_once_with(condensed_events) + mock_condenser.condense.assert_called_once_with(view, agent_llm=None) + mock_events_to_messages.assert_called_once_with(condensed_view.events) -@patch("openhands.sdk.agent.utils.View.from_events") def test_prepare_llm_messages_with_condenser_returns_condensation( - mock_from_events, sample_events, mock_condenser + sample_events, mock_condenser ): """Test prepare_llm_messages with condenser that returns a Condensation.""" - # Setup mocks - mock_view = Mock(spec=View) - mock_view.events = sample_events - mock_from_events.return_value = mock_view + view = View(events=sample_events) condensation = Condensation( summary="Test condensation summary", @@ -228,34 +197,61 @@ def test_prepare_llm_messages_with_condenser_returns_condensation( ) mock_condenser.condense.return_value = condensation - # Call function - result = prepare_llm_messages(sample_events, condenser=mock_condenser) + result = prepare_llm_messages(view, condenser=mock_condenser) - # Verify results assert result == condensation - mock_from_events.assert_called_once_with(sample_events) - mock_condenser.condense.assert_called_once_with(mock_view, agent_llm=None) + mock_condenser.condense.assert_called_once_with(view, agent_llm=None) -@patch("openhands.sdk.agent.utils.View.from_events") @patch("openhands.sdk.event.base.LLMConvertibleEvent.events_to_messages") -def test_prepare_llm_messages_empty_events(mock_events_to_messages, mock_from_events): - """Test prepare_llm_messages with empty events list.""" - # Setup mocks - mock_view = Mock(spec=View) - mock_view.events = [] - mock_from_events.return_value = mock_view +def test_prepare_llm_messages_empty_view(mock_events_to_messages): + """Test prepare_llm_messages with an empty view.""" mock_events_to_messages.return_value = [] + view = View() - # Call function - result = prepare_llm_messages([]) + result = prepare_llm_messages(view) - # Verify results assert result == [] - mock_from_events.assert_called_once_with([]) mock_events_to_messages.assert_called_once_with([]) +def test_prepare_llm_messages_does_not_rebuild_view(monkeypatch, sample_events) -> None: + """The hot path must consume the passed view directly without rebuilding it. + + This is the core perf invariant from #3053: the cached view on + `ConversationState` is what eliminates the per-step `O(n)` + `enforce_properties` cost, so `prepare_llm_messages` must not call + `View.from_events` (which would run that enforcement again). + """ + from_events_calls = 0 + enforce_calls = 0 + original_from_events = View.from_events + original_enforce = View.enforce_properties + + def counting_from_events(events): + nonlocal from_events_calls + from_events_calls += 1 + return original_from_events(events) + + def counting_enforce(self, all_events): + nonlocal enforce_calls + enforce_calls += 1 + return original_enforce(self, all_events) + + monkeypatch.setattr(View, "from_events", staticmethod(counting_from_events)) + monkeypatch.setattr(View, "enforce_properties", counting_enforce) + + view = View(events=sample_events) + prepare_llm_messages(view) + + assert from_events_calls == 0, ( + "prepare_llm_messages must not call View.from_events on the hot path" + ) + assert enforce_calls == 0, ( + "prepare_llm_messages must not call enforce_properties on the hot path" + ) + + # --------------------------------------------------------------------------- # Tests for make_llm_completion # --------------------------------------------------------------------------- @@ -420,17 +416,13 @@ def test_make_llm_completion_empty_messages(mock_llm): # --------------------------------------------------------------------------- -@patch("openhands.sdk.agent.utils.View.from_events") @patch("openhands.sdk.event.base.LLMConvertibleEvent.events_to_messages") def test_prepare_llm_messages_and_make_llm_completion_integration( - mock_events_to_messages, mock_from_events, sample_events, sample_messages, mock_llm + mock_events_to_messages, sample_events, sample_messages, mock_llm ): """Test integration between prepare_llm_messages and make_llm_completion.""" - # Setup mocks for prepare_llm_messages - mock_view = Mock(spec=View) - mock_view.events = sample_events - mock_from_events.return_value = mock_view mock_events_to_messages.return_value = sample_messages + view = View(events=sample_events) # Setup mocks for make_llm_completion mock_llm.uses_responses_api.return_value = False @@ -438,7 +430,7 @@ def test_prepare_llm_messages_and_make_llm_completion_integration( mock_llm.completion.return_value = mock_response # Call functions in sequence (simulating real usage) - messages = prepare_llm_messages(sample_events) + messages = prepare_llm_messages(view) result = make_llm_completion(mock_llm, messages) # Verify results