Replace weakref.proxy with strong references for plugin self.model (#…

[Joao-Dionisio]

Fix #1193

Picking up something I had been playing with. We keep the references to the plugins, and allow Python to garbage collect them itself. This is the proper fix to the problem fixed by #1190. I was just hesitant to use it because, at the time, this would require users to explicitly call dropEvent(). This is no longer the case.

Can you please take a look @DominikKamp ?

[Copilot]

Pull request overview

This PR addresses issue #1193 by replacing weakref.proxy-based plugin.self.model references with strong references and by keeping explicit references to included plugins, so cleanup callbacks (e.g., consfree, eventexit) can safely access self.model during SCIP teardown.

Changes:

• Switch plugin self.model from weakref.proxy(self) to strong references and track included plugins on the Model.
• Add Model.__del__ / refactor cleanup into _free_scip_instance(), plus introduce a public Model.free() for deterministic cleanup and cycle breaking.
• Update tests to validate GC/cleanup behavior and adjust warning handling for unraisable exceptions.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/pyscipopt/scip.pxi	Core change: strong plugin.model, plugin retention list, new finalize/free logic
src/pyscipopt/scip.pxd	Declares new Model fields/method (_plugins, _free_scip_instance)
tests/test_event.py	Adds/updates tests around event cleanup, GC behavior, and strong refs
tests/test_conshdlr.py	Removes ReferenceError workaround and forces GC to ensure callbacks run
tests/test_pricer.py	Filters unraisable-exception warnings for an incomplete pricer test
CHANGELOG.md	Documents the behavioral fix for #1193

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

_free_scip_instance() calls SCIPfree(&self._scip) without checking the return code. If SCIPfree fails, the code still sets self._scip = NULL and self._freescip = False, which can silently leak the SCIP instance and leave the object in an inconsistent state. Please wrap SCIPfree with PY_SCIP_CALL (or explicitly check the returned SCIP_RETCODE) and only NULL/reset fields after a successful free; if this is being avoided to prevent exceptions in finalizers, consider catching/suppressing the exception in __del__ while still surfacing it in free().

[Copilot]

free() assumes every entry in self._plugins has a .model attribute. This is not true for all plugin base classes (e.g., IISfinder in iisfinder.pxi does not define model), so plugin.model = None can raise AttributeError and prevent cleanup. Please either (a) only store plugins that participate in the Model↔plugin reference cycle, (b) add a model attribute to all plugin base classes that can be included, or (c) guard with hasattr(plugin, "model") / try/except AttributeError so cleanup always completes.

Suggested change

	plugin.model = None
	try:
	plugin.model = None
	except AttributeError:
	# Plugin does not expose or allow setting a 'model' attribute.
	# In that case, there is no Model↔plugin reference to break.
	pass

[Copilot]

free() frees the SCIP instance but leaves large Python-side caches/references (_modelvars, _modelconss, _benders_subproblems, possibly _bestSol) intact. Since the docstring says the Model should not be used after free(), it would be safer to also clear these containers to allow Python memory to be reclaimed immediately and to avoid accidental post-free access to stale wrappers.

Suggested change

	# Clear Python-side caches/references that may hold onto SCIP-related objects.
	if hasattr(self, "_modelvars") and self._modelvars is not None:
	self._modelvars.clear()
	if hasattr(self, "_modelconss") and self._modelconss is not None:
	self._modelconss.clear()
	if hasattr(self, "_benders_subproblems") and self._benders_subproblems is not None:
	self._benders_subproblems.clear()
	if hasattr(self, "_bestSol"):
	self._bestSol = None

[Copilot]

__dealloc__ uses PY_SCIP_CALL(SCIPfree(&self._scip)), which can raise Python exceptions during finalization. Exceptions raised from dealloc/finalizers are converted into unraisable-exception warnings (and are currently being filtered in tests), which can hide real cleanup problems. Consider calling SCIPfree without PY_SCIP_CALL here (or wrapping in try/except and logging) to avoid emitting unraisable exceptions at shutdown.

Suggested change

	if self._scip is not NULL and self._freescip and PY_SCIP_CALL:
	PY_SCIP_CALL( SCIPfree(&self._scip) )
	if self._scip is not NULL and self._freescip:
	SCIPfree(&self._scip)
	self._scip = NULL
	self._freescip = False

[DominikKamp]

And when is __del__() called?

[Joao-Dionisio]

And when is __del__() called?

__del__() is one of those Python keywords, and is called just before the object is deleted, I believe.

[DominikKamp]

and is called just before the object is deleted

Triggered by the garbage collector, or who deletes this?

[Joao-Dionisio]

Triggered by the garbage collector, or who deletes this?

The garbage collector at the end, or by the garbage collector when you explicitly call del.

[DominikKamp]

So this makes sure that the model is not freed before the plugins, which makes sense, but still someone has to trigger the cycle breaking, or what was the memory reference issue that justified using weak references?