diff --git a/modal/_functions.py b/modal/_functions.py index 291ec2a347..0e1008df63 100644 --- a/modal/_functions.py +++ b/modal/_functions.py @@ -65,7 +65,7 @@ ) from .gpu import GPU_T, parse_gpu_config from .image import _Image -from .mount import _get_client_mount, _Mount, get_auto_mounts +from .mount import _get_client_mount, _Mount, get_sys_modules_mounts from .network_file_system import _NetworkFileSystem, network_file_system_mount_protos from .output import _get_output_manager from .parallel_map import ( @@ -434,7 +434,7 @@ def _bind_method( return fun @staticmethod - def from_args( + def from_local( info: FunctionInfo, app, image: _Image, @@ -501,18 +501,41 @@ def from_args( if include_source_mode != IncludeSourceMode.INCLUDE_NOTHING: entrypoint_mounts = info.get_entrypoint_mount() else: - entrypoint_mounts = [] + entrypoint_mounts = {} all_mounts = [ _get_client_mount(), *explicit_mounts, - *entrypoint_mounts, + *entrypoint_mounts.values(), ] if include_source_mode is IncludeSourceMode.INCLUDE_FIRST_PARTY: - # TODO(elias): if using INCLUDE_FIRST_PARTY *and* mounts are added that haven't already been - # added to the image via add_local_python_source - all_mounts += get_auto_mounts() + 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", + pending=True, + ) + all_mounts += auto_mounts.values() else: # skip any mount introspection/logic inside containers, since the function # should already be hydrated @@ -561,7 +584,7 @@ def from_args( for k, pf in build_functions: build_function = pf.raw_f snapshot_info = FunctionInfo(build_function, user_cls=info.user_cls) - snapshot_function = _Function.from_args( + snapshot_function = _Function.from_local( snapshot_info, app=None, image=image, diff --git a/modal/_utils/function_utils.py b/modal/_utils/function_utils.py index 56ec4a42e5..b367b5eaee 100644 --- a/modal/_utils/function_utils.py +++ b/modal/_utils/function_utils.py @@ -308,7 +308,7 @@ def class_parameter_info(self) -> api_pb2.ClassParameterInfo: format=api_pb2.ClassParameterInfo.PARAM_SERIALIZATION_FORMAT_PROTO, schema=modal_parameters ) - def get_entrypoint_mount(self) -> list[_Mount]: + def get_entrypoint_mount(self) -> dict[str, _Mount]: """ Includes: * Implicit mount of the function itself (the module or package that the function is part of) @@ -322,7 +322,7 @@ def get_entrypoint_mount(self) -> list[_Mount]: """ if self.is_serialized(): # Don't auto-mount anything for serialized functions (including notebooks) - return [] + return {} # make sure the function's own entrypoint is included: if self._type == FunctionInfoType.PACKAGE: @@ -331,21 +331,23 @@ def get_entrypoint_mount(self) -> list[_Mount]: # includes non-.py files, since we'll want to migrate to .py-only # soon to get it consistent with the `add_local_python_source()` # defaults. - return [_Mount._from_local_python_packages(top_level_package)] + return {top_level_package: _Mount._from_local_python_packages(top_level_package)} elif self._type == FunctionInfoType.FILE: - remote_path = ROOT_DIR / Path(self._file).name + # TODO: inspect if this file is already included as part of + # a package mount, and skip it + reference that package + # instead if that's the case. This avoids possible module + # duplication bugs + module_file = Path(self._file) + container_module_name = module_file.stem + remote_path = ROOT_DIR / module_file.name if not _is_modal_path(remote_path): - # TODO: inspect if this file is already included as part of - # a package mount, and skip it + reference that package - # instead if that's the case. This avoids possible module - # duplication bugs - return [ - _Mount._from_local_file( + return { + container_module_name: _Mount._from_local_file( self._file, remote_path=remote_path, ) - ] - return [] # this should never be reached... + } + return {} # this should never be reached... def get_tag(self): return self.function_name diff --git a/modal/app.py b/modal/app.py index 7643e8193b..c208ba032f 100644 --- a/modal/app.py +++ b/modal/app.py @@ -724,7 +724,7 @@ def f(self, x): raise InvalidError("`region` and `_experimental_scheduler_placement` cannot be used together") scheduler_placement = SchedulerPlacement(region=region) - function = _Function.from_args( + function = _Function.from_local( info, app=self, image=image, @@ -859,7 +859,7 @@ def wrapper(user_cls: CLS_T) -> CLS_T: info = FunctionInfo(None, serialized=serialized, user_cls=user_cls) - cls_func = _Function.from_args( + cls_func = _Function.from_local( info, app=self, image=image or self._get_default_image(), diff --git a/modal/image.py b/modal/image.py index e27720c7d0..369cad291c 100644 --- a/modal/image.py +++ b/modal/image.py @@ -402,12 +402,14 @@ class _Image(_Object, type_prefix="im"): _deferred_mounts: Sequence[ _Mount ] # added as mounts on any container referencing the Image, see `def _mount_layers` + _added_python_source_set: frozenset[str] # used to warn about missing mounts during auto-mount deprecation _metadata: Optional[api_pb2.ImageMetadata] = None # set on hydration, private for now def _initialize_from_empty(self): self.inside_exceptions = [] self._serve_mounts = frozenset() self._deferred_mounts = () + self._added_python_source_set = frozenset() self.force_build = False def _initialize_from_other(self, other: "_Image"): @@ -416,6 +418,7 @@ def _initialize_from_other(self, other: "_Image"): self.force_build = other.force_build self._serve_mounts = other._serve_mounts self._deferred_mounts = other._deferred_mounts + self._added_python_source_set = other._added_python_source_set def _hydrate_metadata(self, metadata: Optional[Message]): env_image_id = config.get("image_id") # set as an env var in containers @@ -440,7 +443,9 @@ async def _load(self2: "_Image", resolver: Resolver, existing_object_id: Optiona self2._deferred_mounts = tuple(base_image._deferred_mounts) + (mount,) self2._serve_mounts = base_image._serve_mounts | ({mount} if mount.is_local() else set()) - return _Image._from_loader(_load, "Image(local files)", deps=lambda: [base_image, mount]) + img = _Image._from_loader(_load, "Image(local files)", deps=lambda: [base_image, mount]) + img._added_python_source_set = base_image._added_python_source_set + return img @property def _mount_layers(self) -> typing.Sequence[_Mount]: @@ -659,6 +664,9 @@ async def _load(self: _Image, resolver: Resolver, existing_object_id: Optional[s rep = f"Image({dockerfile_function})" obj = _Image._from_loader(_load, rep, deps=_deps) obj.force_build = force_build + obj._added_python_source_set = frozenset.union( + frozenset(), *(base._added_python_source_set for base in base_images.values()) + ) return obj def copy_mount(self, mount: _Mount, remote_path: Union[str, Path] = ".") -> "_Image": @@ -843,7 +851,9 @@ def add_local_python_source( ``` """ mount = _Mount._from_local_python_packages(*modules, ignore=ignore) - return self._add_mount_layer_or_copy(mount, copy=copy) + img = self._add_mount_layer_or_copy(mount, copy=copy) + img._added_python_source_set |= set(modules) + return img def copy_local_dir( self, @@ -1954,7 +1964,7 @@ def my_build_function(): info = FunctionInfo(raw_f) - function = _Function.from_args( + function = _Function.from_local( info, app=None, image=self, # type: ignore[reportArgumentType] # TODO: probably conflict with type stub? diff --git a/modal/mount.py b/modal/mount.py index 82d41494a4..f862fbb16e 100644 --- a/modal/mount.py +++ b/modal/mount.py @@ -821,14 +821,14 @@ def _is_modal_path(remote_path: PurePosixPath): return False -def get_auto_mounts() -> list[_Mount]: +def get_sys_modules_mounts() -> dict[str, _Mount]: """mdmd:hidden Auto-mount local modules that have been imported in global scope. This may or may not include the "entrypoint" of the function as well, depending on how modal is invoked Note: sys.modules may change during the iteration """ - auto_mounts = [] + auto_mounts = {} top_level_modules = [] skip_prefixes = set() for name, module in sorted(sys.modules.items(), key=lambda kv: len(kv[0])): @@ -858,6 +858,6 @@ def get_auto_mounts() -> list[_Mount]: # skip any module that has paths in SYS_PREFIXES, or would overwrite the modal Package in the container break else: - auto_mounts.append(potential_mount) + auto_mounts[module_name] = potential_mount return auto_mounts diff --git a/test/cli_test.py b/test/cli_test.py index 4d6fd7f3a3..1cdc16e2b2 100644 --- a/test/cli_test.py +++ b/test/cli_test.py @@ -319,7 +319,7 @@ def test_run_parse_args_entrypoint(servicer, set_env_client, test_dir): assert "Unable to generate command line interface for app entrypoint." in str(res.exception) -def test_run_parse_args_function(servicer, set_env_client, test_dir, recwarn): +def test_run_parse_args_function(servicer, set_env_client, test_dir, recwarn, disable_auto_mount): app_file = test_dir / "supports" / "app_run_tests" / "cli_args.py" res = _run(["run", app_file.as_posix()], expected_exit_code=1, expected_stderr=None) assert "Specify a Modal Function or local entrypoint to run" in res.stderr diff --git a/test/cls_test.py b/test/cls_test.py index 8093f5a4ac..8d61ba0f5e 100644 --- a/test/cls_test.py +++ b/test/cls_test.py @@ -1052,7 +1052,7 @@ def test_modal_object_param_uses_wrapped_type(servicer, set_env_client, client): assert type(kwargs["x"]) == type(dct) -def test_using_method_on_uninstantiated_cls(recwarn): +def test_using_method_on_uninstantiated_cls(recwarn, disable_auto_mount): app = App() @app.cls(serialized=True) diff --git a/test/mount_test.py b/test/mount_test.py index 1a22e668d6..b6d5c388c7 100644 --- a/test/mount_test.py +++ b/test/mount_test.py @@ -4,6 +4,7 @@ import platform import pytest from pathlib import Path, PurePosixPath +from test.helpers import deploy_app_externally from modal import App, FilePatternMatcher from modal._utils.blob_utils import LARGE_FILE_LIMIT @@ -168,3 +169,31 @@ def test_mount_from_local_dir_ignore(test_dir, tmp_path_with_content): file_names = [file.mount_filename for file in Mount._get_files(entries=mount.entries)] assert set(file_names) == expected + + +def test_missing_python_source_warning(servicer, credentials, supports_dir): + # should warn if function doesn't have an imported non-third-party package attached + # either through add OR copy mode, unless automount=False mode is used + def has_warning(output: str): + return '.add_local_python_source("pkg_a")' in output + + output = deploy_app_externally(servicer, credentials, "pkg_d.main", cwd=supports_dir, capture_output=True) + assert has_warning(output) + + # adding the source to the image should make the warning disappear + output = deploy_app_externally( + servicer, credentials, "pkg_d.main", cwd=supports_dir, capture_output=True, env={"ADD_SOURCE": "add"} + ) + assert not has_warning(output) + + # *copying* the source to the image should make the warning disappear too + output = deploy_app_externally( + servicer, credentials, "pkg_d.main", cwd=supports_dir, capture_output=True, env={"ADD_SOURCE": "copy"} + ) + assert not has_warning(output) + + # disabling auto-mount explicitly should make warning disappear + output = deploy_app_externally( + servicer, credentials, "pkg_d.main", cwd=supports_dir, capture_output=True, env={"MODAL_AUTOMOUNT": "0"} + ) + assert not has_warning(output) diff --git a/test/mounted_files_test.py b/test/mounted_files_test.py index eca00d14d0..1338787615 100644 --- a/test/mounted_files_test.py +++ b/test/mounted_files_test.py @@ -9,7 +9,7 @@ import modal from modal import Mount -from modal.mount import get_auto_mounts +from modal.mount import get_sys_modules_mounts from . import helpers from .supports.skip import skip_windows @@ -51,7 +51,7 @@ async def env_mount_files(): # If something is installed using pip -e, it will be bundled up as a part of the environment. # Those are env-specific so we ignore those as a part of the test filenames = [] - for mount in get_auto_mounts(): + for mount in get_sys_modules_mounts().values(): async for file_info in mount._get_files(mount.entries): filenames.append(file_info.mount_filename) diff --git a/test/supports/pkg_d/__init__.py b/test/supports/pkg_d/__init__.py new file mode 100644 index 0000000000..7ad460662a --- /dev/null +++ b/test/supports/pkg_d/__init__.py @@ -0,0 +1 @@ +# Copyright Modal Labs 2025 diff --git a/test/supports/pkg_d/main.py b/test/supports/pkg_d/main.py new file mode 100644 index 0000000000..f227332e86 --- /dev/null +++ b/test/supports/pkg_d/main.py @@ -0,0 +1,23 @@ +# Copyright Modal Labs 2025 +import os + +from pkg_a import a # noqa # this would cause an automount warning + +import modal + +app = modal.App() + +image = modal.Image.debian_slim() + +if os.environ.get("ADD_SOURCE") == "add": + # intentionally makes add local not the last call, to make sure the added modules transfer to downstream layers + image = image.add_local_python_source("pkg_a").add_local_file(__file__, "/tmp/blah") + +elif os.environ.get("ADD_SOURCE") == "copy": + # intentionally makes add local not the last call, to make sure the added modules transfer to downstream layers + image = image.add_local_python_source("pkg_a", copy=True).run_commands("echo hello") + + +@app.function(image=image) +def f(): + pass diff --git a/test/supports/pkg_d/sibling.py b/test/supports/pkg_d/sibling.py new file mode 100644 index 0000000000..7ad460662a --- /dev/null +++ b/test/supports/pkg_d/sibling.py @@ -0,0 +1 @@ +# Copyright Modal Labs 2025