-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
enh: Refactor Event
-> Message
pipeline outside of CodeActAgent
#6715
base: main
Are you sure you want to change the base?
enh: Refactor Event
-> Message
pipeline outside of CodeActAgent
#6715
Conversation
Event
-> Message
pipeline outside of CodeActAgent
openhands/core/message_utils.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also an opportunity to move message.py
, message_format.md
, and message_utils.py
into a sub-module. I didn't do so here to limit the files touched but LMK if this is a good spot for the larger re-org as well.
@@ -130,9 +130,9 @@ def event_to_memory(event: 'Event', max_message_chars: int) -> dict: | |||
return d | |||
|
|||
|
|||
def truncate_content(content: str, max_chars: int) -> str: | |||
def truncate_content(content: str, max_chars: int | None = None) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this function does anything substantial ATM, are there plans to change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I think sometimes it matters a lot, because this is the function used to truncate the obs content.
max_chars is 30_000 by default ~= 9k tokens (seen some with anthropic tokenizer API)
This is because a single observation can be crazy large. A fun example is the result of an innocent ls -R /workspace
on the Django repository. The observation has ~120k tokens. 😭 And we save them all in the stream.
I guess we could:
- in stream.py, return obs already truncated
- not save the full content in the first place (but we may want to keep more of it than 30k, idk). I am not sure about this one, simply conceptually... we do want the event stream to be "the source of truth" so we can find later whatever the underlying content was. The other side of the argument here though is that the full one is never used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're absolutely right -- I was misreading the implementation and thought we were taking half = len(content) // 2
instead of half = max_chars // 2
🙃
End-user friendly description of the problem this fixes or functionality that this introduces
Give a summary of what the PR does, explaining any non-trivial design decisions
This PR pulls the
Event
->Message
pipeline out of the CodeAct agent and intocore
.Additionally:
truncate_content
default behaviorMessage
conversion tests out oftest_codeact_agent.py
and intotest_message_utils.py
CodeActAgent._get_messages
with two new private methods to encapsulate behaviorThese changes enable us to:
Event
->Message
conversionOther agents don't use a similar pipeline: the browsing agents build messages manually from prompts, and the delegating agent just delegates.
Link of any specific issues this addresses
This PR is necessary to resolve #6707, as we need to compute the accurate message structure outside of the agent.