Repository navigation
Fix memory leak from lru_cache on _FanovaTree instance methods - #6859
Closed
kratos0718 wants to merge 1 commit into
Closed
kratos0718 wants to merge 1 commit into
kratos0718 wants to merge 1 commit into
Conversation
_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.
Contributor
|
This pull request has not seen any recent activity. |
Member
|
Thank you. However, this can be resolved with a simpler implementation, so I am closing this PR. |
1 task done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
_FanovaTree._is_node_leaf,_get_node_value, and five other methods inoptuna/importance/_fanova/_tree.pyare decorated with@lru_cache(maxsize=None)directly on the class. Decorating an instance method this way keys the cache onselfas well as the other arguments, so every_FanovaTreeinstance ever constructed stays reachable through the cache for the life of the process, even after the caller has dropped every other reference to it._FanovaTreeis 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 toget_param_importances()builds a batch of these and normally expects them to be freed once the importances are computed. Withmaxsize=None, they instead leak for good, along with whateverself._tree/self._statistics/self._search_spacesthey'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:This keeps the exact same caching behavior for the lifetime of a
_FanovaTreeinstance, 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 intest_tree.pypass unchanged (40 passed), andruff check/ruff format --checkare clean on both changed files.