Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HSM: on_enter inside nested machine calls top level machine methods #248

Closed
Grey-Bit opened this issue Aug 7, 2017 · 12 comments
Closed

Comments

@Grey-Bit
Copy link

Grey-Bit commented Aug 7, 2017

In the snippet below, transition is triggered from one nested state to another.

on_enter callback is attached to the destination nested state, but the library is searching for this callback in the top level machine.

from transitions.extensions import HierarchicalMachine as Machine
from transitions.extensions.nesting import NestedState as State

class Nested(Machine):
    def print_msg(self):
        print("Nested")
    def __init__(self):
        self.states = ['n1', {'name':'n2', 'on_enter':'print_msg'}]
        Machine.__init__(self, states=self.states, initial='n1')
        self.add_transition(trigger='goto_n2',
                            source='*',
                            dest='n2')

class Top(Machine):
    def print_msg(self):
        print("Top")
    def __init__(self):
        self.nested = Nested()

        self.states = [  't1',
                        {'name': 't2',
                        'children': self.nested}]
        Machine.__init__(self, states=self.states, initial='t1')
        self.add_transition(trigger='goto_t2',
                            source='*',
                            dest='t2_n1')



top_machine = Top()
top_machine.goto_t2()
top_machine.goto_n2()

The output is "Top"

If I remove print_msg() from the Top class, then I am getting AttributeError.

While in theory it's possible to have the callbacks in the top machine, it would be better to have states and their accompanying callbacks in the same class for better encapsulation and reuse.

Is there any way to achieve the encapsulation by some other means?

@aleneum
Copy link
Member

aleneum commented Aug 9, 2017

Hi @Grey-Bit,

on_enter callback is attached to the destination nested state, but the library is searching for this callback in the top level machine.

string callbacks are always looked up in the model which in your case is the Top machine.
If you do not pass a specific model instance to Machine it will serve as a model itself.
For more complex scenarios I'd suggest do split the machine (transitions and related rules)
and model (actual state based behaviour).

When you pass a Machine instance to another HSM, the rules and transitions will be copied and not just reused. That's why Nested will use itself as a model but the nested version will use Top instead. Having just copies of Nested's assets has two advantages: first, it does not intervene with the original Nested instance and second, it allows to reuse many functionality provided by core which otherwise had to be rewritten and as a result cause higher code complexity.

Is there any way to achieve the encapsulation by some other means?

The 'easiest' way would be to pass callbacks by reference rather than by name. Instead of 'on_enter':'print_msg' use 'on_enter':self.print_msg. This way the callback reference is already resolved which prevents transitions to look it up in the model.

from transitions.extensions import HierarchicalMachine as Machine


class Nested(Machine):

    def print_msg(self):
        print("Nested")

    def __init__(self):
        self.states = ['n1', {'name': 'n2', 'on_enter': self.print_msg}]
        Machine.__init__(self, states=self.states, initial='n1')
        self.add_transition(trigger='goto_n2',
                            source='*',
                            dest='n2')


class Top(Machine):

    def print_msg(self):
        print("Top")

    def __init__(self):
        self.nested = Nested()

        self.states = ['t1',
                       {'name': 't2',
                        'children': self.nested}]
        Machine.__init__(self, states=self.states, initial='t1')
        self.add_transition(trigger='goto_t2',
                            source='*',
                            dest='t2_n1')


top_machine = Top()
top_machine.goto_t2() 
print(top_machine.state)  # >>> t2_n1 
top_machine.goto_n2()  # >>> Nested
print(top_machine.state)  # >>> t2_n1

As mentioned above, I'd also suggest splitting machine and model. This way you can use MixIns to decorate your main model and keep the code less cluttered. This way you can also use every tool OOP has to offer to alter the state machine's behaviour.

@Grey-Bit
Copy link
Author

Thanks @aleneum - changing to direct references instead of strings worked well.

However, now it introduced another problem - inside the callback, if I try to issue any trigger in the Nested or Top, then it fails with an AttributeError - can't find the trigger functions.

from transitions.extensions import HierarchicalMachine as Machine
from transitions.extensions.nesting import NestedState as State
from transitions.extensions.nesting import NestedTransition as Transition

class Nested(Machine):
    def print_msg(self):
        print("Nested")
        self.top.print_top()
    def __init__(self, top):
        self.top = top
        self.states = ['n1', {'name':'n2', 'on_enter': self.print_msg}]
        Machine.__init__(self, states=self.states, initial='n1')
        self.add_transition(trigger='goto_n2',
                            source='*',
                            dest='n2')

class Top(Machine):
    def print_msg(self):
        print("Top")
    def __init__(self):
        self.nested = Nested(self)

        self.states = [  't1',
                        {'name': 't2',
                        'children': self.nested}]
        Machine.__init__(self, states=self.states, initial='t1')
        self.add_transition(trigger='print_top',
                            source='*',
                            dest='=',
                            after= self.print_msg)
        self.add_transition(trigger='goto_t2',
                            source='*',
                            dest='t2_n1')



top_machine = Top()
top_machine.goto_t2()
top_machine.goto_n2()

I suspect it's the artifact of the copying process and order of transition initializations, but can't figure out the exact cause and how to make it work.

@aleneum
Copy link
Member

aleneum commented Aug 11, 2017

self.top.print_top()

Top does not contain a method called print_top. Change this to self.top.print_msg() and the code above works just fine.

Sorry, my bad. Overlooked the transition which is called print_top. Will have a look shortly.

@aleneum aleneum closed this as completed Aug 11, 2017
@aleneum aleneum reopened this Aug 11, 2017
@aleneum
Copy link
Member

aleneum commented Aug 11, 2017

The problem is this line in extension.nested:

 state = copy.deepcopy(state)

This destroys all the previously created cross-references. deepcopy seems to be smart enough to update bound method references as well. During the copying, at least three new instances of Top are created and all of them do not contain the later created methods.
Maybe we can relax the copying a bit and use copy.copy instead. This would make your code work. Before that, I have to check why I used deepcopy in the first place. Unfortunately, there is no test for that. Actually, there is one: test_blueprint_remap. I will see if I can figure something out to satisfy this as well as your use case.

Remark: self.state is used in Machine. Your values will probably be overridden.

@Grey-Bit
Copy link
Author

Thanks @aleneum,

The copy process introduces another substantial issue:
It's not possible to access any information in the original Top.

In the following example, I am calling print_msg directly, trying to access variable "var",
which exists on the original Top.

class Nested(Machine):
    def print_msg(self):
        print("Nested")
**#raises AttributeError exception**
        **self.top.print_msg()**
    def __init__(self, top):
        self.top = top
        self.states = ['n1', {'name':'n2', 'on_enter': self.print_msg}]
        Machine.__init__(self, states=self.states, initial='n1')
        self.add_transition(trigger='goto_n2',
                            source='*',
                            dest='n2')

class Top(Machine):
    def set_var(self, var):
        self.var = var
    def print_msg(self):
        print("Top: " + self.var)
    def __init__(self):
        self.nested = Nested(self)

        self.states = [  't1',
                        {'name': 't2',
                        'children': self.nested}]
        Machine.__init__(self, states=self.states, initial='t1')
        self.add_transition(trigger='print_top',
                            source='*',
                            dest='=',
                            after= self.print_msg)
        self.add_transition(trigger='goto_t2',
                            source='*',
                            dest='t2_n1')



top_machine = Top()
**top_machine.set_var("var value")**
top_machine.goto_t2()
top_machine.goto_n2()

May be instead of playing with the copy methods, it's better to create a "parent" variable in the nested machines that would reference the original containing machine? This would allow anybody to issue triggers or access any information stored in the containing machines?

For symmetry, we could add an ability to access nested machines through the state that references them.

@aleneum
Copy link
Member

aleneum commented Aug 20, 2017

Hello @Grey-Bit,

... it's better to create a "parent" variable in the nested machines that would reference the original containing machine?

This might work for a scenario with just one level of embedded machines but will not solve the issue for deeper nested configurations. The 'problem' that deepcopy will create new objects for every passed bound function reference still persists.

I guess I put you on the wrong path by mentioning references to bound functions. They seem to NOT work well with HierarchicalMachine.
However, HierarchicalMachine does work as intended for string callbacks. This is why I'd suggest again to have a look at how to share data via the model. You cannot implicitly override model functions for certain substates but transitions offers some tools to help with extending models and enable context sensitive behaviour:

from transitions.extensions.nesting import HierarchicalMachine as Machine

class Nested(object):

    def __init__(self):
        states = ['n1', {'name': 'n2', 'on_enter': 'print_msg'}]
        transitions = [['goto_n2', '*', 'n2']]
        self.machine = Machine(self, states=states, transitions=transitions, initial='n1')

    def print_msg(self):
        print("Nested")


class Top(Nested):

    def __init__(self):
        super(Top, self).__init__()
        self.nested = Nested()
        states = ['t1',
                  {'name': 't2', 'children': self.machine}]
        transitions = [dict(trigger='print', source='*', dest='=', after='print_msg'),
                       ['goto_t2', '*', 't2_n1']]
        self.machine = Machine(self, states=states, transitions=transitions, initial='t1')

    def print_msg(self):
        # call super method in case we are in a substate
        if self.is_t2(allow_substates=True):
            super(Top, self).print_msg()
        else:
            print("Top")

top_machine = Top()
top_machine.print()  # will call Top.print_msg
top_machine.goto_t2()
top_machine.print()  # will call Top.print_msg and forward to Nested.print_msg
top_machine.goto_n2()  # same as above, but triggered by 'on_enter' instead of 'after'

I acknowledge this is an issue since it reduces the flexibility of HierarchicalMachine.
If you or someone else wants to suggest a fix/redesign of HierarchicalMachine, feel free to open a pull request. Running instances of Machine inside another instance of Machine was one of my first attempts but I ended up more or less with a parallel implementation of this class with way to much glue code.

@Grey-Bit
Copy link
Author

Thanks @aleneum
I will try to come up with an alternative and issue a pull request.

In the meantime, I changed deepcopy to copy and at least in my case of two level nesting of machines everything works as expected.

Where should I expect it to fail due to the lack of deep copy?

@aleneum
Copy link
Member

aleneum commented Aug 22, 2017

Where should I expect it to fail due to the lack of deep copy?

I am currently aware of the issue that it breaks the imported machine. The above mentioned test_blueprint_remap will fail in case copy is used. The imported machine counter cannot trigger events any longer since the states had been renamed. Additionally, the passed state dictionary might have been changed (for instance added keywords).

@aleneum
Copy link
Member

aleneum commented Aug 22, 2017

@Grey-Bit: If you mind, you can pull the new changes from master. I added your use case as a test and it seems to work now. Deep copying is now limited to NestedState and NestedTransition which
will omit creating deep copies of callback arrays. This should keep object references intact and also allow references to parents in the imported machine. If you stumble upon more issues let me know.

@Grey-Bit
Copy link
Author

Thanks @aleneum, I will test over the weekend and will report back here if there are any issues.

@Grey-Bit
Copy link
Author

Grey-Bit commented Sep 1, 2017

Sorry for the late response: I validated this change on a more complex scenario that included three level hierarchy and it works well. Thanks @aleneum for taking care of this.

@aleneum
Copy link
Member

aleneum commented Sep 1, 2017

awesome, thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants