bpo-21145: Add cached_property decorator in functools #6982

carljm · 2018-05-19T04:55:23Z

https://bugs.python.org/issue21145

carljm · 2018-05-19T05:32:53Z

(Failing CI builds are asyncio flakiness, not related.)

asvetlov · 2018-05-19T21:22:30Z

Lib/functools.py

When cls is None? When __get__ is called without cls parameter?
Btw in docs for descriptors, the third argument is called owner IIRC.

AFAIK every call to __get__ generated by attribute lookup machinery will include the second (not including self) argument, but both the descriptor documentation (https://docs.python.org/3/reference/datamodel.html#descriptors) and the descriptor howto guide (https://docs.python.org/3/howto/descriptor.html) explicitly include an example of manually calling a descriptor's __get__ method providing only one argument. Also, all of the code examples in the HOWTO show the second argument as optional. Given that the method can be called directly and cls is unused in this implementation, it seems reasonable to follow that lead and make it optional.

Regarding the argument name, considering both the Data Model documentation and the HOWTO, we see examples in Python's own documentation of descriptors whose __get__ second argument is named owner, type, or objtype. All three of these are found in the standard library, along with cls, klass, ownerclass, and _type. There doesn't seem to be a standard usage. Personally I find cls or klass or objtype clearer, as owner doesn't clarify the key difference from the other argument, which is that it's a type. Perhaps naming the two arguments obj and objtype does the best job of clearly indicating their relationship. That said, I don't feel strongly and will rename to owner if you prefer that.

I doubt if howto is 100% correct.

datamodel doesn't allow None for owner, AFAIK Python aways pass a class type.

Using default for always specified argument confuses me.

datamodel uses owner, please do it as well.

If we need to change recommended argument name -- let's discuss it in another issue.

Changed signature of __get__ to (self, instance, owner) to match Data Model documentation.

asvetlov · 2018-05-19T21:25:56Z

Lib/functools.py

__getitem__ / setdefault are atomic operations.
The code could be modified in the following way:

Get attribute value without locking.

If cached attr is not found -- acquire the lock, get the value again under the lock and calculate it if not present.

Sure, will update. Thanks for the review!

bedevere-bot · 2018-05-19T21:26:05Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

carljm · 2018-05-20T07:16:54Z

I have made the requested changes; please review again.

bedevere-bot · 2018-05-20T07:16:56Z

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

carljm · 2018-05-20T18:20:57Z

I have made the requested changes; please review again.

bedevere-bot · 2018-05-20T18:20:59Z

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

asvetlov · 2018-05-22T10:07:12Z

Lib/test/test_functools.py

Looks like the method is never called

Yes, that’s expected. The point of this class is just to test the error message; cached property on classes with slots is not supported. Would you find it clearer if I used pass as the body? I just wrote it the way someone realistically might, to make it clear that the error is not due to a mistake in the implementation of the method.

raise RuntimeError('never executed') is even more clear.

carljm · 2018-05-22T20:30:41Z

I have made the requested changes; please review again.

bedevere-bot · 2018-05-22T20:30:43Z

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

asvetlov

LGTM
Would be nice to get confirmation from code owners, @rhettinger and @ncoghlan

asvetlov · 2018-05-30T10:51:18Z

Ooop. @carljm please resolve merging conflicts.

sir-sigurd

It doesn't work properly in this case:

In [75]: class A:
    ...:     def f(self, x=1):
    ...:         return x
    ...: 
    ...:     cached_f = cached_property(f)

In [76]: a = A()

In [77]: a.f(2)
Out[77]: 2

In [78]: a.cached_f
Out[78]: 1

In [79]: a.f(2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-79-3226073480ca> in <module>()
----> 1 a.f(2)

TypeError: 'int' object is not callable

So __set_name__() should be use to determine attrname.

sir-sigurd · 2018-06-17T10:25:18Z

Lib/test/test_functools.py

Is this needed? It seems to be always shadowed in __init__().

sir-sigurd · 2018-06-17T10:26:04Z

Lib/test/test_functools.py

Redundant line?

methane · 2018-06-28T08:43:22Z

Doc/library/functools.rst

I prefer _data to imply dataset.data = newdata is not supported.

carljm · 2018-06-29T02:16:30Z

Thanks for the comments! Will update next week.

methane · 2018-07-27T09:26:59Z

@carljm ping?

carljm · 2018-07-28T16:36:44Z

I have made the requested changes; please review again.

bedevere-bot · 2018-07-28T16:36:46Z

Thanks for making the requested changes!

@asvetlov, @methane: please review the changes made to this pull request.

sir-sigurd · 2018-07-29T08:48:24Z

Lib/functools.py

+class cached_property:
+    def __init__(self, func):
+        self.func = func
+        self.attrname = func.__name__


This looks redundant since attrname is set in __set_name__()

What do you suggest to do instead? This is the most reasonable fallback in case __set_name__ is not called in some unusual use of this class, and it avoids the object ever being in an incompletely initialized state.

sir-sigurd · 2018-07-29T08:55:24Z

Lib/functools.py

+        self.lock = RLock()
+
+    def __set_name__(self, owner, name):
+        self.attrname = name


Might make sense to raise error in this case, since a will never be cached:

class C: @cached_property def a(self): pass b = a

OK. I guess that also answers the above question; we will raise error in case __set_name__ is ever called twice, and also I guess raise error if __get__ is called and __set_name__ was not.

carljm · 2018-07-29T15:44:43Z

@sir-sigurd How does that look?

methane · 2018-07-30T01:10:40Z

@rhettinger Would you review this?

sir-sigurd · 2018-07-30T05:53:27Z

Lib/functools.py

+        else:
+            raise TypeError(
+                "Cannot assign the same cached_property to two names "
+                f"({self.attrname!r} and {name!r}) on the same class."


This message is inaccurate because the exception is also raised when it's assigned on the different classes.
I'm not sure that we should disallow to assign it on the different classes under the same name.

Good catch. I'd like to avoid tracking owners, but it should be sufficient to just allow repeated __set_name__ as long as the name remains the same.

carljm · 2018-07-30T17:45:37Z

@sir-sigurd Better?

sir-sigurd · 2018-08-03T05:58:54Z

@carljm 👍

ncoghlan

A couple of minor comments and error checking suggestion related to the technicalities of why __dict__ may not exist, and why it may not support __setitem__, but otherwise this looks good to me.

ncoghlan · 2018-08-04T07:17:17Z

Doc/library/functools.rst

+
+   .. note::
+
+      This decorator does not support classes which define ``__slots__``.


I'd word this note slightly differently, as there are multiple ways to end up with instances that don't have a mutable mapping as their __dict__ attribute, and you can also include __dict__ in a __slots__ list if you want the fast C level access for selected fields, but aren't worried about the per-instance dict memory overhead.

Something like:

This decorator requires that the __dict__ attribute on each instance be a mutable mapping. This means it will not work with some types, such as metaclasses (since the __dict__ attributes on type instances are read-only proxies for the class namespace), and those that specify __slots__ without including __dict__ as one of the defined slots (as such classes don't provide a __dict__ attribute at all).

ncoghlan · 2018-08-04T07:20:01Z

Lib/functools.py

+                "Cannot use cached_property instance without calling __set_name__ on it.")
+        try:
+            cache = instance.__dict__
+        except AttributeError:  # objects with __slots__ have no __dict__


Tweak the comment here along similar lines to the note in the docs

ncoghlan · 2018-08-04T07:27:08Z

Lib/functools.py

+                # check if another thread filled cache while we awaited lock
+                val = cache.get(self.attrname, _NOT_FOUND)
+                if val is _NOT_FOUND:
+                    val = cache[self.attrname] = self.func(instance)


Similar to the AttributeError above, it's likely worth wrapping TypeError here to account for cases like:

>>> str.__dict__["x"] = 1 Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: 'mappingproxy' object does not support item assignment

* master: (107 commits) bpo-22057: Clarify eval() documentation (pythonGH-8812) bpo-34318: Convert deprecation warnings to errors in assertRaises() etc. (pythonGH-8623) bpo-22602: Raise an exception in the UTF-7 decoder for ill-formed sequences starting with "+". (pythonGH-8741) bpo-34415: Updated logging.Formatter docstring. (pythonGH-8811) bpo-34432: doc Mention complex and decimal.Decimal on str.format not about locales (pythonGH-8808) bpo-34381: refer to 'Running & Writing Tests' in README.rst (pythonGH-8797) Improve error message when mock.assert_has_calls fails (pythonGH-8205) Warn not to set SIGPIPE to SIG_DFL (python#6773) bpo-34419: selectmodule.c does not compile on HP-UX due to bpo-31938 (pythonGH-8796) bpo-34418: Fix HTTPErrorProcessor documentation (pythonGH-8793) bpo-34391: Fix ftplib test for TLS 1.3 (pythonGH-8787) bpo-34217: Use lowercase for windows headers (pythonGH-8472) bpo-34395: Fix memory leaks caused by incautious usage of PyMem_Resize(). (pythonGH-8756) bpo-34405: Updated to OpenSSL 1.1.0i for Windows builds. (pythonGH-8775) bpo-34384: Fix os.readlink() on Windows (pythonGH-8740) closes bpo-34400: Fix undefined behavior in parsetok(). (pythonGH-4439) bpo-34399: 2048 bits RSA keys and DH params (python#8762) Make regular expressions in test_tasks.py raw strings. (pythonGH-8759) smtplib documentation fixes (pythonGH-8708) Fix misindented yaml in logging how to example (pythonGH-8604) ...

carljm · 2018-08-19T18:54:57Z

@ncoghlan Thanks for the comments! I think they are all addressed in the latest update.

bedevere-bot · 2018-08-28T07:11:59Z

@ncoghlan: Please replace # with GH- in the commit message next time. Thanks!

codingpy · 2019-06-08T03:55:46Z

haha, why not remove the underscore or rename it into cacheproperty like abstractproperty decorator, or reify like Pyramid?
If its name is neat and beautiful, I think it will be more Pythonic.
Just an opinion, never mind.

wimglenn · 2020-02-17T21:06:26Z

Thanks @carljm for this one, sensible to get it in stdlib since every man and his dog were writing their own versions. Is there any PyPI backport avail for people that want to use this stdlib implementation on older Python runtimes? Does the code use any datamodel features that may make copy-pasting the implementation unsafe (apart from the obvious f-strings)?

carljm · 2020-02-17T21:20:30Z

@wimglenn It's not exactly a backport, since (like many other versions) it predated this PR, and it has some additional bells and whistles not found here, but I'd probably use https://pypi.org/project/cached-property/

carljm requested review from ncoghlan and rhettinger as code owners May 19, 2018 04:55

the-knights-who-say-ni added the CLA signed label May 19, 2018

bedevere-bot added the awaiting review label May 19, 2018

asvetlov requested changes May 19, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels May 19, 2018

bedevere-bot removed the awaiting changes label May 20, 2018

bedevere-bot added the awaiting change review label May 20, 2018

asvetlov reviewed May 22, 2018

View reviewed changes

asvetlov approved these changes May 30, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 30, 2018

carljm force-pushed the cached-property branch from 3e53053 to f77ff04 Compare May 30, 2018 15:38

sir-sigurd suggested changes Jun 17, 2018

View reviewed changes

methane reviewed Jun 28, 2018

View reviewed changes

Doc/library/functools.rst Outdated

Copy link

Member

methane Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer _data to imply dataset.data = newdata is not supported.

carljm added 3 commits July 27, 2018 20:41

bpo-21145: Add cached_property decorator in functools

1cda951

Review feedback.

72259cd

More review feedback.

26f3f43

bedevere-bot added the awaiting changes label Jul 28, 2018

Fix consistency of doc example.

f006f68

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 28, 2018

sir-sigurd reviewed Jul 29, 2018

View reviewed changes

Explicitly require calling __set_name__ exactly once on cached_property.

d6bed4b

Refine error message.

b41380d

methane approved these changes Jul 30, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 30, 2018

sir-sigurd reviewed Jul 30, 2018

View reviewed changes

Allow reuse of cached_property as long as name remains the same.

b504a20

ncoghlan reviewed Aug 4, 2018

View reviewed changes

carljm added 2 commits August 19, 2018 12:40

Clearer error message when __dict__ is not mutable.

5bdeaab

ncoghlan merged commit d658dea into python:master Aug 28, 2018

bedevere-bot removed the awaiting merge label Aug 28, 2018

sir-sigurd mentioned this pull request Sep 15, 2018

Fixed #29478 -- Added support for mangled names to cached_property. django/django#10033

Merged


		.. note::

		This decorator does not support classes which define ``__slots__``.

Uh oh!

bpo-21145: Add cached_property decorator in functools #6982

bpo-21145: Add cached_property decorator in functools #6982

Uh oh!

Conversation

carljm commented May 19, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carljm commented May 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 19, 2018

Uh oh!

carljm commented May 20, 2018

Uh oh!

bedevere-bot commented May 20, 2018

Uh oh!

carljm commented May 20, 2018

Uh oh!

bedevere-bot commented May 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carljm commented May 22, 2018

Uh oh!

bedevere-bot commented May 22, 2018

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

asvetlov commented May 30, 2018

Uh oh!

sir-sigurd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carljm commented Jun 29, 2018

Uh oh!

methane commented Jul 27, 2018

Uh oh!

carljm commented Jul 28, 2018

Uh oh!

bedevere-bot commented Jul 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carljm commented Jul 29, 2018

Uh oh!

methane commented Jul 30, 2018

Uh oh!

carljm commented May 19, 2018 •

edited by bedevere-bot

Loading

ncoghlan left a comment •

edited

Loading

codingpy commented Jun 8, 2019 •

edited

Loading

wimglenn commented Feb 17, 2020 •

edited

Loading