Remove ability to reference methods as Functions directly on Cls #3648
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Do you mean so that something like Says something useful instead of just "PartialFunction has no attribute 'remote'"? |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| # 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): |
There was a problem hiding this comment.
😵💫 double checking that we wouldn't expect these to be the internal _PartialFunction type here.
There was a problem hiding this comment.
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 :)
modal/cls.py
Outdated
|
|
||
| return _Function._from_loader( | ||
| method_loader, | ||
| rep=f"Maybe({self._name}.{k})", |
There was a problem hiding this comment.
Ah this wasn't intended to be the final name :) What about UnboundMethod ?
test/cls_test.py
Outdated
| assert len(recwarn) == 0 | ||
| # The following should warn since it's accessed on the class directly | ||
| with pytest.raises(AttributeError, match="Did you forget to instantiate the class first?"): | ||
| C.method.remote() # noqa # triggers a deprecation warning |
There was a problem hiding this comment.
Why is there a deprecation warning still?
There was a problem hiding this comment.
There isn't, but there is a comment about there being one :P Removing
| # 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( |
There was a problem hiding this comment.
Not sure what the right exception type is. Something to clean up eventually?
There was a problem hiding this comment.
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....
This will now return a
PartialFunctioninstead.Not sure if we should be adding some helper
__getattr__onPartialFunctionto help users who will undoubtedly still make this mistake, thinking they have gotten aFunctionthey can use?Compatibility checklist
Check these boxes or delete any item (or this section) if not relevant for this PR.
Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.
Release checklist
If you intend for this commit to trigger a full release to PyPI, please ensure that the following steps have been taken:
modal_version/__init__.py) has been updated with the next logical versionChangelog
Note
Accessing methods on a
Clswithout instantiation now raises AttributeError (with guidance), while static class attributes remain accessible; tests updated and expanded for local, hydrated, and remote cases.modal/cls.py):__getattr__onClsreturns a syntheticFunctionthat always raisesAttributeErrorwhen methods are used, instructing to instantiate first.PartialFunction).test/cls_test.py):test_using_method_on_uninstantiated_clsto expectAttributeErroron class-level method usage and allow static attribute reads.app.run) and remote (Cls.from_name) classes to assert the new error behavior.Written by Cursor Bugbot for commit 9622efc. This will update automatically on new commits. Configure here.