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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions modal/_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

from ._utils.async_utils import TaskContext
from .client import _Client
from .exception import ExecutionError, NotFoundError
from .exception import NotFoundError

if TYPE_CHECKING:
from rich.tree import Tree
Expand Down Expand Up @@ -72,9 +72,7 @@ def __init__(
self._deduplication_cache = {}

@property
def app_id(self) -> str:
if self._app_id is None:
raise ExecutionError("Resolver has no app")
def app_id(self) -> Optional[str]:
return self._app_id

@property
Expand Down
20 changes: 7 additions & 13 deletions modal/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

from ._ipython import is_notebook
from ._output import OutputManager
from ._resolver import Resolver
from ._utils.async_utils import synchronize_api
from ._utils.function_utils import FunctionInfo
from ._utils.mount_utils import validate_volumes
Expand Down Expand Up @@ -775,11 +774,7 @@ async def spawn_sandbox(

Refer to the [docs](/docs/guide/sandbox) on how to spawn and use sandboxes.
"""
if self._running_app:
app_id = self._running_app.app_id
environment_name = self._running_app.environment_name
client = self._client
else:
if not self._running_app:
raise InvalidError("`app.spawn_sandbox` requires a running app.")

if _allow_background_volume_commits is False:
Expand All @@ -790,10 +785,10 @@ async def spawn_sandbox(
elif _allow_background_volume_commits is None:
_allow_background_volume_commits = True

# TODO(erikbern): pulling a lot of app internals here, let's clean up shortly
resolver = Resolver(client, environment_name=environment_name, app_id=app_id)
obj = _Sandbox._new(
entrypoint_args,
return await _Sandbox.create(
*entrypoint_args,
app=self,
environment_name=self._running_app.environment_name,
image=image or _default_image,
mounts=mounts,
secrets=secrets,
Expand All @@ -807,12 +802,11 @@ async def spawn_sandbox(
network_file_systems=network_file_systems,
block_network=block_network,
volumes=volumes,
allow_background_volume_commits=_allow_background_volume_commits,
pty_info=pty_info,
_allow_background_volume_commits=_allow_background_volume_commits,
_experimental_scheduler_placement=_experimental_scheduler_placement,
client=self._client,
)
await resolver.load(obj)
return obj

def include(self, /, other_app: "_App"):
"""Include another app's objects in this one.
Expand Down
2 changes: 2 additions & 0 deletions modal/functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,7 @@ async def _preload(self: _Function, resolver: Resolver, existing_object_id: Opti
else:
function_type = api_pb2.Function.FUNCTION_TYPE_FUNCTION

assert resolver.app_id
req = api_pb2.FunctionPrecreateRequest(
app_id=resolver.app_id,
function_name=info.function_name,
Expand Down Expand Up @@ -810,6 +811,7 @@ async def _load(self: _Function, resolver: Resolver, existing_object_id: Optiona
scheduler_placement=scheduler_placement.proto if scheduler_placement else None,
is_class=info.is_service_class(),
)
assert resolver.app_id
request = api_pb2.FunctionCreateRequest(
app_id=resolver.app_id,
function=function_definition,
Expand Down
9 changes: 8 additions & 1 deletion modal/mount.py
Original file line number Diff line number Diff line change
Expand Up @@ -499,12 +499,19 @@ async def _put_file(file_spec: FileUploadSpec) -> api_pb2.MountFile:
object_creation_type=api_pb2.OBJECT_CREATION_TYPE_CREATE_FAIL_IF_EXISTS,
files=files,
)
else:
elif resolver.app_id is not None:
req = api_pb2.MountGetOrCreateRequest(
object_creation_type=api_pb2.OBJECT_CREATION_TYPE_ANONYMOUS_OWNED_BY_APP,
files=files,
app_id=resolver.app_id,
)
else:
req = api_pb2.MountGetOrCreateRequest(
object_creation_type=api_pb2.OBJECT_CREATION_TYPE_EPHEMERAL,
files=files,
environment_name=resolver.environment_name,
)

resp = await retry_transient_errors(resolver.client.stub.MountGetOrCreate, req, base_delay=1)
status_row.finish(f"Created mount {message_label}")

Expand Down
70 changes: 67 additions & 3 deletions modal/sandbox.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Copyright Modal Labs 2022
import asyncio
import os
from typing import AsyncIterator, Dict, List, Optional, Sequence, Tuple, Union
from typing import TYPE_CHECKING, AsyncIterator, Dict, List, Optional, Sequence, Tuple, Union

from google.protobuf.message import Message
from grpclib.exceptions import GRPCError, StreamTerminatedError
Expand All @@ -27,6 +27,12 @@
from .scheduler_placement import SchedulerPlacement
from .secret import _Secret

_default_image: _Image = _Image.debian_slim()


if TYPE_CHECKING:
import modal.app


class _LogsReader:
"""Provides an interface to buffer and fetch logs from a sandbox stream (`stdout` or `stderr`).
Expand Down Expand Up @@ -248,8 +254,8 @@ def _new(
network_file_systems: Dict[Union[str, os.PathLike], _NetworkFileSystem] = {},
block_network: bool = False,
volumes: Dict[Union[str, os.PathLike], Union[_Volume, _CloudBucketMount]] = {},
allow_background_volume_commits: Optional[bool] = None,
pty_info: Optional[api_pb2.PTYInfo] = None,
_allow_background_volume_commits: Optional[bool] = None,
_experimental_scheduler_placement: Optional[SchedulerPlacement] = None,
) -> "_Sandbox":
"""mdmd:hidden"""
Expand Down Expand Up @@ -289,7 +295,7 @@ async def _load(self: _Sandbox, resolver: Resolver, _existing_object_id: Optiona
api_pb2.VolumeMount(
mount_path=path,
volume_id=volume.object_id,
allow_background_commits=allow_background_volume_commits,
allow_background_commits=_allow_background_volume_commits,
)
for path, volume in validated_volumes
]
Expand All @@ -315,6 +321,7 @@ async def _load(self: _Sandbox, resolver: Resolver, _existing_object_id: Optiona
scheduler_placement=scheduler_placement.proto if scheduler_placement else None,
)

# Note - `resolver.app_id` will be `None` for app-less sandboxes
create_req = api_pb2.SandboxCreateRequest(app_id=resolver.app_id, definition=definition)
create_resp = await retry_transient_errors(resolver.client.stub.SandboxCreate, create_req)

Expand All @@ -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)

app: Optional["modal.app._App"] = None, # Optionally associate the sandbox with an app
environment_name: Optional[str] = None, # Optionally override the default environment
image: Optional[_Image] = None, # The image to run as the container for the sandbox.
mounts: Sequence[_Mount] = (), # Mounts to attach to the sandbox.
secrets: Sequence[_Secret] = (), # Environment variables to inject into the sandbox.
network_file_systems: Dict[Union[str, os.PathLike], _NetworkFileSystem] = {},
timeout: Optional[int] = None, # Maximum execution time of the sandbox in seconds.
workdir: Optional[str] = None, # Working directory of the sandbox.
gpu: GPU_T = None,
cloud: Optional[str] = None,
region: Optional[Union[str, Sequence[str]]] = None, # Region or regions to run the sandbox on.
cpu: Optional[float] = None, # How many CPU cores to request. This is a soft limit.
# Specify, in MiB, a memory request which is the minimum memory required.
# Or, pass (request, limit) to additionally specify a hard limit in MiB.
memory: Optional[Union[int, Tuple[int, int]]] = None,
block_network: bool = False, # Whether to block network access
volumes: Dict[
Union[str, os.PathLike], Union[_Volume, _CloudBucketMount]
] = {}, # 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.

SchedulerPlacement
] = None, # Experimental controls over fine-grained scheduling (alpha).
client: Optional[_Client] = None,
) -> "_Sandbox":
if client is None:
client = await _Client.from_env()

# TODO(erikbern): Get rid of the `_new` method and create an already-hydrated object
obj = _Sandbox._new(
entrypoint_args,
image=image or _default_image,
mounts=mounts,
secrets=secrets,
timeout=timeout,
workdir=workdir,
gpu=gpu,
cloud=cloud,
region=region,
cpu=cpu,
memory=memory,
network_file_systems=network_file_systems,
block_network=block_network,
volumes=volumes,
pty_info=pty_info,
_allow_background_volume_commits=_allow_background_volume_commits,
_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

await resolver.load(obj)
return obj

def _hydrate_metadata(self, handle_metadata: Optional[Message]):
self._stdout = LogsReader(api_pb2.FILE_DESCRIPTOR_STDOUT, self.object_id, self._client)
self._stderr = LogsReader(api_pb2.FILE_DESCRIPTOR_STDERR, self.object_id, self._client)
Expand Down
8 changes: 7 additions & 1 deletion modal/secret.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,16 @@ def run():
raise InvalidError(ENV_DICT_WRONG_TYPE_ERR)

async def _load(self: _Secret, resolver: Resolver, existing_object_id: Optional[str]):
if resolver.app_id is not None:
object_creation_type = api_pb2.OBJECT_CREATION_TYPE_ANONYMOUS_OWNED_BY_APP
else:
object_creation_type = api_pb2.OBJECT_CREATION_TYPE_EPHEMERAL

req = api_pb2.SecretGetOrCreateRequest(
object_creation_type=api_pb2.OBJECT_CREATION_TYPE_ANONYMOUS_OWNED_BY_APP,
object_creation_type=object_creation_type,
env_dict=env_dict_filtered,
app_id=resolver.app_id,
environment_name=resolver.environment_name,
)
try:
resp = await resolver.client.stub.SecretGetOrCreate(req)
Expand Down
6 changes: 6 additions & 0 deletions test/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -879,6 +879,9 @@ async def MountGetOrCreate(self, stream):
elif request.object_creation_type == api_pb2.OBJECT_CREATION_TYPE_ANONYMOUS_OWNED_BY_APP:
self.n_mounts += 1
mount_id = f"mo-{self.n_mounts}"
elif request.object_creation_type == api_pb2.OBJECT_CREATION_TYPE_EPHEMERAL:
self.n_mounts += 1
mount_id = f"mo-{self.n_mounts}"

else:
raise Exception("unsupported creation type")
Expand Down Expand Up @@ -1085,6 +1088,9 @@ async def SecretGetOrCreate(self, stream):
if request.object_creation_type == api_pb2.OBJECT_CREATION_TYPE_ANONYMOUS_OWNED_BY_APP:
secret_id = "st-" + str(len(self.secrets))
self.secrets[secret_id] = request.env_dict
elif request.object_creation_type == api_pb2.OBJECT_CREATION_TYPE_EPHEMERAL:
secret_id = "st-" + str(len(self.secrets))
self.secrets[secret_id] = request.env_dict
elif request.object_creation_type == api_pb2.OBJECT_CREATION_TYPE_CREATE_FAIL_IF_EXISTS:
if k in self.deployed_secrets:
raise GRPCError(Status.ALREADY_EXISTS, f"Secret {k} already exists")
Expand Down
20 changes: 20 additions & 0 deletions test/sandbox_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,3 +189,23 @@ async def test_sandbox_async_for(client, servicer):
# test reading after receiving EOF
assert sb.stdout.read() == ""
assert sb.stderr.read() == ""


@skip_non_linux
def test_appless_sandbox(client, servicer):
# Appless dependencies
image = Image.debian_slim().pip_install("xyz")
secret = Secret.from_dict({"FOO": "bar"})
mount = Mount.from_local_file(__file__, "/xyz")

# Create sandbox
sb = Sandbox.create(
"bash", "-c", "echo bye >&2 && echo hi", image=image, secrets=[secret], mounts=[mount], client=client
)
assert sb.stdout.read() == "hi\n"
assert sb.stderr.read() == "bye\n"

# Make sure ids got assigned
assert image.object_id == "im-2"
assert secret.object_id == "st-0"
assert mount.object_id == "mo-1"
Loading