From 2515c2ea7c337c8d7cdb1af8576f89703b50bf8d Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Wed, 19 Feb 2025 13:46:00 +0000 Subject: [PATCH 01/15] Disconnected runtime now stops the agent loop --- openhands/runtime/impl/docker/docker_runtime.py | 2 +- .../standalone_conversation_manager.py | 6 ++++++ openhands/server/session/agent_session.py | 4 ---- openhands/server/session/session.py | 9 ++++++++- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/openhands/runtime/impl/docker/docker_runtime.py b/openhands/runtime/impl/docker/docker_runtime.py index b1a9caa8cf7f..ecb1386a6843 100644 --- a/openhands/runtime/impl/docker/docker_runtime.py +++ b/openhands/runtime/impl/docker/docker_runtime.py @@ -121,7 +121,7 @@ async def connect(self): 'error', f'Container {self.container_name} not found.', ) - raise e + raise AgentRuntimeDisconnectedError from e if self.runtime_container_image is None: if self.base_container_image is None: raise ValueError( diff --git a/openhands/server/conversation_manager/standalone_conversation_manager.py b/openhands/server/conversation_manager/standalone_conversation_manager.py index 2f49de63dc9e..6fb420ad2013 100644 --- a/openhands/server/conversation_manager/standalone_conversation_manager.py +++ b/openhands/server/conversation_manager/standalone_conversation_manager.py @@ -208,6 +208,7 @@ async def maybe_start_agent_loop( config=self.config, sio=self.sio, user_id=user_id, + status_message_callback=self._status_message_callback, ) self._local_agent_loops_by_sid[sid] = session asyncio.create_task(session.initialize_agent(settings, initial_user_msg)) @@ -274,6 +275,11 @@ async def _close_session(self, sid: str): await session.close() logger.info(f'closed_session:{session.sid}') + async def _status_message_callback(self, sid: str, msg_type: str, id: str): + if msg_type == 'error' and id == 'STATUS$ERROR_RUNTIME_DISCONNECTED': + # If there is no runtime, stop the agent loop. We may need to delete the runtime in question... + asyncio.create_task(self._close_session(sid)) + @classmethod def get_instance( cls, diff --git a/openhands/server/session/agent_session.py b/openhands/server/session/agent_session.py index 79b98733850a..cc8453302e9b 100644 --- a/openhands/server/session/agent_session.py +++ b/openhands/server/session/agent_session.py @@ -163,10 +163,6 @@ async def close(self): if self.security_analyzer is not None: await self.security_analyzer.close() - async def stop_agent_loop_for_error(self): - if self.controller is not None: - await self.controller.set_agent_state_to(AgentState.ERROR) - def _create_security_analyzer(self, security_analyzer: str | None): """Creates a SecurityAnalyzer instance that will be used to analyze the agent actions diff --git a/openhands/server/session/session.py b/openhands/server/session/session.py index d7807fc94740..6a3144ba3fd3 100644 --- a/openhands/server/session/session.py +++ b/openhands/server/session/session.py @@ -1,6 +1,7 @@ import asyncio import time from copy import deepcopy +from typing import Callable import socketio @@ -49,6 +50,7 @@ def __init__( file_store: FileStore, sio: socketio.AsyncServer | None, user_id: str | None = None, + status_message_callback: Callable | None = None, ): self.sid = sid self.sio = sio @@ -67,6 +69,7 @@ def __init__( self.config = deepcopy(config) self.loop = asyncio.get_event_loop() self.user_id = user_id + self._status_message_callback = status_message_callback async def close(self): if self.sio: @@ -242,7 +245,11 @@ async def send_error(self, message: str): async def _send_status_message(self, msg_type: str, id: str, message: str): """Sends a status message to the client.""" if msg_type == 'error': - await self.agent_session.stop_agent_loop_for_error() + controller = self.agent_session.controller + if controller is not None: + await controller.set_agent_state_to(AgentState.ERROR) + if self._status_message_callback: + await self._status_message_callback(self.sid, msg_type, id) await self.send( {'status_update': True, 'type': msg_type, 'id': id, 'message': message} ) From 2a31e4ded04d7668bf72f40ea8df8b7a8cee0a5a Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Wed, 19 Feb 2025 13:47:12 +0000 Subject: [PATCH 02/15] Updated comment --- .../conversation_manager/standalone_conversation_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openhands/server/conversation_manager/standalone_conversation_manager.py b/openhands/server/conversation_manager/standalone_conversation_manager.py index 6fb420ad2013..d678ef8c5a4c 100644 --- a/openhands/server/conversation_manager/standalone_conversation_manager.py +++ b/openhands/server/conversation_manager/standalone_conversation_manager.py @@ -277,7 +277,7 @@ async def _close_session(self, sid: str): async def _status_message_callback(self, sid: str, msg_type: str, id: str): if msg_type == 'error' and id == 'STATUS$ERROR_RUNTIME_DISCONNECTED': - # If there is no runtime, stop the agent loop. We may need to delete the runtime in question... + # If there is no runtime, stop the agent loop. asyncio.create_task(self._close_session(sid)) @classmethod From 288f6257497dbbb22537efb6cc0348be6f025bb6 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Thu, 20 Feb 2025 09:06:32 +0000 Subject: [PATCH 03/15] More logging to catch errors --- openhands/agenthub/codeact_agent/codeact_agent.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/openhands/agenthub/codeact_agent/codeact_agent.py b/openhands/agenthub/codeact_agent/codeact_agent.py index b636e40cb9f6..e02b1e6b5adc 100644 --- a/openhands/agenthub/codeact_agent/codeact_agent.py +++ b/openhands/agenthub/codeact_agent/codeact_agent.py @@ -127,7 +127,13 @@ def step(self, state: State) -> Action: 'messages': self.llm.format_messages_for_llm(messages), } params['tools'] = self.tools - response = self.llm.completion(**params) + try: + response = self.llm.completion(**params) + except Exception: + logger.info('==== START TRACE BROKEN LLM PARAMS ====') + logger.info(str(params)) + logger.info('==== END TRACE BROKEN LLM PARAMS ====') + raise actions = codeact_function_calling.response_to_actions(response) for action in actions: self.pending_actions.append(action) From 35e94dcfcf625af8ccb490bb0bfbdf777f1ecf52 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Thu, 20 Feb 2025 09:53:22 +0000 Subject: [PATCH 04/15] More debugging --- .../conversation_manager/standalone_conversation_manager.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openhands/server/conversation_manager/standalone_conversation_manager.py b/openhands/server/conversation_manager/standalone_conversation_manager.py index d678ef8c5a4c..2763d387b2c6 100644 --- a/openhands/server/conversation_manager/standalone_conversation_manager.py +++ b/openhands/server/conversation_manager/standalone_conversation_manager.py @@ -277,8 +277,9 @@ async def _close_session(self, sid: str): async def _status_message_callback(self, sid: str, msg_type: str, id: str): if msg_type == 'error' and id == 'STATUS$ERROR_RUNTIME_DISCONNECTED': + logger.info('TRACE:I_WOULD_HAVE_STOPPED_THE_AGENT_LOOP_HERE') # If there is no runtime, stop the agent loop. - asyncio.create_task(self._close_session(sid)) + # asyncio.create_task(self._close_session(sid)) @classmethod def get_instance( From 954ccb14239728ce35a3d850e5c43653507b0443 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Thu, 20 Feb 2025 14:46:13 +0000 Subject: [PATCH 05/15] Restart on refresh if event stream in error or stopped state --- .../standalone_conversation_manager.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/openhands/server/conversation_manager/standalone_conversation_manager.py b/openhands/server/conversation_manager/standalone_conversation_manager.py index 2763d387b2c6..5ecbd1b524d5 100644 --- a/openhands/server/conversation_manager/standalone_conversation_manager.py +++ b/openhands/server/conversation_manager/standalone_conversation_manager.py @@ -10,6 +10,7 @@ from openhands.core.logger import openhands_logger as logger from openhands.core.schema.agent import AgentState from openhands.events.action import MessageAction +from openhands.events.observation.agent import AgentStateChangedObservation from openhands.events.stream import EventStream, session_exists from openhands.server.session.conversation import Conversation from openhands.server.session.session import ROOM_KEY, Session @@ -95,6 +96,15 @@ async def join_conversation( event_stream = await self._get_event_stream(sid) if not event_stream: return await self.maybe_start_agent_loop(sid, settings, user_id) + for event in event_stream.get_events(reverse=True): + if isinstance(event, AgentStateChangedObservation): + if event.agent_state in ( + AgentState.STOPPED.value, + AgentState.ERROR.value, + ): + await self.close_session(sid) + return await self.maybe_start_agent_loop(sid, settings, user_id) + break return event_stream async def detach_from_conversation(self, conversation: Conversation): From 9e17c76a01abfa1feab946804fa319507ee5a146 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Thu, 20 Feb 2025 15:30:46 +0000 Subject: [PATCH 06/15] Add prompt caching --- openhands/core/message_utils.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/openhands/core/message_utils.py b/openhands/core/message_utils.py index 25be12873177..acafac37240b 100644 --- a/openhands/core/message_utils.py +++ b/openhands/core/message_utils.py @@ -358,10 +358,11 @@ def apply_prompt_caching(messages: list[Message]) -> None: breakpoints_remaining = 3 # remaining 1 for system/tool for message in reversed(messages): if message.role in ('user', 'tool'): - if breakpoints_remaining > 0: - message.content[ - -1 - ].cache_prompt = True # Last item inside the message content - breakpoints_remaining -= 1 - else: - break + if message.tool_call_id: + continue + message.content[ + -1 + ].cache_prompt = True # Last item inside the message content + breakpoints_remaining -= 1 + if not breakpoints_remaining: + return From 91fe24863649eeb6166c3bca54d11e98aa0e6dff Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Thu, 20 Feb 2025 15:53:23 +0000 Subject: [PATCH 07/15] Trace LLM Params --- openhands/agenthub/codeact_agent/codeact_agent.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/openhands/agenthub/codeact_agent/codeact_agent.py b/openhands/agenthub/codeact_agent/codeact_agent.py index e02b1e6b5adc..ff83d1cb300a 100644 --- a/openhands/agenthub/codeact_agent/codeact_agent.py +++ b/openhands/agenthub/codeact_agent/codeact_agent.py @@ -127,13 +127,10 @@ def step(self, state: State) -> Action: 'messages': self.llm.format_messages_for_llm(messages), } params['tools'] = self.tools - try: - response = self.llm.completion(**params) - except Exception: - logger.info('==== START TRACE BROKEN LLM PARAMS ====') - logger.info(str(params)) - logger.info('==== END TRACE BROKEN LLM PARAMS ====') - raise + logger.info('==== START TRACE LLM PARAMS ====') + logger.info(str(params)) + logger.info('==== END TRACE LLM PARAMS ====') + response = self.llm.completion(**params) actions = codeact_function_calling.response_to_actions(response) for action in actions: self.pending_actions.append(action) From 0448979d91be4b47c540f23aba738e8f38931f76 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Thu, 20 Feb 2025 17:01:02 +0000 Subject: [PATCH 08/15] Checking to see if the api key is getting lost --- openhands/llm/llm.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openhands/llm/llm.py b/openhands/llm/llm.py index b40f11ca8396..de97d3bcefe6 100644 --- a/openhands/llm/llm.py +++ b/openhands/llm/llm.py @@ -144,6 +144,8 @@ def __init__( 'temperature' ) # temperature is not supported for reasoning models + logger.info(f'TRACE:LLM_CONFIG:{self.config.api_key}:{self.config}') + self._completion = partial( litellm_completion, model=self.config.model, From dd5c00c2e4e795b25f44bb14ff17469bd002e32f Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Thu, 20 Feb 2025 18:06:17 +0000 Subject: [PATCH 09/15] I need that API key to reproduce this issue locally --- openhands/llm/llm.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openhands/llm/llm.py b/openhands/llm/llm.py index de97d3bcefe6..bd9da54a435f 100644 --- a/openhands/llm/llm.py +++ b/openhands/llm/llm.py @@ -144,7 +144,9 @@ def __init__( 'temperature' ) # temperature is not supported for reasoning models - logger.info(f'TRACE:LLM_CONFIG:{self.config.api_key}:{self.config}') + if self.config.api_key: + logger.info(f'TRACE:LLM_API_KEY:{self.config.api_key.get_secret_value()}') + logger.info(f'TRACE:LLM_CONFIG:{self.config}') self._completion = partial( litellm_completion, From cfbd26272d8067c948a4f9db81981f1fe4519b2e Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Fri, 21 Feb 2025 10:42:41 +0000 Subject: [PATCH 10/15] WIP --- openhands/core/message_utils.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/openhands/core/message_utils.py b/openhands/core/message_utils.py index acafac37240b..25be12873177 100644 --- a/openhands/core/message_utils.py +++ b/openhands/core/message_utils.py @@ -358,11 +358,10 @@ def apply_prompt_caching(messages: list[Message]) -> None: breakpoints_remaining = 3 # remaining 1 for system/tool for message in reversed(messages): if message.role in ('user', 'tool'): - if message.tool_call_id: - continue - message.content[ - -1 - ].cache_prompt = True # Last item inside the message content - breakpoints_remaining -= 1 - if not breakpoints_remaining: - return + if breakpoints_remaining > 0: + message.content[ + -1 + ].cache_prompt = True # Last item inside the message content + breakpoints_remaining -= 1 + else: + break From c4364d0a2b7136b7610be037d88fd5a8afa6f4be Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Fri, 21 Feb 2025 11:18:25 +0000 Subject: [PATCH 11/15] More logging --- openhands/agenthub/codeact_agent/codeact_agent.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openhands/agenthub/codeact_agent/codeact_agent.py b/openhands/agenthub/codeact_agent/codeact_agent.py index ff83d1cb300a..16e0604269ff 100644 --- a/openhands/agenthub/codeact_agent/codeact_agent.py +++ b/openhands/agenthub/codeact_agent/codeact_agent.py @@ -127,6 +127,8 @@ def step(self, state: State) -> Action: 'messages': self.llm.format_messages_for_llm(messages), } params['tools'] = self.tools + logger.info('==== START TRACE LLM EVENTS ====') + logger.info(str(state.history)) logger.info('==== START TRACE LLM PARAMS ====') logger.info(str(params)) logger.info('==== END TRACE LLM PARAMS ====') From e65a8db391500986b4f271907c9e6c69440c5996 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Sat, 22 Feb 2025 09:20:02 +0000 Subject: [PATCH 12/15] Removed approach that is no longer used --- openhands/agenthub/codeact_agent/codeact_agent.py | 5 ----- openhands/llm/llm.py | 4 ---- .../standalone_conversation_manager.py | 7 ------- openhands/server/session/session.py | 5 ----- 4 files changed, 21 deletions(-) diff --git a/openhands/agenthub/codeact_agent/codeact_agent.py b/openhands/agenthub/codeact_agent/codeact_agent.py index 16e0604269ff..b636e40cb9f6 100644 --- a/openhands/agenthub/codeact_agent/codeact_agent.py +++ b/openhands/agenthub/codeact_agent/codeact_agent.py @@ -127,11 +127,6 @@ def step(self, state: State) -> Action: 'messages': self.llm.format_messages_for_llm(messages), } params['tools'] = self.tools - logger.info('==== START TRACE LLM EVENTS ====') - logger.info(str(state.history)) - logger.info('==== START TRACE LLM PARAMS ====') - logger.info(str(params)) - logger.info('==== END TRACE LLM PARAMS ====') response = self.llm.completion(**params) actions = codeact_function_calling.response_to_actions(response) for action in actions: diff --git a/openhands/llm/llm.py b/openhands/llm/llm.py index 16ea8e6ac4dd..84cf12575008 100644 --- a/openhands/llm/llm.py +++ b/openhands/llm/llm.py @@ -145,10 +145,6 @@ def __init__( 'temperature' ) # temperature is not supported for reasoning models - if self.config.api_key: - logger.info(f'TRACE:LLM_API_KEY:{self.config.api_key.get_secret_value()}') - logger.info(f'TRACE:LLM_CONFIG:{self.config}') - self._completion = partial( litellm_completion, model=self.config.model, diff --git a/openhands/server/conversation_manager/standalone_conversation_manager.py b/openhands/server/conversation_manager/standalone_conversation_manager.py index 5ecbd1b524d5..ca5d634bc9fb 100644 --- a/openhands/server/conversation_manager/standalone_conversation_manager.py +++ b/openhands/server/conversation_manager/standalone_conversation_manager.py @@ -218,7 +218,6 @@ async def maybe_start_agent_loop( config=self.config, sio=self.sio, user_id=user_id, - status_message_callback=self._status_message_callback, ) self._local_agent_loops_by_sid[sid] = session asyncio.create_task(session.initialize_agent(settings, initial_user_msg)) @@ -285,12 +284,6 @@ async def _close_session(self, sid: str): await session.close() logger.info(f'closed_session:{session.sid}') - async def _status_message_callback(self, sid: str, msg_type: str, id: str): - if msg_type == 'error' and id == 'STATUS$ERROR_RUNTIME_DISCONNECTED': - logger.info('TRACE:I_WOULD_HAVE_STOPPED_THE_AGENT_LOOP_HERE') - # If there is no runtime, stop the agent loop. - # asyncio.create_task(self._close_session(sid)) - @classmethod def get_instance( cls, diff --git a/openhands/server/session/session.py b/openhands/server/session/session.py index 6a3144ba3fd3..48f171e0bc74 100644 --- a/openhands/server/session/session.py +++ b/openhands/server/session/session.py @@ -1,7 +1,6 @@ import asyncio import time from copy import deepcopy -from typing import Callable import socketio @@ -50,7 +49,6 @@ def __init__( file_store: FileStore, sio: socketio.AsyncServer | None, user_id: str | None = None, - status_message_callback: Callable | None = None, ): self.sid = sid self.sio = sio @@ -69,7 +67,6 @@ def __init__( self.config = deepcopy(config) self.loop = asyncio.get_event_loop() self.user_id = user_id - self._status_message_callback = status_message_callback async def close(self): if self.sio: @@ -248,8 +245,6 @@ async def _send_status_message(self, msg_type: str, id: str, message: str): controller = self.agent_session.controller if controller is not None: await controller.set_agent_state_to(AgentState.ERROR) - if self._status_message_callback: - await self._status_message_callback(self.sid, msg_type, id) await self.send( {'status_update': True, 'type': msg_type, 'id': id, 'message': message} ) From 4f36ae00d1ae86b74f6442ca112daa4908cdbb61 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Sun, 23 Feb 2025 08:44:55 +0000 Subject: [PATCH 13/15] Revert "Revert "Fix: File Descriptor leak" (#6887)" This reverts commit bf82f75ae490165fd5d1f24b46aace49729caa72. --- openhands/llm/llm.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/openhands/llm/llm.py b/openhands/llm/llm.py index b40f11ca8396..84cf12575008 100644 --- a/openhands/llm/llm.py +++ b/openhands/llm/llm.py @@ -20,6 +20,7 @@ from litellm.exceptions import ( RateLimitError, ) +from litellm.llms.custom_httpx.http_handler import HTTPHandler from litellm.types.utils import CostPerToken, ModelResponse, Usage from litellm.utils import create_pretrained_tokenizer @@ -231,8 +232,17 @@ def wrapper(*args, **kwargs): # Record start time for latency measurement start_time = time.time() - # we don't support streaming here, thus we get a ModelResponse - resp: ModelResponse = self._completion_unwrapped(*args, **kwargs) + # LiteLLM currently have an issue where HttpHandlers are being created but not + # closed. We have submitted a PR to them, (https://github.com/BerriAI/litellm/pull/8711) + # and their dev team say they are in the process of a refactor that will fix this. + # In the meantime, we manage the lifecycle of the HTTPHandler manually. + handler = HTTPHandler(timeout=self.config.timeout) + kwargs['client'] = handler + try: + # we don't support streaming here, thus we get a ModelResponse + resp: ModelResponse = self._completion_unwrapped(*args, **kwargs) + finally: + handler.close() # Calculate and record latency latency = time.time() - start_time From 4bd32905c1a49cfd82cb85f9e0a0e9cc4bf4a103 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Sun, 23 Feb 2025 09:07:30 +0000 Subject: [PATCH 14/15] Fix for log completions --- openhands/llm/llm.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/openhands/llm/llm.py b/openhands/llm/llm.py index 84cf12575008..497578181fc1 100644 --- a/openhands/llm/llm.py +++ b/openhands/llm/llm.py @@ -297,7 +297,11 @@ def wrapper(*args, **kwargs): 'messages': messages, 'response': resp, 'args': args, - 'kwargs': {k: v for k, v in kwargs.items() if k != 'messages'}, + 'kwargs': { + k: v + for k, v in kwargs.items() + if k not in ('messages', 'client') + }, 'timestamp': time.time(), 'cost': cost, } From 8b41359721bf972f36b324d27f039e7f88213520 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Sun, 23 Feb 2025 09:24:58 +0000 Subject: [PATCH 15/15] Added test --- tests/unit/test_llm.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/unit/test_llm.py b/tests/unit/test_llm.py index 1bfee8550698..9c112106f12a 100644 --- a/tests/unit/test_llm.py +++ b/tests/unit/test_llm.py @@ -1,4 +1,6 @@ import copy +import tempfile +from pathlib import Path from unittest.mock import MagicMock, patch import pytest @@ -429,3 +431,27 @@ def test_get_token_count_error_handling( mock_logger.error.assert_called_once_with( 'Error getting token count for\n model gpt-4o\nToken counting failed' ) + + +@patch('openhands.llm.llm.litellm_completion') +def test_completion_with_log_completions(mock_litellm_completion, default_config): + with tempfile.TemporaryDirectory() as temp_dir: + default_config.log_completions = True + default_config.log_completions_folder = temp_dir + mock_response = { + 'choices': [{'message': {'content': 'This is a mocked response.'}}] + } + mock_litellm_completion.return_value = mock_response + + test_llm = LLM(config=default_config) + response = test_llm.completion( + messages=[{'role': 'user', 'content': 'Hello!'}], + stream=False, + drop_params=True, + ) + assert ( + response['choices'][0]['message']['content'] == 'This is a mocked response.' + ) + files = list(Path(temp_dir).iterdir()) + # Expect a log to be generated + assert len(files) == 1