Repository navigation
Conversation
Documentation build overview
252 files changed ·
|
resolves python#102618 This definition of `cached_method` is based on the discussion on DPO: https://discuss.python.org/t/107164 Some tradeoffs need to be made in any version of such a decorator. In this particular implementation, the choices made are as follows: - lru_cache will be used under the hood, and `cache_clear()` and `cache_info()` will be exposed - caches will be stored in a separate dict, indexed by `id(self)` -- meaning instances do not need to be hashable and will not share caches - weakrefs will be used to delete entries from the cache, so the instances must be weak-referencable - lru_cache itself is not threadsafe, but initialization of the caches is threadsafe -- this avoids confusing scenarios in which cache entries "disappear" New documentation is included, marked for 3.16, and a small number of new tests are added.
Thanks to feedback from @jaraco, this adjustment makes the public interface for `cached_method` a function which is responsible for the argument management, rather than trying to use `__call__` on the descriptor itself. This frees up `__call__` to lookup the relevant LRU-cached function and invoke it, which makes the `_cached_method` descriptor suitable for use as a `property.fget` callable. Tests are updated to indicate that a decorator can be prepared and then used repeatedly (which was previously an explicit error), and can be used under a property decorator.
- Fix doc references - Use imperative mood in a comment Co-authored-by: Jason R. Coombs <[email protected]>
|
Jelle pointed out to me that although I have tried to be careful about the locking I included, to minimize contention, the lock I defined is per- I'll push a change to remove the locking and clarify the thread-safety documentation. |
|
@willingc , this is the PR that I mentioned to you; you said you wanted a ping to take a look. 🙂 |
Although this logic tried to avoid lock contention, there were still scenarios under which it was possible and could negatively impact users. Removal and adjustment of the documentation puts responsibility for locking/safety onto users, similar to other parts of the stdlib.
|
FWIW. The |
Co-authored-by: Edgar Ramírez Mondragón <[email protected]>
jaraco
left a comment
jaraco
left a comment
There was a problem hiding this comment.
In my opinion, this implementation is fairly clean and simple. The documentation shows pretty clearly what the user's expectations should be. The tradeoffs chosen have been well considered and a reasonable path elected. I propose to get this merged and give it some exposure through the beta releases. From there iterate or pivot.
StanFromIreland
left a comment
StanFromIreland
left a comment
There was a problem hiding this comment.
This needs a What's New in 3.16 entry.
Rather than doing an old-style deferred import, with explanatory comments for why the deferred import style is used. Convert these to a top-level `lazy import weakref`. No explanatory comment is used, on the grounds that the new syntax is sufficiently low in cognitive cost that experienced maintainers will quickly recognize it and assume that the pattern is used to either break a cycle or reduce import costs.
|
I've applied the lazy import change. My commit message explains why I did not preserve any comment about it, but I'm open to feedback on that. I also added the What's New entry. |
hugovk
left a comment
hugovk
left a comment
There was a problem hiding this comment.
We have docs tooling that will update next to whatever the next pre-release is, for example:
"Added in version 3.16.0a0 (unreleased)"
Co-authored-by: Hugo van Kemenade <[email protected]> Co-authored-by: Stan Ulbrych <[email protected]>
Co-authored-by: Stan Ulbrych <[email protected]>
And add it to functools' `__all__` list.
Multiple weakrefs can be created when there are concurrent writes to a "new cache". When they finalize, their attempts to clear the dict should be kept safe by using a pop() default.
In order for `help(functools.cached_method)` to show the docstring, it needs to be attached to the public interface, not the implementation. This docstring was "misplaced" in the course of refactors to this code during review.
Co-authored-by: Stan Ulbrych <[email protected]>
|
I just saw #154069 . If that goes in, this should get updated to also use |
|
This PR is stale because it has been open for 90 days with no activity. |
|
The docs should specify what happens in code like this: @dataclass
class Equity:
str: symbol
@cached_method(maxsize=16)
def opening_price(self, date):
return expensive_query(self.symbol, date)
e1 = Equity('GOOG')
print(e1.opening_price, date(2020, 2, 20)) # Miss ? Unhashable instances allowed
print(e1.opening_price, date(2020, 2, 20)) # Hit
print(e1.opening_price, date(2020, 2, 25)) # Miss
e2 = Equity('GOOG')
print(e2.opening_price, date(2020, 2, 20)) # ? Cross-instance caching
e1.symbol = 'ALPB'
print(e1.opening_price, date(2020, 2, 20)) # ? Detect attribute change
del e1 # ? Immediate clear |
|
|
||
| When :func:`cache` or :func:`lru_cache` are used on an instance method, the | ||
| instance (``self``) will be stored in the cache. As a result, instances cannot be | ||
| garbage collected until the relevant caches are cleared. |
There was a problem hiding this comment.
or until the entry ages out of the lru (pretty much like any other entry).
There was a problem hiding this comment.
Yep! I tried to improve this in a subtle way, by correcting "relevant caches are cleared" to "relevant cache entries are cleared".
We can adjust it more -- my goal is to be both precise and concise. 😁
- Note "cache entries" rather than "caches", thereby capturing that eviction from an LRU cache covers the GC issue for cache/lru_cache - Add a note on hashability, next to the note about weakref. This provides a forum for explaining that `id()` is used and what the implications of that choice are.
|
Thanks for the review! I added a doc note about hashability, which explains that
I'm not sure what, if anything, to add about two of these noted cases:
|
resolves #102618
This definition of
cached_methodis based on the discussion on DPO:https://discuss.python.org/t/107164
Some tradeoffs need to be made in any version of such a decorator. In this particular implementation, the choices made are as follows:
lru_cache will be used under the hood, and
cache_clear()andcache_info()will be exposedcaches will be stored in a separate dict, indexed by
id(self)-- meaning instances do not need to be hashable and will not share cachesweakrefs will be used to delete entries from the cache, so the instances must be weak-referencable
lru_cache itself is not threadsafe, but initialization of the caches is threadsafe -- this avoids confusing scenarios in which cache entries "disappear"EDIT: in response to review feedback, this locking was removed; users can lock if they want thread safety
New documentation is included, marked for 3.16, and a small number of new
tests are added.
In addition to @rhettinger's review, as the listed owner for functools, @sobolevn expressed interest in reviewing changes.
@kenahoo and @stevendaprano may also want a look based on their prior interest.
Everyone I've spoken with about this change1 has been supportive of the core idea. In particular @jaraco noted that he would support this addition, and maintains a very different implementation. There are many other ways of writing this, with different tradeoffs. Perhaps the most notable class of those are versions which store caches on the instance itself, like this one suggested here in the DPO discussion -- this writes the cached method into
__dict__, similarly tocached_property.Of course, I'm happy to alter or re-approach any part of this per review! 😄
Footnotes
at PyCon US 2026 ↩