same state machine instance for multiple object instances

[gemerden]

In our use case we have many objects that could be handled with the same state machine (e.g. from database queries). Currently a state machine is instantiated for each object which creates (perhaps unnecessary) overhead.

Would it be possible to have a version of the state machine that points self.machine in multiple objects (of the same class) to a single state machine instance?

[tyarkoni]

This is an interesting idea. In theory there's no principled reason why you shouldn't be able to use the same Machine for multiple models, provided you want identical behavior. Right now the only barrier I see to doing that is that we track the current State object within the Machine instance rather than the model instance. This is actually a design flaw, in my opinion; I think it would be cleaner and more sensible to maintain state only in the model. As it stands we already track the current state name within the model, but not the State instance itself.

This should be easy to fix without breaking the API. Assuming we move all state out of the Machine, we could then modify the initializer to take a list of models rather than just a single model. Any changes applied to the machine (e.g., adding new transitions) would thereafter be applied to all available models. So that would work fine in cases where you have all of your models instantiated in advance of Machine creation. But note that you wouldn't be able to easily add extra models after initializing the Machine. Simply pointing to the right Machine in your models wouldn't give you the desired behavior, since you wouldn't automatically have all the dynamically added triggers and callbacks. For that, we'd need to implement some kind of add_model() method in Machine that re-applies all existing transitions and callbacks. That's also potentially doable, but I'm not sure I want to take that step unless this is going to be a fairly common use case (whereas simply managing multiple existing models should be trivial).

Anyway, thanks for opening this issue. I'll think about it some more, and we should be able to do at least something to accommodate this use case without having to change much.

[gemerden]

Thank you for your quick reply. I think having just the state name in the model is not a problem; it is always possible to access the actual state from the model and from a database persistence point of view having just strings for the state is much easier to handle.

After reading some more of the transitions code i think it would be possible to maintain (just extend) the API, and optionally move all methods (callback, triggers) to the state machine itself, by introducing an optional base class for the models. E.g. for triggers something like this:

class BaseModel(object):

    machine = None # e.g. have one state machine instance for all instances 

    def __getattr__(self, trigger_name):  # only called if  trigger_name was not found on the model
        if trigger_name in self.machine.triggers:  # or some such, maybe not even required
            return partial(getattr(self.machine, trigger_name), model=self)
        raise AttributeError

if __name__ == "__main__":
    class SomeMachine(object):
        triggers = ["some_trigger"]

        def some_trigger(self, model):  # access to machine and model
            print "triggered from "+ type(model).__name__    

    class SomeModel(BaseModel):
        machine = SomeMachine()

    model = SomeModel()
    model.some_trigger()

[tyarkoni]

Unfortunately, I don't think it's that simple; the State instance currently stored in machine.current_state is used for a number of things--e.g., to determine whether the requested transition is valid for the current state, or to update the EventData instance passed to callbacks. So if you have just one Machine with a lot of models, you're going to run into serious problems if you want to allow those models to be in different states. To make things work properly given the current code, current_state will need to be a property of the model. And like I said, I think it's a design flaw to maintain individual model states in the Machine class anyway; in my view, the Machine should focus on the transition logic and support as little state as possible. This isn't hard to do, but would require patching most of core.py as well as all of the subclasses in extensions.py, so it may have to wait until I have some time (unless you want to submit a PR for it).

[tyarkoni]

Oh, and just to clarify: having only the state name in the model is fine; like you say, the corresponding State instance can be looked up in the Machine. But that still requires rewriting various parts of the code to access the right State via the current model (which would have to be passed in from the trigger; right now it isn't) rather than directly from self.current_state.

[aleneum]

Hi,

interesting thoughts. So the machine would manage the 'ruleset' of valid transitions but the actual state is part of the model. I see how this increases the reuse value. I do have some (initial) questions about the desired behaviour and relationship between model and machine though.

• Is a one (machine) to many (models) relationship or a many to many relationship desired?
• Should all models provide all triggers registered in the machine or should the event triggers be spread across all models?
• If add_transition is executed, should that add the trigger to all models related to the machine instance?
• Should the State instances/objects be managed by the machine or also be part of the model?

I would suggest to extend a Event.trigger with a model parameter which is bound with partial during transition creation. This requires that a machine has a list of managed models (if add_transition(s) should be able to register triggers on every model). Some things have to be discussed though (like where to store the state in the model or how to deal with Machine.is_state)

I give it a try if one of you can provide a test case. Just to be sure I try to achieve the right thing :).

[tyarkoni]

@aleneum, agreed, that's exactly the implementation I would go with as well. I've played with this a bit already and have a version of core.py that passes all tests in test_core with machine.current_state completely removed and all references occurring via the model. I'll push the branch shortly for comment. If it looks good, we'd just need to amend the extensions accordingly.

[gemerden]

@aleneum: I don't know the code enough to understand the Event.trigger part, but in answer to your questions:

• Is a one (machine) to many (models) relationship or a many to many relationship desired?:
  I could see a use-case for many to many, but our use-case is one-to-many.
• Should all models provide all triggers registered in the machine or should the event triggers be spread across all models?
  Preferably all state related data and methods be in the machine, and the triggers possibly be made accessible though a call as in the code in my seconds post, so the model would only contain a string for the current_state and a reference to it's state machine (either on class level, as a property or in its __dict__), the state machine would not need the model beforehand, because the model is passed when a trigger on it is called.
• If add_transition is executed, should that add the trigger to all models related to the machine instance?
  preferably to the model (reducing overhead)
• Should the State instances/objects be managed by the machine or also be part of the model?
  They would be constant and part of the machine instance, the current_state string of the model would be they way to access it's State instance.

[tyarkoni]

I just pushed a multiple-models branch that modifies the core Machine instance to handle multiple models. As noted above, right now this only works for models passed in at machine initialization. It will not automatically update new models with existing transitions. We can talk about whether that's worth supporting, but for now I lean towards no.

Right now all changes are completely compatible with the existing API. The one potential sticking point here is that we might want to rename self.model to self.models. Right now, to prevent API breakage, I internally store models in self.models, but define Machine.model as a property that returns a single model if the list has length 1, and the full list otherwise. I'm not thrilled about this, but it seems like the best way to preserve the API, and given that right now probably nobody in the wild is expecting multiple models, it doesn't seem wise to tamper with this.

I also had to remove a test that in my view shouldn't be a supported use case anyway (passing optional arguments with send_event=False when the callback function is not explicitly defined to handle such an argument), as it was interfering with passing the model as the first argument to trigger.

@aleneum, see what you think. If the changes look good, we'll need to similarly update all the extensions etc. A bit tedious, but should be straightforward, I think. @gemerden, assuming you're not using any of the extensions, do you want to test these changes out and see if they work for your use case?

[aleneum]

The tests should pass now but there is still some work to do for graphing. Right now, only the last transition executed will be shown in the graph. My plan would be to move the graph instance also to the model so that every model has a separate graph. What do you think?

[tyarkoni]

+1 on moving the graph to the model.

[tyarkoni]

And thanks for fixing all the other modules so quickly!

[aleneum]

Graph issues have been fixed and nesting also seems to work. But we might want to extend testing of this feature a bit before calling it stable enough for the master branch.

[aleneum]

I guess we can consider this solved with #135. Feel free to reopen this issue if you encounter bugs/limitations or open a new issue.