OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

feat(sandbox): Candidate Implementation of Sandbox Plugin to Support Jupyter

Open xingyaoww opened this issue 1 year ago • 4 comments

As discussed in https://github.com/OpenDevin/OpenDevin/issues/1251, we need a better way (instead of the not ideal attempt in https://github.com/OpenDevin/OpenDevin/pull/1215) to incorporate different plugins/tools (e.g., jupyter kernel for interactive python execution).

The goal is "If the agent implementer wants to make any assumptions about the environment beyond "it's bash", the agent needs to make sure everything is set up." The current implementation assumes different behavior for each sandbox that may go against the goal.

In this PR, I attempt to implement one that satisfies this goal by allowing any arbitrary sandbox to be initialized with plugins (a setup.sh that automatically sets everything needed). I added PluginMixin, which can be mixed with all Sandbox that leverage their .execute method to complete the setup process.

I also mapped opendevin/.cache to ~/.cache inside the sandbox so that the user doesn't need to download the pip install packages everytime they re-start the container -- these will be automatically cached. The user can also clean-up these caches (e.g., if they want to switch to a different sandbox) by running make clean.

I have only implemented this for DockerSSHBox yet, and i hope to get some feedback before proceeding to include this Mixin for all boxes :)

image

Once setup.sh is completed, you can send code to jupyter kernel for execution by using echo "import math" | execute_cli. image

xingyaoww avatar Apr 20 '24 17:04 xingyaoww

That was super fast! This is looking awesome. I think you're definitely on the right track

rbren avatar Apr 20 '24 18:04 rbren

@rbren After trying to extend this Plugin capability to different sandboxes, I realized that it could be challenging since different sandbox would assume different "user" and levels of system access:

  • ExecBox can potentially work, but its .execute is not stateful, which make this Sandbox different (deviating from our original goal that each Sandbox should has a consistent interface?)
  • LocalBox is executed locally, which means it may not have sudo commands available; Also, it is not stateful (e.g., environment variables set in the previous turn is no longer accessible in subsequent turns).

If we managed to get the DockerSSHBox working stably with the DOOD route, do you think if we can deprecate LocalBox and ExecBox and solely focus on SSHBox for future development? It could saves a tons of efforts (but of course we need to double-check Windows user can run this DOOD without issues)

xingyaoww avatar Apr 21 '24 08:04 xingyaoww

@xingyaoww I definitely want to get rid of ExecBox, for the reason you stated--it's not stateful, and we want something that's stateful. I think with the --add-host trick we might be ready?

LocalBox seems potentially useful still. And we want to keep supporting e2b.

I think so long as we make sure the plugin script fails with a good error (e.g. "Failed to install $PLUGIN on $SANDBOX_TYPE: $ERROR_MESSAGE"), we can make things more consistent over time. Really what I'd like is for sandbox maintainers (e.g. mlejva for e2b) to be responsible for ensuring their sandbox env is as consistent as possible, with SSHBox being our "north star".

rbren avatar Apr 21 '24 19:04 rbren

@xingyaoww I definitely want to get rid of ExecBox, for the reason you stated--it's not stateful, and we want something that's stateful. I think with the --add-host trick we might be ready?

LocalBox seems potentially useful still. And we want to keep supporting e2b.

I think so long as we make sure the plugin script fails with a good error (e.g. "Failed to install $PLUGIN on $SANDBOX_TYPE: $ERROR_MESSAGE"), we can make things more consistent over time. Really what I'd like is for sandbox maintainers (e.g. mlejva for e2b) to be responsible for ensuring their sandbox env is as consistent as possible, with SSHBox being our "north star".

Agree! Btw, I think ExecBox will suffer from the exact same issue of --network=host, since it also relies on docker, right? i.e., flask server started by ExecBox also cannot be accessed from outside the container.

Also, just realizing the --network=host is supported with the newest version of Docker Desktop! If this works, i guess we can probably deprecate the ExecBox, and having DOOD with DockerSSHBox as the default setting (if they also work well on Linux).

Version Docker Desktop 4.29.0 Settings ⇒ Features in development ⇒ Enable host networking Host networking allows containers that are started with --net=host to use localhost to connect to TCP and UDP services on the host. It will automatically allow software on the host to use localhost to connect to TCP and UDP services in the container. Sign in required.

Enable host networking

Originally posted by @4-FLOSS-Free-Libre-Open-Source-Software in https://github.com/docker/roadmap/issues/238#issuecomment-2044688144

xingyaoww avatar Apr 22 '24 07:04 xingyaoww

@xingyaoww @rbren I implemented the copy_to method for E2BBox in a separate PR #1298

mlejva avatar Apr 23 '24 00:04 mlejva