-
Notifications
You must be signed in to change notification settings - Fork 44
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
Adds a -m
flag to modal run/shell/deploy/serve
#2852
Changes from 13 commits
767485f
9560c90
6f7b929
f4c389d
16a1c08
e39df18
445d654
43c8368
a553270
c03b639
96653ff
4ce83c4
f1816f4
af587a9
8bec717
0a00df2
0a57d16
39e4ca7
bcc1b10
93cb099
09fb449
479318e
bb22f2c
96d8c97
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
from rich.console import Console | ||
from rich.markdown import Markdown | ||
|
||
from modal._utils.deprecation import deprecation_warning | ||
from modal.app import App, LocalEntrypoint | ||
from modal.cls import Cls | ||
from modal.exception import InvalidError, _CliUserExecutionError | ||
|
@@ -31,51 +32,66 @@ | |
@dataclasses.dataclass | ||
class ImportRef: | ||
file_or_module: str | ||
is_module: bool | ||
|
||
# object_path is a .-delimited path to the object to execute, or a parent from which to infer the object | ||
# e.g. | ||
# function or local_entrypoint in module scope | ||
# app in module scope [+ method name] | ||
# app [+ function/entrypoint on that app] | ||
object_path: str | ||
object_path: str = dataclasses.field(default="") | ||
|
||
|
||
def parse_import_ref(object_ref: str) -> ImportRef: | ||
def parse_import_ref(object_ref: str, is_module: bool = False) -> ImportRef: | ||
if object_ref.find("::") > 1: | ||
file_or_module, object_path = object_ref.split("::", 1) | ||
elif object_ref.find(":") > 1: | ||
raise InvalidError(f"Invalid object reference: {object_ref}. Did you mean '::' instead of ':'?") | ||
else: | ||
file_or_module, object_path = object_ref, "" | ||
|
||
return ImportRef(file_or_module, object_path) | ||
return ImportRef(file_or_module, is_module, object_path) | ||
|
||
|
||
DEFAULT_APP_NAME = "app" | ||
|
||
|
||
def import_file_or_module(file_or_module: str): | ||
def import_file_or_module(import_ref: ImportRef, base_cmd: str = ""): | ||
# TODO: Remove the base_cmd argument here once we have removed the deprecation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll still want to error out with a useful message when users invoke the entrypoint the wrong way right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, removing comment |
||
if "" not in sys.path: | ||
# When running from a CLI like `modal run` | ||
# the current working directory isn't added to sys.path | ||
# so we add it in order to make module path specification possible | ||
sys.path.insert(0, "") # "" means the current working directory | ||
|
||
if file_or_module.endswith(".py"): | ||
if not import_ref.file_or_module.endswith(".py") or import_ref.is_module: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In what situations could the first condition be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be the case when users specify a module ref but don't send along the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. It's maybe a little confusing when reading the code in this context that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, maybe it should be called |
||
if not import_ref.is_module: | ||
deprecation_warning( | ||
(2025, 2, 6), | ||
f"Using Python module paths will require using the -m flag in a future version of Modal.\n" | ||
f"Use `{base_cmd} -m {import_ref.file_or_module}` instead.", | ||
pending=True, | ||
show_source=False, | ||
) | ||
try: | ||
module = importlib.import_module(import_ref.file_or_module) | ||
except Exception as exc: | ||
raise _CliUserExecutionError(import_ref.file_or_module) from exc | ||
else: | ||
# when using a script path, that scripts directory should also be on the path as it is | ||
# with `python some/script.py` | ||
full_path = Path(file_or_module).resolve() | ||
full_path = Path(import_ref.file_or_module).resolve() | ||
if "." in full_path.name.removesuffix(".py"): | ||
raise InvalidError( | ||
f"Invalid Modal source filename: {full_path.name!r}." | ||
"\n\nSource filename cannot contain additional period characters." | ||
) | ||
sys.path.insert(0, str(full_path.parent)) | ||
|
||
module_name = inspect.getmodulename(file_or_module) | ||
module_name = inspect.getmodulename(import_ref.file_or_module) | ||
assert module_name is not None | ||
# Import the module - see https://docs.python.org/3/library/importlib.html#importing-a-source-file-directly | ||
spec = importlib.util.spec_from_file_location(module_name, file_or_module) | ||
spec = importlib.util.spec_from_file_location(module_name, import_ref.file_or_module) | ||
assert spec is not None | ||
module = importlib.util.module_from_spec(spec) | ||
sys.modules[module_name] = module | ||
|
@@ -84,11 +100,6 @@ def import_file_or_module(file_or_module: str): | |
spec.loader.exec_module(module) | ||
except Exception as exc: | ||
raise _CliUserExecutionError(str(full_path)) from exc | ||
else: | ||
try: | ||
module = importlib.import_module(file_or_module) | ||
except Exception as exc: | ||
raise _CliUserExecutionError(file_or_module) from exc | ||
|
||
return module | ||
|
||
|
@@ -227,15 +238,15 @@ def _is_accepted_type(cli_command: CLICommand) -> bool: | |
return res | ||
|
||
|
||
def import_app(app_ref: str) -> App: | ||
import_ref = parse_import_ref(app_ref) | ||
def import_app(app_ref: str, is_module: bool, base_cmd: str) -> App: | ||
import_ref = parse_import_ref(app_ref, is_module=is_module) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if there's a way to reduce the amount that we need to thread
That issues a deprecation warning when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I also wanted to achieve something like that, but otoh it would be nice to force module mode when the user specifies it (and once we make this non-optional we should force script mode when not specifying it). If the import code itself still infers the import from the file name, there are edge cases where we would make the wrong choice without the option. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One idea is to eagerly parse the app ref and thread the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hah yeah that's an annoying edge case! But yeah I guess I still feel like passing around There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but I think it's a little bit less smelly now that we at least encapsulate it in a single There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, TBF I responded here before I saw that you'd done some of refactoring, that said it does feel like properly handling the |
||
|
||
# TODO: default could be to just pick up any app regardless if it's called DEFAULT_APP_NAME | ||
# as long as there is a single app in the module? | ||
import_path = import_ref.file_or_module | ||
object_path = import_ref.object_path or DEFAULT_APP_NAME | ||
|
||
module = import_file_or_module(import_ref.file_or_module) | ||
module = import_file_or_module(import_ref, base_cmd) | ||
|
||
if "." in object_path: | ||
raise click.UsageError(f"{object_path} is not a Modal App") | ||
|
@@ -305,7 +316,7 @@ def _get_runnable_app(runnable: Runnable) -> App: | |
|
||
|
||
def import_and_filter( | ||
import_ref: ImportRef, accept_local_entrypoint=True, accept_webhook=False | ||
import_ref: ImportRef, *, base_cmd: str, accept_local_entrypoint=True, accept_webhook=False | ||
) -> tuple[Optional[Runnable], list[CLICommand]]: | ||
"""Takes a function ref string and returns a single determined "runnable" to use, and a list of all available | ||
runnables. | ||
|
@@ -321,7 +332,7 @@ def import_and_filter( | |
3. if there is a single method (within a class) that one is used | ||
""" | ||
# all commands: | ||
module = import_file_or_module(import_ref.file_or_module) | ||
module = import_file_or_module(import_ref, base_cmd) | ||
cli_commands = list_cli_commands(module) | ||
|
||
# all commands that satisfy local entrypoint/accept webhook limitations AND object path prefix | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -325,9 +325,9 @@ def get_command(self, ctx, func_ref): | |
ctx.ensure_object(dict) | ||
ctx.obj["env"] = ensure_env(ctx.params["env"]) | ||
|
||
import_ref = parse_import_ref(func_ref) | ||
import_ref = parse_import_ref(func_ref, is_module=ctx.params["module"]) | ||
runnable, all_usable_commands = import_and_filter( | ||
import_ref, accept_local_entrypoint=True, accept_webhook=False | ||
import_ref, base_cmd="modal run", accept_local_entrypoint=True, accept_webhook=False | ||
) | ||
if not runnable: | ||
help_header = ( | ||
|
@@ -369,8 +369,9 @@ def get_command(self, ctx, func_ref): | |
@click.option("-d", "--detach", is_flag=True, help="Don't stop the app if the local process dies or disconnects.") | ||
@click.option("-i", "--interactive", is_flag=True, help="Run the app in interactive mode.") | ||
@click.option("-e", "--env", help=ENV_OPTION_HELP, default=None) | ||
@click.option("-m", "--module", is_flag=True, help="use library module as a script") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It migh be helpful to align this explanation with the terminology that we use in the guide page on the two modes of App reference. In particular, we call the other mode "Script mode" so it's a little confusing to say that this flag invokes the module "as a script". Also, nit: can we match the capitalization of the other help entries? |
||
@click.pass_context | ||
def run(ctx, write_result, detach, quiet, interactive, env): | ||
def run(ctx, write_result, detach, quiet, interactive, env, module): | ||
"""Run a Modal function or local entrypoint. | ||
|
||
`FUNC_REF` should be of the format `{file or module}::{function name}`. | ||
|
@@ -386,38 +387,47 @@ def run(ctx, write_result, detach, quiet, interactive, env): | |
modal run my_app.py::hello_world | ||
``` | ||
|
||
If your module only has a single app called `app` and your app has a | ||
If your module only has a single app and your app has a | ||
single local entrypoint (or single function), you can omit the app and | ||
function parts: | ||
|
||
``` | ||
modal run my_app.py | ||
``` | ||
|
||
Instead of pointing to a file, you can also use the Python module path: | ||
Instead of pointing to a file, you can also use the Python module path, which | ||
by default will ensure that your remote functions will use the same module | ||
names as they do locally. | ||
|
||
``` | ||
modal run my_project.my_app | ||
modal run -m my_project.my_app | ||
``` | ||
""" | ||
ctx.ensure_object(dict) | ||
ctx.obj["result_path"] = write_result | ||
ctx.obj["detach"] = detach # if subcommand would be a click command... | ||
ctx.obj["show_progress"] = False if quiet else True | ||
ctx.obj["interactive"] = interactive | ||
ctx.obj["use_module"] = module | ||
|
||
|
||
def deploy( | ||
app_ref: str = typer.Argument(..., help="Path to a Python file with an app."), | ||
app_ref: str = typer.Argument(..., help="Path to a Python file with an app to deploy"), | ||
name: str = typer.Option("", help="Name of the deployment."), | ||
env: str = ENV_OPTION, | ||
stream_logs: bool = typer.Option(False, help="Stream logs from the app upon deployment."), | ||
tag: str = typer.Option("", help="Tag the deployment with a version."), | ||
module_mode: bool = typer.Option(False, "--module", "-m", help="Use a Python module path instead of a file path"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're trying to match Python, I don't think it actually supports |
||
): | ||
"""Deploy a Modal application. | ||
|
||
**Usage:** | ||
hello | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi! |
||
""" | ||
# this ensures that lookups without environment specification use the same env as specified | ||
env = ensure_env(env) | ||
|
||
app = import_app(app_ref) | ||
app = import_app(app_ref, module_mode, base_cmd="modal deploy") | ||
|
||
if name is None: | ||
name = app.name | ||
|
@@ -433,6 +443,7 @@ def serve( | |
app_ref: str = typer.Argument(..., help="Path to a Python file with an app."), | ||
timeout: Optional[float] = None, | ||
env: str = ENV_OPTION, | ||
is_module: bool = typer.Option(False, "--module", "-m", help="Use a Python module path instead of a file path"), | ||
): | ||
"""Run a web endpoint(s) associated with a Modal app and hot-reload code. | ||
|
||
|
@@ -444,12 +455,12 @@ def serve( | |
""" | ||
env = ensure_env(env) | ||
|
||
app = import_app(app_ref) | ||
app = import_app(app_ref, is_module, base_cmd="modal serve") | ||
if app.description is None: | ||
app.set_description(_get_clean_app_description(app_ref)) | ||
|
||
with enable_output(): | ||
with serve_app(app, app_ref, environment_name=env): | ||
with serve_app(app, app_ref, is_module=is_module, environment_name=env): | ||
if timeout is None: | ||
timeout = config["serve_timeout"] | ||
if timeout is None: | ||
|
@@ -564,7 +575,7 @@ def shell( | |
|
||
import_ref = parse_import_ref(container_or_function) | ||
runnable, all_usable_commands = import_and_filter( | ||
import_ref, accept_local_entrypoint=False, accept_webhook=True | ||
import_ref, base_cmd="modal shell", accept_local_entrypoint=False, accept_webhook=True | ||
) | ||
if not runnable: | ||
help_header = ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,23 +26,27 @@ | |
_App = TypeVar("_App") | ||
|
||
|
||
def _run_serve(app_ref: str, existing_app_id: str, is_ready: Event, environment_name: str, show_progress: bool): | ||
def _run_serve( | ||
app_ref: str, is_module: bool, existing_app_id: str, is_ready: Event, environment_name: str, show_progress: bool | ||
): | ||
# subprocess entrypoint | ||
_app = import_app(app_ref) | ||
_app = import_app(app_ref, is_module=is_module, base_cmd="modal serve") | ||
blocking_app = synchronizer._translate_out(_app) | ||
|
||
with enable_output(show_progress=show_progress): | ||
serve_update(blocking_app, existing_app_id, is_ready, environment_name) | ||
|
||
|
||
async def _restart_serve( | ||
app_ref: str, existing_app_id: str, environment_name: str, timeout: float = 5.0 | ||
app_ref: str, *, is_module: bool, existing_app_id: str, environment_name: str, timeout: float = 5.0 | ||
) -> SpawnProcess: | ||
ctx = multiprocessing.get_context("spawn") # Needed to reload the interpreter | ||
is_ready = ctx.Event() | ||
output_mgr = OutputManager.get() | ||
show_progress = output_mgr is not None | ||
p = ctx.Process(target=_run_serve, args=(app_ref, existing_app_id, is_ready, environment_name, show_progress)) | ||
p = ctx.Process( | ||
target=_run_serve, args=(app_ref, is_module, existing_app_id, is_ready, environment_name, show_progress) | ||
) | ||
p.start() | ||
await asyncify(is_ready.wait)(timeout) | ||
# TODO(erikbern): we don't fail if the above times out, but that's somewhat intentional, since | ||
|
@@ -69,6 +73,8 @@ async def _terminate(proc: Optional[SpawnProcess], timeout: float = 5.0): | |
|
||
async def _run_watch_loop( | ||
app_ref: str, | ||
*, | ||
is_module: bool, | ||
app_id: str, | ||
watcher: AsyncGenerator[set[str], None], | ||
environment_name: str, | ||
|
@@ -88,7 +94,9 @@ async def _run_watch_loop( | |
async for trigger_files in watcher: | ||
logger.debug(f"The following files triggered an app update: {', '.join(trigger_files)}") | ||
await _terminate(curr_proc) | ||
curr_proc = await _restart_serve(app_ref, existing_app_id=app_id, environment_name=environment_name) | ||
curr_proc = await _restart_serve( | ||
app_ref, is_module=is_module, existing_app_id=app_id, environment_name=environment_name | ||
) | ||
finally: | ||
await _terminate(curr_proc) | ||
|
||
|
@@ -97,6 +105,8 @@ async def _run_watch_loop( | |
async def _serve_app( | ||
app: "_App", | ||
app_ref: str, | ||
*, | ||
is_module: bool, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we consider It bears on whether we want "internal" parameters like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should make it public since it's quite clunky at the moment in having to specify the "app_ref" which is a very cli thing imo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I guess that makes sense. Is there a reason it's written around the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's because of how the serve command works when files change ("auto reloading") - it starts a new interpreter that should reload everything based on the original command, so it needs the app ref for that reason :( |
||
_watcher: Optional[AsyncGenerator[set[str], None]] = None, # for testing | ||
environment_name: Optional[str] = None, | ||
) -> AsyncGenerator["_App", None]: | ||
|
@@ -112,7 +122,11 @@ async def _serve_app( | |
mounts_to_watch = app._get_watch_mounts() | ||
watcher = watch(mounts_to_watch) | ||
async with TaskContext(grace=0.1) as tc: | ||
tc.create_task(_run_watch_loop(app_ref, app.app_id, watcher, environment_name)) | ||
tc.create_task( | ||
_run_watch_loop( | ||
app_ref, is_module=is_module, app_id=app.app_id, watcher=watcher, environment_name=environment_name | ||
) | ||
) | ||
yield app | ||
|
||
|
||
|
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.
Should we be introducing an error here (or somewhere else?) for when you do
modal run -m foo.py
wherefoo.py
is a local script, andfoo.py
is not a path to a module? Or will that trigger an error later on?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.
That would trigger an error later when we try to import the module since is_module=True
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.
Would it make sense to consolidate the code that does the validation in one place?
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.
Not sure what you mean - all of the module/script validation is in the
import_file_or_module
function here."Validation" of the import itself being ok (file existing, having valid python syntax etc) is delegated to importlib.import_module which is also called in this function, so it should all be contained here afaik?
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 meant checking all the various ways that the spelling of the import ref and the use or non-use of the
-m
flag can disagree. But maybe you can't determine that a path without.py
isn't a valid module until you actually try to import it?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 the problem is that i just didn't read far enough down to understand when "trigger an error later" actually applies 😁