gh-102618: Introduce a functools.cached_method decorator#150002
gh-102618: Introduce a functools.cached_method decorator#150002sirosen wants to merge 6 commits into
functools.cached_method decorator#150002Conversation
Documentation build overview
19 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 <308610+jaraco@users.noreply.github.com>
|
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. 🙂 |
| from abc import get_cache_token | ||
| from collections import namedtuple | ||
| # import weakref # Deferred to single_dispatch() | ||
| # import weakref # Deferred to single_dispatch() and cached_method() |
There was a problem hiding this comment.
Could we use a lazy import here instead of adding more inline imports?
There was a problem hiding this comment.
I prefer to avoid that as not-in-scope for this change, if that's okay.
But I would be happy to file an issue and PR for follow-up (and maybe look for other cases as well?).
| def __init__(self, func, /, maxsize=None, typed=False): | ||
| self._function_table = {} | ||
| # we need a lock when initializing per-instance caches | ||
| self._cache_init_lock = RLock() |
There was a problem hiding this comment.
It looks like this got left behind when you removed the locking logic and isn't used any more.
There was a problem hiding this comment.
Oof, thanks for the catch! I'll do an amend to remove it as part of that commit. (Is that the typical/appropriate way to handle CPython PRs? I know rebase is frowned upon.)
There was a problem hiding this comment.
(Is that the typical/appropriate way to handle CPython PRs? I know rebase is frowned upon.)
See this note on force pushing/amending: https://devguide.python.org/getting-started/pull-request-lifecycle/#don-t-force-push
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.
| a weakref and ensures that caches are cleared when instances are garbage collected. | ||
|
|
||
| This is useful for expensive computations which are consistent with respect to | ||
| an instance, e.g., those which depend only on immutable attributes. |
There was a problem hiding this comment.
Please avoid latin abbreviations: https://devguide.python.org/documentation/style-guide/#use-simple-language
4 participants
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 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.
In addition to @rhettinger's review, as the listed owner for functools, @sobolevn expressed interest in reviewing changes.
@kenahoo and @stevendaprano 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 ↩