pytransitions / transitions

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

Why is graphing a separate type of machine? #466

Closed ghost closed 3 years ago

ghost commented 4 years ago

Isn't the graphical representation of the machine a separate domain?

aleneum commented 4 years ago

Hello @StephenCarboni,

could you elaborate on your question? Is this a feature request? Do you consider isolated diagram support inconvenient?

ghost commented 4 years ago

I'm actually just questioning the API design.

The type of Machine chosen is relevant to the application's needs. But in production, I don't care that my Machine can graph itself and also don't want to worry about graphviz being available. But a GraphMachine will fail to init when graphviz is not available, this leads to all sorts of complications. If you are inheriting a Machine like I am, you need to make the choice at import time. There's ways to make it work, but I would be choosing one just because of pytransitions. It would make things simple if none of the Machine extensions introduced dependencies.

Something like:

from transitions.tools import graph_machine
from transitions.extensions import LockedHierarchicalMachine

class MyMachine(LockedHierarchicalMachine): pass

m = MyMachine()

g = graph_machine(m, title="My Amazing Machine", show_conditions=True)
g.graph_attr.update(dpi="300")
g.draw("state_diagram.png", prog="dot")

Let graph_machine raise an error if graphviz is not available.

Graphing isn't a responsibility of the machine, let the caller worry about it.

aleneum commented 4 years ago

Hello @StephenCarboni,

I don't care that my Machine can graph itself and also don't want to worry about graphviz being available.

there is no need to use GraphMachine anywhere. You can stick with other variants of Machine and HSM if you like and don't need to install pygraphivz or graphviz at all. The graphviz dependency is entirely optional which is reflected in setup.py where you have to install the diagrams extra and in requirements_diagrams.txt which isolates graphing dependencies from the rest. You can even test without Graphviz. Short: Graphviz is no mandatory dependency of transitions.

It would make things simple if none of the Machine extensions introduced dependencies.

Being able to visualize a machine configuration can be helpful and is a feature many transitions users use. The fact that it is an extension (just like AsyncMachine) is by design since not everyone needs it and doesn't like the requirements (asyncio -> Python 3.7+).

bmxp commented 4 years ago

@aleneum I would like to implement a surface that models the statemachines but I already have a webserver and it is not tornado but cherrypy. Is there any chance to decouple the webserving part from Tornado? (FYI We use CherryPy)

aleneum commented 4 years ago

Hello @bmxp,

that's a bit off-topic ;). You could head over to https://github.com/pytransitions/transitions-gui and create a ticket there. Let's see what we can do.

aleneum commented 3 years ago

Closing this due to a lack of feedback. However, feel free to clarify if you think your matter hasnt been discussed yet.

ghost commented 3 years ago

@aleneum Sorry for the confusion and lack of feedback. I should have been more forward with the issue I was facing.

For me, graphviz is a dependency in development because it's convenient to graph the machines. However, when it comes to production, it is difficult to get graphviz on the environment the code is running and graphing is not needed in production, either.

So in dev, my model inherits from LockedGraphMachine, but for production, I want it to inherit from LockedMachine.

I can use MachineFactory to get the MachineCls I want at runtime, but if I want to use inheritance, I can only think of building the class behind a method.

wtgee commented 3 years ago

@StephenCarboni you can just detect if graphviz is installed at runtime.

can_graph = False
try:  # pragma: no cover
    import pygraphviz  # pragma: no flakes
    from transitions.extensions import GraphMachine as Machine
    can_graph = True
except ImportError:  # pragma: no cover
    from transitions import Machine

What I do is actually generate images for each of the possible states (we have <20) with names that correspond to the transition/state combo and then trigger a callback for every transition that just looks up the image and displays it on our gui by symlinking to a latest.svg. That way I'm not constantly generating the images and don't have the production graphviz dependency. This is old code from the transitions earlier days so it might be slightly differerent to implement now.

I'm sure this could be done cleaner somehow but should give you the idea.

# ... somewhere else
if can_graph:
    s.add_callback('enter', '_update_graph')

def _update_graph(self, event_data):  # pragma: no cover
    model = event_data.model

    try:
        state_id = 'state_{}_{}'.format(event_data.event.name, event_data.state.name)

        image_dir = self.config['directories']['images']
        os.makedirs('{}/state_images/'.format(image_dir), exist_ok=True)

        fn = '{}/state_images/{}.svg'.format(image_dir, state_id)
        ln_fn = '{}/state.svg'.format(image_dir)

        # Only make the file once
        if not os.path.exists(fn):
            model.graph.draw(fn, prog='dot')

        # Link current image
        if os.path.exists(ln_fn):
            os.remove(ln_fn)

        os.symlink(fn, ln_fn)

    except Exception as e:
        self.logger.warning("Can't generate state graph: {}".format(e))

Hopefully that helps.

As a one-time but now silent maintainer of the library I would definitely say we do not want graphviz in the core dependencies.