From a2d4683c5a2e95cd885e618fe6a6e44468076b3d Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Thu, 20 Feb 2025 10:21:15 +0100 Subject: [PATCH 1/4] Remove/move is_local checks to make it possible to deploy apps with mounts from within running functions iirc, the checks were mostly in place to prevent unnecessary logic from running in global scope of containers, but just adding pre-defined mounts to a list wouldn't account for many clock cycles, and this change still prevents auto-mounting from running within containers (soon to be deprecated anyways) --- blah.py | 0 modal/_functions.py | 78 ++++++++++++++++++++------------------------- 2 files changed, 34 insertions(+), 44 deletions(-) create mode 100644 blah.py diff --git a/blah.py b/blah.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/modal/_functions.py b/modal/_functions.py index 5c09af94fd..406a21c071 100644 --- a/modal/_functions.py +++ b/modal/_functions.py @@ -473,51 +473,41 @@ def from_local( explicit_mounts = mounts - if is_local(): - include_source_mode = get_include_source_mode(include_source) - if include_source_mode != IncludeSourceMode.INCLUDE_NOTHING: - entrypoint_mounts = info.get_entrypoint_mount() - else: - entrypoint_mounts = {} - - all_mounts = [ - _get_client_mount(), - *explicit_mounts, - *entrypoint_mounts.values(), - ] - - if include_source_mode is IncludeSourceMode.INCLUDE_FIRST_PARTY: - auto_mounts = get_sys_modules_mounts() - # don't need to add entrypoint modules to automounts: - for entrypoint_module in entrypoint_mounts: - auto_mounts.pop(entrypoint_module, None) - - warn_missing_modules = set(auto_mounts.keys()) - image._added_python_source_set - - if warn_missing_modules: - python_stringified_modules = ", ".join(f'"{mod}"' for mod in sorted(warn_missing_modules)) - deprecation_warning( - (2025, 2, 3), - ( - 'Modal will stop implicitly adding local Python modules to the Image ("automounting") in a ' - "future update. The following modules need to be explicitly added for future " - "compatibility:\n" - ) - + "\n".join(sorted([f"* {m}" for m in warn_missing_modules])) - + "\n\n" - + ( - "e.g.:\n" - f"image_with_source = my_image.add_local_python_source({python_stringified_modules})\n\n" - ) - + "For more information, see https://modal.com/docs/guide/modal-1-0-migration", - ) - all_mounts += auto_mounts.values() + include_source_mode = get_include_source_mode(include_source) + if include_source_mode != IncludeSourceMode.INCLUDE_NOTHING: + entrypoint_mounts = info.get_entrypoint_mount() else: - # skip any mount introspection/logic inside containers, since the function - # should already be hydrated - # TODO: maybe the entire from_args loader should be exited early if not local? - # since it will be hydrated - all_mounts = [] + entrypoint_mounts = {} + + all_mounts = [ + _get_client_mount(), + *explicit_mounts, + *entrypoint_mounts.values(), + ] + + if include_source_mode is IncludeSourceMode.INCLUDE_FIRST_PARTY and is_local(): + auto_mounts = get_sys_modules_mounts() + # don't need to add entrypoint modules to automounts: + for entrypoint_module in entrypoint_mounts: + auto_mounts.pop(entrypoint_module, None) + + warn_missing_modules = set(auto_mounts.keys()) - image._added_python_source_set + + if warn_missing_modules: + python_stringified_modules = ", ".join(f'"{mod}"' for mod in sorted(warn_missing_modules)) + deprecation_warning( + (2025, 2, 3), + ( + 'Modal will stop implicitly adding local Python modules to the Image ("automounting") in a ' + "future update. The following modules need to be explicitly added for future " + "compatibility:\n" + ) + + "\n".join(sorted([f"* {m}" for m in warn_missing_modules])) + + "\n\n" + + (f"e.g.:\nimage_with_source = my_image.add_local_python_source({python_stringified_modules})\n\n") + + "For more information, see https://modal.com/docs/guide/modal-1-0-migration", + ) + all_mounts += auto_mounts.values() retry_policy = _parse_retries( retries, f"Function '{info.get_tag()}'" if info.raw_f else f"Class '{info.get_tag()}'" From 89e5db4c647675519324e593c63b15b4f183c3ed Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Thu, 20 Feb 2025 10:38:52 +0100 Subject: [PATCH 2/4] Remove dummy file --- blah.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 blah.py diff --git a/blah.py b/blah.py deleted file mode 100644 index e69de29bb2..0000000000 From daa054c71ba0eecebe5cf0abbfcaa3f727740232 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Thu, 20 Feb 2025 10:44:54 +0100 Subject: [PATCH 3/4] Removes prevention for running app.run within Modal functions The intent of it was to prevent usage of app.run in global scope of modules that would then get loaded in containers causing an infinite recursion. This is not as likely to happen anymore now that we have the `modal run` cli instead of `if __name__ == __main__` as the main app launcher mechanic --- modal/runner.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/modal/runner.py b/modal/runner.py index 7c773d8920..7b798b32da 100644 --- a/modal/runner.py +++ b/modal/runner.py @@ -19,7 +19,6 @@ from ._object import _get_environment_name, _Object from ._pty import get_pty_info from ._resolver import Resolver -from ._runtime.execution_context import is_local from ._traceback import print_server_warnings, traceback_contains_remote_call from ._utils.async_utils import TaskContext, gather_cancel_on_exc, synchronize_api from ._utils.deprecation import deprecation_error @@ -262,12 +261,6 @@ async def _run_app( if environment_name is None: environment_name = typing.cast(str, config.get("environment")) - if not is_local(): - raise InvalidError( - "Can not run an app from within a container." - " Are you calling app.run() directly?" - " Consider using the `modal run` shell command." - ) if app._running_app: raise InvalidError( "App is already running and can't be started again.\n" From 68855cbcf2a72869fe0716dd45f615efe8d5590e Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 24 Feb 2025 14:07:48 +0100 Subject: [PATCH 4/4] Apply Erik's suggestion of still erroring during module imports, but allowing app.run within functions themselves --- modal/_container_entrypoint.py | 32 +++++++++++++++-------------- modal/_runtime/execution_context.py | 13 ++++++++++++ modal/runner.py | 4 ++++ test/container_test.py | 5 +---- 4 files changed, 35 insertions(+), 19 deletions(-) diff --git a/modal/_container_entrypoint.py b/modal/_container_entrypoint.py index bba1fed067..39b2e26a02 100644 --- a/modal/_container_entrypoint.py +++ b/modal/_container_entrypoint.py @@ -2,6 +2,7 @@ # ruff: noqa: E402 import os +from modal._runtime import execution_context from modal._runtime.user_code_imports import Service, import_class_service, import_single_function_service telemetry_socket = os.environ.get("MODAL_TELEMETRY_SOCKET") @@ -428,21 +429,22 @@ def main(container_args: api_pb2.ContainerArguments, client: Client): param_args = () param_kwargs = {} - if function_def.is_class: - service = import_class_service( - function_def, - ser_cls, - param_args, - param_kwargs, - ) - else: - service = import_single_function_service( - function_def, - ser_cls, - ser_fun, - param_args, - param_kwargs, - ) + with execution_context._import_context(): + if function_def.is_class: + service = import_class_service( + function_def, + ser_cls, + param_args, + param_kwargs, + ) + else: + service = import_single_function_service( + function_def, + ser_cls, + ser_fun, + param_args, + param_kwargs, + ) # If the cls/function decorator was applied in local scope, but the app is global, we can look it up if service.app is not None: diff --git a/modal/_runtime/execution_context.py b/modal/_runtime/execution_context.py index 95f4806990..d3592ac410 100644 --- a/modal/_runtime/execution_context.py +++ b/modal/_runtime/execution_context.py @@ -1,4 +1,5 @@ # Copyright Modal Labs 2024 +from contextlib import contextmanager from contextvars import ContextVar from typing import Callable, Optional @@ -87,3 +88,15 @@ def _reset_current_context_ids(): _current_input_id: ContextVar = ContextVar("_current_input_id") _current_function_call_id: ContextVar = ContextVar("_current_function_call_id") + +_is_currently_importing = False # we set this to True while a container is importing user code + + +@contextmanager +def _import_context(): + global _is_currently_importing + _is_currently_importing = True + try: + yield + finally: + _is_currently_importing = False diff --git a/modal/runner.py b/modal/runner.py index 7b798b32da..3b1b92ae46 100644 --- a/modal/runner.py +++ b/modal/runner.py @@ -12,6 +12,7 @@ from grpclib import GRPCError, Status from synchronicity.async_wrap import asynccontextmanager +import modal._runtime.execution_context import modal_proto.api_pb2 from modal_proto import api_pb2 @@ -261,6 +262,9 @@ async def _run_app( if environment_name is None: environment_name = typing.cast(str, config.get("environment")) + if modal._runtime.execution_context._is_currently_importing: + raise InvalidError("Can not run an app in global scope within a container") + if app._running_app: raise InvalidError( "App is already running and can't be started again.\n" diff --git a/test/container_test.py b/test/container_test.py index c153b83a4d..6beb206172 100644 --- a/test/container_test.py +++ b/test/container_test.py @@ -536,11 +536,8 @@ def test_grpc_failure(servicer, event_loop): def test_missing_main_conditional(servicer, capsys): _run_container(servicer, "test.supports.missing_main_conditional", "square") output = capsys.readouterr() - assert "Can not run an app from within a container" in output.err - + assert "Can not run an app in global scope within a container" in output.err assert servicer.task_result.status == api_pb2.GenericResult.GENERIC_STATUS_FAILURE - assert "modal run" in servicer.task_result.traceback - exc = deserialize(servicer.task_result.data, None) assert isinstance(exc, InvalidError)