Remove incorrect Py_INCREF/DECREF on Model in catchEvent/dropEvent

[Joao-Dionisio]

No description provided.

[Copilot]

Pull request overview

This PR removes manual Py_INCREF/ Py_DECREF refcount adjustments on Model inside catchEvent/dropEvent to prevent Model instances from being kept alive when event handlers don’t reliably call dropEvent, and adds a regression test to ensure the Model is garbage-collected.

Changes:

• Removed Py_INCREF(self) in Model.catchEvent() and Py_DECREF(self) in Model.dropEvent().
• Added a regression test that uses weakref + gc.collect() to assert Model is not leaked when dropEvent is skipped.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
tests/test_event.py	Adds a regression test asserting catchEvent does not keep a Model alive after deletion.
src/pyscipopt/scip.pxi	Removes the refcount manipulation in catchEvent/dropEvent that could cause Model leaks.

Comments suppressed due to low confidence (1)

tests/test_event.py:3

• sys is imported but never used in this test module. Please remove the unused import to keep the test file clean (and to avoid failures if a linter is added/enabled for tests).

import gc
import sys
import weakref

---

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

[DominikKamp]

And what prevents event handlers from calling dropEvent()?

[Joao-Dionisio]

@DominikKamp Nothing, but I don't think anything bad will happen if they do.

If the question is why wouldn't a user just do it, I don't think we should expect Python users to worry about these things.

[DominikKamp]

Suggested change

	del ev
	del m
	gc.collect()
	del m
	gc.collect()
	assert ref() is not None, "Model was accidentally garbage collected — event handler active"
	del ev
	gc.collect()
	del m
	gc.collect()
	assert ref() is not None, "Model was accidentally garbage collected — event handler active"
	del ev
	gc.collect()

GitHub messed up the suggestion.

[Joao-Dionisio]

Wait, why would you want the event handler to remain active after deleting the model?

[Joao-Dionisio]

Also, we're using weakref proxies, so event handlers aren't directly tying Models down.

I have a branch somewhere that reverts the usage of these weakrefs, but never got around to creating a PR.

[DominikKamp]

The event handler references the model, so it should not be garbage collected before the event handler is deleted. If del kills the model, why do we expect reference counters to prevent this?

[Joao-Dionisio]

Hang on, the weak ref branch fixes this more cleanly, I'll create another PR in a bit

[Joao-Dionisio]

ok not more clean, as users would need to explicitly free their models.
But you can take a look at the branch remove-weakref-proxy for this alternative.

[DominikKamp]

Sounds very clean. But I just do not understand the test. Did it fail before the fix?

[Joao-Dionisio]

Regarding your point, the event handler does not reference the model. It only has a weakly-referenced proxy. This was first discussed in #26

[DominikKamp]

It only has a weakly-referenced proxy.

And why does this make sense or is required?

[Joao-Dionisio]

Felipe was saying this:

I understand that removing the weakref alternative is attractive for someone coming from the C world, but from a Python perspective, it's not intuitive that you start having to worry about memory.