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
| 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
| 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
| """ | ||
| 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! |
3 participants
Fixes #1028
@kernc I followed your suggestion to use
localsbut still need a deepcopy or else the reference cycle will still be present.