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

modal shell takes container IDs now #2336

Merged
merged 2 commits into from
Oct 14, 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
46 changes: 28 additions & 18 deletions modal/cli/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -331,49 +331,46 @@ def serve(


def shell(
func_ref: Optional[str] = typer.Argument(
container_or_function: Optional[str] = typer.Argument(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, but one tricky thing about container_or_function is that they're not equivalent — the "container" mode is a container_id but while we do have a function_id concept, that isn't what you would pass here 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that makes sense! It's only internal though since the metavar in the help is REF so maybe fine.

default=None,
help=(
"Path to a Python file with an App or Modal function with container parameters."
" Can also include a function specifier, like `module.py::func`, when the file defines multiple functions."
"ID of running container, or path to a Python file containing a Modal App."
" Can also include a function specifier, like `module.py::func`, if the file defines multiple functions."
),
metavar="FUNC_REF",
metavar="REF",
),
cmd: str = typer.Option(default="/bin/bash", help="Command to run inside the Modal image."),
env: str = ENV_OPTION,
image: Optional[str] = typer.Option(
default=None, help="Container image tag for inside the shell (if not using FUNC_REF)."
default=None, help="Container image tag for inside the shell (if not using REF)."
),
add_python: Optional[str] = typer.Option(default=None, help="Add Python to the image (if not using FUNC_REF)."),
add_python: Optional[str] = typer.Option(default=None, help="Add Python to the image (if not using REF)."),
volume: Optional[typing.List[str]] = typer.Option(
default=None,
help=(
"Name of a `modal.Volume` to mount inside the shell at `/mnt/{name}` (if not using FUNC_REF)."
"Name of a `modal.Volume` to mount inside the shell at `/mnt/{name}` (if not using REF)."
" Can be used multiple times."
),
),
cpu: Optional[int] = typer.Option(
default=None, help="Number of CPUs to allocate to the shell (if not using FUNC_REF)."
),
cpu: Optional[int] = typer.Option(default=None, help="Number of CPUs to allocate to the shell (if not using REF)."),
memory: Optional[int] = typer.Option(
default=None, help="Memory to allocate for the shell, in MiB (if not using FUNC_REF)."
default=None, help="Memory to allocate for the shell, in MiB (if not using REF)."
),
gpu: Optional[str] = typer.Option(
default=None,
help="GPUs to request for the shell, if any. Examples are `any`, `a10g`, `a100:4` (if not using FUNC_REF).",
help="GPUs to request for the shell, if any. Examples are `any`, `a10g`, `a100:4` (if not using REF).",
),
cloud: Optional[str] = typer.Option(
default=None,
help=(
"Cloud provider to run the shell on. "
"Possible values are `aws`, `gcp`, `oci`, `auto` (if not using FUNC_REF)."
"Cloud provider to run the shell on. " "Possible values are `aws`, `gcp`, `oci`, `auto` (if not using REF)."
),
),
region: Optional[str] = typer.Option(
default=None,
help=(
"Region(s) to run the shell on. "
"Can be a single region or a comma-separated list to choose from (if not using FUNC_REF)."
"Region(s) to run the container on. "
"Can be a single region or a comma-separated list to choose from (if not using REF)."
),
),
):
Expand Down Expand Up @@ -410,8 +407,21 @@ def shell(

app = App("modal shell")

if func_ref is not None:
function = import_function(func_ref, accept_local_entrypoint=False, accept_webhook=True, base_cmd="modal shell")
if container_or_function is not None:
# `modal shell` with a container ID is a special case, alias for `modal container exec`.
if (
container_or_function.startswith("ta-")
and len(container_or_function[3:]) > 0
and container_or_function[3:].isalnum()
):
from .container import exec

exec(container_id=container_or_function, command=shlex.split(cmd), pty=True)
return

function = import_function(
container_or_function, accept_local_entrypoint=False, accept_webhook=True, base_cmd="modal shell"
)
assert isinstance(function, Function)
function_spec: _FunctionSpec = function.spec
start_shell = partial(
Expand Down
11 changes: 0 additions & 11 deletions modal/serving.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# Copyright Modal Labs 2023
import multiprocessing
import platform
import sys
from multiprocessing.context import SpawnProcess
from multiprocessing.synchronize import Event
from typing import TYPE_CHECKING, AsyncGenerator, Optional, Set, TypeVar
Expand Down Expand Up @@ -88,16 +87,6 @@ async def _run_watch_loop(
await _terminate(curr_proc)


def _get_clean_app_description(app_ref: str) -> str:
# If possible, consider the 'ref' argument the start of the app's args. Everything
# before it Modal CLI cruft (eg. `modal serve --timeout 1.0`).
try:
func_ref_arg_idx = sys.argv.index(app_ref)
return " ".join(sys.argv[func_ref_arg_idx:])
except ValueError:
return " ".join(sys.argv)


@asynccontextmanager
async def _serve_app(
app: "_App",
Expand Down
Loading