[ENH] Multiple inputs for OWPythonScript

[janezd]

Issue

Alternative to #2418.

Description of changes

Allow multiple inputs, store them in dictionaries and expose them under their existing names (e.g. in_data) in case there is only a single signal, and with plural names (e.g. in_datas) bound to lists of inputs (zero, one or more).

Creation of inputs and outputs is automatized through a meta class. The advantage of this is, beside easier adding of new inputs and outputs, sharing the common code for all inputs. For instance, when testing multiple inputs, we only need to test in_data (in_datas) since other inputs work the same way.

Includes

• Code changes
• Tests
• Documentation

[codecov-io]

Codecov Report

Merging #2506 into master will increase coverage by <.01%.
The diff coverage is 95.61%.

@@            Coverage Diff             @@
##           master    #2506      +/-   ##
==========================================
+ Coverage   75.02%   75.03%   +<.01%     
==========================================
  Files         327      327              
  Lines       57641    57700      +59     
==========================================
+ Hits        43248    43297      +49     
- Misses      14393    14403      +10

[astaric]

I like the idea, hate the metaclass.

IIUC the metaclass (1) creates the signals, (2) handlers and (3) fills the input_name and output_names lists with the names of the signals.

(1) I would keep the input/output declarations on the widget, it is a bit more verbose, but allows us easier modification. If at some point we decide to rename classifier input to model, and need to add replaces attribute to the Input/Output, that is much easier with explicit signal declarations.

(2) I would much rather have the handler method on the widget, with metaclass adding (and registering) thin methods that call the actual handler with the correct parameters.
def handle_input(input_name, data, id): ...

(3) do we really need these? They could be a property that calls getmembers on Intputs/Outputs

[janezd]

At first, I almost did it without the meta class. Everything that the meta class does can be done in the widget class itself, but too late: the meta class of OWWidget (WidgetMetaClass) calls a method convert_signals which expects that the signals are already defined. Note that the meta class in this PR does not defined __new__ but __prepare__ to insert the definition into the namespace before executing the inherited __new__.

(3) are not needed. I added them just to minimize the number of changes, but I'm happy to remove them.

Suggestions (1) and (2) are incompatible. Handlers must exist before the inherited metaclass new is called. Therefore, they must be inserted via __prepare__. At the moment when __prepare__ is called though, the class body is not yet executed, so __prepare__ wouldn't yet have the signal definitions to wrap the handlers if the signals were defined in the class and not in the meta class.

A: I can keep the signal definitions in the meta class (1), move the handler to the widget (2) and remove (3)

B: I can move everything to the widget (I guess!), but define the meta class to overload convert_signals so that it is called at a later moment, when the widget is ready.

B hacks into signal discovery, but may be nicer, so I'm giving it a try tomorrow.

[astaric]

What about something like this:

@Inputs.in_data(input="in_data")
@Inputs.in_learner(input="in_learner")
def handle_input(self, data, id, input=None):
   pass

(any kwargs passed to the signal's call would get forwarded to the handler)

[janezd]

@ is followed by an expression that evaluates to a function which gets a function as an argument and returns a function. With this proposal, Inputs.in_data would have to be a function that returns a "decorator". In other words, the current in_data would have to be thunked.

To avoid breaking compatibility with the existing decorator, we could provide a thunked decorator (e.g. @Inputs.in_data.with_args) or change in_data to be smarter:

def __call__(self, method=None, **kwargs):
    def decorate(method):
        if self.handler:
            raise ValueError("Input {} is already bound to method {}".
                             format(self.name, self.handler))
        self.handler = method.__name__
        return method
    if method is None:
        return decorate  # Thunked decorator
    else:
        return decorate(method)  # Existing behaviour

This solves just potential thunking, but not passing **kwargs. At first I thought that changing return method to return partial(method, **kwargs) would do it, but ... no, if the decorators are chained like in your example. The current decorator doesn't construct a new function, a wrapper, but just stores the name. To store kwargs, it would have to create an, ugh, function with kwargs in the closure, and this function would act as unbound signal to be latter bound to instances of widgets.

While this is a fun exercise, it's terribly complex --- and this may be about the only widget where it would be used.

[astaric]

Yeah, I forgot that the only the name of the method is stored as handler, which means that each signal would have to have its own named handler (no lambdas or partials).

Another idea (the last one I promise :))

If WidgetManager was modified a bit (line 822)

                if hasattr(handler, "universal"):
                    handler(*args, signal=link.sink_channel)
                else:
                    handler(*args)

we would just have to mark the method, and it would be called with the signal as a parameter, which should be sufficient to perform all the logic in a single method

[janezd]

What about this?

            if "signal" in inspect.signature(handler).parameters:
                handler(*args, signal=link.sink_channel)
            else:
                handler(*args)

If the handler expects a signal name, it receives it. No flags.

[janezd]

I removed the meta class.

Lists of inputs and outputs are again within the widget. The handler is also again a method. Creation and population of Inputs and Outputs is outside of the class to avoid mixing declarations and methods. It could also be a class method. Alternatively, classes could be defined in the widget and populated outside. I've no strong opinion.

Classes Inputs and Outputs are no longer constructed by calling typeto allow manual adding of additional signals that behave differently or have a specific handler.

I left input_names and output_names; they are useful and don't hurt.

@astaric, do you like it better?

@ales-erjavec, would you object to sending the signal name to the handler, as proposed in this commit (https://github.com/biolab/orange3/pull/2506/files#diff-436cd70924fc901a93215a371a0f8ed2R820)? Handlers belong to widgets, so there seem to be no reason to hide signal names from them. While this widget may be the only place in Orange where we need it (so far?), supporting this may be useful in extensions or when Orange Canvas is used in different contexts, like physics, with different sets of different-minded widgets. Or does it introduce any problems -- or do you consider it inappropriate for some other reason?

[ales-erjavec]

Or does it introduce any problems. -- or do you consider it inappropriate for some other reason?

It would break (in a surprising manner) any existing (or future) definition of

@Inputs.some_name
def set_some_name(self, signal)

I.e. It breaks the principle of least astonishment. If this is done it would be better adding an explicit flag the way @astaric suggested.

I like that widgets define an standard setter API for setting the inputs; i.e set_data, set_learner, ... as apposed to signal_handler(..., signal="in_data").
Therefore I like the explicit approach of gh-2418 better.

[janezd]

It's been fun, but @ales-erjavec is right. This is now essentially #2418 + some refactoring and tests.

When somebody approves, I'll squash the commits.