Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
32 changes: 21 additions & 11 deletions modal/cls.py
Original file line number Diff line number Diff line change
Expand Up @@ -863,18 +863,28 @@ def __call__(self, *args, **kwargs) -> _Obj:
)

def __getattr__(self, k):
# TODO: remove this method - access to attributes on classes (not instances) should be discouraged
if not self._is_local() or k in self._method_partials:
# if not local (== k *could* be a method) or it is local and we know k is a method
deprecation_warning(
(2025, 1, 13),
"Calling a method on an uninstantiated class will soon be deprecated; "
"update your code to instantiate the class first, i.e.:\n"
f"{self._name}().{k} instead of {self._name}.{k}",
if self._user_cls is not None:
# local class, we can check if there are static attributes and let the user access them
# except if they are PartialFunction (i.e. methods)
v = getattr(self._user_cls, k)
if not isinstance(v, modal.partial_function.PartialFunction):
Copy link
Contributor

Choose a reason for hiding this comment

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

😵‍💫 double checking that we wouldn't expect these to be the internal _PartialFunction type here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is a bit tbh 😵 But @method is itself a synchronicity-wrapped method, so it will output wrapped types (PartialFunction in this case) that will be part of the class namespace of the underlying class, which is what we're getting here. It would break the tests if this wasn't so, so even if this was to change we would detect it in CI :)

return v
Copy link

Choose a reason for hiding this comment

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

Bug: AttributeError and Import Issue in Local Classes

When accessing attributes on local classes, getattr(self._user_cls, k) directly raises an AttributeError for non-existent attributes, bypassing the helpful synthetic _Function error message. Additionally, the code references modal.partial_function.PartialFunction without importing it, causing a NameError; _PartialFunction should be used instead.

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentional, we want a regular AttributeError in those cases


# We create a synthetic dummy Function that is guaranteed to raise an AttributeError when
# a user tries to use any of its "live methods" - this lets us raise exceptions for users
# only if they try to access methods on a Cls as if they were methods on the instance.
async def method_loader(fun: _Function, resolver: Resolver, existing_object_id: Optional[str]):
raise AttributeError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what the right exception type is. Something to clean up eventually?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure AttributeError is right. For normal Python classes this is a TypeError: the method attribute exists, but it's not been bound and so the arglist is incomplete.

I'm still not sure exactly what error type to captures the modal-specific issue. It turns out that modal.Cls is really not exactly like a class....

"You can't access methods on a Cls directly - Did you forget to instantiate the class first?\n"
"e.g. instead of MyClass.method.remote(), do MyClass().method.remote()"
)
return getattr(self(), k)
# non-method attribute access on local class - arguably shouldn't be used either:
return getattr(self._user_cls, k)

return _Function._from_loader(
method_loader,
rep=f"UnboundMethod({self._name}.{k})",
deps=lambda: [],
hydrate_lazily=True,
)

def _is_local(self) -> bool:
return self._user_cls is not None
Expand Down
57 changes: 41 additions & 16 deletions test/cls_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1126,30 +1126,55 @@ def f(self):
pass


def test_using_method_on_uninstantiated_cls(recwarn):
def test_using_method_on_uninstantiated_cls():
app = App()

@app.cls(serialized=True)
class C:
some_non_param_variable = 10

@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 the class first" in warning_string
assert "C().method instead of C.method" in warning_string
assert C.some_non_param_variable == 10

with pytest.raises(AttributeError, match="blah"):
C.blah # type: ignore

with pytest.raises(AttributeError, match="Did you forget to instantiate the class first?"):
# The following should error since the class is supposed to be instantiated first
C.method.remote() # noqa


def test_using_method_on_uninstantiated_hydrated_cls(set_env_client, client):
app = App()

@app.cls(serialized=True)
class C:
some_non_param_variable = 10

@method()
def method(self):
pass

with app.run(client=client):
assert C.some_non_param_variable == 10

with pytest.raises(AttributeError, match="blah"):
C.blah # type: ignore

with pytest.raises(AttributeError, match="Did you forget to instantiate the class first?"):
# The following should error since the class is supposed to be instantiated first
C.method.remote() # noqa


def test_using_method_on_uninstantiated_remote_cls(set_env_client):
C = modal.Cls.from_name("app", "C")

with pytest.raises(AttributeError, match="Did you forget to instantiate the class first?"):
# The following should error since the class is supposed to be instantiated first
C.method.remote() # noqa


def test_bytes_serialization_validation(servicer, client, set_env_client):
Expand Down
Loading