Adds a `-m` flag to modal run/shell/deploy/serve #2852

freider · 2025-02-10T12:06:13Z

Warn if using cli with "module syntax" without -m, so we can enforce it later

Note that unlike python we are using -m as a flag rather than an option marker, so you can have it at any point in your argument list, not just last. I think it also makes sense since our func_ref can have more parts than the module (with the :: function qualifier)

Check these boxes or delete any item (or this section) if not relevant for this PR.

Client+Server: this change is compatible with old servers
Client forward compatibility: this change ensures client can accept data intended for later versions of itself

Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.

Changelog

Introduces an -m flag to modal run, modal shell, modal serve and modal deploy, which indicates that the modal app/function file is specified using python "module syntax" rather than a file path. In the future this will be a required flag when using module syntax.

Old syntax:
```
modal run my_package/modal_main.py
modal run my_package.modal_main
```
New syntax (note the -m on the second line):
```
modal run my_package/modal_main.py
modal run -m my_package.modal_main
```

freider · 2025-02-10T15:22:37Z

test/cli_test.py

+    servicer, set_env_client, supports_dir, recwarn, monkeypatch, disable_auto_mount
+):
+    monkeypatch.chdir(supports_dir)
+    _run(["run", "-m", f"{app_module}::foo"])


Can't use "expected stderr" since this is actually running inside the same process and pytest captures warnings separately from stderr...

freider · 2025-02-10T15:23:10Z

test/supports/app_run_tests/webhook.py

@@ -1,7 +1,7 @@
 # Copyright Modal Labs 2022
 from modal import App, web_endpoint

-app = App()
+app = App(include_source=True)


random automount fix...

mwaskom

Sorry for the slow review here — but had a few thoughts!

mwaskom · 2025-02-14T22:13:37Z

modal/cli/run.py

@@ -368,8 +368,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")


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?

mwaskom · 2025-02-14T22:17:08Z

modal/cli/run.py

+    """Deploy a Modal application.
+
+    **Usage:**
+    hello


mwaskom · 2025-02-14T22:17:50Z

modal/cli/run.py

    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"),


If we're trying to match Python, I don't think it actually supports --module, just -m.

mwaskom · 2025-02-14T22:23:30Z

modal/cli/import_refs.py

-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)


I wonder if there's a way to reduce the amount that we need to thread is_module down through the various functions involved here. Do we actually need to hold the is_module state? It feels like we might just need a validation function that we can use in the CLI entrypoints like

def validate_app_ref(app_ref: str, is_module: bool): if uses_module_spelling(app_ref) and not is_module: # Deprecation warning elif not uses_module_spelling(app_ref) and is_module: # Usage error

That issues a deprecation warning when app_ref does not use the module spelling but is_module is True (or vice versa)

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.
In most cases we can infer the import mode from file name, but there are edge cases like -m foo.py where py is actually a module named py.py, in which case we'd have to pass along the option to import it correctly

One idea is to eagerly parse the app ref and thread the ImportRef object through everywhere, then at least its a single object that has all of the attributes related to what should be imported and how

there are edge cases like -m foo.py where py is actually a module named py.py

Hah yeah that's an annoying edge case!

But yeah I guess I still feel like passing around app_ref, is_module is a smell.

I agree, but I think it's a little bit less smelly now that we at least encapsulate it in a single ImportRef that defines "what/how to import" something. One refactoring option would be to "pull" the import logic up to the CLI functions themselves instead of delegating to import_and_filter etc, and then pass around a (module, object_path) instead? It would be a few lines of repeated code across the different CLI methods, but perhaps a tad less smelly :)

Yeah, TBF I responded here before I saw that you'd done some of refactoring, that said it does feel like properly handling the -m flag is purely about the CLI and should probably be handled within the CLI itself.

mwaskom · 2025-02-14T22:26:00Z

modal/serving.py

@@ -97,6 +105,8 @@ async def _run_watch_loop(
 async def _serve_app(
    app: "_App",
    app_ref: str,
+    *,
+    is_module: bool,


Do we consider serve_app to be public API? It looks a lot like the App.run and modal.runner.deploy_app API analogues for the core CLI, so it feels like we should expose it just for completeness sake. OTOH it's a little less obvious how it would be useful to use it programmatically.

It bears on whether we want "internal" parameters like is_module to appear in the signature. But see comment above about whether we can reduce the need to be passing is_module around.

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

Yeah I guess that makes sense. Is there a reason it's written around the app_ref and not the App object itself?

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 :(

freider · 2025-02-17T14:18:03Z

@mwaskom I changed the code to do an early parse_import_ref and then simply pass along the ImportRef object everywhere. This allows us to do both the validation and actual import logic using the is_module flag, regardless at which depth the import is actually performed, which is a bit cleaner

mwaskom

In terms of aligning with Python semantics, do we need to do anything special to look in a __main.py__ module when running in Module mode and the provided path goes only to a package object? Not sure what makes the most sense there, but something to think about in terms of eliminating subtle differences with Python behavior.

mwaskom · 2025-02-18T16:46:38Z

modal/cli/import_refs.py

    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:


In what situations could the first condition be True and the second False? (i.e., a path that doesn't end in .py but is not a module?)

That would be the case when users specify a module ref but don't send along the -m flag - i.e the case where we issue the warning below

I see. It's maybe a little confusing when reading the code in this context that import_ref.is_module is data about how the user invoked Modal, not a statement about what sort of path we actually have.

Yeah, maybe it should be called load_as_module or use_module_mode?

mwaskom · 2025-02-18T16:47:18Z

modal/cli/import_refs.py



 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


We'll still want to error out with a useful message when users invoke the entrypoint the wrong way right?

Good point, removing comment

mwaskom · 2025-02-18T16:50:46Z

modal/cli/import_refs.py



 DEFAULT_APP_NAME = "app"


-def import_file_or_module(file_or_module: str):
+def import_file_or_module(import_ref: ImportRef, base_cmd: str = ""):


Should we be introducing an error here (or somewhere else?) for when you do modal run -m foo.py where foo.py is a local script, and foo.py is not a path to a module? Or will that trigger an error later on?

That would trigger an error later when we try to import the module since is_module=True

Would it make sense to consolidate the code that does the validation in one place?

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?

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?

Oh maybe the problem is that i just didn't read far enough down to understand when "trigger an error later" actually applies 😁

mwaskom · 2025-02-18T16:55:43Z

modal/cli/run.py

@@ -368,8 +375,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", is_flag=True, help="Use a Python module path instead of a file path")


Should the verb here be something like "Interpret"? Since it's just going to use whatever you give it, but we're trying to parameterize how it uses it.

freider · 2025-02-20T12:25:48Z

@prbot approve

modal-pr-review-automation · 2025-02-20T12:25:55Z

Please request a reviewer to follow up with a post-merge review.

freider · 2025-02-20T12:27:52Z

Merging now so we get the deprecation in, happy to do a followup refactor if we feel like the remaining smell is too bad :)

freider · 2025-02-20T12:28:06Z

@prbot approve

modal-pr-review-automation

Approved 👍. @mwaskom will follow-up review this.

mwaskom

LGTM. We probably have a bunch of suggested command lines in the guide / examples that we need to update? In most cases I don't think we actually exercise the code in tests so it won't warn, and it will be a little annoying to find :/

freider added 13 commits February 6, 2025 14:02

Parameterize test

767485f

Clean up test some more

9560c90

Adding failing test

6f7b929

test case with -m

f4c389d

Move deploy help into docstring

16a1c08

Fix modal deploy

e39df18

Merge remote-tracking branch 'origin/main' into freider/dash-m

445d654

Stuff

43c8368

Fix tests

a553270

Fix some automount warnings

c03b639

Serve fixes

96653ff

wackamole

4ce83c4

more

f1816f4

freider commented Feb 10, 2025

View reviewed changes

freider requested a review from mwaskom February 10, 2025 15:24

freider commented Feb 11, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into freider/dash-m

af587a9

freider force-pushed the freider/dash-m branch from 62591d0 to af587a9 Compare February 13, 2025 10:17

mwaskom reviewed Feb 14, 2025

View reviewed changes

freider added 6 commits February 17, 2025 13:19

Refactor

8bec717

Blarh

0a00df2

Michael comments

0a57d16

Merge remote-tracking branch 'origin/main' into freider/dash-m

39e4ca7

fixes

bcc1b10

more fix

93cb099

freider requested a review from mwaskom February 17, 2025 14:16

Merge remote-tracking branch 'origin/main' into freider/dash-m

09fb449

mwaskom reviewed Feb 18, 2025

View reviewed changes

Small updates

479318e

freider and others added 2 commits February 19, 2025 15:18

Rename is_module to use_module_mode

bb22f2c

Merge branch 'main' into freider/dash-m

96d8c97

freider requested a review from mwaskom February 20, 2025 12:28

modal-pr-review-automation bot approved these changes Feb 20, 2025

View reviewed changes

freider merged commit 0fe73d2 into main Feb 20, 2025
24 checks passed

freider deleted the freider/dash-m branch February 20, 2025 12:28

mwaskom reviewed Feb 20, 2025

View reviewed changes

Adds a -m flag to modal run/shell/deploy/serve #2852

Adds a -m flag to modal run/shell/deploy/serve #2852

Conversation

freider commented Feb 10, 2025 • edited Loading

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaskom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freider commented Feb 17, 2025

mwaskom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freider commented Feb 20, 2025

modal-pr-review-automation bot commented Feb 20, 2025

freider commented Feb 20, 2025

freider commented Feb 20, 2025

modal-pr-review-automation bot left a comment

Choose a reason for hiding this comment

mwaskom left a comment

Choose a reason for hiding this comment

Adds a `-m` flag to modal run/shell/deploy/serve #2852

Adds a `-m` flag to modal run/shell/deploy/serve #2852

freider commented Feb 10, 2025 •

edited

Loading