From 12904f53a4c22057f57d62b232fe832b9e42bfca Mon Sep 17 00:00:00 2001 From: Michael Waskom Date: Wed, 22 Jan 2025 09:36:45 -0500 Subject: [PATCH] Expose Object.hydrate as public API / deprecate Object.resolve (#2769) * Expose Object.hydrate as public API / deprecate Object.resolve * Add some basic test coverage * Polish: add docstring, return self * Fix ruff * Downgrade .resolve deprecation to pending; we use it in integration tests * Require synchronicity 0.9.9 * Fix docstring --- modal/_object.py | 35 +++++++++++++++++++++++++++-------- modal/functions.py | 3 ++- modal/sandbox.py | 3 ++- setup.cfg | 2 +- test/cls_test.py | 2 +- test/container_test.py | 6 +++--- test/object_test.py | 17 +++++++++++++++-- 7 files changed, 51 insertions(+), 17 deletions(-) diff --git a/modal/_object.py b/modal/_object.py index ff5dd89a4..02a487e76 100644 --- a/modal/_object.py +++ b/modal/_object.py @@ -8,9 +8,9 @@ from google.protobuf.message import Message from typing_extensions import Self -from modal._utils.async_utils import aclosing - from ._resolver import Resolver +from ._utils.async_utils import aclosing +from ._utils.deprecation import deprecation_warning from .client import _Client from .config import config, logger from .exception import ExecutionError, InvalidError @@ -238,10 +238,29 @@ def default_deps(*args, **kwargs) -> Sequence["_Object"]: async def resolve(self, client: Optional[_Client] = None): """mdmd:hidden""" + obj = self.__class__.__name__.strip("_") + deprecation_warning( + (2025, 1, 16), + f"The `{obj}.resolve` method is deprecated and will be removed in a future release." + f" Please use `{obj}.hydrate()` or `await {obj}.hydrate.aio()` instead." + "\n\nNote that it is rarely necessary to explicitly hydrate objects, as most methods" + " will lazily hydrate when needed.", + show_source=False, # synchronicity interferes with attributing source correctly + pending=True, + ) + await self.hydrate(client) + + async def hydrate(self, client: Optional[_Client] = None) -> Self: + """Synchronize the local object with its identity on the Modal server. + + It is rarely necessary to call this method explicitly, as most operations + will lazily hydrate when needed. The main use case is when you need to + access object metadata, such as its ID. + """ if self._is_hydrated: - # memory snapshots capture references which must be rehydrated - # on restore to handle staleness. if self.client._snapshotted and not self._is_rehydrated: + # memory snapshots capture references which must be rehydrated + # on restore to handle staleness. logger.debug(f"rehydrating {self} after snapshot") self._is_hydrated = False # un-hydrate and re-resolve c = client if client is not None else await _Client.from_env() @@ -249,20 +268,20 @@ async def resolve(self, client: Optional[_Client] = None): await resolver.load(typing.cast(_Object, self)) self._is_rehydrated = True logger.debug(f"rehydrated {self} with client {id(c)}") - return elif not self._hydrate_lazily: + # TODO(michael) can remove _hydrate lazily? I think all objects support it now? self._validate_is_hydrated() else: - # TODO: this client and/or resolver can't be changed by a caller to X.from_name() c = client if client is not None else await _Client.from_env() resolver = Resolver(c) await resolver.load(self) + return self def live_method(method): @wraps(method) async def wrapped(self, *args, **kwargs): - await self.resolve() + await self.hydrate() return await method(self, *args, **kwargs) return wrapped @@ -271,7 +290,7 @@ async def wrapped(self, *args, **kwargs): def live_method_gen(method): @wraps(method) async def wrapped(self, *args, **kwargs): - await self.resolve() + await self.hydrate() async with aclosing(method(self, *args, **kwargs)) as stream: async for item in stream: yield item diff --git a/modal/functions.py b/modal/functions.py index f240b2418..e00125da0 100644 --- a/modal/functions.py +++ b/modal/functions.py @@ -1207,12 +1207,13 @@ async def web_url(self) -> str: async def is_generator(self) -> bool: """mdmd:hidden""" # hacky: kind of like @live_method, but not hydrating if we have the value already from local source + # TODO(michael) use a common / lightweight method for handling unhydrated metadata properties if self._is_generator is not None: # this is set if the function or class is local return self._is_generator # not set - this is a from_name lookup - hydrate - await self.resolve() + await self.hydrate() assert self._is_generator is not None # should be set now return self._is_generator diff --git a/modal/sandbox.py b/modal/sandbox.py index ea460a4e7..2655ab4a6 100644 --- a/modal/sandbox.py +++ b/modal/sandbox.py @@ -517,8 +517,9 @@ async def exec( raise InvalidError(f"workdir must be an absolute path, got: {workdir}") # Force secret resolution so we can pass the secret IDs to the backend. + # TODO should we parallelize this? for secret in secrets: - await secret.resolve(client=self._client) + await secret.hydrate(client=self._client) task_id = await self._get_task_id() req = api_pb2.ContainerExecRequest( diff --git a/setup.cfg b/setup.cfg index 115f27228..ba49584e6 100644 --- a/setup.cfg +++ b/setup.cfg @@ -26,7 +26,7 @@ install_requires = grpclib==0.4.7 protobuf>=3.19,<6.0,!=4.24.0 rich>=12.0.0 - synchronicity~=0.9.8 + synchronicity~=0.9.9 toml typer>=0.9 types-certifi diff --git a/test/cls_test.py b/test/cls_test.py index 6f826e6a9..e28fb9547 100644 --- a/test/cls_test.py +++ b/test/cls_test.py @@ -358,7 +358,7 @@ def test_lookup(client, servicer): assert obj.bar.local(1, 2) -def test_from_name_lazy_method_resolve(client, servicer): +def test_from_name_lazy_method_hydration(client, servicer): deploy_app(app, "my-cls-app", client=client) cls: Cls = Cls.from_name("my-cls-app", "Foo") diff --git a/test/container_test.py b/test/container_test.py index 1d08f9bd4..eb08ad4b4 100644 --- a/test/container_test.py +++ b/test/container_test.py @@ -2242,10 +2242,10 @@ def test_class_as_service_serialized(servicer): @skip_github_non_linux -def test_function_lazy_resolution(servicer, credentials, set_env_client): +def test_function_lazy_hydration(servicer, credentials, set_env_client): # Deploy some global objects - Volume.from_name("my-vol", create_if_missing=True).resolve() - Queue.from_name("my-queue", create_if_missing=True).resolve() + Volume.from_name("my-vol", create_if_missing=True).hydrate() + Queue.from_name("my-queue", create_if_missing=True).hydrate() # Run container deploy_app_externally(servicer, credentials, "test.supports.lazy_hydration", "app", capture_output=False) diff --git a/test/object_test.py b/test/object_test.py index 0ecea8890..9dd4bf97e 100644 --- a/test/object_test.py +++ b/test/object_test.py @@ -3,8 +3,8 @@ from modal import Secret from modal._object import _Object -from modal.dict import _Dict -from modal.exception import InvalidError +from modal.dict import Dict, _Dict +from modal.exception import InvalidError, PendingDeprecationError from modal.queue import _Queue @@ -22,6 +22,19 @@ def test_new_hydrated(client): _Object._new_hydrated("xy-123", client, None) +def test_on_demand_hydration(client): + obj = Dict.from_name("test-dict", create_if_missing=True).hydrate(client) + assert obj.object_id is not None + + +def test_resolve_deprecation(client): + obj = Dict.from_name("test-dict", create_if_missing=True) + warning = r"Please use `Dict.hydrate\(\)` or `await Dict.hydrate.aio\(\)`" + with pytest.warns(PendingDeprecationError, match=warning): + obj.resolve(client) + assert obj.object_id is not None + + def test_constructor(): with pytest.raises(InvalidError) as excinfo: Secret({"foo": 123})