PySCIPOpt callbacks and Model lifetime

[gasse]

Discussed in https://github.com/ds4dm/ecole/discussions/193

A partial fix has been merged into PySCIPOpt here. Still, I understand that one must keep a reference to the PySCIPOpt object that was used to create the event handler somewhere to use event handlers registered through PySCIPOpt within Ecole (comment here).

What if the model.as_pyscipopt() method would store such a reference internally on the first call, and then simply return that reference again on later calls, as long as the SCIP model does not change ? @AntoinePrv do you think that would work ?

Best

[AntoinePrv]

Adding an attribute might be a bit trickier than it seems because the oject is a C++ one, but perhaps a keep alive policy could help here.

[AntoinePrv]

I've successfully reproduce the problem in a test in #281.

The problem is that PySCIPOpt callback use a weak reference (a reference that does not do reference counting) to the PySCIPOpt Model. That is the case because otherwise they would have a cyclic reference loop: the Model holds a reference to the callback and the callback would holds a reference to the Model. The reference count never goes to 0.

We have the same problem in the sense that the following code is legal in Ecole

pyscipopt_model = next(ecole.instance.SetCoverGenerator()).as_pyscipopt()

For this to work, the pyscipopt_model (somehow) keeps a reference to the Ecole Model itself (because the SCIP* ownership remains with Ecole). So if we do the opposite, we create a loop ourselves. Giving the ownership of the SCIP* to PySCIPOpt to remove the reference to the Ecole Model is also a no go because then the following code would crash (let alone the ReverseControl).

model = next(ecole.instance.SetCoverGenerator())
model.as_pyscipopt().getSomething()  # SCIP* deallocated after this point
model.solve()

The correct way to do this would be for Ecole and PySCIPOpt model not to rely on each other for the ownership of the SCIP*, but to share it in a PyCapsule for reference counting. However, that's not currently possible for us, because in C++ the Model uses a unique_ptr<SCIP>. For it to work, we'd have to refactor Scimpl, so that it can be replaced with a PyCapsule, and find somewhere else to put the reverseControl/thread (maybe attached data to the SCIP*) so that it does not crash when transferring ownership

[gasse]

Wow, that looks like big progress. Well done @AntoinePrv . Let's discuss what to do with that in the next meeting.