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

feat: Add LocalRuntime and rename EventStreamRuntime to DockerRuntime #5284

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

xingyaoww
Copy link
Collaborator

@xingyaoww xingyaoww commented Nov 26, 2024

This PR adds a new LocalRuntime implementation and renames EventStreamRuntime to LocalDockerRuntime for better clarity.

Changes

  • Add new LocalRuntime implementation that runs action_execution_server directly on the host machine
  • Rename EventStreamRuntime to LocalDockerRuntime for better clarity
  • Move runtime implementations to dedicated directories (local/ and docker/)
  • Update documentation to reflect runtime changes and add LocalRuntime description

Benefits

  • Provides a simpler setup for users who don't need container isolation
  • Clearer naming convention for runtime implementations
  • Better organized codebase with dedicated directories for each runtime type
  • Updated documentation to help users choose the right runtime for their needs

Issue fixed: #3903


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:724c1b0-nikolaik   --name openhands-app-724c1b0   docker.all-hands.dev/all-hands-ai/openhands:724c1b0

…ntime

- Add new LocalRuntime implementation that runs action_execution_server directly on host
- Rename EventStreamRuntime to LocalDockerRuntime for clarity
- Move runtime implementations to dedicated directories
- Update documentation to reflect runtime changes
- Update imports to use LocalDockerRuntime and LocalRuntime
- Add LocalRuntime to get_runtime_classes()
- Update _close_test_runtime to handle LocalDockerRuntime
- Add proper type hints for server_process
- Fix stdout access safety
- Fix async/await type hints
- Improve error handling
openhands/runtime/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

OpenHands started fixing the pr! You can monitor the progress here.

@openhands-agent
Copy link
Contributor

New OpenHands update

@enyst
Copy link
Collaborator

enyst commented Nov 29, 2024

@openhands-agent This PR tests fail with

File "/home/runner/work/OpenHands/OpenHands/openhands/runtime/init.py", line 3, in
from openhands.runtime.impl.eventstream.eventstream_runtime import (

ERROR:root:<class 'ModuleNotFoundError'>: No module named 'openhands.runtime.impl.eventstream'

Fix this error. Keep your changes minimal.

Copy link
Contributor

OpenHands started fixing the pr! You can monitor the progress here.

tests/runtime/conftest.py Outdated Show resolved Hide resolved
tests/runtime/conftest.py Outdated Show resolved Hide resolved
tests/runtime/conftest.py Outdated Show resolved Hide resolved
@enyst
Copy link
Collaborator

enyst commented Nov 29, 2024

@openhands-agent We have done a lot to fix the original. This PR is doing these:

  • Add new LocalRuntime implementation that runs action_execution_server directly on the host machine
  • Rename EventStreamRuntime to DockerRuntime for better clarity
  • Move runtime implementations to dedicated directories (local/ and docker/)
  • Update documentation to reflect runtime changes and add LocalRuntime description.

The PR is mostly finished now. It has only one detail that went wrong: instead of the name LocalDockerRuntime, we now want the name DockerRuntime. Find all occurrences and fix them. Additionally, look also for the name EventStreamRuntime, and if you find any, update it to the new name, DockerRuntime.

Copy link
Contributor

OpenHands started fixing the pr! You can monitor the progress here.

@All-Hands-AI All-Hands-AI deleted a comment from openhands-agent Nov 29, 2024
@All-Hands-AI All-Hands-AI deleted a comment from github-actions bot Nov 29, 2024
@All-Hands-AI All-Hands-AI deleted a comment from github-actions bot Nov 29, 2024
@All-Hands-AI All-Hands-AI deleted a comment from openhands-agent Nov 29, 2024
@All-Hands-AI All-Hands-AI deleted a comment from github-actions bot Nov 29, 2024
@enyst
Copy link
Collaborator

enyst commented Nov 30, 2024

@xingyaoww

  • the LLM changed to "DockerRuntime" at some point, and I went with it: it's now "DockerRuntime" (instead of "LocalDockerRuntime") everywhere
  • it doesn't have unit tests. Do we want for this runtime the equivalent of all tests of the eventstream runtime? Or only some?

openhands/runtime/README.md Outdated Show resolved Hide resolved
openhands/runtime/README.md Outdated Show resolved Hide resolved
@xingyaoww
Copy link
Collaborator Author

@enyst going back from thanksgiving break and see this.. I'm very impressed :D - I think we can just see if we can run the same suites of runtime tests on LocalRuntime 🤔

Copy link
Contributor

OpenHands started fixing the pr! You can monitor the progress here.

@All-Hands-AI All-Hands-AI deleted a comment from github-actions bot Nov 30, 2024
@All-Hands-AI All-Hands-AI deleted a comment from openhands-agent Nov 30, 2024
@xingyaoww xingyaoww changed the title feat: Add LocalRuntime and rename EventStreamRuntime to LocalDockerRuntime feat: Add LocalRuntime and rename EventStreamRuntime to DockerRuntime Dec 1, 2024
@xingyaoww
Copy link
Collaborator Author

@openhands-agent Can you try to set up environment first, then try to run

TEST_RUNTIME=local \
          TEST_IN_CI=true \
          RUN_AS_OPENHANDS=true \
          poetry run pytest -raRs --reruns 2 --reruns-delay 5 --cov=openhands --cov-report=xml -s ./tests/runtime

And get the testcase to pass? Note you are allowed to override some of the SandboxConfig in openhands/runtime/impl/local/local_runtime.py, e.g., You can ignore config.run_as_openhands and sandbox.user_id, and instead, running as the current USER name and ID. Also, you need to override self.config.workspace_mount_path_in_sandbox to a locally accessible directory.

Copy link
Contributor

github-actions bot commented Dec 1, 2024

OpenHands started fixing the pr! You can monitor the progress here.

@openhands-agent
Copy link
Contributor

New OpenHands update

Copy link
Contributor

github-actions bot commented Dec 1, 2024

The workflow to fix this issue encountered an error. Please check the workflow logs for more information.

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.

3 participants