From 767485fb024de34b87580116778651594a6b6162 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Thu, 6 Feb 2025 14:02:03 +0100 Subject: [PATCH 01/19] Parameterize test --- modal/cli/run.py | 12 ++++++++---- test/cli_test.py | 32 +++++++++++++++++++++++--------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/modal/cli/run.py b/modal/cli/run.py index 40e908ced..5a597cc2a 100644 --- a/modal/cli/run.py +++ b/modal/cli/run.py @@ -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") @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,7 +387,7 @@ 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: @@ -394,10 +395,12 @@ def run(ctx, write_result, detach, quiet, interactive, env): 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) @@ -405,6 +408,7 @@ def run(ctx, write_result, detach, quiet, interactive, env): 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( diff --git a/test/cli_test.py b/test/cli_test.py index e3d3a60f1..441a737cf 100644 --- a/test/cli_test.py +++ b/test/cli_test.py @@ -129,16 +129,30 @@ def test_app_setup(servicer, set_env_client, server_url_env, modal_config): assert "_test" in toml.load(config_file_path) -def test_run(servicer, set_env_client, test_dir): - app_file = test_dir / "supports" / "app_run_tests" / "default_app.py" +supports_dir = Path(__file__).parent / "supports" +app_file = supports_dir / "app_run_tests" / "default_app.py" +file_with_entrypoint = supports_dir / "app_run_tests" / "local_entrypoint.py" + + +@pytest.mark.parametrize( + ("file_or_module", "expected_exit_code"), + [ + (supports_dir / app_file, 0), + (str(supports_dir / app_file) + "::app", 0), + (str(supports_dir / app_file) + "::foo", 0), + (str(supports_dir / app_file) + "::bar", 1), + (str(supports_dir / app_file) + "::app", 0), + (supports_dir / file_with_entrypoint, 0), + (str(supports_dir / file_with_entrypoint) + "::main", 0), + (str(supports_dir / file_with_entrypoint) + "::app.main", 0), + ], +) +def test_run(servicer, set_env_client, file_or_module, expected_exit_code): + _run(["run", str(file_or_module)], expected_exit_code=expected_exit_code) + + +def test_run_as_module(): _run(["run", app_file.as_posix()]) - _run(["run", app_file.as_posix() + "::app"]) - _run(["run", app_file.as_posix() + "::foo"]) - _run(["run", app_file.as_posix() + "::bar"], expected_exit_code=1, expected_stderr=None) - file_with_entrypoint = test_dir / "supports" / "app_run_tests" / "local_entrypoint.py" - _run(["run", file_with_entrypoint.as_posix()]) - _run(["run", file_with_entrypoint.as_posix() + "::main"]) - _run(["run", file_with_entrypoint.as_posix() + "::app.main"]) def test_run_stub(servicer, set_env_client, test_dir): From 9560c90e70aa6d66f26ca3d0959e26ffecd289f2 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Thu, 6 Feb 2025 14:18:14 +0100 Subject: [PATCH 02/19] Clean up test some more --- test/cli_test.py | 24 ++++++++++++------------ test/conftest.py | 4 +--- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/test/cli_test.py b/test/cli_test.py index 441a737cf..dd0dfebd0 100644 --- a/test/cli_test.py +++ b/test/cli_test.py @@ -129,25 +129,25 @@ def test_app_setup(servicer, set_env_client, server_url_env, modal_config): assert "_test" in toml.load(config_file_path) -supports_dir = Path(__file__).parent / "supports" -app_file = supports_dir / "app_run_tests" / "default_app.py" -file_with_entrypoint = supports_dir / "app_run_tests" / "local_entrypoint.py" +app_file = Path("app_run_tests") / "default_app.py" +file_with_entrypoint = Path("app_run_tests") / "local_entrypoint.py" @pytest.mark.parametrize( ("file_or_module", "expected_exit_code"), [ - (supports_dir / app_file, 0), - (str(supports_dir / app_file) + "::app", 0), - (str(supports_dir / app_file) + "::foo", 0), - (str(supports_dir / app_file) + "::bar", 1), - (str(supports_dir / app_file) + "::app", 0), - (supports_dir / file_with_entrypoint, 0), - (str(supports_dir / file_with_entrypoint) + "::main", 0), - (str(supports_dir / file_with_entrypoint) + "::app.main", 0), + (f"{app_file}", 0), + (f"{app_file}::app", 0), + (f"{app_file}::foo", 0), + (f"{app_file}::bar", 1), + (f"{file_with_entrypoint}", 0), + (f"{file_with_entrypoint}::main", 0), + (f"{file_with_entrypoint}::app.main", 0), + (f"{file_with_entrypoint}::foo", 0), ], ) -def test_run(servicer, set_env_client, file_or_module, expected_exit_code): +def test_run(servicer, set_env_client, supports_dir, monkeypatch, file_or_module, expected_exit_code): + monkeypatch.chdir(supports_dir) _run(["run", str(file_or_module)], expected_exit_code=expected_exit_code) diff --git a/test/conftest.py b/test/conftest.py index a2037a78a..0e3874b9e 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -2093,9 +2093,7 @@ def repo_root(request): @pytest.fixture(scope="module") def test_dir(request): """Absolute path to directory containing test file.""" - root_dir = Path(request.config.rootdir) - test_dir = Path(os.getenv("PYTEST_CURRENT_TEST")).parent - return root_dir / test_dir + return Path(__file__).parent @pytest.fixture(scope="function") From 6f7b9294f049502ff15fe25def59ae99317a11ee Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Thu, 6 Feb 2025 14:22:08 +0100 Subject: [PATCH 03/19] Adding failing test --- test/cli_test.py | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/test/cli_test.py b/test/cli_test.py index dd0dfebd0..96b33f6aa 100644 --- a/test/cli_test.py +++ b/test/cli_test.py @@ -134,21 +134,26 @@ def test_app_setup(servicer, set_env_client, server_url_env, modal_config): @pytest.mark.parametrize( - ("file_or_module", "expected_exit_code"), + ("file_or_module", "expected_exit_code", "expected_output"), [ - (f"{app_file}", 0), - (f"{app_file}::app", 0), - (f"{app_file}::foo", 0), - (f"{app_file}::bar", 1), - (f"{file_with_entrypoint}", 0), - (f"{file_with_entrypoint}::main", 0), - (f"{file_with_entrypoint}::app.main", 0), - (f"{file_with_entrypoint}::foo", 0), + (f"{app_file}", 0, ""), + (f"{app_file}::app", 0, ""), + (f"{app_file}::foo", 0, ""), + (f"{app_file}::bar", 1, ""), + ("app_run_tests.default_app::foo", 0, "-m"), # module mode without -m - warn + (f"{file_with_entrypoint}", 0, ""), + (f"{file_with_entrypoint}::main", 0, ""), + (f"{file_with_entrypoint}::app.main", 0, ""), + (f"{file_with_entrypoint}::foo", 0, ""), ], ) -def test_run(servicer, set_env_client, supports_dir, monkeypatch, file_or_module, expected_exit_code): +def test_run(servicer, set_env_client, supports_dir, monkeypatch, file_or_module, expected_exit_code, expected_output): monkeypatch.chdir(supports_dir) - _run(["run", str(file_or_module)], expected_exit_code=expected_exit_code) + res = _run(["run", str(file_or_module)], expected_exit_code=expected_exit_code) + if expected_output: + assert re.search(expected_output, res.stdout) or re.search( + expected_output, res.stderr + ), "output does not match expected string" def test_run_as_module(): From f4c389d04e207cdc8a91f329f644faa862e620b0 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Thu, 6 Feb 2025 14:29:11 +0100 Subject: [PATCH 04/19] test case with -m --- test/cli_test.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/test/cli_test.py b/test/cli_test.py index 96b33f6aa..2aa2ef88d 100644 --- a/test/cli_test.py +++ b/test/cli_test.py @@ -134,22 +134,23 @@ def test_app_setup(servicer, set_env_client, server_url_env, modal_config): @pytest.mark.parametrize( - ("file_or_module", "expected_exit_code", "expected_output"), + ("run_command", "expected_exit_code", "expected_output"), [ - (f"{app_file}", 0, ""), - (f"{app_file}::app", 0, ""), - (f"{app_file}::foo", 0, ""), - (f"{app_file}::bar", 1, ""), - ("app_run_tests.default_app::foo", 0, "-m"), # module mode without -m - warn - (f"{file_with_entrypoint}", 0, ""), - (f"{file_with_entrypoint}::main", 0, ""), - (f"{file_with_entrypoint}::app.main", 0, ""), - (f"{file_with_entrypoint}::foo", 0, ""), + ([f"{app_file}"], 0, ""), + ([f"{app_file}::app"], 0, ""), + ([f"{app_file}::foo"], 0, ""), + ([f"{app_file}::bar"], 1, ""), + (["app_run_tests.default_app::foo"], 0, "-m"), # module mode without -m - warn + (["-m", "app_run_tests.default_app::foo"], 0, ""), # module with -m + ([f"{file_with_entrypoint}"], 0, ""), + ([f"{file_with_entrypoint}::main"], 0, ""), + ([f"{file_with_entrypoint}::app.main"], 0, ""), + ([f"{file_with_entrypoint}::foo"], 0, ""), ], ) -def test_run(servicer, set_env_client, supports_dir, monkeypatch, file_or_module, expected_exit_code, expected_output): +def test_run(servicer, set_env_client, supports_dir, monkeypatch, run_command, expected_exit_code, expected_output): monkeypatch.chdir(supports_dir) - res = _run(["run", str(file_or_module)], expected_exit_code=expected_exit_code) + res = _run(["run"] + run_command, expected_exit_code=expected_exit_code) if expected_output: assert re.search(expected_output, res.stdout) or re.search( expected_output, res.stderr From 16a1c086b4ccc86675768d31be32f88378dd5b10 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Thu, 6 Feb 2025 14:30:32 +0100 Subject: [PATCH 05/19] Move deploy help into docstring --- modal/cli/entry_point.py | 2 +- modal/cli/run.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/modal/cli/entry_point.py b/modal/cli/entry_point.py index 7a037149f..0431708f3 100644 --- a/modal/cli/entry_point.py +++ b/modal/cli/entry_point.py @@ -84,7 +84,7 @@ async def setup(profile: Optional[str] = None): # Commands -entrypoint_cli_typer.command("deploy", help="Deploy a Modal application.", no_args_is_help=True)(run.deploy) +entrypoint_cli_typer.command("deploy", no_args_is_help=True)(run.deploy) entrypoint_cli_typer.command("serve", no_args_is_help=True)(run.serve) entrypoint_cli_typer.command("shell")(run.shell) entrypoint_cli_typer.add_typer(launch_cli) diff --git a/modal/cli/run.py b/modal/cli/run.py index 5a597cc2a..0bda11994 100644 --- a/modal/cli/run.py +++ b/modal/cli/run.py @@ -418,6 +418,7 @@ def deploy( 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."), ): + """Deploy a Modal application.""" # this ensures that lookups without environment specification use the same env as specified env = ensure_env(env) From e39df18d270fc81dbeb03294b1ce5db066a3c753 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Thu, 6 Feb 2025 16:47:52 +0100 Subject: [PATCH 06/19] Fix modal deploy --- modal/cli/import_refs.py | 27 ++++++++++++++++++--------- modal/cli/run.py | 11 ++++++++--- test/cli_test.py | 33 +++++++++++++++++++++++---------- 3 files changed, 49 insertions(+), 22 deletions(-) diff --git a/modal/cli/import_refs.py b/modal/cli/import_refs.py index 50ac4195a..fd8ffec7a 100644 --- a/modal/cli/import_refs.py +++ b/modal/cli/import_refs.py @@ -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 @@ -54,14 +55,27 @@ def parse_import_ref(object_ref: str) -> ImportRef: DEFAULT_APP_NAME = "app" -def import_file_or_module(file_or_module: str): +def import_file_or_module(file_or_module: str, module_mode: bool, base_cmd: str): 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 file_or_module.endswith(".py") or module_mode: + if not module_mode: + 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 {file_or_module}` instead.", + pending=True, + show_source=False, + ) + try: + module = importlib.import_module(file_or_module) + except Exception as exc: + raise _CliUserExecutionError(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() @@ -84,11 +98,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,7 +236,7 @@ def _is_accepted_type(cli_command: CLICommand) -> bool: return res -def import_app(app_ref: str) -> App: +def import_app(app_ref: str, module_mode: bool, base_cmd: str) -> App: import_ref = parse_import_ref(app_ref) # TODO: default could be to just pick up any app regardless if it's called DEFAULT_APP_NAME @@ -235,7 +244,7 @@ def import_app(app_ref: str) -> App: 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.file_or_module, module_mode, base_cmd) if "." in object_path: raise click.UsageError(f"{object_path} is not a Modal App") diff --git a/modal/cli/run.py b/modal/cli/run.py index 0bda11994..ff16ea246 100644 --- a/modal/cli/run.py +++ b/modal/cli/run.py @@ -412,17 +412,22 @@ def run(ctx, write_result, detach, quiet, interactive, env, 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"), ): - """Deploy a Modal application.""" + """Deploy a Modal application. + + **Usage:** + hello + """ # 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 diff --git a/test/cli_test.py b/test/cli_test.py index 2aa2ef88d..1b10eb976 100644 --- a/test/cli_test.py +++ b/test/cli_test.py @@ -130,6 +130,7 @@ def test_app_setup(servicer, set_env_client, server_url_env, modal_config): app_file = Path("app_run_tests") / "default_app.py" +app_module = "app_run_tests.default_app" file_with_entrypoint = Path("app_run_tests") / "local_entrypoint.py" @@ -140,8 +141,8 @@ def test_app_setup(servicer, set_env_client, server_url_env, modal_config): ([f"{app_file}::app"], 0, ""), ([f"{app_file}::foo"], 0, ""), ([f"{app_file}::bar"], 1, ""), - (["app_run_tests.default_app::foo"], 0, "-m"), # module mode without -m - warn - (["-m", "app_run_tests.default_app::foo"], 0, ""), # module with -m + ([f"{app_module}::foo"], 0, "-m"), # module mode without -m - warn + (["-m", f"{app_module}::foo"], 0, ""), # module with -m ([f"{file_with_entrypoint}"], 0, ""), ([f"{file_with_entrypoint}::main"], 0, ""), ([f"{file_with_entrypoint}::app.main"], 0, ""), @@ -157,10 +158,6 @@ def test_run(servicer, set_env_client, supports_dir, monkeypatch, run_command, e ), "output does not match expected string" -def test_run_as_module(): - _run(["run", app_file.as_posix()]) - - def test_run_stub(servicer, set_env_client, test_dir): app_file = test_dir / "supports" / "app_run_tests" / "app_was_once_stub.py" with pytest.warns(match="App"): @@ -249,10 +246,26 @@ def test_run_write_result(servicer, set_env_client, test_dir): ) -def test_deploy(servicer, set_env_client, test_dir): - app_file = test_dir / "supports" / "app_run_tests" / "default_app.py" - _run(["deploy", "--name=deployment_name", app_file.as_posix()]) - assert servicer.app_state_history["ap-1"] == [api_pb2.APP_STATE_INITIALIZING, api_pb2.APP_STATE_DEPLOYED] +@pytest.mark.parametrize( + ["args", "success", "expected_warning"], + [ + (["--name=deployment_name", str(app_file)], True, ""), + (["--name=deployment_name", app_module], True, f"modal deploy -m {app_module}"), + (["--name=deployment_name", "-m", app_module], True, ""), + ], +) +def test_deploy( + servicer, set_env_client, supports_dir, monkeypatch, args, success, expected_warning, disable_auto_mount, recwarn +): + monkeypatch.chdir(supports_dir) + _run(["deploy"] + args, expected_exit_code=0 if success else 1) + if success: + assert servicer.app_state_history["ap-1"] == [api_pb2.APP_STATE_INITIALIZING, api_pb2.APP_STATE_DEPLOYED] + else: + assert api_pb2.APP_STATE_DEPLOYED not in servicer.app_state_history["ap-1"] + if expected_warning: + assert len(recwarn) == 1 + assert expected_warning in str(recwarn[0].message) def test_run_custom_app(servicer, set_env_client, test_dir): From 43c83684a1065ece8ea3fda68a4c2aa46308a020 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 10 Feb 2025 13:45:08 +0100 Subject: [PATCH 07/19] Stuff --- modal/cli/import_refs.py | 36 ++++++++++++++++--------------- modal/cli/launch.py | 13 +++++------ modal/cli/programs/run_jupyter.py | 2 +- modal/cli/programs/vscode.py | 2 +- modal/cli/run.py | 9 ++++---- test/cli_imports_test.py | 19 ++++++++++------ 6 files changed, 43 insertions(+), 38 deletions(-) diff --git a/modal/cli/import_refs.py b/modal/cli/import_refs.py index fd8ffec7a..70e7b00ad 100644 --- a/modal/cli/import_refs.py +++ b/modal/cli/import_refs.py @@ -32,16 +32,17 @@ @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: @@ -49,36 +50,37 @@ def parse_import_ref(object_ref: str) -> ImportRef: 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, module_mode: bool, base_cmd: 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 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 not file_or_module.endswith(".py") or module_mode: - if not module_mode: + if not import_ref.file_or_module.endswith(".py") or import_ref.is_module: + 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 {file_or_module}` instead.", + f"Use `{base_cmd} -m {import_ref.file_or_module}` instead.", pending=True, show_source=False, ) try: - module = importlib.import_module(file_or_module) + module = importlib.import_module(import_ref.file_or_module) except Exception as exc: - raise _CliUserExecutionError(file_or_module) from 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}." @@ -86,10 +88,10 @@ def import_file_or_module(file_or_module: str, module_mode: bool, base_cmd: str) ) 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 @@ -236,15 +238,15 @@ def _is_accepted_type(cli_command: CLICommand) -> bool: return res -def import_app(app_ref: str, module_mode: bool, base_cmd: 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) # 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_mode, base_cmd) + module = import_file_or_module(import_ref, base_cmd) if "." in object_path: raise click.UsageError(f"{object_path} is not a Modal App") @@ -314,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. @@ -330,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 diff --git a/modal/cli/launch.py b/modal/cli/launch.py index 60226f523..034ccf4ab 100644 --- a/modal/cli/launch.py +++ b/modal/cli/launch.py @@ -8,11 +8,10 @@ from typer import Typer -from ..app import LocalEntrypoint from ..exception import _CliUserExecutionError from ..output import enable_output from ..runner import run_app -from .import_refs import _get_runnable_app, import_and_filter, parse_import_ref +from .import_refs import ImportRef, _get_runnable_app, import_file_or_module launch_cli = Typer( name="launch", @@ -29,14 +28,12 @@ def _launch_program(name: str, filename: str, detach: bool, args: dict[str, Any] os.environ["MODAL_LAUNCH_ARGS"] = json.dumps(args) program_path = str(Path(__file__).parent / "programs" / filename) - entrypoint, _ = import_and_filter( - parse_import_ref(program_path), accept_local_entrypoint=True, accept_webhook=False - ) - if not isinstance(entrypoint, LocalEntrypoint): - raise ValueError(f"{program_path} has no single local_entrypoint") + base_cmd = f"modal launch {name}" + module = import_file_or_module(ImportRef(program_path, is_module=False), base_cmd=base_cmd) + entrypoint = module.main app = _get_runnable_app(entrypoint) - app.set_description(f"modal launch {name}") + app.set_description(base_cmd) # `launch/` scripts must have a `local_entrypoint()` with no args, for simplicity here. func = entrypoint.info.raw_f diff --git a/modal/cli/programs/run_jupyter.py b/modal/cli/programs/run_jupyter.py index 2b7fa00e4..888e7ea87 100644 --- a/modal/cli/programs/run_jupyter.py +++ b/modal/cli/programs/run_jupyter.py @@ -15,7 +15,7 @@ # Passed by `modal launch` locally via CLI, plumbed to remote runner through secrets. args: dict[str, Any] = json.loads(os.environ.get("MODAL_LAUNCH_ARGS", "{}")) -app = App() +app = App(include_source=True) image = Image.from_registry(args.get("image"), add_python=args.get("add_python")).pip_install("jupyterlab") diff --git a/modal/cli/programs/vscode.py b/modal/cli/programs/vscode.py index 22f02d838..0f152cc12 100644 --- a/modal/cli/programs/vscode.py +++ b/modal/cli/programs/vscode.py @@ -22,7 +22,7 @@ FIXUD_INSTALLER = "https://github.com/boxboat/fixuid/releases/download/v0.6.0/fixuid-0.6.0-linux-$ARCH.tar.gz" -app = App() +app = App(include_source=True) image = ( Image.from_registry(args.get("image"), add_python="3.11") .apt_install("curl", "dumb-init", "git", "git-lfs") diff --git a/modal/cli/run.py b/modal/cli/run.py index ff16ea246..7d19e2bb6 100644 --- a/modal/cli/run.py +++ b/modal/cli/run.py @@ -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 = ( @@ -443,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. @@ -454,7 +455,7 @@ 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)) @@ -574,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 = ( diff --git a/test/cli_imports_test.py b/test/cli_imports_test.py index 7ad99589f..eea0349ad 100644 --- a/test/cli_imports_test.py +++ b/test/cli_imports_test.py @@ -14,7 +14,7 @@ list_cli_commands, parse_import_ref, ) -from modal.exception import InvalidError +from modal.exception import InvalidError, PendingDeprecationError from modal.functions import Function from modal.partial_function import method, web_server @@ -114,7 +114,7 @@ def test_import_and_filter(dir_structure, ref, mock_dir, returned_runnable_type, with mock_dir(dir_structure): import_ref = parse_import_ref(ref) runnable, all_usable_commands = import_and_filter( - import_ref, accept_local_entrypoint=True, accept_webhook=False + import_ref, base_cmd="dummy", accept_local_entrypoint=True, accept_webhook=False ) print(all_usable_commands) assert isinstance(runnable, returned_runnable_type) @@ -124,7 +124,10 @@ def test_import_and_filter(dir_structure, ref, mock_dir, returned_runnable_type, def test_import_and_filter_2(monkeypatch, supports_on_path): def import_runnable(name_prefix, accept_local_entrypoint=False, accept_webhook=False): return import_and_filter( - ImportRef("import_and_filter_source", name_prefix), accept_local_entrypoint, accept_webhook + ImportRef("import_and_filter_source", name_prefix), + base_cmd="", + accept_local_entrypoint=accept_local_entrypoint, + accept_webhook=accept_webhook, ) runnable, all_usable_commands = import_runnable( @@ -163,23 +166,25 @@ def test_import_package_and_module_names(monkeypatch, supports_dir): # is __main__ when using `python` but in the Modal runtime it's the name of the # file minus the ".py", since Modal has its own __main__ monkeypatch.chdir(supports_dir) - mod1 = import_file_or_module("assert_package") + mod1 = import_file_or_module(ImportRef("assert_package", is_module=True)) assert mod1.__package__ == "" assert mod1.__name__ == "assert_package" monkeypatch.chdir(supports_dir.parent) - mod2 = import_file_or_module("test.supports.assert_package") + with pytest.warns(PendingDeprecationError, match=r"\s-m\s"): + mod2 = import_file_or_module(ImportRef("test.supports.assert_package", is_module=False)) # TODO: is_module=True + assert mod2.__package__ == "test.supports" assert mod2.__name__ == "test.supports.assert_package" - mod3 = import_file_or_module("supports/assert_package.py") + mod3 = import_file_or_module(ImportRef("supports/assert_package.py", is_module=False)) assert mod3.__package__ == "" assert mod3.__name__ == "assert_package" def test_invalid_source_file_exception(): with pytest.raises(InvalidError, match="Invalid Modal source filename: 'foo.bar.py'"): - import_file_or_module("path/to/foo.bar.py") + import_file_or_module(ImportRef("path/to/foo.bar.py", is_module=False)) def test_list_cli_commands(): From a55327090099fa2ffa2c36a4b5e741db620e3ecb Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 10 Feb 2025 14:24:34 +0100 Subject: [PATCH 08/19] Fix tests --- test/cli_test.py | 15 ++++++++++++--- test/supports/app_run_tests/variadic_args.py | 12 +++++++----- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/test/cli_test.py b/test/cli_test.py index 1b10eb976..951064cad 100644 --- a/test/cli_test.py +++ b/test/cli_test.py @@ -141,8 +141,6 @@ def test_app_setup(servicer, set_env_client, server_url_env, modal_config): ([f"{app_file}::app"], 0, ""), ([f"{app_file}::foo"], 0, ""), ([f"{app_file}::bar"], 1, ""), - ([f"{app_module}::foo"], 0, "-m"), # module mode without -m - warn - (["-m", f"{app_module}::foo"], 0, ""), # module with -m ([f"{file_with_entrypoint}"], 0, ""), ([f"{file_with_entrypoint}::main"], 0, ""), ([f"{file_with_entrypoint}::app.main"], 0, ""), @@ -158,6 +156,17 @@ def test_run(servicer, set_env_client, supports_dir, monkeypatch, run_command, e ), "output does not match expected string" +def test_run_warns_without_module_flag( + servicer, set_env_client, supports_dir, recwarn, monkeypatch, disable_auto_mount +): + monkeypatch.chdir(supports_dir) + _run(["run", "-m", f"{app_module}::foo"]) + assert not len(recwarn) + + with pytest.warns(match=" -m "): + _run(["run", f"{app_module}::foo"]) + + def test_run_stub(servicer, set_env_client, test_dir): app_file = test_dir / "supports" / "app_run_tests" / "app_was_once_stub.py" with pytest.warns(match="App"): @@ -1215,7 +1224,7 @@ def test_run_auto_infer_prefer_target_module(servicer, supports_dir, set_env_cli @pytest.mark.parametrize("func", ["va_entrypoint", "va_function", "VaClass.va_method"]) -def test_cli_run_variadic_args(servicer, set_env_client, test_dir, func): +def test_cli_run_variadic_args(servicer, set_env_client, test_dir, func, disable_auto_mount): app_file = test_dir / "supports" / "app_run_tests" / "variadic_args.py" @servicer.function_body diff --git a/test/supports/app_run_tests/variadic_args.py b/test/supports/app_run_tests/variadic_args.py index 58679304c..64633958a 100644 --- a/test/supports/app_run_tests/variadic_args.py +++ b/test/supports/app_run_tests/variadic_args.py @@ -1,26 +1,28 @@ # Copyright Modal Labs 2022 from modal import App, method -app = App() +app = App(include_source=True) + @app.cls() class VaClass: @method() def va_method(self, *args): - pass # Set via @servicer.function_body + pass # Set via @servicer.function_body @method() def va_method_invalid(self, x: int, *args): - pass # Set via @servicer.function_body + pass # Set via @servicer.function_body + @app.function() def va_function(*args): - pass # Set via @servicer.function_body + pass # Set via @servicer.function_body @app.function() def va_function_invalid(x: int, *args): - pass # Set via @servicer.function_body + pass # Set via @servicer.function_body @app.local_entrypoint() From c03b639111e6f2b269295ce710153bb0fa94d8b7 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 10 Feb 2025 14:33:27 +0100 Subject: [PATCH 09/19] Fix some automount warnings --- test/cli_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cli_test.py b/test/cli_test.py index 951064cad..d2f17c1aa 100644 --- a/test/cli_test.py +++ b/test/cli_test.py @@ -1168,7 +1168,7 @@ def test_container_exec(servicer, set_env_client, mock_shell_pty, app): sb.terminate() -def test_can_run_all_listed_functions_with_includes(supports_on_path, monkeypatch, set_env_client): +def test_can_run_all_listed_functions_with_includes(supports_on_path, monkeypatch, set_env_client, disable_auto_mount): monkeypatch.setenv("TERM", "dumb") # prevents looking at ansi escape sequences res = _run(["run", "multifile_project.main"], expected_exit_code=1) @@ -1217,7 +1217,7 @@ def test_run_file_with_global_lookups(servicer, set_env_client, supports_dir): assert len(ctx.get_requests("FunctionGet")) == 0 -def test_run_auto_infer_prefer_target_module(servicer, supports_dir, set_env_client, monkeypatch): +def test_run_auto_infer_prefer_target_module(servicer, supports_dir, set_env_client, monkeypatch, disable_auto_mount): monkeypatch.syspath_prepend(supports_dir / "app_run_tests") res = _run(["run", "multifile.util"]) assert "ran util\nmain func" in res.stdout From 96653ff3266a0dff234d78851934ba4aeecad9e8 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 10 Feb 2025 15:14:06 +0100 Subject: [PATCH 10/19] Serve fixes --- modal/serving.py | 26 ++++++++++++++++++++------ test/cli_imports_test.py | 4 ++-- test/live_reload_test.py | 7 +++++-- test/supports/app_run_tests/webhook.py | 2 +- 4 files changed, 28 insertions(+), 11 deletions(-) diff --git a/modal/serving.py b/modal/serving.py index 8a8f169ba..e3b062d7f 100644 --- a/modal/serving.py +++ b/modal/serving.py @@ -26,9 +26,11 @@ _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): @@ -36,13 +38,15 @@ def _run_serve(app_ref: str, existing_app_id: str, is_ready: Event, environment_ 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, _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 diff --git a/test/cli_imports_test.py b/test/cli_imports_test.py index eea0349ad..4594fbcec 100644 --- a/test/cli_imports_test.py +++ b/test/cli_imports_test.py @@ -122,9 +122,9 @@ def test_import_and_filter(dir_structure, ref, mock_dir, returned_runnable_type, def test_import_and_filter_2(monkeypatch, supports_on_path): - def import_runnable(name_prefix, accept_local_entrypoint=False, accept_webhook=False): + def import_runnable(object_path, accept_local_entrypoint=False, accept_webhook=False): return import_and_filter( - ImportRef("import_and_filter_source", name_prefix), + ImportRef("import_and_filter_source", is_module=True, object_path=object_path), base_cmd="", accept_local_entrypoint=accept_local_entrypoint, accept_webhook=accept_webhook, diff --git a/test/live_reload_test.py b/test/live_reload_test.py index ee74c4621..0ab920dbb 100644 --- a/test/live_reload_test.py +++ b/test/live_reload_test.py @@ -37,7 +37,7 @@ async def test_live_reload_with_logs(app_ref, server_url_env, token_env, service @skip_windows("live-reload not supported on windows") -def test_file_changes_trigger_reloads(app_ref, server_url_env, token_env, servicer): +def test_file_changes_trigger_reloads(app_ref, server_url_env, token_env, servicer, capfd): watcher_done = threading.Event() async def fake_watch(): @@ -45,11 +45,14 @@ async def fake_watch(): yield {"/some/file"} watcher_done.set() - with serve_app(app, app_ref, _watcher=fake_watch()): + with serve_app(app, app_ref, is_module=False, _watcher=fake_watch()): watcher_done.wait() # wait until watcher loop is done foo: Function = app.registered_functions["foo"] assert foo.web_url.startswith("http://") + stderr = capfd.readouterr().err + print(stderr) + assert "Traceback" not in stderr # TODO ideally we would assert the specific expected number here, but this test # is consistently flaking in CI and I cannot reproduce locally to debug. # I'm relaxing the assertion for now to stop the test from blocking deployments. diff --git a/test/supports/app_run_tests/webhook.py b/test/supports/app_run_tests/webhook.py index cee6ace62..5529833ff 100644 --- a/test/supports/app_run_tests/webhook.py +++ b/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) @app.function() From 4ce83c4e6aea3c6687a21048d2e04525c0edfdb9 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 10 Feb 2025 16:00:54 +0100 Subject: [PATCH 11/19] wackamole --- modal/cli/run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modal/cli/run.py b/modal/cli/run.py index 7d19e2bb6..8809c2e3f 100644 --- a/modal/cli/run.py +++ b/modal/cli/run.py @@ -460,7 +460,7 @@ def serve( 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: From f1816f4edd1579eaa1d8dccc0e13a86e4a3d07fc Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 10 Feb 2025 16:08:11 +0100 Subject: [PATCH 12/19] more --- test/live_reload_test.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/live_reload_test.py b/test/live_reload_test.py index 0ab920dbb..a20a39a75 100644 --- a/test/live_reload_test.py +++ b/test/live_reload_test.py @@ -19,7 +19,7 @@ def app_ref(test_dir): @pytest.mark.asyncio async def test_live_reload(app_ref, server_url_env, token_env, servicer): - async with serve_app.aio(app, app_ref): + async with serve_app.aio(app, app_ref, is_module=False): await asyncio.sleep(3.0) assert servicer.app_publish_count == 1 assert servicer.app_client_disconnect_count == 1 @@ -29,7 +29,7 @@ async def test_live_reload(app_ref, server_url_env, token_env, servicer): @pytest.mark.asyncio async def test_live_reload_with_logs(app_ref, server_url_env, token_env, servicer): with enable_output(): - async with serve_app.aio(app, app_ref): + async with serve_app.aio(app, app_ref, is_module=False): await asyncio.sleep(3.0) assert servicer.app_publish_count == 1 assert servicer.app_client_disconnect_count == 1 @@ -68,7 +68,7 @@ async def fake_watch(): if False: yield - async with serve_app.aio(app, app_ref, _watcher=fake_watch()): + async with serve_app.aio(app, app_ref, _watcher=fake_watch(), is_module=False): pass assert servicer.app_publish_count == 1 # Should create the initial app once @@ -79,7 +79,7 @@ async def fake_watch(): async def test_heartbeats(app_ref, server_url_env, token_env, servicer): with mock.patch("modal.runner.HEARTBEAT_INTERVAL", 1): t0 = time.time() - async with serve_app.aio(app, app_ref): + async with serve_app.aio(app, app_ref, is_module=False): await asyncio.sleep(3.1) total_secs = int(time.time() - t0) From 8bec717e813a15cfedf391f1110fbe7c88cad229 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 17 Feb 2025 13:19:30 +0100 Subject: [PATCH 13/19] Refactor --- modal/cli/import_refs.py | 4 +--- modal/cli/run.py | 11 ++++++----- modal/serving.py | 26 +++++++++----------------- 3 files changed, 16 insertions(+), 25 deletions(-) diff --git a/modal/cli/import_refs.py b/modal/cli/import_refs.py index 70e7b00ad..1e033b63d 100644 --- a/modal/cli/import_refs.py +++ b/modal/cli/import_refs.py @@ -238,9 +238,7 @@ def _is_accepted_type(cli_command: CLICommand) -> bool: return res -def import_app(app_ref: str, is_module: bool, base_cmd: str) -> App: - import_ref = parse_import_ref(app_ref, is_module=is_module) - +def import_app(import_ref: ImportRef, base_cmd: str) -> App: # 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 diff --git a/modal/cli/run.py b/modal/cli/run.py index 8d31c7852..a0dd4c9a4 100644 --- a/modal/cli/run.py +++ b/modal/cli/run.py @@ -416,7 +416,7 @@ def deploy( 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"), + is_module: bool = typer.Option(False, "-m", help="Use a Python module path instead of a file path"), ): """Deploy a Modal application. @@ -426,7 +426,8 @@ def deploy( # this ensures that lookups without environment specification use the same env as specified env = ensure_env(env) - app = import_app(app_ref, module_mode, base_cmd="modal deploy") + import_ref = parse_import_ref(app_ref, is_module=is_module) + app = import_app(import_ref, base_cmd="modal deploy") if name is None: name = app.name @@ -453,13 +454,13 @@ def serve( ``` """ env = ensure_env(env) - - app = import_app(app_ref, is_module, base_cmd="modal serve") + import_ref = parse_import_ref(app_ref, is_module=is_module) + app = import_app(import_ref, 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, is_module=is_module, environment_name=env): + with serve_app(app, import_ref, environment_name=env): if timeout is None: timeout = config["serve_timeout"] if timeout is None: diff --git a/modal/serving.py b/modal/serving.py index e3b062d7f..1a889d439 100644 --- a/modal/serving.py +++ b/modal/serving.py @@ -14,7 +14,7 @@ from ._utils.deprecation import deprecation_error from ._utils.logger import logger from ._watcher import watch -from .cli.import_refs import import_app +from .cli.import_refs import ImportRef, import_app from .client import _Client from .config import config from .output import _get_output_manager, enable_output @@ -27,10 +27,10 @@ def _run_serve( - app_ref: str, is_module: bool, existing_app_id: str, is_ready: Event, environment_name: str, show_progress: bool + import_ref: ImportRef, existing_app_id: str, is_ready: Event, environment_name: str, show_progress: bool ): # subprocess entrypoint - _app = import_app(app_ref, is_module=is_module, base_cmd="modal serve") + _app = import_app(import_ref, base_cmd="modal serve") blocking_app = synchronizer._translate_out(_app) with enable_output(show_progress=show_progress): @@ -38,15 +38,13 @@ def _run_serve( async def _restart_serve( - app_ref: str, *, is_module: bool, existing_app_id: str, environment_name: str, timeout: float = 5.0 + import_ref: ImportRef, *, 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, is_module, existing_app_id, is_ready, environment_name, show_progress) - ) + p = ctx.Process(target=_run_serve, args=(import_ref, 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 @@ -72,9 +70,8 @@ async def _terminate(proc: Optional[SpawnProcess], timeout: float = 5.0): async def _run_watch_loop( - app_ref: str, + import_ref: ImportRef, *, - is_module: bool, app_id: str, watcher: AsyncGenerator[set[str], None], environment_name: str, @@ -94,9 +91,7 @@ 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, is_module=is_module, existing_app_id=app_id, environment_name=environment_name - ) + curr_proc = await _restart_serve(import_ref, existing_app_id=app_id, environment_name=environment_name) finally: await _terminate(curr_proc) @@ -104,9 +99,8 @@ async def _run_watch_loop( @asynccontextmanager async def _serve_app( app: "_App", - app_ref: str, + import_ref: ImportRef, *, - is_module: bool, _watcher: Optional[AsyncGenerator[set[str], None]] = None, # for testing environment_name: Optional[str] = None, ) -> AsyncGenerator["_App", None]: @@ -123,9 +117,7 @@ async def _serve_app( watcher = watch(mounts_to_watch) async with TaskContext(grace=0.1) as tc: tc.create_task( - _run_watch_loop( - app_ref, is_module=is_module, app_id=app.app_id, watcher=watcher, environment_name=environment_name - ) + _run_watch_loop(import_ref, app_id=app.app_id, watcher=watcher, environment_name=environment_name) ) yield app From 0a00df2729cf6aab45bee435083c6ed3f6208008 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 17 Feb 2025 13:23:58 +0100 Subject: [PATCH 14/19] Blarh --- modal/cli/import_refs.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/modal/cli/import_refs.py b/modal/cli/import_refs.py index 1e033b63d..fc6a07aca 100644 --- a/modal/cli/import_refs.py +++ b/modal/cli/import_refs.py @@ -238,7 +238,12 @@ def _is_accepted_type(cli_command: CLICommand) -> bool: return res -def import_app(import_ref: ImportRef, base_cmd: str) -> App: +def import_app(app_ref: str): + # TODO: remove when integration tests have been migrated to import_app_from_ref + return import_app_from_ref(parse_import_ref(app_ref)) + + +def import_app_from_ref(import_ref: ImportRef, base_cmd: str = "") -> App: # 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 From 0a57d16d3358e753485a80fb522f4d46744d7d2e Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 17 Feb 2025 13:31:47 +0100 Subject: [PATCH 15/19] Michael comments --- modal/cli/run.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/modal/cli/run.py b/modal/cli/run.py index a0dd4c9a4..9736ebf66 100644 --- a/modal/cli/run.py +++ b/modal/cli/run.py @@ -368,7 +368,7 @@ 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") +@click.option("-m", is_flag=True, help="Use a Python module path instead of a file path") @click.pass_context def run(ctx, write_result, detach, quiet, interactive, env, module): """Run a Modal function or local entrypoint. @@ -421,7 +421,8 @@ def deploy( """Deploy a Modal application. **Usage:** - hello + modal deploy my_script.py + modal deploy -m my_package.my_mod """ # this ensures that lookups without environment specification use the same env as specified env = ensure_env(env) @@ -443,7 +444,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"), + is_module: bool = typer.Option(False, "-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. From bcc1b106913275b75facbd6d0b012188278a14f6 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 17 Feb 2025 14:06:55 +0100 Subject: [PATCH 16/19] fixes --- modal/cli/run.py | 13 ++++++++++--- modal/serving.py | 4 ++-- test/live_reload_test.py | 25 +++++++++++++------------ 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/modal/cli/run.py b/modal/cli/run.py index 9736ebf66..3c66292bc 100644 --- a/modal/cli/run.py +++ b/modal/cli/run.py @@ -27,7 +27,14 @@ from ..runner import deploy_app, interactive_shell, run_app from ..serving import serve_app from ..volume import Volume -from .import_refs import CLICommand, MethodReference, _get_runnable_app, import_and_filter, import_app, parse_import_ref +from .import_refs import ( + CLICommand, + MethodReference, + _get_runnable_app, + import_and_filter, + import_app_from_ref, + parse_import_ref, +) from .utils import ENV_OPTION, ENV_OPTION_HELP, is_tty, stream_app_logs @@ -428,7 +435,7 @@ def deploy( env = ensure_env(env) import_ref = parse_import_ref(app_ref, is_module=is_module) - app = import_app(import_ref, base_cmd="modal deploy") + app = import_app_from_ref(import_ref, base_cmd="modal deploy") if name is None: name = app.name @@ -456,7 +463,7 @@ def serve( """ env = ensure_env(env) import_ref = parse_import_ref(app_ref, is_module=is_module) - app = import_app(import_ref, base_cmd="modal serve") + app = import_app_from_ref(import_ref, base_cmd="modal serve") if app.description is None: app.set_description(_get_clean_app_description(app_ref)) diff --git a/modal/serving.py b/modal/serving.py index 1a889d439..1a1cae867 100644 --- a/modal/serving.py +++ b/modal/serving.py @@ -14,7 +14,7 @@ from ._utils.deprecation import deprecation_error from ._utils.logger import logger from ._watcher import watch -from .cli.import_refs import ImportRef, import_app +from .cli.import_refs import ImportRef, import_app_from_ref from .client import _Client from .config import config from .output import _get_output_manager, enable_output @@ -30,7 +30,7 @@ def _run_serve( import_ref: ImportRef, existing_app_id: str, is_ready: Event, environment_name: str, show_progress: bool ): # subprocess entrypoint - _app = import_app(import_ref, base_cmd="modal serve") + _app = import_app_from_ref(import_ref, base_cmd="modal serve") blocking_app = synchronizer._translate_out(_app) with enable_output(show_progress=show_progress): diff --git a/test/live_reload_test.py b/test/live_reload_test.py index a20a39a75..ac4726324 100644 --- a/test/live_reload_test.py +++ b/test/live_reload_test.py @@ -6,6 +6,7 @@ from unittest import mock from modal import Function, enable_output +from modal.cli.import_refs import ImportRef from modal.serving import serve_app from .supports.app_run_tests.webhook import app @@ -13,13 +14,13 @@ @pytest.fixture -def app_ref(test_dir): - return str(test_dir / "supports" / "app_run_tests" / "webhook.py") +def import_ref(test_dir): + return ImportRef(str(test_dir / "supports" / "app_run_tests" / "webhook.py"), is_module=False) @pytest.mark.asyncio -async def test_live_reload(app_ref, server_url_env, token_env, servicer): - async with serve_app.aio(app, app_ref, is_module=False): +async def test_live_reload(import_ref, server_url_env, token_env, servicer): + async with serve_app.aio(app, import_ref): await asyncio.sleep(3.0) assert servicer.app_publish_count == 1 assert servicer.app_client_disconnect_count == 1 @@ -27,9 +28,9 @@ async def test_live_reload(app_ref, server_url_env, token_env, servicer): @pytest.mark.asyncio -async def test_live_reload_with_logs(app_ref, server_url_env, token_env, servicer): +async def test_live_reload_with_logs(import_ref, server_url_env, token_env, servicer): with enable_output(): - async with serve_app.aio(app, app_ref, is_module=False): + async with serve_app.aio(app, import_ref): await asyncio.sleep(3.0) assert servicer.app_publish_count == 1 assert servicer.app_client_disconnect_count == 1 @@ -37,7 +38,7 @@ async def test_live_reload_with_logs(app_ref, server_url_env, token_env, service @skip_windows("live-reload not supported on windows") -def test_file_changes_trigger_reloads(app_ref, server_url_env, token_env, servicer, capfd): +def test_file_changes_trigger_reloads(import_ref, server_url_env, token_env, servicer, capfd): watcher_done = threading.Event() async def fake_watch(): @@ -45,7 +46,7 @@ async def fake_watch(): yield {"/some/file"} watcher_done.set() - with serve_app(app, app_ref, is_module=False, _watcher=fake_watch()): + with serve_app(app, import_ref, _watcher=fake_watch()): watcher_done.wait() # wait until watcher loop is done foo: Function = app.registered_functions["foo"] assert foo.web_url.startswith("http://") @@ -62,13 +63,13 @@ async def fake_watch(): @pytest.mark.asyncio -async def test_no_change(app_ref, server_url_env, token_env, servicer): +async def test_no_change(import_ref, server_url_env, token_env, servicer): async def fake_watch(): # Iterator that returns immediately, yielding nothing if False: yield - async with serve_app.aio(app, app_ref, _watcher=fake_watch(), is_module=False): + async with serve_app.aio(app, import_ref, _watcher=fake_watch()): pass assert servicer.app_publish_count == 1 # Should create the initial app once @@ -76,10 +77,10 @@ async def fake_watch(): @pytest.mark.asyncio -async def test_heartbeats(app_ref, server_url_env, token_env, servicer): +async def test_heartbeats(import_ref, server_url_env, token_env, servicer): with mock.patch("modal.runner.HEARTBEAT_INTERVAL", 1): t0 = time.time() - async with serve_app.aio(app, app_ref, is_module=False): + async with serve_app.aio(app, import_ref): await asyncio.sleep(3.1) total_secs = int(time.time() - t0) From 93cb099a6b3ca9adc444bfff565b15a1dcabe387 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 17 Feb 2025 14:34:51 +0100 Subject: [PATCH 17/19] more fix --- modal/cli/run.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/modal/cli/run.py b/modal/cli/run.py index 3c66292bc..c5362770b 100644 --- a/modal/cli/run.py +++ b/modal/cli/run.py @@ -331,7 +331,7 @@ 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, is_module=ctx.params["module"]) + import_ref = parse_import_ref(func_ref, is_module=ctx.params["m"]) runnable, all_usable_commands = import_and_filter( import_ref, base_cmd="modal run", accept_local_entrypoint=True, accept_webhook=False ) @@ -377,7 +377,7 @@ def get_command(self, ctx, func_ref): @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") @click.pass_context -def run(ctx, write_result, detach, quiet, interactive, env, module): +def run(ctx, write_result, detach, quiet, interactive, env, m): """Run a Modal function or local entrypoint. `FUNC_REF` should be of the format `{file or module}::{function name}`. @@ -414,7 +414,6 @@ def run(ctx, write_result, detach, quiet, interactive, env, module): 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( From 479318e90b723e9b68b1bbafe10c963581f73dce Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Wed, 19 Feb 2025 15:16:07 +0100 Subject: [PATCH 18/19] Small updates --- modal/cli/import_refs.py | 1 - modal/cli/run.py | 10 +++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/modal/cli/import_refs.py b/modal/cli/import_refs.py index fc6a07aca..9d2d3ded0 100644 --- a/modal/cli/import_refs.py +++ b/modal/cli/import_refs.py @@ -57,7 +57,6 @@ def parse_import_ref(object_ref: str, is_module: bool = False) -> ImportRef: def import_file_or_module(import_ref: ImportRef, base_cmd: str = ""): - # TODO: Remove the base_cmd argument here once we have removed the deprecation if "" not in sys.path: # When running from a CLI like `modal run` # the current working directory isn't added to sys.path diff --git a/modal/cli/run.py b/modal/cli/run.py index c5362770b..01e8622c6 100644 --- a/modal/cli/run.py +++ b/modal/cli/run.py @@ -375,7 +375,7 @@ 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") +@click.option("-m", is_flag=True, help="Interpret argument as a Python module path instead of a file/script path") @click.pass_context def run(ctx, write_result, detach, quiet, interactive, env, m): """Run a Modal function or local entrypoint. @@ -422,7 +422,9 @@ def deploy( 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."), - is_module: bool = typer.Option(False, "-m", help="Use a Python module path instead of a file path"), + is_module: bool = typer.Option( + False, "-m", help="Interpret argument as a Python module path instead of a file/script path" + ), ): """Deploy a Modal application. @@ -450,7 +452,9 @@ 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, "-m", help="Use a Python module path instead of a file path"), + is_module: bool = typer.Option( + False, "-m", help="Interpret argument as a Python module path instead of a file/script path" + ), ): """Run a web endpoint(s) associated with a Modal app and hot-reload code. From bb22f2c36de718e4bc7fdb4905ca0373e4b4f86a Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Wed, 19 Feb 2025 15:18:40 +0100 Subject: [PATCH 19/19] Rename is_module to use_module_mode --- modal/cli/import_refs.py | 10 +++++----- modal/cli/launch.py | 2 +- modal/cli/run.py | 10 +++++----- test/cli_imports_test.py | 11 ++++++----- test/live_reload_test.py | 2 +- 5 files changed, 18 insertions(+), 17 deletions(-) diff --git a/modal/cli/import_refs.py b/modal/cli/import_refs.py index 9d2d3ded0..6e6f72618 100644 --- a/modal/cli/import_refs.py +++ b/modal/cli/import_refs.py @@ -32,7 +32,7 @@ @dataclasses.dataclass class ImportRef: file_or_module: str - is_module: bool + use_module_mode: bool # i.e. using the -m flag # object_path is a .-delimited path to the object to execute, or a parent from which to infer the object # e.g. @@ -42,7 +42,7 @@ class ImportRef: object_path: str = dataclasses.field(default="") -def parse_import_ref(object_ref: str, is_module: bool = False) -> ImportRef: +def parse_import_ref(object_ref: str, use_module_mode: bool = False) -> ImportRef: if object_ref.find("::") > 1: file_or_module, object_path = object_ref.split("::", 1) elif object_ref.find(":") > 1: @@ -50,7 +50,7 @@ def parse_import_ref(object_ref: str, is_module: bool = False) -> ImportRef: else: file_or_module, object_path = object_ref, "" - return ImportRef(file_or_module, is_module, object_path) + return ImportRef(file_or_module, use_module_mode, object_path) DEFAULT_APP_NAME = "app" @@ -63,8 +63,8 @@ def import_file_or_module(import_ref: ImportRef, base_cmd: str = ""): # so we add it in order to make module path specification possible sys.path.insert(0, "") # "" means the current working directory - if not import_ref.file_or_module.endswith(".py") or import_ref.is_module: - if not import_ref.is_module: + if not import_ref.file_or_module.endswith(".py") or import_ref.use_module_mode: + if not import_ref.use_module_mode: deprecation_warning( (2025, 2, 6), f"Using Python module paths will require using the -m flag in a future version of Modal.\n" diff --git a/modal/cli/launch.py b/modal/cli/launch.py index 034ccf4ab..0fda64753 100644 --- a/modal/cli/launch.py +++ b/modal/cli/launch.py @@ -29,7 +29,7 @@ def _launch_program(name: str, filename: str, detach: bool, args: dict[str, Any] program_path = str(Path(__file__).parent / "programs" / filename) base_cmd = f"modal launch {name}" - module = import_file_or_module(ImportRef(program_path, is_module=False), base_cmd=base_cmd) + module = import_file_or_module(ImportRef(program_path, use_module_mode=False), base_cmd=base_cmd) entrypoint = module.main app = _get_runnable_app(entrypoint) diff --git a/modal/cli/run.py b/modal/cli/run.py index 01e8622c6..4e0597eb0 100644 --- a/modal/cli/run.py +++ b/modal/cli/run.py @@ -331,7 +331,7 @@ 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, is_module=ctx.params["m"]) + import_ref = parse_import_ref(func_ref, use_module_mode=ctx.params["m"]) runnable, all_usable_commands = import_and_filter( import_ref, base_cmd="modal run", accept_local_entrypoint=True, accept_webhook=False ) @@ -422,7 +422,7 @@ def deploy( 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."), - is_module: bool = typer.Option( + use_module_mode: bool = typer.Option( False, "-m", help="Interpret argument as a Python module path instead of a file/script path" ), ): @@ -435,7 +435,7 @@ def deploy( # this ensures that lookups without environment specification use the same env as specified env = ensure_env(env) - import_ref = parse_import_ref(app_ref, is_module=is_module) + import_ref = parse_import_ref(app_ref, use_module_mode=use_module_mode) app = import_app_from_ref(import_ref, base_cmd="modal deploy") if name is None: @@ -452,7 +452,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( + use_module_mode: bool = typer.Option( False, "-m", help="Interpret argument as a Python module path instead of a file/script path" ), ): @@ -465,7 +465,7 @@ def serve( ``` """ env = ensure_env(env) - import_ref = parse_import_ref(app_ref, is_module=is_module) + import_ref = parse_import_ref(app_ref, use_module_mode=use_module_mode) app = import_app_from_ref(import_ref, base_cmd="modal serve") if app.description is None: app.set_description(_get_clean_app_description(app_ref)) diff --git a/test/cli_imports_test.py b/test/cli_imports_test.py index 6915482d6..16730bd0a 100644 --- a/test/cli_imports_test.py +++ b/test/cli_imports_test.py @@ -124,7 +124,7 @@ def test_import_and_filter(dir_structure, ref, mock_dir, returned_runnable_type, def test_import_and_filter_2(monkeypatch, supports_on_path): def import_runnable(object_path, accept_local_entrypoint=False, accept_webhook=False): return import_and_filter( - ImportRef("import_and_filter_source", is_module=True, object_path=object_path), + ImportRef("import_and_filter_source", use_module_mode=True, object_path=object_path), base_cmd="", accept_local_entrypoint=accept_local_entrypoint, accept_webhook=accept_webhook, @@ -166,25 +166,26 @@ def test_import_package_and_module_names(monkeypatch, supports_dir): # is __main__ when using `python` but in the Modal runtime it's the name of the # file minus the ".py", since Modal has its own __main__ monkeypatch.chdir(supports_dir) - mod1 = import_file_or_module(ImportRef("assert_package", is_module=True)) + mod1 = import_file_or_module(ImportRef("assert_package", use_module_mode=True)) assert mod1.__package__ == "" assert mod1.__name__ == "assert_package" monkeypatch.chdir(supports_dir.parent) with pytest.warns(PendingDeprecationError, match=r"\s-m\s"): - mod2 = import_file_or_module(ImportRef("test.supports.assert_package", is_module=False)) # TODO: is_module=True + # TODO: this should use use_module_mode=True once we remove the deprecation warning + mod2 = import_file_or_module(ImportRef("test.supports.assert_package", use_module_mode=False)) assert mod2.__package__ == "test.supports" assert mod2.__name__ == "test.supports.assert_package" - mod3 = import_file_or_module(ImportRef("supports/assert_package.py", is_module=False)) + mod3 = import_file_or_module(ImportRef("supports/assert_package.py", use_module_mode=False)) assert mod3.__package__ == "" assert mod3.__name__ == "assert_package" def test_invalid_source_file_exception(): with pytest.raises(InvalidError, match="Invalid Modal source filename: 'foo.bar.py'"): - import_file_or_module(ImportRef("path/to/foo.bar.py", is_module=False)) + import_file_or_module(ImportRef("path/to/foo.bar.py", use_module_mode=False)) def test_list_cli_commands(): diff --git a/test/live_reload_test.py b/test/live_reload_test.py index ac4726324..cf24515d9 100644 --- a/test/live_reload_test.py +++ b/test/live_reload_test.py @@ -15,7 +15,7 @@ @pytest.fixture def import_ref(test_dir): - return ImportRef(str(test_dir / "supports" / "app_run_tests" / "webhook.py"), is_module=False) + return ImportRef(str(test_dir / "supports" / "app_run_tests" / "webhook.py"), use_module_mode=False) @pytest.mark.asyncio