Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix automount deprecations #2860

Merged
merged 8 commits into from
Feb 12, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modal/_functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,6 @@ def from_local(
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:
Expand Down Expand Up @@ -604,6 +603,7 @@ def from_local(
is_builder_function=True,
is_auto_snapshot=True,
scheduler_placement=scheduler_placement,
include_source=include_source,
)
image = _Image._from_args(
base_images={"base": image},
Expand Down
2 changes: 1 addition & 1 deletion modal/cli/programs/run_jupyter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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) # TODO: remove include_source=True when automount is disabled by default

image = Image.from_registry(args.get("image"), add_python=args.get("add_python")).pip_install("jupyterlab")

Expand Down
2 changes: 1 addition & 1 deletion modal/cli/programs/vscode.py
Original file line number Diff line number Diff line change
Expand Up @@ -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) # TODO: remove include_source=True when automount is disabled by default
image = (
Image.from_registry(args.get("image"), add_python="3.11")
.apt_install("curl", "dumb-init", "git", "git-lfs")
Expand Down
3 changes: 3 additions & 0 deletions modal/image.py
Original file line number Diff line number Diff line change
Expand Up @@ -1924,6 +1924,8 @@ def run_function(
region: Optional[Union[str, Sequence[str]]] = None, # Region or regions to run the function on.
args: Sequence[Any] = (), # Positional arguments to the function.
kwargs: dict[str, Any] = {}, # Keyword arguments to the function.
*,
include_source: Optional[bool] = None,
) -> "_Image":
"""Run user-defined function `raw_f` as an image build step. The function runs just like an ordinary Modal
function, and any kwargs accepted by `@app.function` (such as `Mount`s, `NetworkFileSystem`s,
Expand Down Expand Up @@ -1979,6 +1981,7 @@ def my_build_function():
timeout=timeout,
cpu=cpu,
is_builder_function=True,
include_source=include_source,
)
if len(args) + len(kwargs) > 0:
args_serialized = serialize((args, kwargs))
Expand Down
1 change: 0 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ module = [
]
ignore_errors = true


[tool.pytest.ini_options]
timeout = 300
addopts = "--ignore=modal/cli/programs"
Expand Down
16 changes: 8 additions & 8 deletions test/cli_imports_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ def main():
"""
python_module_src = """
import modal
app = modal.App("FOO")
other_app = modal.App("BAR")
app = modal.App("FOO", include_source=True) # TODO: remove include_source=True)
other_app = modal.App("BAR", include_source=True) # TODO: remove include_source=True)
@other_app.function()
def func():
pass
Expand All @@ -45,8 +45,8 @@ def meth(self):

python_package_src = """
import modal
app = modal.App("FOO")
other_app = modal.App("BAR")
app = modal.App("FOO", include_source=True) # TODO: remove include_source=True)
other_app = modal.App("BAR", include_source=True) # TODO: remove include_source=True)
@other_app.function()
def func():
pass
Expand All @@ -55,8 +55,8 @@ def func():

python_subpackage_src = """
import modal
app = modal.App("FOO")
other_app = modal.App("BAR")
app = modal.App("FOO", include_source=True) # TODO: remove include_source=True)
other_app = modal.App("BAR", include_source=True) # TODO: remove include_source=True)
@other_app.function()
def func():
pass
Expand All @@ -65,8 +65,8 @@ def func():

python_file_src = """
import modal
app = modal.App("FOO")
other_app = modal.App("BAR")
app = modal.App("FOO", include_source=True) # TODO: remove include_source=True)
other_app = modal.App("BAR", include_source=True) # TODO: remove include_source=True)
@other_app.function()
def func():
pass
Expand Down
2 changes: 1 addition & 1 deletion test/cli_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@

import other_module

app = modal.App("my_app")
app = modal.App("my_app", include_source=True) # TODO: remove include_source=True)

# Sanity check that the module is imported properly
import sys
Expand Down
45 changes: 23 additions & 22 deletions test/cls_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

from .supports.base_class import BaseCls2

app = App("app")
app = App("app", include_source=True)


@pytest.fixture(autouse=True)
Expand Down Expand Up @@ -68,7 +68,7 @@ def test_run_class(client, servicer):
assert len(servicer.precreated_functions) == 0
assert servicer.n_functions == 0
with app.run(client=client):
method_handle_object_id = Foo.bar.object_id # method handle object id will probably go away
method_handle_object_id = Foo._get_class_service_function().object_id # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the subsequent assertion actually doing anything useful with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does ensure that the underlying class service function has been hydrated by app.run, which we probably test in other places too, but it doesn't hurt to have another assertion about it here I think

assert isinstance(Foo, Cls)
assert isinstance(NoParamsCls, Cls)
class_id = Foo.object_id
Expand Down Expand Up @@ -152,7 +152,7 @@ def test_class_with_options_need_hydrating(client, servicer):
# Reusing the app runs into an issue with stale function handles.
# TODO (akshat): have all the client tests use separate apps, and throw
# an exception if the user tries to reuse an app.
app_remote = App()
app_remote = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@app_remote.cls(cpu=42)
Expand Down Expand Up @@ -192,7 +192,7 @@ def test_call_cls_remote_modal_type(client):
FooRemote(42, q) # type: ignore


app_2 = App()
app_2 = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@app_2.cls(cpu=42)
Expand Down Expand Up @@ -234,7 +234,7 @@ def bar(self, x):
assert bound_bar(100) == 1000000


app_remote_2 = App()
app_remote_2 = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@app_remote_2.cls(cpu=42)
Expand All @@ -255,7 +255,7 @@ async def test_call_cls_remote_async(client):
assert await bar_remote.baz.remote.aio(8) == 64 # Mock servicer just squares the argument


app_local = App()
app_local = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@app_local.cls(cpu=42, enable_memory_snapshot=True)
Expand Down Expand Up @@ -298,7 +298,7 @@ def test_can_call_remotely_from_local(client):
assert foo.baz.remote(9) == 81


app_remote_3 = App()
app_remote_3 = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@app_remote_3.cls(cpu=42)
Expand Down Expand Up @@ -334,10 +334,10 @@ def test_lookup(client, servicer):

# objects are resolved
assert cls.object_id.startswith("cs-")
assert cls.bar.object_id.startswith("fu-")
assert cls._get_class_service_function().object_id.startswith("fu-")

# Check that function properties are preserved
assert cls.bar.is_generator is False
assert cls().bar.is_generator is False

# Make sure we can instantiate the class
with servicer.intercept() as ctx:
Expand Down Expand Up @@ -412,7 +412,7 @@ def test_lookup_lazy_spawn(client, servicer):
assert function_call.get() == 7693


baz_app = App()
baz_app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@baz_app.cls()
Expand All @@ -430,7 +430,7 @@ def test_call_not_modal_method():
assert baz.not_modal_method(7) == 35


cls_with_enter_app = App()
cls_with_enter_app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


def get_thread_id():
Expand Down Expand Up @@ -504,7 +504,7 @@ async def test_async_enter_on_local_modal_call():
assert obj.entered


inheritance_app = App()
inheritance_app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


class BaseCls:
Expand All @@ -528,7 +528,7 @@ def test_derived_cls(client, servicer):
assert DerivedCls().run.remote(3) == 9


inheritance_app_2 = App()
inheritance_app_2 = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@inheritance_app_2.cls()
Expand Down Expand Up @@ -563,7 +563,7 @@ def test_rehydrate(client, servicer, reset_container_app):
assert obj.bar.local(7) == 343


app_unhydrated = App()
app_unhydrated = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@app_unhydrated.cls()
Expand All @@ -578,7 +578,7 @@ def test_unhydrated():
foo.bar.remote(42)


app_method_args = App()
app_method_args = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@app_method_args.cls(keep_warm=5)
Expand Down Expand Up @@ -684,7 +684,7 @@ def test_handlers():
assert list(pfs.keys()) == ["my_exit"]


web_app_app = App()
web_app_app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@web_app_app.cls()
Expand All @@ -705,7 +705,7 @@ def test_web_cls(client):
assert c.asgi.web_url == "http://asgi.internal"


handler_app = App("handler-app")
handler_app = App("handler-app", include_source=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you intend not to remove this later or did you just get tired of writing the comment everywhere? 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was one of the instances where my regex didn't trigger so I made the edit manually :D



image = Image.debian_slim().pip_install("xyz")
Expand Down Expand Up @@ -733,7 +733,7 @@ def test_build_image(client, servicer):
assert servicer.force_built_images == []


other_handler_app = App("other-handler-app")
other_handler_app = App("other-handler-app", include_source=True)


with pytest.warns(DeprecationError, match="@modal.build"):
Expand All @@ -758,7 +758,7 @@ def test_force_build_image(client, servicer):
assert servicer.force_built_images == ["im-3"]


build_timeout_handler_app = App("build-timeout-handler-app")
build_timeout_handler_app = App("build-timeout-handler-app", include_source=True)


with pytest.warns(DeprecationError, match="@modal.build"):
Expand Down Expand Up @@ -867,7 +867,7 @@ def test_cross_process_userclass_serde(supports_dir):
assert revived_cls().method() == "a" # this should be bound to the object


app2 = modal.App("app2")
app2 = App("app2", include_source=True)


@app2.cls()
Expand Down Expand Up @@ -954,7 +954,7 @@ def __init__(self):
pass


app_batched = App()
app_batched = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


def test_batched_method_duplicate_error(client):
Expand Down Expand Up @@ -1074,4 +1074,5 @@ def method(self):


def test_method_on_cls_access_warns():
print(Foo.bar)
with pytest.warns(match="instantiate classes before using methods"):
print(Foo.bar)
2 changes: 1 addition & 1 deletion test/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -2168,7 +2168,7 @@ def import_fail_for_rich(name: str, *args, **kwargs) -> ModuleType:
yield


@pytest.fixture
@pytest.fixture(autouse=True)
def disable_auto_mount(monkeypatch):
monkeypatch.setenv("MODAL_AUTOMOUNT", "0")
yield
Expand Down
2 changes: 1 addition & 1 deletion test/container_buffer_test.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Copyright Modal Labs 2024
from modal import App

app = App()
app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@app.function(
Expand Down
4 changes: 2 additions & 2 deletions test/function_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from modal_proto import api_pb2
from test.helpers import deploy_app_externally

app = App()
app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


if os.environ.get("GITHUB_ACTIONS") == "true":
Expand Down Expand Up @@ -716,7 +716,7 @@ def test_from_id_iter_gen(client, servicer, is_generator):
assert rehydrated_function_call.get() == "hello"


lc_app = App()
lc_app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@lc_app.function()
Expand Down
2 changes: 1 addition & 1 deletion test/gpu_fallbacks_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from modal import App
from modal_proto import api_pb2

app = App()
app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@app.function(gpu=["a10g"])
Expand Down
2 changes: 1 addition & 1 deletion test/i6pn_clustered_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import modal.experimental
from modal import App

app = App()
app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@app.function()
Expand Down
4 changes: 2 additions & 2 deletions test/image_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1060,7 +1060,7 @@ def test_hydration_metadata(servicer, client):
assert_metadata(img, dummy_metadata)


cls_app = App()
cls_app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default

VARIABLE_5 = 1
VARIABLE_6 = 1
Expand Down Expand Up @@ -1448,7 +1448,7 @@ def get_hash(img: Image) -> str:
assert get_hash(img) == "78d579f243c21dcaa59e5daf97f732e2453b004bc2122de692617d4d725c6184"


parallel_app = App()
parallel_app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@parallel_app.function(image=Image.debian_slim().run_commands("sleep 1", "echo hi"))
Expand Down
1 change: 1 addition & 0 deletions test/mounted_files_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ def symlinked_python_installation_venv_path(tmp_path, repo_root):
def test_mounted_files_symlinked_python_install(
symlinked_python_installation_venv_path, supports_dir, server_url_env, token_env, servicer
):
# TODO(elias): This test fails when run from a uv-managed virtualenv
subprocess.check_call(
[symlinked_python_installation_venv_path / "bin" / "modal", "run", supports_dir / "imports_ast.py"]
)
Expand Down
2 changes: 1 addition & 1 deletion test/schedule_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from modal import App, Period
from modal_proto import api_pb2

app = App()
app = App(include_source=True) # TODO: remove include_source=True when automount is disabled by default


@app.function(schedule=Period(seconds=5))
Expand Down
Loading
Loading