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.

Sign up for GitHub

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

Jump to bottom

we should not recommend using functools.cache on methods #104112

Closed
carljm opened this issue May 3, 2023 · 2 comments · Fixed by #104113
Closed

we should not recommend using functools.cache on methods #104112

carljm opened this issue May 3, 2023 · 2 comments · Fixed by #104113
Labels
docs Documentation in the Doc dir

Comments

@carljm
Copy link
Member

carljm commented May 3, 2023 •
edited by bedevere-bot
Loading

Documentation

Currently the documentation for functools.cached_property recommends stacking @property and @functools.cache decorators to "achieve an effect similar to" functools.cached_property. But this omits a critical problem with using @functools.cache on instance methods: it is an unbounded cache, and will use self as a cache key, so any instance on which this "cached property" is ever accessed will be kept alive by the cache forever (unless the cache is manually cleared), and the memory used by these cached instances can grow unboundedly.

The FAQ entry on caching methods does not recommend (or even mention) functools.cache, and it explicitly recommends a bounded functools.lru_cache in order to avoid keeping alive all instances indefinitely.

Specific proposals:

  1. The cached_property documentation should not recommend using functools.cache on a method.
  2. It also doesn't need to give a full example of using lru_cache for a method cache; it can instead just mention the possibility, and link to the FAQ entry for more detailed discussion.

Linked PRs
@carljm carljm added the docs Documentation in the Doc dir label May 3, 2023
carljm added a commit to carljm/cpython that referenced this issue May 3, 2023
@carljm
pythongh-104112: link from cached_property docs to method-caching FAQ
b6643b4
@carljm carljm mentioned this issue May 3, 2023
Merged
@sunmy2019
Copy link
Member

LGTM. May backport to earlier versions?

@hauntsaninja
Copy link
Contributor

Yup, we usually backport documentation fixes to maintained Python versions.

Jelle wrote up some general guidelines around backporting here: https://github.com/JelleZijlstra/JelleZijlstra.github.io/blob/main/cpython.md#backporting

carljm added a commit that referenced this issue May 4, 2023
@carljm @hauntsaninja
gh-104112: link from cached_property docs to method-caching FAQ (#104113
fa86a77 
)

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@bedevere-bot bedevere-bot mentioned this issue May 4, 2023
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 4, 2023
@carljm @hauntsaninja @miss-islington
pythongh-104112: link from cached_property docs to method-caching FAQ (…
0271e1c 
…pythonGH-104113)

(cherry picked from commit fa86a77)

Co-authored-by: Carl Meyer <carl@oddbird.net>
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
carljm added a commit that referenced this issue May 4, 2023
@miss-islington @carljm @hauntsaninja
[3.11] gh-104112: link from cached_property docs to method-caching FAQ (
bb4369e 
GH-104113) (#104182)

gh-104112: link from cached_property docs to method-caching FAQ (GH-104113)
(cherry picked from commit fa86a77)

Co-authored-by: Carl Meyer <carl@oddbird.net>
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
carljm added a commit to carljm/cpython that referenced this issue May 5, 2023
@carljm
Merge branch 'main' into inlinecomp2
8a78a36 
* main: (61 commits)
  pythongh-64595: Argument Clinic: Touch source file if any output file changed (python#104152)
  pythongh-64631: Test exception messages in cloned Argument Clinic funcs (python#104167)
  pythongh-68395: Avoid naming conflicts by mangling variable names in Argument Clinic (python#104065)
  pythongh-64658: Expand Argument Clinic return converter docs (python#104175)
  pythonGH-103092: port `_asyncio` freelist to module state (python#104196)
  pythongh-104051: fix crash in test_xxtestfuzz with -We (python#104052)
  pythongh-104190: fix ubsan crash (python#104191)
  pythongh-104106: Add gcc fallback of mkfifoat/mknodat for macOS (pythongh-104129)
  pythonGH-104142: Fix _Py_RefcntAdd to respect immortality (pythonGH-104143)
  pythongh-104112: link from cached_property docs to method-caching FAQ (python#104113)
  pythongh-68968: Correcting message display issue with assertEqual (python#103937)
  pythonGH-103899: Provide a hint when accidentally calling a module (pythonGH-103900)
  pythongh-103963: fix 'make regen-opcode' in out-of-tree builds (python#104177)
  pythongh-102500: Add PEP 688 and 698 to the 3.12 release highlights (python#104174)
  pythonGH-81079: Add case_sensitive argument to `pathlib.Path.glob()` (pythonGH-102710)
  pythongh-91896: Deprecate collections.abc.ByteString (python#102096)
  pythongh-99593: Add tests for Unicode C API (part 2) (python#99868)
  pythongh-102500: Document PEP 688 (python#102571)
  pythongh-102500: Implement PEP 688 (python#102521)
  pythongh-96534: socketmodule: support FreeBSD divert(4) socket (python#96536)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
docs Documentation in the Doc dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

gh-104112: link from cached_property docs to method-caching FAQ carljm/cpython
3 participants
@carljm @hauntsaninja @sunmy2019