Refactor signals during evaluation

[PaulSchweizer]

Signals

Signals are meant for external processes that want to observe the graph evaluation, mainly GUI applications or other visualizations and update their state according to the signals they receive.

Currently we have two different types of signals being emitted during graph.evaluate.

from flowpipe.log_observer import LogObserver
from flowpipe.stats_reporter import StatsReporter

Both of these were put in because I needed something quick. I don't like this and want to refactor this into a more generic Signal/Slot system while still keeping it very simple.

This is how it could be implemented:

flowpipe/signal.py:

class Signal(object):

    def __init__(self, name, signature=None):
        """
        Args:
            name (str): The (unique) name of the signal
            signature (tuple of type): A tuple of types that the signal will
                send to the listeners when emitted
        """
        self.name = name
        self.signature = signature
        self._listeners = []

    def emit(self, *args):
        for listener in self._listeners:
            listener(*args)

    def connect(self, listener):
        if not self.is_connected(listener):
            self._listeners.append(listener)

    def disconnect(self, listener):
        if self.is_connected(listener):
            self._listeners.pop(self._listeners.index(listener))

    def is_connected(self, listener):
        return listener in self._listeners

flowpipe/node.py:

class INode(object):

   def evaluate(self):
        if self.omit:
            INode.SIGNALS["evaluation-omitted"].emit(self)
            return {}

        INode.SIGNALS["evaluation-started"].emit(self)

        inputs = {}
        for name, plug in self.inputs.items():
            inputs[name] = plug.value

        # Compute and redirect the output to the output plugs
        outputs = self.compute(**inputs) or dict()

        # all_outputs = self.all_outputs()
        for name, value in outputs.items():
            if '.' in name:
                parent_plug, sub_plug = name.split('.')
                self.outputs[parent_plug][sub_plug].value = value
            else:
                self.outputs[name].value = value

        # Set the inputs clean
        for input_ in self.all_inputs().values():
            input_.is_dirty = False

        INode.SIGNALS["evaluation-finished"].emit(self)

        return outputs

# A bit unfortunate that we'd have to add the signals afterwards due to the
# signature, maybe we can find a better pattern for this?
#
INode.SIGNALS = {
    "evaluation-omitted": Signal("evaluation-omitted", (INode, )),
    "evaluation-started": Signal("evaluation-started", (INode, )),
    "evaluation-finished": Signal("evaluation-finished", (INode, ))
}

The client code would then do something like this:

def started(node):
    print("started:", node.name)

def finished(node):
    print("finished:", node.name)

INode.SIGNALS["evaluation-started"].connect(started)
INode.SIGNALS["evaluation-finished"].connect(finished)

Let me know what you think and what changes you'd suggest. Also, what other signals might be helpful.

LogObserver

We should remove the logger in the flowpipe.__init__.py and replace it with this in each module (if needed) and then just use that log directly:

import logging

log = logging.getLogger(__name__)

Users can create their own logs or messages from the signals, there is no need to wrap the logging the way we are wrapping it right now. log_observer.py should therefore be removed.

StatsReporter

The original idea was to calculate statistics and report them to a db. These statistics could then be used to predict evaluation times and required resources for future evaluations of the same/similar graphs.
It never want past the idea stage though and besides that, flowpipe should not contain any such code anyways. If one would want to do something like this, the signals are the perfect solution to hook into the system and extract this information.
We should therefore remove the stats_reporter.py from the repo. This idea could instead be added to an example file for inspiration.

[neuneck]

First off, as a disclaimer, I have never ever worked with signals before and am just now stumbling upon the subject. Therefore, please bear with me if this is obvious, but could the python core library module signal help us out? I guess that it could help people use our signals in larger contexts, since we would adhere to the standard way of going about things.

Ref:
python 3: https://docs.python.org/3/library/signal.html
python 2: https://docs.python.org/2.7/library/signal.html

[TheElderMindseeker]

@neuneck IMHO: although Unix signals, which Python's signal is based upon, is well-known and time tested mechanism it is too low-level for the purposes of flowpipe. Those signals were designed to allow developers handle technical issues such as graceful shutdown on SIGTERM (Ctrl-C) or child process management using SIGCHLD. The other problem that may arise is the allocation of a new signal for the application goals (see this post on StackOverflow). To my mind, the solution proposed by @PaulSchweizer with custom Signal class is good except maybe the name. It may be a bit confusing considering the signal module. I would suggest Event or Notification.

As to the signal declaration issue, maybe we can use metaclasses to dynamically create INode.SIGNALS dictionary using the class variable?

class INode(metaclass=SignalMeta):
    signals = ['evaluation-omitted', 'evaluation-started', 'evaluation-finished']

[PaulSchweizer]

Thanks for the comments guys. I agree, the name should be changed to "event" that makes more sense and does not confuse it with the system "signals" as mentioned. I'm not quite sure about the proposed metaclass solution could work but I'll take a closer look at it next week (I'm not in the office this week). Another solution would be not to specify the arguments for an event at all. It might actually be ok to just state this information in the docstring.

[Onandon11]

We should remove the logger in the flowpipe.init.py and replace it with this in each module (if needed) and then just use that log directly:

import logging

log = logging.getLogger(name)
Users can create their own logs or messages from the signals, there is no need to wrap the logging the way we are wrapping it right now. log_observer.py should therefore be removed.

I agree on this. Currently the flowpipe logger cannot be integrated into other modules, like my own, because it uses it's own logger dependency. I think we should handle the logger part and the signal part apart from each other.

[TheElderMindseeker]

@PaulSchweizer in your initial post you wrote about StatsReporter which was aimed at providing the users of flowpipe with the runtime statistics of the node. Now that StatsReporter is a subject for removal I wonder what will be the mechanism for obtaining this statistics. In my opinion we should give the possibility to attach this information to the emitted event in some way. I am concerned with this issue because I already see a potential use case in my own practice.

[PaulSchweizer]

@TheElderMindseeker , currently the statistics only contain the evaluation time and a timestamp of the start time.
We could introduce a INode.stats field that gets filled with this infromation after evaluation, then you can inspect that data without having to necessarily tap into the event system.

n = MyNode().evaluate()
print n.stats

{
    "eval_time": 0.123
}

What do you think, would something like that suffice?

Also, if there are any other stats you think we should gather, pease let me know.

[PaulSchweizer]

I'll submit a PR for this tomorrow

[PaulSchweizer]

The PR is submitted: #100