-
Notifications
You must be signed in to change notification settings - Fork 44
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
Changes from all commits
c930436
0892dab
fca8452
bca01ff
b840789
7dd4b5b
9e8abe1
7e398e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ | |
|
||
from .supports.base_class import BaseCls2 | ||
|
||
app = App("app") | ||
app = App("app", include_source=True) | ||
|
||
|
||
@pytest.fixture(autouse=True) | ||
|
@@ -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 | ||
assert isinstance(Foo, Cls) | ||
assert isinstance(NoParamsCls, Cls) | ||
class_id = Foo.object_id | ||
|
@@ -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) | ||
|
@@ -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) | ||
|
@@ -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) | ||
|
@@ -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) | ||
|
@@ -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) | ||
|
@@ -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: | ||
|
@@ -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() | ||
|
@@ -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(): | ||
|
@@ -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: | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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) | ||
|
@@ -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() | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? 😁 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
|
@@ -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"): | ||
|
@@ -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"): | ||
|
@@ -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() | ||
|
@@ -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): | ||
|
@@ -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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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