pytransitions / transitions

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

GraphMachine markup should update if machine structure changes #600

Closed drpjm closed 1 month ago

drpjm commented 1 year ago

I am experimenting with modifying HierarchicalGraphMachine for my particular use case: using remove_transition and/or removing states. I noticed that the GraphMachine family does not update its markup when structural changes occur.

Here is some example code I was messing with...

from transitions.extensions import HierarchicalGraphMachine as HGM
states = ['a','b','c']
initial = 'a'
machine = HGM(states=states, initial='a', auto_transitions=False)
machine.add_transition(trigger='t_ab', source='a',dest='b')
machine.add_transition(trigger='t_bc', source='b',dest='c')
machine.add_transition(trigger='t_ca', source='c',dest='a')
machine.get_graph().draw('machine1.png', format="png", prog='dot')

And this produces: machine1

Then I take a couple steps:

machine.t_ab()
machine.t_bc()

machine3 Now I remove a transition for fun:

machine.remove_transition(trigger='t_ca')

The transition is removed correctly (no more t_ca), but the markup still has transition as evidenced in this final graph drawing:

machine4 and by looking at the markup:

machine.markup['transitions']

[{'source': 'a', 'dest': 'b', 'trigger': 't_ab'}, {'source': 'b', 'dest': 'c', 'trigger': 't_bc'}, {'source': 'c', 'dest': 'a', 'trigger': 't_ca'}]

It could be I am doing this wrong, but I would expect that removing a transition or state should update the markup that is used to drive the graph output. If this seems reasonable, where would such a feature be implemented? Thanks!

aleneum commented 1 year ago

Hello @drpjm,

very well presented issue. Many thanks for that. Markup as well as diagrams should update when a transition is removed. Changes in 5c89a24 should address this. However, regeneration of graphs will remove the styling of previous transitions. This information is not stored anywhere and just 'applied' to a model's graph when a state changes. Consequently, your code example would end up with a graph like this:

machine2

drpjm commented 1 year ago

Great! The current state is still indicated, which is just fine. Would it make sense for there to be a method for removal of states too and that would be reflected in the markup? I did not find a remove_state function, so I have been doing something like this (based on example above):

del machine.states['a']

This operates on the OrderedDict directly. Would it make sense for there to be a wrapping function similar to remove_transition, but it safely removes the state? Or is there a better way?

aleneum commented 1 year ago

A briefly scanned the code and it seems like deleting states dynamically has not been a use case so far. But since removing transitions is supported, supporting the removal of states is not far-fetched at all. I don't see any particular reason why this should not be supported.

[...] so I have been doing something like this (based on example above): del machine.states['a']

I haven't tested in but it looks okay-ish. It will not remove convenience functions previously attached to models like "mode.is_a()" though.

drpjm commented 1 year ago

Thanks for the fix and discussion. I will play with cleanly removing states dynamically and let you know if I come up with a way to do it that would be good for the library.

aleneum commented 1 year ago

I will play with cleanly removing states dynamically and let you know if I come up with a way to do it that would be good for the library.

sounds good. adding this feature is quite straight forward I guess. remove_transition, add_state and add_model_to_state are the methods to have a look at. We usually decorate the model via setattr as far as I remember.

cc-stjm commented 2 months ago

In case it helps, I've found that calling machine.get_graph(force_new=True) to obtain the graph means it will pick up anything that has changed.