pytransitions / transitions

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

Providing a child FSM to an HFSM does not create a copy of the transitions/states #509

Closed thedrow closed 3 years ago

thedrow commented 3 years ago
from transitions.extensions import HierarchicalMachine
m1 = HierarchicalMachine(states=['a', 'b'], initial='a')
m2 = HierarchicalMachine(states=m1, initial='a')
m1.add_transition('foo', 'a', 'b', after=lambda: print("hi"))
m1.get_transitions('foo', 'a', 'b')[0].before.append(lambda: print("BEFORE IS SHARED"))
print(m2.get_transitions('foo', 'a', 'b')[0].before)
m2.foo()

Output:

[<function <lambda> at 0x7ff97f5ae8b0>]
BEFORE IS SHARED
hi

As we can see below https://github.com/pytransitions/transitions/blob/b6a1d15bd19738b7790136a66ee71611ccff82a1/transitions/extensions/nesting.py#L538-L543 The events are copied by reference as is which means that the transitions list is copied by reference. This in turn means that each transition holds the same before and after callbacks.

I tried deepcopying ev.transitions before adding the states (by moving the loop before self.add_states(new_states) and after adding the states. I also tried deepcopying the entire event which doesn't work.

Note that the states callbacks are also shared.

I'm guessing that there is a missing __deepcopy__ in Event or something similar but I'm at a loss. If this is intended, please let me know.

I'd appreciate any guidance in resolving this issue.

thedrow commented 3 years ago

Deepcopying the states in https://github.com/pytransitions/transitions/blob/b6a1d15bd19738b7790136a66ee71611ccff82a1/transitions/extensions/nesting.py#L538 does the trick for state callbacks but the transitions remain.

thedrow commented 3 years ago

Also, note that ev.machine is not self.

thedrow commented 3 years ago

Also, note that ev.machine is not self.

Wait, my test case is incorrect. But this is true...

thedrow commented 3 years ago

I'm unable to reproduce this with the corrected test case but it does reproduce in my code.

thedrow commented 3 years ago

Deepcopying the entire state machine before passing it resolves this issue but only the entire state machine.

aleneum commented 3 years ago

From the documentation:

Before 0.8.0, a HierarchicalMachine would not integrate the machine instance itself but the states and transitions by creating copies of them. However, since 0.8.0 (Nested)State instances are just referenced which means changes in one machine's collection of states and events will influence the other machine instance. Models and their state will not be shared though. Note that events and transitions are also copied by reference and will be shared by both instances if you do not use the remap keyword.

Unfortunately, I couldnt find the issues where this change was requested.

Also, note that ev.machine is not self.

that's exactly the reason. Since events are shared, multiple machines can trigger the same event. Sharing State, Transition and Event should not be an issue unless some callbacks are passed by reference and not by name.

If you want to create independent states, events and transition from running machines, you can have a look at the markup property of any machine class derived from MarkupMachine (e.g. GraphMachine, HierarchicalGraphMachine).

thedrow commented 3 years ago

So the intention is that if I nest a machine B in machine A like so:

A = HierarchicalMachine(states=['a', 'b'], initial='a')
A.add_transition('foo', 'a', 'b')
B = HierarchicalMachine(states={'name': 'c', 'children': A}, initial='c')
B.foo()
print(B.state)
print(A.state)

Would print:

c↦b
b

Because it prints:

c↦b
a

And the following:

A = HierarchicalMachine(states=['a', 'b'], initial='a')
A.add_transition('foo', 'a', 'b')
B = HierarchicalMachine(states=A, initial='a')
B.foo()
print(B.state)
print(A.state)

Also prints:

b
a

Some of my callbacks are passed by reference. What I want is to copy the state machine with the callbacks but not share references between the machines.

This API is awfully confusing at the moment. Deepcopying the machine does just what I need but the current behavior doesn't make much sense unless I'm missing something and I must be.

aleneum commented 3 years ago

This API is awfully confusing at the moment. Deepcopying the machine does just what I need but the current behavior doesn't make much sense unless I'm missing something and I must be.

This if of course open for discussion. I had two reasons to make this change:

First, imho it's more in line with the usual behavior of passing initialized states to Machine

from transitions import Machine, State

s = State(name='A')
m = Machine(states=[s], initial=s.name)
assert id(m.states[s.name]) == id(s)

I'd outline this as 'use references (objects and methods) and create from value (string and enum)'. If you find an occasion where this is violated, let me know.

Second, it is easier to deepcopy a machine's states and events right now then it was to use references prior to 0.8:

from transitions.extensions.nesting import HierarchicalMachine, NestedState
from copy import deepcopy

count_states = ['1', '2', '3', 'done']
count_trans = [
    ['increase', '1', '2'],
    ['increase', '2', '3'],
    ['decrease', '3', '2'],
    ['decrease', '2', '1'],
    ['done', '3', 'done'],
    ['reset', '*', '1']
]

counter = HierarchicalMachine(states=count_states, transitions=count_trans, initial='1')

state = NestedState(name="counting", initial='1')
state.states = deepcopy(counter.states)
state.events = deepcopy(counter.events)

states = ['waiting', 'collecting', state]

transitions = [
    ['collect', '*', 'collecting'],
    ['wait', '*', 'waiting'],
    ['count', 'collecting', 'counting']
]

collector = HierarchicalMachine(states=states, transitions=transitions, initial='waiting')
collector.collect()
collector.count()
collector.increase()
print(collector.state)

I'd say the change enables a new workflow with just minor inconvenience for another.

I will close this issue for now but feel free to comment anyway. Thanks again for sharing your thoughts on this. I will try to improve the documentation which hopefully will prevent confusion for future readers.