pytransitions / transitions

A lightweight, object-oriented finite state machine implementation in Python with many extensions
MIT License
5.49k stars 525 forks source link

Multiple machines on the same model only triggers state updates of first one #588

Closed skion closed 1 year ago

skion commented 1 year ago

Describe the bug The top-level README describes how you can attach multiple machines to the same model by using the model_attribute parameter. However, when subsequently using the transition functions added to the model, only the first machine appears to be invoked.

Minimal working example

from transitions import Machine

class Model:
    pass

lump = Model()

states = ["state1", "state2"]
transitions = [["switch", "state1", "state2"]]

machine1 = Machine(model=lump, model_attribute="attr1", states=states, transitions=transitions, initial="state1")
machine2 = Machine(model=lump, model_attribute="attr2", states=states, transitions=transitions, initial="state1")

lump.switch()

assert lump.attr1 == 'state2'

However, this fails:

assert lump.attr2 == 'state2'

Expected behavior

Based on the documentation I would expect the call to lump.switch() to then update state of both of the machines attached to the model and hence lump.attr2 to be equal to state2.

Alternatively I'd vote to remove this part of the documentation to avoid confusion.

aleneum commented 1 year ago

Hello @skion,

you can use multiple machines with the same model. Convenience functions are not overridden though. This becomes an issue when you use the same trigger/event names. The ReadMe also states

To be more precise, your model should not already contain methods with the same name as event triggers since transitions will only attach convenience methods to your model if the spot is not already taken. If you want to modify that behaviour, have a look at the FAQ.

If you add basic logging to your example:

import logging
# ... 
logging.basicConfig()

This will be the output:

WARNING:transitions.core:Model already contains an attribute 'trigger'. Skip binding.
WARNING:transitions.core:Model already contains an attribute 'switch'. Skip binding.
WARNING:transitions.core:Model already contains an attribute 'may_switch'. Skip binding.

The mentioned FAQ contains this section:

There is a high chance that your model already contained a trigger method or methods with the same name as your even trigger. In this case, transitions will not add convenience methods to not accidentally break your model and only emit a warning. If you defined these methods on purpose and want them to be overriden or maybe even call both -- the trigger event AND your predefined method, you can extend/override Machine._checked_assignment which is always called when something needs to be added to a model

from transitions import State, Machine

class StateMachineModel:

    state = None

    def __init__(self):
        pass

    def transition_one(self):
        print('transitioning states...')

    def transition_two(self):
        print('transitioning states...')

class OverrideMachine(Machine):

    def _checked_assignment(self, model, name, func):
        setattr(model, name, func)

class CallingMachine(Machine):

    def _checked_assignment(self, model, name, func):
        if hasattr(model, name):
            predefined_func = getattr(model, name)
            def nested_func(*args, **kwargs):
                predefined_func()
                func(*args, **kwargs)
            setattr(model, name, nested_func)
        else:
            setattr(model, name, func)

states = [State(name='A'), State(name='B'), State(name='C'), State(name='D')]
transitions = [
    {'trigger': 'transition_one', 'source': 'A', 'dest': 'B'},
    {'trigger': 'transition_two', 'source': 'B', 'dest': 'C'},
    {'trigger': 'transition_three', 'source': 'C', 'dest': 'D'}
]
state_machine_model = StateMachineModel()

print('OverrideMachine ...')
state_machine = OverrideMachine(model=state_machine_model, states=states, transitions=transitions, initial=states[0])
print('state_machine_model (current state): %s' % state_machine_model.state)
state_machine_model.transition_one()
print('state_machine_model (current state): %s' % state_machine_model.state)
state_machine_model.transition_two()
print('state_machine_model (current state): %s' % state_machine_model.state)

print('\nCallingMachine ...')
state_machine_model = StateMachineModel()
state_machine = CallingMachine(model=state_machine_model, states=states, transitions=transitions, initial=states[0])
print('state_machine_model (current state): %s' % state_machine_model.state)
state_machine_model.transition_one()
print('state_machine_model (current state): %s' % state_machine_model.state)
state_machine_model.transition_two()
print('state_machine_model (current state): %s' % state_machine_model.state)

I hope this contains info you need to resolve your issue.

aleneum commented 1 year ago

E.g. this would result in what you are trying to achieve:

from transitions import Machine
import logging

class Model:
    pass

class CallingMachine(Machine):

    def _checked_assignment(self, model, name, func):
        if hasattr(model, name):
            predefined_func = getattr(model, name)
            def nested_func(*args, **kwargs):
                predefined_func()
                func(*args, **kwargs)
            setattr(model, name, nested_func)
        else:
            setattr(model, name, func)

logging.basicConfig()
lump = Model()

states = ["state1", "state2"]
transitions = [["switch", "state1", "state2"]]

machine1 = Machine(model=lump, model_attribute="attr1", states=states, transitions=transitions, initial="state1")
# since lump now has convenience functions attached to it, we need to override the standard behavior where
# name collisions would skip model binding.
machine2 = CallingMachine(model=lump, model_attribute="attr2", states=states, transitions=transitions, initial="state1")

lump.switch()

assert lump.attr1 == 'state2'
skion commented 1 year ago

Thanks @aleneum, although this behaviour was slightly unexpected to me, you've given me some good clues to work around it!

aleneum commented 1 year ago

I guess I will add a remark to the Readme to clarify this. Thank you for the feedback. Another remark:

If you plan to add more than two models, an 'event bus' model and triggering events by name might be a suitable approach:

from transitions.extensions import HierarchicalMachine

class Model:

    def __init__(self):
        self.machines = []

    def trigger(self, event_name):
        return [machine.trigger_event(self, event_name) for machine in self.machines]

lump = Model()

states = ["state1", "state2"]
transitions = [["switch", "state1", "state2"]]

lump.machines.append(HierarchicalMachine(model=lump, model_attribute="attr1", states=states, transitions=transitions, initial="state1"))
lump.machines.append(HierarchicalMachine(model=lump, model_attribute="attr2", states=states, transitions=transitions, initial="state1"))

lump.trigger("switch")

assert lump.attr1 == 'state2'

I just use HSMs here since Hierarchical.trigger_event is a public function and slightly easier to comprehend (imo) than Machine._get_trigger which does the same though. Since convenience functions are not needed in this setting, (Hierarhical)Machine._checked_assignment could be overridden with pass.

skion commented 1 year ago

FWIW, I'm now using something similar to the event bus you suggested in the last comment...