fix(solvers): dispose the solver model before its env on close - #899
Conversation
Solver.close() closed the env ExitStack before dropping solver_model, so the native model was collected against freed memory — a Fatal Python error or Windows access violation from an unrelated GC pass. COPT also closed its env in a finally while returning the model built in it; the env now lives on the solver's ExitStack.
Merging this PR will degrade performance by 3.51%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | test_to_lp[milp-n=50] |
2 MB | 2.7 MB | -24.39% |
| ❌ | Memory | test_to_lp[nodal_balance-severity=50] |
2.8 MB | 3.4 MB | -18.18% |
| ❌ | Memory | test_to_lp[merge_balance-severity=0] |
2.6 MB | 3.2 MB | -17.89% |
| ❌ | Memory | test_to_lp[rolling-severity=0] |
2.7 MB | 3.1 MB | -12.56% |
| ⚡ | Memory | test_to_lp[rolling-severity=50] |
429.7 MB | 305.6 MB | +40.61% |
| ⚡ | Memory | test_to_lp[masked-n=100] |
2.7 MB | 2.1 MB | +29.24% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/solver-close-ordering (ee50956) with master (09c34dd)
Footnotes
-
175 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
d97a8d1 to
f3a4405
Compare
Build cost — v1 vs legacyv1 build peak & time relative to legacy, on this commit — not a comparison against master (that is CodSpeed).
Full table (time + peak, mean)📊 Interactive plots + CSV: download the semantics-report-v1-vs-legacy artifact from this run. Report-only · not a gate · refreshed on every push · obsolete once legacy is dropped. |
|
@FabianHofmann Lets land this to get the other PRs green again! |
…is a list coptpy returns a coptcore.VarArray, so the isinstance check could never pass where COPT is installed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A Solver is only ever reachable in a Model/Solver reference cycle, so it is reclaimed by the cyclic collector and never by refcount. Its __del__ therefore ran mid-collection, tearing down native solver handles at an arbitrary point in an unrelated call stack — the interpreter aborted in coptpy.Envr(), cplex.Cplex(), GLPK's result parsing and inside xarray alignment, in a different CI job on every run. close() is unchanged and is still called when a new solve replaces the solver and when model.solver is reassigned. Solvers that are never closed are left to the vendor wrappers, which dispose in their own finalizers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The file interface built a cplex.Cplex() per solve, handed it back as solver_model and never ended it, so every solve left a live CPLEX environment for the garbage collector. Collecting one re-entered the CPLEX library while another environment was being constructed, which aborted the interpreter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This reverts commit c420696.
Keeping the environment on the solver's ExitStack leaked it: with no finalizer, a solver that is merely dropped — every solve in the test suite builds a fresh Model and drops it — never runs close(), so the callback never fires. Measured on the file-based path: 20 solves created 20 COPT environments and closed none, against 20/20 on master. Close per solve as before, but do not hand back a solver model whose environment has just been closed; model.solver_model is None after a file-based COPT solve rather than a handle into freed memory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MindOpt disposes both the model and the environment before returning, so the handle it handed back as solver_model pointed at freed memory — the same defect as COPT. Knitro already does this correctly: it extracts what it needs into a plain dataclass before freeing the native context. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Restore an unrelated blank line, shorten the close() docstring and drop the typing import the spy no longer needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removing Solver.__del__ withdrew a guarantee announced in 0.8.0, so say so where it was promised: Model.solve still claimed the solver is released on garbage collection, and the note sat under bug fixes rather than breaking changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@FabianHofmann This is quite tidious. Maybe making Solver.model a weakref could fix it... |
|
@FabianHofmann This PR now fixes the bug with a small tradeoff. |
|
we can live with that |
The current CI on master is flaky with COPT, let's test if it has to do with the potential issue raised and fixed here
Note
The following content was generated by AI.
Changes proposed in this Pull Request
Solver.close()closed the environment before dropping the model that lives in it:Solvers that register their model on the environment's
ExitStack— Gurobi (since #826), Xpress, Mosek — get the right LIFO ordering and were unaffected. HiGHS, SCIP, COPT and MindOpt only dereference the model, so its native destructor ran against a freed environment. Becauseclose()is also called from__del__, the crash lands in whatever code happened to trigger the collection, which makes it look unrelated and intermittent.This is what CI has been hitting on
master:Windows fatal exception: access violation, faulting framesolvers.pyclose()atself.solver_model = None, reached from__del__during a garbage collection pass inside unrelatedxarrayalignment code.Fatal Python error: AbortedinsideCOPT._run_fileatenv_.createModel()— a later COPT call aborting on state a previous freed model corrupted.Two changes:
Solver.close()dropssolver_modelfirst, then closes the environment stack. For solvers whose model sits on the stack this is a no-op (the stack still holds the reference and disposes it first); for the others the model is now released while its environment is still alive.COPT._run_fileno longer closes its environment in afinallywhile returning the model built in it. The environment is registered on the solver'sExitStack, so it is released bySolver.close()andmodel.solver_modelstays usable aftermodel.solve("copt")— matching Gurobi's behaviour.Verification
Both crashes are macOS/Windows-only and were not reproducible on Linux, so this fix is validated by CI rather than locally. Locally (Linux, Python 3.13) the full suite passes: 2980 passed, 33 skipped, with
test/remoteand Xpress excluded — Xpress is deselected because of an unrelated local install problem (NameError: name 'xpress' is not defined), andtest/remoteneeds OETC credentials.mypyreports no new errors, andruff check/ruff formatare clean.test_copt_env_persists_after_solvepins the COPT invariant. It is skipped locally (no valid COPT licence on this machine) and runs on the CI hosts where COPT is licensed.Crash traceback (Windows, master and PR #717)
Crash traceback (macOS, master run 32129308681)
Checklist
AGENTS.md).doc.doc/release_notes.rstof the upcoming release is included.