-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Integrate E2B sandbox as an alternative to a Docker container #727
Conversation
curious: Can e2b run inside docker? |
No. E2B sandbox is a VM so it sits on a "lower level" compared to a container. What will be possible in the future is to run Docker inside E2B. |
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.
Looking good!
We can probably formalize the sandbox interface with an abstract class--doesn't need to be in this PR though
I'm planning on updating the PR today/tomorrow with proper config and env vars setup and make it match the functionality of the current sandbox. |
@rbren a short update, still work in progress. I updated the PR so the E2B sandbox matches the functionality of the current Docker sandbox. Specifically the background commands are now supported. Also rebased to main to stay up to date with the new code style rules. Next step is to:
I think I should be able to finish the whole PR this Sunday |
@rbren similar to what @foragerr did in #874 I think we should also build a common interface for process (what's known as command at the moment). It'd be something fairly simple like from abc import ABC, abstractmethod
class Process(ABC):
@abstractmethod
def kill(self):
pass
@abstractmethod
def read_logs(self) -> str:
pass
@property
@abstractmethod
def id(self) -> int:
pass
@property
@abstractmethod
def command(self) -> str:
pass
@property
@abstractmethod
def exit_code(self) -> int | None: # The reason it can be none is because the process can still be running
pass What do you think? |
@mlejva that seems reasonable to me! |
@foragerr I realized I might have misunderstood your question. I thought you're talking about self-hosting. With self-hosting what I wrote stands - E2B sandbox is a VM so it sits on a "lower level" compared to a container. What will be possible in the future is to run Docker inside E2B. But you can definitely use E2B SDK for launching sandboxes in our cloud from Docker and Dockerize the whole app. I just wanted to clear that up |
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.
Looks great, thanks a bunch for the contribution! I just have a few small comments.
with open(whole_path, "w", encoding="utf-8") as file: | ||
file.write(self.content) | ||
return FileWriteObservation(content="", path=self.path) | ||
if isinstance(controller.command_manager.sandbox, E2BBox): |
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 feels leaky to me. But passing controller
into run
is already kind of leaky. Curious if you have any ideas on how to improve this.
Maybe sandbox
should handle all the run
logic...
We can come back to this in a future PR
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.
Yeah I agree this doesn't feel right. Happy to refactorize this once we merge this PR
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.
Excited to get this in!
Hey @rbren thank you so much for the code review. I merged the I built it locally with both Python 3.11 and 3.12 and it looks like it's passing. The errors in GitHub actions seem to only show error code and nothing else though:
I'm not sure why it's not passing, I you have any tips, it would be super helpful! |
I also did a bit of refactorization in this PR.
|
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 LGTM as-is! A couple minor notes though
|
||
1. Build the sandbox | ||
```sh | ||
e2b template build --dockerfile ./Dockerfile --name "open-devin" |
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.
Can we e2b template
from a remote image like ubuntu
or ghcr.io/opendevin/sandbox
? Or does the user have to build it from scratch?
Would be nice not to have to maintain a new Dockerfile, or to at least build it for them in our CI/CD pipeline
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.
You can share the same Dockerfile and use a remote image. We just have some restrictions on what images and Dockerfile commands we support. Generally, you should be good. I'll set it up to use the same Dockerfile image
@@ -0,0 +1,14 @@ | |||
# This is a config for E2B sandbox template. |
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 don't see this file getting used anywhere?
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.
It's used by the E2B CLI, it's something like Cloudflare Workers have their wrangler.toml
opendevin/sandbox/e2b/README.md
Outdated
|
||
1. [Get your API key](https://e2b.dev/docs/getting-started/api-key) | ||
|
||
1. Set your E2B API key in the `config.toml` file as `E2B_API_KEY` |
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.
We're mostly getting rid of config.toml
now that everything is running inside docker. Maybe this should be passed into docker run
with -e
?
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.
Yeah, that should work, I can update it. So config.py
isn't used anymore and instead we should pass the env vars, right?
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 updated the readme to instruct users to set the E2B API key to the E2B_API_KEY
env var.
If there's nothing else you think should change, this PR should be good to go!
auto-merge enabled! 🚀 Thanks for all the work here! |
Sorry, merge conflicts 😭 SWE-agent implemented start and end lines for read/write operations, which I suppose we'll need to support with e2b. Hopefully that's not too hard... |
Looking into it in 1-2 hours! |
Head branch was pushed to by a user without write access
Hey @rbren I finally got to resolving the conflicts. Sorry it took longer. Thank you! |
Head branch was pushed to by a user without write access
This is a draft PR for integrating E2B sandbox for OpenDevin's agents. The E2B sandbox is a secure micro VM made for running AI agents and AI generated code in the cloud.
I'll be updating the following todo list as I go:
sandbox.py
Use E2B sandbox for running swe-bench evals end-to-end(will be done in separate PR)