This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: functools.cached_property docs should explain that it is non-overriding
Type: Stage: resolved
Components: Documentation Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: docs@python, miss-islington, ramalho, rhettinger
Priority: normal Keywords: patch

Created on 2020-12-29 18:12 by ramalho, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 24031 merged rhettinger, 2020-12-31 20:36
PR 24035 merged miss-islington, 2021-01-01 01:06
Messages (5)
msg384019 - (view) Author: Luciano Ramalho (ramalho) * Date: 2020-12-29 18:12
functools.cached_property is a great addition to the standard library, thanks!

However, the docs do not say that @cached_property produces a non-overriding descriptor, in contrast with @property.

If a user replaces a @property with a @cached_property, her code may or may not break depending on the existence of an instance attribute with the same name as the decorated method. This is surprising and may affect correctness, so it deserves even more attention than the possible performance loss already mentioned in the docs, related to the shared-dict optimization.

In the future, perhaps we can add an argument to @cached_property to optionally make it produce overriding descriptors.
msg384024 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-12-29 19:09
FYI, the usual term is "data descriptor" instead of "overriding descriptor".

Normally the docs for things like property() and cached_property() don't mention the term descriptor at all.  From the user point of view, that is an implementation detail.  What is important is what it does and how to use it.

In the case of cached_property(), the docs do a pretty good job, "Transform a method of a class into a property whose value is computed once and then cached as a normal attribute for the life of the instance."

From a user point of view, that is exactly what happens.  The method is called exactly once.  The result is cached in an attribute and it behaves like a normal attribute for the remaining life (effectively, the descriptor is gone.

I would suggest a small amendment, and say "cached as a normal attribute with the same name".   The choice of attribute isn't arbitrary, it is exactly the same as the cached property.

> In the future, perhaps we can add an argument to @cached_property
> to optionally make it produce overriding descriptors.

That doesn't make any sense to me.  It would defeat the entire mechanism for cached_property().

> If a user replaces a @property with a @cached_property, her 
> code may or may not break depending on the existence of an 
> instance attribute with the same name as the decorated method. 

We've never had a report of an issue like this and I don't expect to see over.  In general, if someone is using property(), it is already a bit challenging to store and retrieve attributes that have the same name as the property.

I think it is sufficient to just state that cached_property() uses the attribute with the same name as the property.  That way, it won't be a "surprise" that attribute with the same name gets used :-)

One other possible addition is to note that the attribute is writeable:

class A:
    @cached_property
    def x(self):
        print('!')
        return 42

>>> a = A()
>>> vars(a)           # Initially, there is no instance variable
{}
>>> a.x               # Method is called
!
42
>>> vars(a)           # Data is stored in the instance variable
{'x': 42}
>>> a.x               # Method is not called again
42  
>>> a.x = 10          # Attribue is writeable
>>> vars(a)
{'x': 10}
>>> a.x
10
msg384032 - (view) Author: Luciano Ramalho (ramalho) * Date: 2020-12-29 22:36
> FYI, the usual term is "data descriptor" instead of "overriding descriptor".

Yes, the Python docs are consistent in always using "data descriptor", but I've adopted "overriding descriptor" from Martelli's Python in a Nutshell--in Fluent Python I also put a note saying that "data descriptor" is used in the docs. I think "overriding descriptor" is more descriptive. In particular, I find "non-data descriptor" quite puzzling. But this issue is not about changing the jargon.

> Normally the docs for things like property() and cached_property()
> don't mention the term descriptor at all.  From the user point of
> view, that is an implementation detail.

I'd agree, if I wasn't personally bitten by the issue I reported. I was refactoring an existing class which had hand-made caches in a couple of methods decorated with @property. When I discovered @cached_property, I tried to simplify my code by using it, and it broke my code in one place, but not in the other. Leonardo Rochael had experience with cached_property and told me about the difference.

I suggest a warning noting the different behavior regarding existing instance attributes. The warning doesn't need to assume the user knows what is a descriptor, but it should in my opinion point to your excellent Descriptor HowTo Guide for more information.

> I would suggest a small amendment, and say "cached as a normal attribute with the same name". The choice of attribute isn't arbitrary, it is exactly the same as the cached property.

That's good too.

>> In the future, perhaps we can add an argument to @cached_property
>> to optionally make it produce overriding descriptors.

> That doesn't make any sense to me.  It would defeat the entire mechanism for cached_property().

My idea would be to add a dummy __set__ method if the overriding option was True (default would be False). The method could raise an exception. But its presence would make @cached_property behave more like @property in the most common use case of @property: as an overriding descriptor emulating a read-only attribute.

>> If a user replaces a @property with a @cached_property, her 
>> code may or may not break depending on the existence of an 
>> instance attribute with the same name as the decorated method. 

> We've never had a report of an issue like this and I don't expect to see over.

You just did ;-). I filed this issue after spending hours trying to figure out what the problem was in my code on my second attempt at using @cached_property. I expected @cached_property would work as a drop-in replacement for @property when emulating a read-only attribute, and it did not.

> One other possible addition is to note that the attribute is writeable:

Yes, the code snippet you suggested is a good way of saying "this produces a non-overriding descriptor". 

However we want to say it, I think it is important to contrast the behavior of @cached_property v. @property regarding the presence of attributes with the same name.

Cheers and a happy 2021 with two doses of vaccine for you and your loved ones, Raymond!
msg384145 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-01-01 01:06
New changeset c8a7b8fa1b5f9a6d3052db792018dc27c5a3a794 by Raymond Hettinger in branch 'master':
bpo-42781: Document the mechanics of cached_property from a user viewpoint (GH-24031)
https://github.com/python/cpython/commit/c8a7b8fa1b5f9a6d3052db792018dc27c5a3a794
msg384146 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-01-01 01:36
New changeset 8333d421c0d7b90de3ff92002af9fd2c5d5f373c by Miss Islington (bot) in branch '3.9':
bpo-42781: Document the mechanics of cached_property from a user viewpoint (GH-24031) (#24035)
https://github.com/python/cpython/commit/8333d421c0d7b90de3ff92002af9fd2c5d5f373c
History
Date User Action Args
2022-04-11 14:59:39adminsetgithub: 86947
2021-01-01 01:36:21rhettingersetmessages: + msg384146
2021-01-01 01:12:21rhettingersetstatus: open -> closed
assignee: docs@python -> rhettinger
resolution: fixed
stage: patch review -> resolved
2021-01-01 01:06:41miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request22876
2021-01-01 01:06:07rhettingersetmessages: + msg384145
2020-12-31 20:36:46rhettingersetkeywords: + patch
nosy: + rhettinger

pull_requests: + pull_request22872
stage: patch review
2020-12-30 00:53:25rhettingersetnosy: - rhettinger
2020-12-29 22:36:32ramalhosetmessages: + msg384032
2020-12-29 19:09:02rhettingersetnosy: + rhettinger
messages: + msg384024
2020-12-29 18:12:58ramalhocreate