Skip to content

Fix memory leak from lru_cache on _FanovaTree instance methods - #6859

Closed
kratos0718 wants to merge 1 commit into
optuna:masterfrom
kratos0718:fix/fanova-tree-lru-cache-leak
Closed

kratos0718 wants to merge 1 commit into
optuna:masterfrom
kratos0718:fix/fanova-tree-lru-cache-leak

Conversation

@kratos0718

Copy link
Copy Markdown

Motivation

_FanovaTree._is_node_leaf, _get_node_value, and five other methods in optuna/importance/_fanova/_tree.py are decorated with @lru_cache(maxsize=None) directly on the class. Decorating an instance method this way keys the cache on self as well as the other arguments, so every _FanovaTree instance ever constructed stays reachable through the cache for the life of the process, even after the caller has dropped every other reference to it.

_FanovaTree is constructed once per estimator in the random forest fitted inside _Fanova ([_FanovaTree(e.tree_, search_spaces) for e in self._forest.estimators_]), so every call to get_param_importances() builds a batch of these and normally expects them to be freed once the importances are computed. With maxsize=None, they instead leak for good, along with whatever self._tree/self._statistics/self._search_spaces they're holding onto — this adds up in any long-running process that computes feature importances repeatedly (a dashboard, a service, a notebook running many studies).

Description of the changes

Moved the caching from class-level decorators to per-instance caching, by wrapping the bound methods in __init__ instead of decorating them at class scope:

self._is_node_leaf = lru_cache(maxsize=None)(self._is_node_leaf)

This keeps the exact same caching behavior for the lifetime of a _FanovaTree instance, but the cache (and everything it holds) is now freed along with the instance instead of outliving it.

Added a regression test (test_tree_is_garbage_collected) that constructs a tree, exercises the cached methods, drops the only reference, and asserts the instance is actually collected. It fails against the old code and passes with this fix. All existing tests in test_tree.py pass unchanged (40 passed), and ruff check/ruff format --check are clean on both changed files.

_FanovaTree._is_node_leaf, _get_node_value, and five other methods were
decorated with @lru_cache(maxsize=None) directly on the class. Decorating
an instance method that way keys the cache on self too, so every
_FanovaTree ever constructed (one per random-forest estimator, on every
get_param_importances() call) stayed reachable through the cache for the
life of the process instead of being garbage-collected once the caller
was done with it.

Moved the caching to per-instance by wrapping the bound methods in
__init__ instead of decorating them at class scope, so the cache (and
everything it holds onto) is freed along with the instance. Added a
regression test that constructs a tree, drops the only reference, and
asserts it gets collected - it fails on the old code and passes with
this fix.
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has not seen any recent activity.

@github-actions github-actions Bot added the stale Exempt from stale bot labeling. label Sep 24, 2026
@not522

not522 commented Sep 25, 2026

Copy link
Copy Markdown
Member

Thank you. However, this can be resolved with a simpler implementation, so I am closing this PR.

@not522 not522 closed this Sep 25, 2026
@not522 not522 mentioned this pull request Oct 13, 2026
1 task done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Exempt from stale bot labeling.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants