Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

functools.cached_property does not maintain the wrapped method's __isabstractmethod__ #79176

Closed
mwilbz mannequin opened this issue Oct 15, 2018 · 20 comments
Closed

functools.cached_property does not maintain the wrapped method's __isabstractmethod__ #79176

mwilbz mannequin opened this issue Oct 15, 2018 · 20 comments
Labels
3.8 only security fixes stdlib Python modules in the Lib dir

Comments

@mwilbz
Copy link
Mannequin

mwilbz mannequin commented Oct 15, 2018

BPO 34995
Nosy @vstinner, @carljm, @methane, @serhiy-storchaka, @ilevkivskyi, @sir-sigurd, @mwilbz
PRs
  • bpo-34995: Maintain func.__isabstractmethod__ in functools.cached_property #9904
  • 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']

    @mwilbz mwilbz mannequin added 3.8 only security fixes stdlib Python modules in the Lib dir labels Oct 15, 2018
    @serhiy-storchaka
    Copy link
    Member

    What is the use case of this?

    @mwilbz
    Copy link
    Mannequin Author

    mwilbz mannequin commented 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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    mwilbz mannequin commented 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
    Copy link
    Mannequin Author

    mwilbz mannequin commented 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

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

    @serhiy-storchaka
    Copy link
    Member

    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
    Copy link
    Member

    methane commented Oct 20, 2018

    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()

    @vstinner
    Copy link
    Member

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

    @vstinner
    Copy link
    Member

    Context: @functools.cached_property has been added by bpo-21145.

    @carljm
    Copy link
    Member

    carljm commented Oct 25, 2018

    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.

    @methane
    Copy link
    Member

    methane commented Oct 26, 2018

    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.

    @methane
    Copy link
    Member

    methane commented Nov 17, 2018

    I want to hear mypy developer's comment.

    @gvanrossum
    Copy link
    Member

    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
    Copy link
    Member

    methane commented Nov 17, 2018

    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?

    @gvanrossum
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    methane commented 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
    Copy link
    Member

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

    @mwilbz
    Copy link
    Mannequin Author

    mwilbz mannequin commented 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
    Copy link
    Member

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

    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.

    @methane methane closed this as completed Mar 28, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants