Support weak references to models

[thedrow]

Is your feature request related to a problem? Please describe. As evident by https://github.com/celery/jumpstarter/pull/36, using weakref.proxy is very hard to do correctly with transitions. proxies are not hashable which interferes with many checks in the code base. Currently, transitions do not resolve weakref.ref objects into the objects they are pointing to. This results in exceptions when a weakref is provided as a model since the library expects certain attributes to be attached to the model object but they are not present in weakref.ref.

Describe the solution you'd like We should always check if a model is a weakref.ref and if it is, we need to dereference it by calling model() first.

Additional context None

[aleneum]

Hello @thedrow,

As evident by celery/jumpstarter#36, using weakref.proxy is very hard to do correctly with transitions. proxies are not hashable which interferes with many checks in the code base.

you are right, transitions uses model hashes to bind some machine-managed attributes like graphs to models. But I don't think these must be hashes. My initial idea would be to use object id instead.

We should always check if a model is a weakref.ref and if it is, we need to dereference it by calling model() first.

phew... this sounds like a quite massive change if every passed model has to be checked...

[thedrow]

I actually had a proposal in mind but I didn't have time to implement it. What if we make models a property that checks for weak references and dereferences them? If they are None, they should be excluded.

[aleneum]

Just tried to test my local changes from model_graphs[model] to model_graphs[id(model)] with model = weakref.proxy(actual_model) and stumbled upon this issue.

[aleneum]

I added some simple test cases for weakref.proxy models and things like

from transitions.extensions import GraphMachine
import weakref

class Model:
    pass

m = Model()
r = weakref.proxy(m)

machine = GraphMachine(r, states=['A', 'B'], transitions=[['go', 'A', 'B']], initial='A')
r.go()
r.get_graph().draw('tst1.png', prog='dot')

work now (also for Python 3.8 and earlier).

[aleneum]

FYI: Seems like hash forwarding for weakref.proxy will be rolled back in Python 3.9 and following. This means that soon, weakref.proxy will raise a TypeError again for instance when one attempts to use a proxy as a key in a dict.

[aleneum]

I assume switching from objects to ids for internal cache management did the trick. Since 0.8.9 has been released, I am closing this for now. Feel free to comment anyway. I will reopen the issue if necessary.

[hhamana]

Recently updated to 0.8.10 from 0.8.8, turns out using id() as keys for caches was actually a breaking change for us, despite the patch semver.

[thedrow]

So let's release 1.0 and make that breaking change?

[aleneum]

Hello @hhamana,

turns out using id() as keys for caches was actually a breaking change for us, despite the patch semver.

I am sorry to hear that. I expected this to be a purely internal issue but it seem I was wrong. Could you elaborate how this change interfered with your use case?

@thedrow: As of now 'id-based model management' is part of 0.8.10 and will be part of 0.9 which is currently in the next branch. Depending on what @hhamana reports I might roll this change back in 0.8.11 but will keep 'id-based model management' for 0.9 unless there is a major drawback when handling models by id. If that's the case, I might need to find another way to reference models.

[hhamana]

@aleneum The breaking change lies in the model_graphs cache iirc, which we were using to override the nested graphviz styling when using get_graph. We were overriding with the machine object as key, as that was how it worked. After updating, would not resolve as it was looking for the id(machine) instead. I'm not sure customizing graphviz styling through this cache was even fully documented in the first place though, but, that's our use case. Changing to using id(machine) as keys did fix it once I figured it out.

[thedrow]

Then this is a bug caused by this change which we should fix. Can you please open an issue about it or submit a PR to fix the problem?

[hhamana]

@thedrow hmm, I don't really consider it a bug personally, just an unexpected api change, the scope of which might simply have been underestimated. Sorry for the confusion.