classification
Title: functools.cached_property does not maintain the wrapped method's __isabstractmethod__
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: carljm, levkivskyi, methane, mwilbz, serhiy.storchaka, sir-sigurd, vstinner
Priority: normal Keywords: patch

Created on 2018-10-15 23:30 by mwilbz, last changed 2019-03-28 06:18 by methane. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9904 closed mwilbz, 2018-10-15 23:47
Messages (20)
msg327808 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-16 05:45
What is the use case of this?
msg327810 - (view) Author: Matt Wilber (mwilbz) * Date: 2018-10-16 06:01
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.
msg327880 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-17 09:26
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.
msg327908 - (view) Author: Matt Wilber (mwilbz) * Date: 2018-10-17 15:50
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.
msg327910 - (view) Author: Matt Wilber (mwilbz) * Date: 2018-10-17 16:27
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

@property
@abstractmethod

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.
msg327951 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-18 09:40
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.
msg328136 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2018-10-20 13:38
I agree with Serhiy.

For static hinting, `@property` should be enough.
I think tools like mypy should support this pattern:

class MyABC(metaclass=ABCMeta):
    @property
    @abstractmethod
    def myprop(self):
        ...

class MyConcrete(MyABC):
    @cached_property
    def myprop(self):
        return self._some_heavy_work()
msg328491 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-25 21:04
Exmaple 1:
---
import abc
import functools

class AbstractExpensiveCalculator(abc.ABC):
    @abc.abstractmethod
    @functools.cached_property
    def calculate(self):
        pass

AbstractExpensiveCalculator()
---

Exmaple 2:
---
import abc
import functools

class AbstractExpensiveCalculator(abc.ABC):
    @functools.cached_property
    @abc.abstractmethod
    def calculate(self):
        pass

AbstractExpensiveCalculator()
---

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.


> 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.

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".
msg328492 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-25 21:06
Context: @functools.cached_property has been added by bpo-21145.
msg328502 - (view) Author: Carl Meyer (carljm) * Date: 2018-10-25 22:22
FWIW, it seems to me (author of `cached_property` patch) that while just using `@property` on 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_property` shouldn't propagate this flag in the same way `@property` does. It seems reasonable to me to consider this behavior discrepancy a bug; I'd have fixed it if made aware of it while developing `cached_property`.
msg328510 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2018-10-26 04:55
>  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.

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.
msg330019 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2018-11-17 03:48
I want to hear mypy developer's comment.
msg330020 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-11-17 04:51
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.
msg330021 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2018-11-17 06:11
> Guido van Rossum <guido@python.org> added the comment:
>
> 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.
>
>

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?
msg330022 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-11-17 06:31
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.)
msg330027 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-17 09:03
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.
msg330123 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2018-11-20 07:57
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.
msg330127 - (view) Author: Ivan Levkivskyi (levkivskyi) * (Python committer) Date: 2018-11-20 11:17
I am with Inada-san actually. I would go as far as saying that

@cached_property
@abstractmethod
def something(): ...

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_.
msg330135 - (view) Author: Matt Wilber (mwilbz) * Date: 2018-11-20 16:25
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 :)
msg330171 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2018-11-21 06:24
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.

class MyABC(abc.ABC):
    @abstractmethod
    def set_foo(self, v):
        pass
    reset_foo = partialmethod(set_foo, None)

When they subclass of MyABC, they need to override both of `set_foo` and `reset_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:

class MyABC(abc.ABC):
    @abstractmethod
    def set_foo(self, v):
        pass
    @abstractmethod
    def reset_foo(self):
        pass

Or it can use lazy binding too:

class MyABC(abc.ABC):
    @abstractmethod
    def set_foo(self, v):
        pass

    # No need to override if default implementation is OK
    def reset_foo(self):
        self.set_foo(None)

I am not sure __isabstractmethod__ support in partialmethod is really useful.
History
Date User Action Args
2019-03-28 06:18:08methanesetstatus: open -> closed
resolution: rejected
stage: patch review -> resolved
2018-11-21 06:24:35methanesetmessages: + msg330171
2018-11-20 16:25:27mwilbzsetmessages: + msg330135
2018-11-20 15:59:07gvanrossumsetnosy: - gvanrossum
2018-11-20 11:17:54levkivskyisetmessages: + msg330127
2018-11-20 07:57:19methanesetmessages: + msg330123
2018-11-17 09:03:51vstinnersetmessages: + msg330027
2018-11-17 06:31:19gvanrossumsetmessages: + msg330022
2018-11-17 06:11:43methanesetmessages: + msg330021
2018-11-17 04:51:07gvanrossumsetmessages: + msg330020
2018-11-17 03:48:13methanesetnosy: + gvanrossum, levkivskyi
messages: + msg330019
2018-10-27 06:16:50sir-sigurdsetnosy: + sir-sigurd
2018-10-26 04:55:03methanesetmessages: + msg328510
2018-10-25 22:22:30carljmsetnosy: + carljm
messages: + msg328502
2018-10-25 21:06:16vstinnersetmessages: + msg328492
2018-10-25 21:04:37vstinnersetnosy: + vstinner
messages: + msg328491
2018-10-20 13:38:08methanesetnosy: + methane
messages: + msg328136
2018-10-18 09:40:47serhiy.storchakasetmessages: + msg327951
2018-10-17 16:27:15mwilbzsetmessages: + msg327910
2018-10-17 15:50:39mwilbzsetmessages: + msg327908
2018-10-17 09:26:49serhiy.storchakasetmessages: + msg327880
2018-10-16 06:01:59mwilbzsetmessages: + msg327810
2018-10-16 05:45:05serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg327808
2018-10-15 23:47:31mwilbzsetkeywords: + patch
stage: patch review
pull_requests: + pull_request9266
2018-10-15 23:30:29mwilbzcreate