Widgets' controls call `unconditional_commit`

[janezd]

For example, OWAggregate has

 var_list = gui.listView(
            box, self, "variables", model=self.variable_model,
            callback=self.commit)

It should have callback=lambda: self.comit() instead. God knows how many widgets have the same problem. Even veteran developers make this mistake; beginners don't even know why the heck should they use lambda here.

As @irgolic once said, it should be the other way around -- the widget should have an unconditional_commit and auto_commit should add the conditional one. At the same time, self.commit should be available at the time of __init__ and refer to the conditional commit, which is added later. Furthermore, this must be resolved so that it doesn't break backward compatibility and it doesn't introduce to much magic. No meta classes, for instance.

[markotoplak]

Thanks, Janez. I was complaining about that multiple times and no one wanted to back me up.

My original complaint was that adding a button renames a function and it was confusing where it came from.

I only work on Orange since 2007 - I have no idea if that counts as a veteran, but this keeps confusing me. A few weeks ago I asked @VesnaT why did she use that unneeded lambda in a callback while I was reviewing her PR. It turned out it was needed.

I suggest deprecating the current button and just make a new one with the appropriate semantics.

[irgolic]

One option is, we could remove autocommits and enhance the UX of freezing the workflow.

Another option, keep the UX as is, and (as Janez suggests) make self.commit refer to the conditional commit already at __init__ time. We could add this to WidgetMetaclass, wherein the original commit is delegated to a hidden method. It wouldn't really add any magic that isn't already present. @markotoplak and I have talked about this before, I think self.unconditional_commit really shouldn't be public. Right now, all widgets manually implement (and test) calling it upon receiving a new data source. That should be made consistent and implemented in OWBaseWidget. Also note, it can be called unconditional_apply too if the commit method is called apply. We should probably consolidate that too.

Really, both workflow freezing and autocommits are only there because there's no way to interrupt processing. We should separate orange-canvas-core into a GUI process (purely for canvas GUI - toolbox, connecting widgets) and a worker process (wherein widgets run - widget interfaces, SignalManager).

P.S.: widgets like File really really need to follow the convention and get an autocommit button. I've spent the past few months closely observing users, it genuinely confuses them.

[janezd]

Here's an idea that requires minimal changes.

Widget has to decorate the commit function, like this:

    @gui.delayed
    def commit(self):
        ...

This provides the method with an attribute delayed, which is a delayed call (a "conditional commit"), and are used like this

        var_list = gui.listView(
            self.controlArea, self, "selected_vars", box="Group by",
            model=self.var_model, callback=self.commit.delayed)

Function gui.auto_commit recognizes this and for such methods it does not replace commit with conditional commit and does not add another method, unconditional commit. Hence, calls like

    self.commit()

become unconditional.

In short, to migrate, a widget must

• decorate the commit method
• change callbacks to commit.delayed
• replace calls of commit with commit.delayed (where conditional commit was indeed intended)
• replace calls of unconditional_commit with commit.

What we gain is that there are no sneaky method replacements and renames in the background.

All unchanged existing code should work as before. Changes only take effect if delayed decorator is used.

Required magic? Some, but not much. This is the decorator:

def delayed(func):
    func.delayed = True
    return func

gui.auto_commit exposes it local function conditional_commit as an attribute of commit:

    if getattr(commit, "delayed", False):
        commit.conditional_commit = conditional_commit
    else:

And the magic is in the constructor. commit.delayed must be a bound method, but decorator cannot attach bound methos because it receives a pure function (it doesn't have self, doesn't even know the class!). The first place where self exists is in the init. So the base class' init must comb the class namespace for function mark as delayed, bound the commit (to be able to attach instance-related attributes) and add a function that calls the conditional commit that is provided by auto_commit.

        for func in type(self).__dict__.values():
            if getattr(func, "delayed", False):
                self.commit = lambda func=func: func(self)
                self.commit.delayed = lambda: self.commit.conditional_commit()

I haven't tested this thoroughly, but I think it works.

[irgolic]

Great idea using a decorator!

My two cents: if I use a decorator, I expect it to change the method I decorated. Also, callback=self.commit.delayed is very weird, I've never seen somebody attach an attribute to a function or bound method like this. Either use the decorator and let it change the method, or don't, and add a new attribute, preferably to a class instance.

The decorator should change the commit into a conditional commit; unconditional commit should only ever be called from within the conditional commit or when a new input is received – never manually within a widget's code. I've gone through the calls to unconditional_commit, they all either relate to receiving a new signal (which should be consolidated in OWBaseWidget) or are inappropriate.

Also, consider using deferred or smth instead of delayed, which carries a different connotation in other prominent libraries. E.g., dask defines a dask.delayed decorator for lazy function computation. I suppose it would still be conceptually appropriate, but it might confuse more experienced coders, as its semantic meaning is slightly different. https://docs.dask.org/en/latest/delayed.html

Edit: Maybe you could even name the decorator commit, to make apparent that it's related to auto_commit.

[janezd]

Thanks for deferred! I didn't like delayed, but haven't found a better word.

I've never seen somebody attach an attribute to a function or bound method like this

It's a typical pattern. lru_cache adds cache_info, singledispatch adds register and dispatch. (Also property seems to add setter, but it adds it to the function.)

add a new attribute, preferably to a class instance.

I can't think of any decorator that adds methods to namespace. It looks sneaky (this is exactly what we are doing now, and I don't like it) and can also cause collisions, at least in general. lru_cache does not add <function-name>_cache_info.

if I use a decorator, I expect it to change the method I decorated.

There's no reason why decorator should change the function. Many don't. From the top of my head: register just registers the function with the dispatcher, unittest.patch doesn't change the function (except for sometimes adding an argument), wraps only changes some attributes (which is so inconsequential that we usually neglect to call it). I'm sure there are others, too. This one is funny: https://wiki.python.org/moin/Decorators#A__main__.

Adding attributes to the function - without changing it - is actually listed as a use case in https://www.python.org/dev/peps/pep-0318/.

I prefer a decorator that adds another mechanism for calling this function in deferred form over one that changes it to a deferred version, but lets the function that we are defining (and decorating) be found under a different name inserted into the namespace.

[irgolic]

Cool, thanks for sharing those examples.

I suppose my intuition was coming from dask.delayed, which changes the function to a delayed version.

I prefer a decorator that adds another mechanism for calling this function in deferred form over one that changes it to a deferred version, but lets the function that we are defining (and decorating) be found under a different name inserted into the namespace.

I don't like what we have now. But what about changing it to a deferred version and NOT letting the function be found under a different name in the namespace? Is there a valid use for unconditional_commit outside the above described? If not, it's better to discourage its use, people will intuitively want to write callback=self.commit. To stay on the safe side, it could be added to the namespace as a private method (_OWBaseWidget__unconditional_commit), or to the bound (now conditional) method as self.commit.unconditional.

[markotoplak]

I prefer a decorator that adds another mechanism for calling this function in deferred form over one that changes it to a deferred version, but lets the function that we are defining (and decorating) be found under a different name inserted into the namespace.

Adding a deferred version, as @janezd suggests, makes the behavior more explicit. This makes the behavior easier to understand. And for me, this means easier development. Yes, it might make it harder to write in the beginning, as @irgolic pointed out, but to me, this short-term disadvantage is far outweighed by easier reasoning about the code. To me, adding a deferred commit and not modifying the original function is the step forward here.

If we eventually find the unconditional version superfluous, we could modify the deferred decorator to wrap it with a nice warning. Voila!

[markotoplak]

Alternatively, the deferred decorated could wrap the unconditional commit so that it always produces a warning unless the user calls it with an argument i_really_want_unconditiional_commit=True. Or perhaps some other name. :)

This could both (1) guide users relying on autocomplete and (2) make the code easy to read. Personally, I do not care about (1) so I would skip it, but if the others feel differently a compromise solution like this would work for me.

[janezd]

Private methods are not intended as indicators that a method is probably not the one that developer needs to call. Plus, they're problematic for derived classes. Plus (I don't want to go there again), there's nothing wrong with adding attributes to functions.

We are not to discourage the use of immediate commit. It's perfectly proper to call it when needed -- which is typically once in each widget!

By the way, if decorator changes the behaviour, I'd call the original self.commit.undeferred() or, better, self.commit.now(). unconditional is weird.

For me, the only dilemma that remains is whether to modify and provide the original or vice versa. @markotoplak, wouldn't you say that marking the function as deferred is explicit enough? It requires explicit, conscious action from the programmer. (I just realized: gui.auto_commit can show a warning when given an undecorated commit!)

• For changing the behaviour: there will be more calls to deferred commit, so let it have the shorter form.
• Against: using immediate instead of deferred causes unnecessary computation. The opposite may stop the workflow execution if auto apply is off.

We need to choose the better solution, not the one easier for a hypothetical beginner. If GvR had beginners in mind, Python would have a goto statement.

[markotoplak]

For me, the only dilemma that remains is whether to modify and provide the original or vice versa. @markotoplak, wouldn't you say that marking the function as deferred is explicit enough? It requires explicit, conscious action from the programmer.

Modifying with the decorator is very explicit indeed when you look at the function's code, but not if you are looking at the code where the function is called. As you suggest, requiring the commit to be decorated would make it less confusing.

* **For changing the behaviour:** 
* **Against:** 

So this is inherently confusing. I suggest we make things even more explicit: the decorator could add self.commit.now() and self.commit.deferred(), while making self.commit() invalid.

[janezd]

the decorator could add self.commit.now() and self.commit.deferred(), while making self.commit() invalid.

This crossed my mind, too. :) You're right, if we can't decide, we can/should expose both options equivalently.

But the best thing about this is that it forces you to always think what you're doing.

"Beginners" won't find the decorator anyway, but if they do, the decorator will replace the function with one that throws an exception directing the programmer to call either "now" or "deferred".

What shall we name the decorator? deferred is no longer a good match, and I don't like naming it commit because we (at least I) often find the name apply more appropriate for the context.

[irgolic]

We are not to discourage the use of immediate commit. It's perfectly proper to call it when needed -- which is typically once in each widget!

Could you describe when it's appropriate to call immediate commit? I looked and couldn't find an example. As mentioned above, the only real uses I've found are within conditional commit and when receiving new inputs (which should be consolidated in OWBaseWidget). When using Orange, users learn that plugging a new signal into a widget causes commit to trigger. It's a primary mechanic in that users don't even think about it, they unconsciously rely on it. That's why this shouldn't be implemented in each widget separately but in the base class.

We need to choose the better solution, not the one easier for a hypothetical beginner.

Please don't disregard the learning curve, better is subjective.

[janezd]

Could you describe when it's appropriate to call immediate commit?

As you wrote: when receiving new inputs. I can't see a consolidation of that because there are so many paths the computation can take. It can be even in a separate thread.

[janezd]

I like the end result, https://github.com/biolab/orange3/pull/5495/files, as well as the implementation, https://github.com/biolab/orange-widget-base/pull/157/files.

The hypothetical beginner was probably just copying auto_commit calls without knowing the magic and necessary rituals surrounding it. So (s)he was doing it half-wrong. Now, auto_commit issues a warning f"decorate {commit_name} with @gui.deferred and then explicitly call {commit_name}.now or {commit_name}.deferred"). If the method is decorated, calling commit raises an exception instructing her/him to explicitly call now or deferred.

This will also prevent us, "seniors", from making this mistake.