Add new Event parameter type

[jlstevens]

Very WIP PR to illustrate an idea we have been discussing. The API will probably change but here is the current behavior:

The idea is to add a new Trigger parameter type which would replace many uses of Action. What you want to know about a Trigger parameter is whether it has been triggered (e.g a button clicked). The value is True if the parameter has just been triggered, otherwise False (which means a trigger call has happened since not involving that parameter).

One possibility which is more complicated is to avoid b.param.trigger('a') by setting b.a = True (setting to True triggers, whether or not the value is True already). This involves working inside __set__ which can get quite tricky.

At this point, this PR aims to be a place to discuss this suggestion more than something to merge. The idea is that this parameter would mean holoviz/holoviews#4611 is not necessary (though panel would need to be updated to display this parameter type as appropriate buttons).

TODO

• Make this work with class parameters (doesn't work with Bar at the moment in the example above)
• Improve API, possibly via __set__

[jbednar]

Thanks. Can you add a more complete example using depends or watch to show how the end-to-end behavior would look? I'm having trouble seeing how the full circle of triggering an action that then completes would look.

[jbednar]

I tried adding such an example myself, but can't make sense of the results:

I had expected that fn would be executed when a gets triggered, and no other time. Instead it gets executed twice for a being triggered, and also gets executed when c is triggered? I'm confused!

[jbednar]

Ok, I guess the two triggers are because 'a' is being set to True then False:

Not sure about the third one, though, and having two in the first case doesn't seem desirable for something that's explicitly for triggering, and is thus not about the value.

[jlstevens]

Now works with classes and no double triggering:

[jlstevens]

@jbednar The value should now always be False outside of the triggered function.

[jbednar]

As far as I can see, this is a reasonable set of behavior now:

Thanks for updating it and addressing those quirks!

So let's see if we can keep this behavior as a reference and consider the implementation, the API, and the naming.

Taking a step back, the user story here is nominally about a Trigger parameter, but actually the Trigger Parameter itself is a no-op as currently implemented. Every Parameter can already have .trigger called on it, and the only thing special about the current param.Trigger happens for special casing for param.Trigger within .trigger. That's spooky-action-at-a-distance stuff rather than clean object-oriented design; e.g. people can't inspect the source code of Trigger to find out anything about how it works. So my gut reaction is not to like having a parameter called Trigger that doesn't actually implement anything about triggering. If we do want triggering to be implemented in this way, I would steer more towards some flag declared by a Parameter that enables this behavior rather than a specially named dummy Parameter type.

Would the set approach be able to be localized into the Parameter's definition, if it's feasible? If so that would be a big advantage of doing it that way. Plus using set would map more cleanly to the semantics of Trigger, because the value does change during triggering, so it's nice to have that be explicit (even if the changing back to False is implicit). I'd be happy to have a set-based Trigger be called param.Trigger if the implementation is inside the Trigger class, whereas the current Trigger struggles to have the name Trigger be appropriate for it and seems like it shouldn't exist in that form.

[jlstevens]

To clarify my understanding of what you are suggesting, the set approach means running the trigger with something like:

foo.button = True

Instead of foo.param.trigger('button').

That might be possible by overriding the __set__ method on this new parameter (I can't be sure yet) but I am sure that the special logic in the trigger method (in the Parameters namespace object) would still be required.

[jbednar]

I'll see once there's an implementation. In any case, ideally we'd have something other than the name Trigger to govern that special behavior if it's still needed.

[jlstevens]

I sort of have it working:

It now relies on the change in value to run the callback (apparently most things don't even use trigger). Unfortunately discard_events doesn't seem to work inside __set__ (events were still being generated when setting to False) so I copied the core parts of __set__ without all the watcher stuff to reset to False.

[jlstevens]

At this point in the history, the implementation is simple as I've removed the changes from the trigger method. One thing that isn't clear to me, should a become True when running b.param.trigger('a')? That would need special handling...

[coveralls]

Coverage increased (+0.2%) to 79.645% when pulling df0f4e2 on trigger_parameter into f68fffc on master.

[jlstevens]

Explicit triggering now handled:

This does what I would expect now so I'll leave this PR alone till I get some feedback.

[jbednar]

I've tested the current commits, and it doesn't seem like it's working reasonably, because the callback keeps getting fired for things that have nothing to do with a:

I'd only have expected it to be triggered for the single call to .trigger('a'), but it's also being called when other things are triggered? I suspect the changes to .trigger are forcing triggering for all Trigger parameters even if they weren't the ones being called here.

One thing that isn't clear to me, should a become True when running b.param.trigger('a')? That would need special handling...

I don't think a should become True when merely triggering a; I think the value should change only when a is set (and then only transiently), leaving .trigger to simply trigger without a change in value. I think that's more consistent with other parameters, leaving the only weird aspect of this one to be the fact that it auto-resets itself whenever you try to set it.

If I remove the changes to .trigger, it seems to behave reasonably to me:

Without those changes, it's now fully self contained, fully local in its implementation and effects, and easy to describe: a Trigger parameter is one that can be set, but where setting is always transient, used only to trigger other actions, with the value immediately returning to the baseline by the end of the setting operation.

So I'm happy with it at the param level, if you go ahead and remove the .trigger code changes altogether. If you agree, then it's up to you (@jlstevens) and @philippjfr to say whether this is sufficient for HoloViews stream needs -- does this transient setting achieve the correct set of values and triggering events for you to use as part of the overall stream handling? If it does, then we're done and ended up with only a few, fully localized lines of code!

[jlstevens]

Ok, I am happy to remove the latest changes to the trigger method again as the behavior on explicit triggering wasn't obvious to me (and not introducing special handling might well be the most sensible option).

As soon as there is some panel code that can display Trigger parameters as buttons, I'll be able to test to see if this properly supersedes the HoloViews PR.

[jlstevens]

@jbednar Note, I spotted the bug in the code I just reverted: I simply forgot to filter the trigger parameters against param_names. So I think I could make foo.param.trigger('a') have a be True transiently (without executing unrelated triggers). Would you like to try it out or are you sure this behavior isn't a good idea?

[philippjfr]

So I think I could make foo.param.trigger('a') have a be True transiently (without executing unrelated triggers). Would you like to try it out or are you sure this behavior isn't a good idea?

I think that should work because otherwise someone could trigger it and get False as a value which seems incorrect.

[jlstevens]

Ok, both versions can work! With the latest commit, unrelated triggers (e.g b.param.trigger('c')) aren't being executed:

I think I prefer this than leaving the value False. As either version works, I am happy to pick either as long as we have a consensus on the correct behavior. Then I'll write up some unit tests.

[jbednar]

I see arguments above saying that it could be confusing to invoke .trigger and not see the value change, but I don't personally think that's confusing, because .trigger never changes the value for any other Parameter, so I don't see why people would expect to see it do so here. I think it's up to @philippjfr ; if the changes to .trigger here aren't needed for holoviews or panel, I'd rather keep the code and behavior localized to the Trigger class definition, but if it makes things work more consistently then it's ok to keep the changes to .trigger.

[jlstevens]

... so I don't see why people would expect to see it do so here

In general I would agree, but for a parameter named Trigger it doesn't seem unreasonable to have semantics specially related to .trigger. I have a slight preference for consistency in terms of having a value of True while triggering (which is special relative to other parameters that don't care about being triggered) but I am also happy to let @philippjfr decide.

[jbednar]

for a parameter named Trigger it doesn't seem unreasonable to have semantics specially related to .trigger

Right; I was thinking about that too. Maybe it needs a better name? param.Transient? param.Pulse? In electronics it's called a momentary switch, but I don't think that's useful here.

It's hard to think of something that will make sense at the app level and at the Param level, and I haven't come up with anything better than Trigger.

[jlstevens]

param.Event (as in a 'button press event')? Though to me events are typically more structured than this...

[jbednar]

That's a contender. Right, events typically have a payload and this doesn't, but it's true it's an event...

[jlstevens]

I prefer Event to Pulse.

[philippjfr]

I've also been considering param.Event, my only reservation was that param.parameterized.Event exists for the watch Events.

[jlstevens]

I don't think it is a problem to have an Event parameter as well as an actual Event object...

[philippjfr]

There's a few things I'm struggling with the discussion here.

This would have different semantics from what we discussed earlier. In particular, an Event parameter could have a permanent value of True if no watchers are hooked up to it. Only when something is actually triggered would it go back to False. This might be a way to notice that something is not hooked up if you type foo.a = True and then look at foo.a and it is still True, nothing was triggered.

This seems totally bizarre to me, to me the point of the True value is to say it is true that there is currently an event, so this totally breaks the semantics I've always maintained. Also changing the behavior depending on whether there is a watcher or not is also very strange to me. I'm totally missing any argument that would justify this behavior.

I like the simplicity of knowing that the only weird thing about Event is that when the event is processed the value is reset. It seems a lot easier to reason about than the approach currently in this PR, where I have to just close my eyes and hope that it's all ok.

Totally disagree here, I think that is really weird behavior because it breaks the concept of this parameter signaling whether there is an event or not, but we've had sufficient back and forth here to establish our disagreement here 😄

You and Philipp agreed that the value needed to change when calling .trigger, so I guess you're right, you have to store both the reset and non-reset values still. :-(

This is the part I'm still working through so let's go through a chain of events. The main problem I see here is to do with batching of these events, which is a problem whether or not we implement the .trigger behavior, so I struggling to see how anything will be simpler without that. I also don't quite gather the need for storing a reset and non-reset values from this discussion. What I absolutely don't think will work is somehow making the resetting dependent on whether there are watchers, because if you add a watcher dynamically the value will be True and setting it will not trigger any event at all, which is simply wrong behavior, whichever semantics you ascribe to param.Event. It always has to reset to False.

That just leaves the resetting behavior in the batched which I'd have to work through with detailed examples but that doesn't at all depend on implementing the .trigger semantics.

[jlstevens]

What I absolutely don't think will work is somehow making the resetting dependent on whether there are watchers, because if you add a watcher dynamically the value will be True and setting it will not trigger any event at all, which is simply wrong behavior, whichever semantics you ascribe to param.Event. It always has to reset to False.

I agree. The semantics of the parameter just look like they need a more complicated implementation than we may like in order to make sense.

[jbednar]

I think it's probably most efficient if @philippjfr and @jlstevens together agree on an implementation that you like. I can give feedback at that point, but it won't be binding, just an opinion. So please work it out so you're happy, and we can go from there!

[jlstevens]

@philippjfr Tests are green and I've addressed your suggestions. Ready to merge and cut a new release?

[philippjfr]

Looks good.