From 45a4df9a78edd25bc0b8ec8af20a1e3f392e7745 Mon Sep 17 00:00:00 2001 From: Elias Freider Date: Mon, 13 Jan 2025 10:14:19 +0100 Subject: [PATCH] Adds deprecation warning when trying to access method directly on class [CLI-302] (#2755) * Date * Types --- modal/cls.py | 14 +++++++++++++- test/cls_test.py | 26 ++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/modal/cls.py b/modal/cls.py index 24ae7ad2e..b14c5d402 100644 --- a/modal/cls.py +++ b/modal/cls.py @@ -16,7 +16,7 @@ from ._serialization import check_valid_cls_constructor_arg from ._traceback import print_server_warnings from ._utils.async_utils import synchronize_api, synchronizer -from ._utils.deprecation import renamed_parameter +from ._utils.deprecation import deprecation_warning, renamed_parameter from ._utils.grpc_utils import retry_transient_errors from ._utils.mount_utils import validate_volumes from .client import _Client @@ -374,12 +374,14 @@ class _Cls(_Object, type_prefix="cs"): _options: Optional[api_pb2.FunctionOptions] _callables: dict[str, Callable[..., Any]] _app: Optional["modal.app._App"] = None # not set for lookups + _name: Optional[str] def _initialize_from_empty(self): self._user_cls = None self._class_service_function = None self._options = None self._callables = {} + self._name = None def _initialize_from_other(self, other: "_Cls"): super()._initialize_from_other(other) @@ -388,6 +390,7 @@ def _initialize_from_other(self, other: "_Cls"): self._method_functions = other._method_functions self._options = other._options self._callables = other._callables + self._name = other._name def _get_partial_functions(self) -> dict[str, _PartialFunction]: if not self._user_cls: @@ -506,6 +509,7 @@ async def _load(self: "_Cls", resolver: Resolver, existing_object_id: Optional[s cls._class_service_function = class_service_function cls._method_functions = method_functions cls._callables = callables + cls._name = user_cls.__name__ return cls def _uses_common_service_function(self): @@ -576,6 +580,7 @@ async def _load_remote(obj: _Object, resolver: Resolver, existing_object_id: Opt rep = f"Ref({app_name})" cls = cls._from_loader(_load_remote, rep, is_another_app=True, hydrate_lazily=True) # TODO: when pre 0.63 is phased out, we can set class_service_function here instead + cls._name = name return cls def with_options( @@ -681,6 +686,13 @@ def __getattr__(self, k): # Used by CLI and container entrypoint # TODO: remove this method - access to attributes on classes should be discouraged if k in self._method_functions: + deprecation_warning( + (2025, 1, 13), + "Usage of methods directly on the class will soon be deprecated, " + "instantiate classes before using methods, e.g.:\n" + f"{self._name}().{k} instead of {self._name}.{k}", + pending=True, + ) return self._method_functions[k] return getattr(self._user_cls, k) diff --git a/test/cls_test.py b/test/cls_test.py index 68d33c5f9..9819dd54d 100644 --- a/test/cls_test.py +++ b/test/cls_test.py @@ -1041,3 +1041,29 @@ def test_modal_object_param_uses_wrapped_type(servicer, set_env_client, client): container_params = deserialize_params(req.serialized_params, function_def, _client) args, kwargs = container_params assert type(kwargs["x"]) == type(dct) + + +def test_using_method_on_uninstantiated_cls(recwarn): + app = App() + + @app.cls(serialized=True) + class C: + @method() + def method(self): + pass + + assert len(recwarn) == 0 + with pytest.raises(AttributeError): + C.blah # type: ignore # noqa + assert len(recwarn) == 0 + + assert isinstance(C().method, Function) # should be fine to access on an instance of the class + assert len(recwarn) == 0 + + # The following should warn since it's accessed on the class directly + C.method # noqa # triggers a deprecation warning + # TODO: this will be an AttributeError or return a non-modal unbound function in the future: + assert len(recwarn) == 1 + warning_string = str(recwarn[0].message) + assert "instantiate classes before using methods" in warning_string + assert "C().method instead of C.method" in warning_string