#17320: Memory leaks with LP Solvers are back
-------------------------------------+-------------------------------------
Reporter: pmueller | Owner:
Type: defect | Status: new
Priority: major | Milestone: sage-6.4
Component: linear | Resolution:
programming | Merged in:
Keywords: | Reviewers:
Authors: | Work issues:
Report Upstream: N/A | Commit:
Branch: | bc20667184ca8209776d8c363df2aa3e65994287
u/nbruin/memory_leaks_with_lp_solvers_are_back| Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Comment (by ncohen):
Hello,
> It seems to me that only committing to the new model by assigning it to
`self.model` at the very end is a design feature:
I wrote this file myself, and I do not think that this had the slightest
importance to me at that time.
> it means that if the computation fails for any reason,
When an exception is raised in that code, it is almost always because the
problem is infeasible. This is not a "failure", it is the actual answer
the user is looking for.
> the object `self` is unaffected and hence still in a consistent state.
It's also better for future development, because as long as model is
private, there is no lock required to modify it. Once it's exported to
`self.model`, you'd need to lock self before `self.model` is changed.
Python currently doesn't support such multiprocessing programming anyway,
but having changes in global state as atomic as possible is generally a
good feature, because it reduces the possibility for unexpected
interactions between multiple threads (the GIL does get released every now
and again ...)
If your worry is that in a multithread Python program with a
MixedIntegerLinearProgram instance `p` the function `p.solve()` may be
called simultaneously from different processors, I believe that you can
forget it. Even if that made the sligthest sense, you would have many
other places in the code to re-read and adapt.
> With your change, the model that just experienced an error is now stored
in `self`. It could be convenient to have it available for further
analysis, so a priori it's not clear which design is more useful. It
depends on the details of what the code does and how it does it.
Well, again what you call an 'error' is just a certificate of
infeasibiity, and this is definitely good to have around. But the fact
that all doctests pass (even in the graph/ folder) indicates that this
change brings no real problem.
> Furthermore, there is still this code between `model` allocation and
committing to `self.model=model`.
> {{{
> model.setLogLevel(self.model.logLevel())
>
> # multithreading
> import multiprocessing
> model.setNumberThreads(multiprocessing.cpu_count())
>
> model.branchAndBound()
> }}}
> which looks like it's quite nontrivial and hence could raise errors by
itself. You'd still have a leak then. So either guard that with a
`try/finally`, in which case you might as well stick with the guard all
the way, or move the `self.model=model` even higher.
Please move that line higher if you care. I believe that you are being
worried unnecessarily.
Nathann
--
Ticket URL: <http://trac.sagemath.org/ticket/17320#comment:15>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
