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.

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

Make sure result objects don't store references to corresponding solver objects #2262

Merged
merged 3 commits into from Nov 14, 2023

Conversation

pmenczel
Copy link
Member

Description
Currently, result objects store a reference to the solver object that created them. I think this is an accident. The options field of a solver is an instance of _SolverOptions (see here), which gets stored in the result here. _SolverOptions has a _feedback field which is assigned an instance method of the solver and thus keeps a reference to the solver.

I noticed this issue when I called qsave on a result object and the resulting file was ~100 MB instead of the expected ~100 KB. Setting result.options._feedback = None before calling qsave reduced the file size by a factor ~2000. But also without pickling, I think that this is a memory leak where solver objects can't be garbage collected.

The fix I am submitting here makes results create a copy of the options passed to it, setting _feedback to None if it exists. I considered converting _SolverOptions objects to plain dicts in the result class, but that would discard a little bit of information. (Note however that in many places, the options passed to the results are plain dicts anyway, for example.) I also considered making _feedback a weak reference, but that would make pickling more complicated.

I am not sure what tests to add about this, if any.

@coveralls
Copy link

Coverage Status

coverage: 84.849% (+0.004%) from 84.845%
when pulling 250692b on pmenczel:result-opt-no-feedback
into 454b615 on qutip:master.

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you for fixing.

Changing options in result to plain dict would also be fine.

I am fine to not have a test for this, but I think adding a check on the size of file created in test_qsave_qload could work.

@pmenczel pmenczel merged commit 4ce8829 into qutip:master Nov 14, 2023
12 checks passed
@pmenczel pmenczel deleted the result-opt-no-feedback branch November 14, 2023 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants