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

Client-side app-less sandboxes #1957

Merged
merged 6 commits into from
Jul 3, 2024
Merged

Client-side app-less sandboxes #1957

merged 6 commits into from
Jul 3, 2024

Conversation

erikbern
Copy link
Contributor

@erikbern erikbern commented Jul 2, 2024

Implements the basics of this. Demo:

>>> from modal import Sandbox
>>> s = Sandbox.create("/bin/bash", "-c", "echo hello world")
>>> for line in s.stdout:
...     print(line)
... 
hello world

>>> s.poll()
0

TODO for this PR:

  • Support apps optionally
  • Add tests
  • Add tests for custom image and secrets

Will do in subsequent PRs:

  • Update docstrings to use new syntax
  • Add deprecation warnings for old syntax

@erikbern erikbern force-pushed the erikbern/app-less-sandboxes branch from b16f498 to edb9b13 Compare July 2, 2024 23:03
@erikbern erikbern changed the title Client-side app-less sandboxes (WIP) Client-side app-less sandboxes Jul 2, 2024
Copy link
Contributor

@aksh-at aksh-at left a comment

Choose a reason for hiding this comment

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

LGTM! Should we add a deprecation message for the old code path?

_experimental_scheduler_placement=_experimental_scheduler_placement,
)
app_id: Optional[str] = app.app_id if app else None
resolver = Resolver(client, environment_name=environment_name, app_id=app_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to resolve all deps without an app ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah should be fine – it's just images that need it and i fixed that in the backend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually it's possible secrets need it too, let me check

] = {}, # Mount points for Modal Volumes and CloudBucketMounts
pty_info: Optional[api_pb2.PTYInfo] = None,
_allow_background_volume_commits: Optional[bool] = None,
_experimental_scheduler_placement: Optional[
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah looks like we still need to add the non-experimental scheduler param, now that it's out. Outside the scope of this PR.

@@ -323,6 +330,63 @@ async def _load(self: _Sandbox, resolver: Resolver, _existing_object_id: Optiona

return _Sandbox._from_loader(_load, "Sandbox()", deps=_deps)

@staticmethod
async def create(
*entrypoint_args: str,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make the following args keyword-only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually believe they are keyword-only because of the * automatically (i just learned something new about python!)

Copy link
Contributor

@aksh-at aksh-at Jul 3, 2024

Choose a reason for hiding this comment

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

oh cool, makes sense! (any args would be spread into entrypoint_args so it has to be this way)

@erikbern
Copy link
Contributor Author

erikbern commented Jul 3, 2024

Ok slightly bad news: in order for this to work, we need to support app-less secrets and mounts as well.

The good news is that this is something I've been hoping to add support for anyway for a while. But it will take a bit of time to figure out how to do it, unfortunately.

@erikbern
Copy link
Contributor Author

erikbern commented Jul 3, 2024

LGTM! Should we add a deprecation message for the old code path?

Was planning to do that in a subsequent PR (I want to update some integration tests and docs first)

@erikbern
Copy link
Contributor Author

erikbern commented Jul 3, 2024

This works now, but lack of output is a bit confusing with custom images.

>>> image = Image.debian_slim().pip_install("numpy").run_commands("sleep 30")
>>> s = Sandbox.create("/bin/bash", "-c", "echo hello world", image=image)  # This blocks for >30s
>>> s.poll()
0
>>> [line for line in s.stdout]
['hello world\n']

@aksh-at
Copy link
Contributor

aksh-at commented Jul 3, 2024

This works now, but lack of output is a bit confusing with custom images.

Ah yeah, I think we used to be able to see output in the app logs, but decoupling makes that difficult 🤔

@erikbern
Copy link
Contributor Author

erikbern commented Jul 3, 2024

Ah yeah, I think we used to be able to see output in the app logs, but decoupling makes that difficult 🤔

I think we can fix this in a different way. I've had it on my backlog for a long time to break out the output management into a singleton (separate from the resolver) that you can activate using a context manager (with the idea that all CLI commands enable this automatically). Something like

with modal.enable_logs():
    ...

I don't love singletons but in this case I think it's fine since stdout is already a singleton

@erikbern
Copy link
Contributor Author

erikbern commented Jul 3, 2024

ok this FINALLY works

>>> from modal import Image, Secret, Mount, Sandbox
>>> secret = Secret.from_dict({"FOO": "bar"})
>>> s = Sandbox.create("/bin/bash", "-c", "echo $FOO", secrets=[secret], environment_name="main")
>>> s.poll()
0
>>> [line for line in s.stdout]
['bar\n']

@erikbern erikbern merged commit 42e09a0 into main Jul 3, 2024
20 checks passed
@erikbern erikbern deleted the erikbern/app-less-sandboxes branch July 3, 2024 23:12
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.

2 participants