Repository navigation
functools.cached_property does not maintain the wrapped method's __isabstractmethod__ #79176
Description
| BPO | 34995 |
|---|---|
| Nosy | @vstinner, @carljm, @methane, @serhiy-storchaka, @ilevkivskyi, @sir-sigurd, @mwilbz |
| PRs |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2019-03-28.06:18:08.277>
created_at = <Date 2018-10-15.23:30:29.130>
labels = ['3.8', 'library']
title = "functools.cached_property does not maintain the wrapped method's __isabstractmethod__"
updated_at = <Date 2019-03-28.06:18:08.277>
user = 'https://github.com/mwilbz'bugs.python.org fields:
activity = <Date 2019-03-28.06:18:08.277>
actor = 'methane'
assignee = 'none'
closed = True
closed_date = <Date 2019-03-28.06:18:08.277>
closer = 'methane'
components = ['Library (Lib)']
creation = <Date 2018-10-15.23:30:29.130>
creator = 'mwilbz'
dependencies = []
files = []
hgrepos = []
issue_num = 34995
keywords = ['patch']
message_count = 20.0
messages = ['327808', '327810', '327880', '327908', '327910', '327951', '328136', '328491', '328492', '328502', '328510', '330019', '330020', '330021', '330022', '330027', '330123', '330127', '330135', '330171']
nosy_count = 7.0
nosy_names = ['vstinner', 'carljm', 'methane', 'serhiy.storchaka', 'levkivskyi', 'sir-sigurd', 'mwilbz']
pr_nums = ['9904']
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue34995'
versions = ['Python 3.8']
Activity
serhiy-storchaka commentedon Oct 16, 2018
What is the use case of this?
mwilbz commentedon Oct 16, 2018
This allows a developer to add a @cached_property to a method with the @AbstractMethod decorator, without breaking the check for abstract methods on ABC instantiation. That is, if you tried to instantiate an ABC with a method that had a method decorated with @cached_property and @AbstractMethod now, it would succeed, instead of throwing a TypeError as you might expect.
As for why you'd put @cached_property on an abstract method, it's useful for IDEs and type checkers to know that the method is implemented with a property, and that users of the method (and its implementations) can reasonably call it multiple times and know caching is occurring.
serhiy-storchaka commentedon Oct 17, 2018
The comment can serve the same purpose.
The caching affects the behavior at run time. Abstract methods should not be executed. The cached_property decorator is not inherited by overriding properties. I don't think that combining the cached_property and the @AbstractMethod decorators should be supported.
mwilbz commentedon Oct 17, 2018
I agree, a comment can serve the same purpose. But for the same reasons it's useful to express typing hints in Python with real syntax, and the reasons it's useful to have the abc module in the first place, I think it is useful to be able to annotate an abstract method with @cachedproperty.
The purpose of this change is not to add code developers would try to use at runtime, it is to allow developers to communicate their intentions to static code analysis tools and to other developers, using standard decorators from builtins that do not break one another. Indeed, abstract methods should not be executed (on purpose), but Python still supports abstract methods to protect developers who want to explicitly label their code as "do not execute".
Therefore, please do not think of this change as something someone would try to execute. It is for hinting functionality, and it is to protect developers who would decorate a method with @cached_property and @AbstractMethod and find that abc's functionality was unexpectedly broken.
mwilbz commentedon Oct 17, 2018
To add some more context, the existing Python documentation is a little misleading in this situation:
https://docs.python.org/3/library/abc.html#abc.abstractmethod
It shows that labeling a method with
ought to be done in the order above, but this ordering currently breaks the abstract method check if you swap our @Property for @cached_property. The documentation also says
"In order to correctly interoperate with the abstract base class machinery, the descriptor must identify itself as abstract using __isabstractmethod__." which @cached_property does not do, but as the documentation then shows, @Property does do this properly.
serhiy-storchaka commentedon Oct 18, 2018
Well, you can combine @AbstractMethod with @Property in an abstract class. This will give you type hints etc. @cached_property is a detail of concrete implementation.
methane commentedon Oct 20, 2018
I agree with Serhiy.
For static hinting,
@propertyshould be enough.I think tools like mypy should support this pattern:
vstinner commentedon Oct 25, 2018
Exmaple 1:
---
Exmaple 2:
---
Current behavior: Example 1 raises an exception as expected, Example 2 instanciate the object: no exception is raised.
PR 9904 looks like a reasonable change to me.
Well, maybe we can hack something to make Example 2 fail as well, but I like the idea of using @functools.cached_property in an abstract class as "documentation". To announce that the property will be cached, even if technically it will not be cached. It's more to use the code as documentation than to execute any code.
PR 9904 is just 4 lines of code to make the code "works as expected".
vstinner commentedon Oct 25, 2018
Context: @functools.cached_property has been added by bpo-21145.
carljm commentedon Oct 25, 2018
FWIW, it seems to me (author of
cached_propertypatch) that while just using@propertyon the abstract method plus a comment is a reasonable and functional workaround that sacrifices only a small documentation value, there's no reason why@cached_propertyshouldn't propagate this flag in the same way@propertydoes. It seems reasonable to me to consider this behavior discrepancy a bug; I'd have fixed it if made aware of it while developingcached_property.methane commentedon Oct 26, 2018
When thinking about "code as documentation", we should think not only Python iterpreter, but also IDE, static analystics tools.
If we support abstractmethod + cached_property, it means we encourage to all IDEs & tools to support it.
Otherwise, code completion or static analytics will be broken.
I think "this property is likely (but not must be) cached" hint is not worth enough to add such complexity to all IDEs / static analytics tools.
methane commentedon Nov 17, 2018
I want to hear mypy developer's comment.
gvanrossum commentedon Nov 17, 2018
This is runtime behavior, mypy doesn't care either way. It triggers on the presence of the decorator, not on what attributes it sets at runtime on the object.
methane commentedon Nov 17, 2018
But it's only motivation is static hinting.
My point is, is cached_property important static hint?
For example, do you want support cached_property in mypy Protocol with same
semantics?
gvanrossum commentedon Nov 17, 2018
In mypy there would be no difference between a cached property and a normal one -- the type is the same either way. Caching is the quintessential runtime behavior -- it shouldn't matter for semantics, only for performance. (True, sometimes there are more subtle semantics that require caching past the first call, but this is beyond the realm of a type checker, which cannot typically tell whether a call is the first or not.)
vstinner commentedon Nov 17, 2018
INADA-san: do you prefer to raise an exception? If yes, can we ensure that
an exception will always be raised? See my 2 examples.
methane commentedon Nov 20, 2018
I worried about people think we recommend it when we support it, while this is just a request from one person, not recommendation after wide discussion.
But it doesn't affect to static tools than I suspected. Tools like autocompletion should support cached_property in normal class anyway.
And this pull request doesn't promote the cached_property support in the document.
So I don't have a strong objections for now.
Generally speaking, I don't like this type of changes.
Some people think it's improvement, but since there are no clear vision/recommendation how ABCs are used, it's just a random increasing code.
We can't maintain "preferably only one obvious way" for now, especially about ABCs.
ilevkivskyi commentedon Nov 20, 2018
I am with Inada-san actually. I would go as far as saying that
should unconditionally raise on definition. Mostly because this is just misleading. This declaration doesn't guarantee that the implementation will use caching (neither Python nor mypy can enforce this). Caching is an _implementation_ detail, while ABCs are used to specify _interface_.
mwilbz commentedon Nov 20, 2018
Inada-san, I think it's fair to ask for a broader vision about how ABCs are used. In that respect I'm wondering about some inconsistencies in the existing functools module about whether wrappers should maintain the wrapped function's __isabstractmethod__ attribute.
Both singledispatchmethod and partialmethod implement their own way of copying __isabstractmethod__ from the wrapped function. singledispatchmethod does this in addition to calling update_wrapper, whereas partialmethod doesn't seem to use update_wrapper at all.
In terms of a broader vision, then, should the decorators in functools be more consistent about whether the __isabstractmethod__ attribute of a wrapped function is preserved in the wrapped version? Should update_wrapper be the way to standardize this? If so, it would make sense either to remove the __isabstractmethod__ copying from those decorators, or otherwise to add __isabstractmethod__ to the list of attributes that are copied from the wrapped function by update_wrapper.
Hopefully viewing the problem in this way avoids the pitfalls of adding random code to the codebase, as you are pointing out. Let me know if this discussion belongs in a broader issue than this one. Thank you :)
methane commentedon Nov 21, 2018
My personal opinion is: support abstractmethod only when the descriptor is useful for define interface with ABC.
In case of cached_property, it's not. It is just a property as interface level. Caching is just an implementation.
In case of update_wrapper, it's too generic. A decorated function may be used while define interface, but it's a rare case. So no need to support it.
In case of singledispatch, I think it is not used in ABC, like cached_property. But it has shipped in Python already. We shouldn't remove it easily.
In case of partialmethod... it's considerable. I don't use ABC much, and I never use partialmethod. So this example is very artificial.
When they subclass of MyABC, they need to override both of
set_fooandreset_foo. Otherwise, reset_foo is bound to MyABC.set_foo, not subclass' one.So isabstractmethod support in partialmethod is not just a "commet as a code".
On the other hand, this example can be written as:
Or it can use lazy binding too:
I am not sure __isabstractmethod__ support in partialmethod is really useful.