-
Notifications
You must be signed in to change notification settings - Fork 860
feat: sandboxed home #7702
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
base: main
Are you sure you want to change the base?
feat: sandboxed home #7702
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
akshayka
left a comment
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.
Before releasing this feature, we should make sure the UX issues you alluded to in the PR description, in particular when the frontend handles the case in which sandbox notebooks fail to start (e.g., uv sync fails):
- the error should be communicated to the user;
- we should either fail fast (show the error and stop trying to connect to the kernel), or provide a way for the user to fix the error and try again (edit script metadata, then click a button to try creating the venv again)
If you prefer to work in multiple PRs, sandbox home could be feature flagged until the FE handles the failure case
akshayka
left a comment
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.
Shutting down the server on the frontend should also shutdown running notebooks, but that does not appear to be happening right now
Co-authored-by: Akshay Agrawal <[email protected]>
for more information, see https://pre-commit.ci
| KernelStartupErrorNotification(error=error_message) | ||
| ) | ||
| text = f'{{"op": "kernel-startup-error", "data": {msg.decode("utf-8")}}}' | ||
| await self.websocket.send_text(text) |
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.
self.session.notify( KernelStartupErrorNotification(error=error_message))
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 maybe you don't have the session? this bit feels fragile
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.
Good point. Changing this up, an advantage to having a session is that we could potentially stream the errors/ startup before closing. Will keep that for a future PR because this is quiet long, but will do the non-blocking kernel launch
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 did try to refactor this such that we:
- async attempted startup kernel
- started session and the web socket loop
- push notifications from kernel startup directly to the queue
This also has the advantage of being able to stream messages during startup

In theory. In practice the installation modal popped up after it was done (not sure what I was missing there), I had a couple race condition issues where kernel closed out before WS was ready.
There's also not really a clear mechanism to turn off the session from inside the kernel, and started going down the route of a process interrupt- I didn't want to rely on the FE for teardown since it was leaving orphaned sessions. Then remembered if we just fail before we set up our scaffolding, then we don't have to do all this- so I reverted and just cleaned up.
Maybe we can revisit, but it was looking to be like a fair amount of more work on an already long PR. Maybe there's a simpler way but seemed marginal for just a slight code smell
akshayka
left a comment
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.
Might not be related to this change, but I created a new notebook and marimo was not added to its script metadata:
# /// script
# requires-python = ">=3.13"
# dependencies = [
# "matplotlib==3.10.8",
# ]
# ///Additionally, I am finding a bad interaction with format on save, see video:
ruff-install.mp4
for more information, see https://pre-commit.ci
Co-authored-by: Akshay Agrawal <[email protected]>
for more information, see https://pre-commit.ci
|
marimo not being added a a bug particular to this (but we should fix). Opening the motebook again and it works as expected. For the ruff error, I'm just going to throw an error and revisit if we need to. Ruff is installed in the sandbox, but the wrong sandbox! Installation path goes through the kernel |
for more information, see https://pre-commit.ci
📝 Summary
Adds
--sandboxtomarimo edit --sandbox directory/. Leveraging pep-723 where ever it can.closes #2598 rebases and supercedes #7640
marimo edit --sandbox(home mode)UX changes on kernel launch failure: