Use deepcopy to prevent reference cycles in Optimizer #1029
Conversation
skopt/optimizer/base.py
Outdated
| specs = {"args": copy.copy(inspect.currentframe().f_locals), | ||
| "function": inspect.currentframe().f_code.co_name} | ||
| specs = {"args": copy.deepcopy(locals()), | ||
| "function": copy.deepcopy(inspect.currentframe().f_code.co_name)} |
There was a problem hiding this comment.
What if we replace function with literal 'base_minimize' as in the other example? I don't like manual accounting, but what is the function of specs/'function' anyway.
skopt/optimizer/optimizer.py
Outdated
| @@ -171,7 +170,7 @@ def __init__(self, dimensions, base_estimator="gp", | |||
| model_queue_size=None, | |||
| acq_func_kwargs=None, | |||
| acq_optimizer_kwargs=None): | |||
| self.specs = {"args": copy.copy(inspect.currentframe().f_locals), | |||
| self.specs = {"args": copy.deepcopy(locals()), | |||
There was a problem hiding this comment.
I'm thinking, if you drop self from locals(), copying shouldn't be necessary any longer.
There was a problem hiding this comment.
Great call - didn't know self wasn't needed!
skopt/optimizer/optimizer.py
Outdated
| @@ -171,7 +169,9 @@ def __init__(self, dimensions, base_estimator="gp", | |||
| model_queue_size=None, | |||
| acq_func_kwargs=None, | |||
| acq_optimizer_kwargs=None): | |||
| self.specs = {"args": copy.copy(inspect.currentframe().f_locals), | |||
| args = locals() | |||
There was a problem hiding this comment.
I guess the function call constructs a dict on the fly, but I'd do a locals().copy() to comply with the note here.
skopt/optimizer/base.py
Outdated
| @@ -2 8000 09,8 +209,8 @@ def base_minimize(func, dimensions, base_estimator, | |||
| For more details related to the OptimizeResult object, refer | |||
| http://docs.scipy.org/doc/scipy/reference/generated/scipy.optimize.OptimizeResult.html | |||
| """ | |||
| specs = {"args": copy.copy(inspect.currentframe().f_locals), | |||
| "function": inspect.currentframe().f_code.co_name} | |||
| specs = {"args": copy.deepcopy(locals()), | |||
There was a problem hiding this comment.
Given that there's no deepcopying (or copying even) in the other example below, is deepcopy here necessary?
Did you maybe check the garbage graph afterwards? Are we safe?
There was a problem hiding this comment.
Good point I kept the copy to be safe but after visualizing the graph of OptimizeResult and I don't see any circular references. The obj graph is quite big because the specs contain the global branin function but that's expected. Worth noting that if you pass an object method as the func argument to base_minimize, that object will be referred to by OptimizeResult as long as OptimizeResult lives.
|
Thanks! |
Fixes #1028
@kernc I followed your suggestion to use
localsbut still need a deepcopy or else the reference cycle will still be present.