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

Remove while True in AgentController #5868

Merged
merged 69 commits into from
Dec 31, 2024
Merged

Remove while True in AgentController #5868

merged 69 commits into from
Dec 31, 2024

Conversation

rbren
Copy link
Collaborator

@rbren rbren commented Dec 27, 2024

End-user friendly description of the problem this fixes or functionality that this introduces

  • Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below
    no changelog

Give a summary of what the PR does, explaining any non-trivial design decisions

The while True here is causing a lot of problems. It's easy for it to get stuck permanently.

Removing that loop is a surprisingly simple change--we just _step the agent whenever a new event comes in that it might react to. Haven't fully tested this yet but at first blush it's working well.

However, doing that surfaced a bunch of subtle issues with EventStream callbacks blocking each other, as well as main parts of the app. So now:

  • All events are added to a queue, which is processed in order
  • The queue is processed on its own thread
  • Every subscriber gets its own thread, with an asyncio loop set up for it

Link of any specific issues this addresses


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:ac520c1-nikolaik   --name openhands-app-ac520c1   docker.all-hands.dev/all-hands-ai/openhands:ac520c1

@rbren rbren marked this pull request as ready for review December 27, 2024 23:37
Copy link
Collaborator

@enyst enyst left a comment

Choose a reason for hiding this comment

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

The exclusions in should_step include the filter_out class var values. That looks right to me.

FWIW, I did look for ways that will break this PR, related to should_step. I didn't find something that will break. 😅 🤷‍♂️

@rbren rbren merged commit d29cc61 into main Dec 31, 2024
14 of 15 checks passed
@rbren rbren deleted the rb/controller-loop branch December 31, 2024 21:10
@rbren rbren mentioned this pull request Jan 6, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants