All-Hands-AI / OpenHands

🙌 OpenHands: Code Less, Make More
https://all-hands.dev
MIT License
34.19k stars 3.89k forks source link

[Refactor]: Simplify ShortTermHistory/EventStream Logic #3731

Closed neubig closed 1 day ago

neubig commented 2 months ago

Summary

The following classes are a bit complex with non-intuitive interfaces, it'd be good to make them simpler:

https://github.com/All-Hands-AI/OpenHands/blob/add4653335218eb04d0b7960fa67c21f8e313b8e/openhands/events/stream.py#L23

https://github.com/All-Hands-AI/OpenHands/blob/add4653335218eb04d0b7960fa67c21f8e313b8e/openhands/memory/history.py#L21

enyst commented 2 months ago

@neubig I tried to move filtering in the event stream completely, and the rest of history. It looks like this: https://github.com/All-Hands-AI/OpenHands/compare/main...enyst/eventstream-state

Is that closer to what you have in mind?

I noticed two things, if we go this route:

Should we try to let all agents and the rest reference the event stream directly? If we use another intermediary, for the second case that might be the new PromptManager class. But in that case, I'm not sure we simplify things, if PromptManager basically takes over what history was doing. 🤔

neubig commented 2 months ago

Thanks @enyst , much appreciated!

EventStream needs a reference to State.

We can mostly avoid this by making start_id and end_id in get_events() default to the beginning and end IDs of the event stream, right? So start_id defaults to 0, and end_id defaults to self._cur_id-1

The one place that's tricky is self.state.delegates.items(), and that might require a little thought, but I think the IDs to be filtered out could also probably be passed in as necessary.

Should we try to let all agents and the rest reference the event stream directly?

That seems fine to me?

Also small comment, it's a little easier to comment on code if you send a draft PR, but it's not a big deal.

github-actions[bot] commented 1 month ago

This issue is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.