Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(fix): Condensation events to reconstruct contexts added to event stream #7353

Merged
merged 49 commits into from
Mar 27, 2025
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
f6f2a71
condenser return value split into view and condensation
Mar 19, 2025
5ceaeaa
fixing tests by adding dunder methods to view obj
Mar 19, 2025
e07f57a
rework of rolling condenser to use views and condensations
Mar 19, 2025
4eb73ae
fixing up llm attention condenser to use rolling condenser interface
Mar 19, 2025
345cd44
moving amortized forgetting condenser to new rolling condenser impl"
Mar 19, 2025
6588e35
rolling condenser harness to make testing easier + updated amortized …
Mar 19, 2025
1da4e8d
udpating remaining llm attention condenser tests to use new test harness
Mar 19, 2025
ea280a2
exporting formula for expected view size to harness
Mar 19, 2025
a6102eb
cleaning up amortized forgetting tests
Mar 19, 2025
7ab7feb
updating llm attention condenser tests
Mar 19, 2025
f9561f9
intermediate transition of llm summarizing to new condenser interface
Mar 19, 2025
5ec4e58
fixing llm summarizing impl and associated tests
Mar 19, 2025
46c5d3b
condensation result to event stream now an action (originates from th…
Mar 19, 2025
1eed189
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 19, 2025
d85a423
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 20, 2025
e0d9781
fixing unit tests relying on
Mar 20, 2025
facb4e7
Merge branch 'fix/condenser-visibility' of github.com:csmith49/OpenHa…
Mar 20, 2025
e04ea81
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 20, 2025
2f5e104
unified condensation action base class
Mar 20, 2025
6092fce
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 20, 2025
1bb75d2
Merge branch 'fix/condenser-visibility' of github.com:csmith49/OpenHa…
Mar 20, 2025
894adca
single action for now -- easier serialization
Mar 20, 2025
e6fd3e1
condensation in the event stream -> triggers actual condensation mult…
Mar 20, 2025
7fd10c5
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 20, 2025
2d9e790
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 20, 2025
119c6bb
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 21, 2025
5750bfe
minor documentation improvements
Mar 21, 2025
e9d054d
Merge branch 'fix/condenser-visibility' of github.com:csmith49/OpenHa…
Mar 21, 2025
c28bc63
adding condenser class field
Mar 24, 2025
4ee94f3
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 24, 2025
0d1e9e4
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 24, 2025
d1ff924
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 24, 2025
397cc47
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 24, 2025
ecfbb4c
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 25, 2025
1d37b8b
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 25, 2025
b7c0a1c
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 25, 2025
307ea48
minor doc pass
Mar 25, 2025
88a9d35
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 25, 2025
1be62b0
minor
Mar 25, 2025
a411d2f
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 25, 2025
5f04259
event spec polymorphic event
Mar 26, 2025
f4039cb
fixing tests failing from lack of ids
Mar 26, 2025
f3812e7
extended condensation action, view reconstruction moved to view itself
Mar 27, 2025
1a1424a
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 27, 2025
0d9e7ed
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 27, 2025
00f4f17
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 27, 2025
eb12df8
Merge branch 'main' into fix/condenser-visibility
csmith49 Mar 27, 2025
d10137b
minor
Mar 27, 2025
7f6f31a
Merge branch 'fix/condenser-visibility' of github.com:csmith49/OpenHa…
Mar 27, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions openhands/agenthub/codeact_agent/codeact_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@
Action,
AgentFinishAction,
)
from openhands.events.event import Event
from openhands.llm.llm import LLM
from openhands.memory.condenser import Condenser
from openhands.memory.condenser.condenser import Condensation, View
from openhands.memory.conversation_memory import ConversationMemory
from openhands.runtime.plugins import (
AgentSkillsRequirement,
Expand Down Expand Up @@ -92,6 +94,7 @@ def reset(self) -> None:

def step(self, state: State) -> Action:
"""Performs one step using the CodeAct Agent.

This includes gathering info on previous steps and prompting the model to make a command to execute.

Parameters:
Expand All @@ -113,8 +116,23 @@ def step(self, state: State) -> Action:
if latest_user_message and latest_user_message.content.strip() == '/exit':
return AgentFinishAction()

# prepare what we want to send to the LLM
messages = self._get_messages(state)
# Condense the events from the state. If we get a view we'll pass those
# to the conversation manager for processing, but if we get a condensation
# event we'll just return that instead of an action. The controller will
# immediately ask the agent to step again with the new view.
condensed_history: list[Event] = []
match self.condenser.condensed_history(state):
case View(events=events):
condensed_history = events

case Condensation(action=condensation_action):
return condensation_action

logger.debug(
f'Processing {len(condensed_history)} events from a total of {len(state.history)} events'
)

messages = self._get_messages(condensed_history)
params: dict = {
'messages': self.llm.format_messages_for_llm(messages),
}
Expand All @@ -127,7 +145,7 @@ def step(self, state: State) -> Action:
self.pending_actions.append(action)
return self.pending_actions.popleft()

def _get_messages(self, state: State) -> list[Message]:
def _get_messages(self, events: list[Event]) -> list[Message]:
"""Constructs the message history for the LLM conversation.

This method builds a structured conversation history by processing events from the state
Expand All @@ -143,7 +161,7 @@ def _get_messages(self, state: State) -> list[Message]:
6. Adds environment reminders for non-function-calling mode

Args:
state (State): The current state object containing conversation history and other metadata
events: The list of events to convert to messages

Returns:
list[Message]: A list of formatted messages ready for LLM consumption, including:
Expand All @@ -167,13 +185,6 @@ def _get_messages(self, state: State) -> list[Message]:
with_caching=self.llm.is_caching_prompt_active()
)

# Condense the events from the state.
events = self.condenser.condensed_history(state)

logger.debug(
f'Processing {len(events)} events from a total of {len(state.history)} events'
)

# Use ConversationMemory to process events
messages = self.conversation_memory.process_events(
condensed_history=events,
Expand Down
4 changes: 3 additions & 1 deletion openhands/controller/agent_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
MessageAction,
NullAction,
)
from openhands.events.action.agent import RecallAction
from openhands.events.action.agent import CondensationAction, RecallAction
from openhands.events.event import Event
from openhands.events.observation import (
AgentCondensationObservation,
Expand Down Expand Up @@ -305,6 +305,8 @@ def should_step(self, event: Event) -> bool:
return True
if isinstance(event, AgentDelegateAction):
return True
if isinstance(event, CondensationAction):
return True
return False
if isinstance(event, Observation):
if (
Expand Down
3 changes: 3 additions & 0 deletions openhands/core/schema/action.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,6 @@ class ActionType(str, Enum):

RECALL = 'recall'
"""Retrieves content from a user workspace, microagent, or other source."""

CONDENSATION = 'condensation'
"""Condenses a list of events into a summary."""
84 changes: 84 additions & 0 deletions openhands/events/action/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,87 @@ def __str__(self) -> str:
ret = '**RecallAction**\n'
ret += f'QUERY: {self.query[:50]}'
return ret


@dataclass
class CondensationAction(Action):
"""This action indicates a condensation of the conversation history is happening.

There are two ways to specify the events to be forgotten:
1. By providing a list of event IDs.
2. By providing the start and end IDs of a range of events.
Comment on lines +120 to +122
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading this class again, do we actually need these two code paths? Couldn't we just do the first one and the functionality would be a superset of the second one? Sure it'd be a little bit less efficient memory-wise, but storing a list of ints is not really a big deal I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first path is a functional super-set of the second, yes. Engel expressed concerns about the size of the messages so I added the suggested range representation as an optimization. I don't have numbers on how much it saves but I think the overall memory impact is minor either way (maybe ~0.1% of the data in the whole trajectory?).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm sorry, it may be just me and my inefficiency hate: they're consecutive, so unnecessary and they would be saved/read in every condensation action. 🥹 They would also be sent via websocket.

Our usual case is the summarizer, where they're consecutive. In fact, all of them, except for llm_attention, specify, at most, a range: from start to end.

IMHO a way to represent these would be with polymorphism and discriminated union. But the event stream is "flat". When we represented this kind of case before, we made a flattened representation with an enum as discriminator for serialization and some validation, and an umbrella class for the necessary fields. Calvin doesn't like that, quite understandably - and here they would be more than two. 😅

This is an umbrella class, but with just minimal options, which is two. It's a middle-ground I'm happy with, but maybe it's bad?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, personally I think code complexity is worse than small amounts of inefficiency. But it's not a hill that I'll die on :)


In the second case, we assume that event IDs are monotonically increasing, and that _all_ events between the start and end IDs are to be forgotten.
Copy link
Collaborator

@enyst enyst Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, I also noticed again while rebuilding lists that this is completely reasonable, but for the condense actions themselves. They will be in the stream/history interweaved with the last big list to be forgotten.

This isn't just an assumption, it's by definition. We can define the start id and end id to be the range between which most condensers (the exception being attention) have their forgotten events in that step.

In fact, whether we need the other condense actions is a question for the condenser implementation. Outside the condensers, if we want to send them to the agent LLM, we can pick them up. They're in the stream, so they're in history, so we don't, strictly speaking, need the condensers to also provide those, we can pick them up if we want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm so we can resolve this comment -- all event IDs are ordered monotonically increasing as they get added to the event stream? So if event_i is added to the event stream, then event_j is added, it will always be the case that event_i.id < event_j.id?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The stream itself assigns ids in ascending order, in the add_event method, which is the only way to do it, and the _id field is otherwise never set.


Raises:
ValueError: If the optional fields are not instantiated in a valid configuration.
"""

action: str = ActionType.CONDENSATION

forgotten_event_ids: list[int] | None = None
"""The IDs of the events that are being forgotten (removed from the `View` given to the LLM)."""

forgotten_events_start_id: int | None = None
"""The ID of the first event to be forgotten in a range of events."""

forgotten_events_end_id: int | None = None
"""The ID of the last event to be forgotten in a range of events."""

summary: str | None = None
"""An optional summary of the events being forgotten."""

summary_offset: int | None = None
"""An optional offset to the start of the resulting view indicating where the summary should be inserted."""

def _validate_field_polymorphism(self) -> bool:
"""Check if the optional fields are instantiated in a valid configuration."""
# For the forgotton events, there are only two valid configurations:
# 1. We're forgetting events based on the list of provided IDs, or
using_event_ids = self.forgotten_event_ids is not None
# 2. We're forgetting events based on the range of IDs.
using_event_range = (
self.forgotten_events_start_id is not None
and self.forgotten_events_end_id is not None
)

# Either way, we can only have one of the two valid configurations.
forgotten_event_configuration = using_event_ids ^ using_event_range

# We also need to check that if the summary is provided, so is the
# offset (and vice versa).
summary_configuration = (
self.summary is None and self.summary_offset is None
) or (self.summary is not None and self.summary_offset is not None)

return forgotten_event_configuration and summary_configuration

def __post_init__(self):
if not self._validate_field_polymorphism():
raise ValueError('Invalid configuration of the optional fields.')

@property
def forgotten(self) -> list[int]:
"""The list of event IDs that should be forgotten."""
# Start by making sure the fields are instantiated in a valid
# configuration. We check this whenever the event is initialized, but we
# can't make the dataclass immutable so we need to check it again here
# to make sure the configuration is still valid.
if not self._validate_field_polymorphism():
raise ValueError('Invalid configuration of the optional fields.')

if self.forgotten_event_ids is not None:
return self.forgotten_event_ids

# If we've gotten this far, the start/end IDs are not None.
assert self.forgotten_events_start_id is not None
assert self.forgotten_events_end_id is not None
return list(
range(self.forgotten_events_start_id, self.forgotten_events_end_id + 1)
)

@property
def message(self) -> str:
if self.summary:
return f'Summary: {self.summary}'
return f'Condenser is dropping the events: {self.forgotten}.'
2 changes: 2 additions & 0 deletions openhands/events/serialization/action.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
AgentRejectAction,
AgentThinkAction,
ChangeAgentStateAction,
CondensationAction,
RecallAction,
)
from openhands.events.action.browse import BrowseInteractiveAction, BrowseURLAction
Expand Down Expand Up @@ -39,6 +40,7 @@
RecallAction,
ChangeAgentStateAction,
MessageAction,
CondensationAction,
)

ACTION_TYPE_TO_CLASS = {action_class.action: action_class for action_class in actions} # type: ignore[attr-defined]
Expand Down
15 changes: 13 additions & 2 deletions openhands/memory/condenser/__init__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
import openhands.memory.condenser.impl # noqa F401 (we import this to get the condensers registered)
from openhands.memory.condenser.condenser import Condenser, get_condensation_metadata
from openhands.memory.condenser.condenser import (
Condenser,
get_condensation_metadata,
View,
Condensation,
)

__all__ = ['Condenser', 'get_condensation_metadata', 'CONDENSER_REGISTRY']
__all__ = [
'Condenser',
'get_condensation_metadata',
'CONDENSER_REGISTRY',
'View',
'Condensation',
]
Loading