Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add eager option to watch calls #351

Merged
merged 2 commits into from Oct 1, 2019
Merged

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Jul 31, 2019

In order to fix #322 I implemented a scheme by which watch calls would execute in the order the user scheduled them. This meant that if a watch callback triggered another event, that event would not get called until all previous events had been processed.

This fix was important for certain (relatively rare) scenarios and ensured correctness, however it also has the quite annoying side-effect that you cannot define a watch callback which temporarily sets and unsets a parameter that has some side-effect, e.g. when you click a button in panel you might want to disable that button while you perform some computation (e.g. see #350), in this scenario you want param to eagerly execute any callbacks as they are triggered. The distinction here is akin to depth-first vs. breadth-first execution of callbacks.

To control this I've added an eager keyword to watch calls. I'd be happy to rename that to something that reflects the depth- vs. breadth-first distinction. Additionally I'm now also not clear whether the current default of breadth-first execution is more desirable, and would consider changing the default. I think it's a very common usecase particularly in panel to trigger some temporary side-effect in a callback and teaching users about the execution model seems difficult.

Closes #350

  • Decide on API and naming
  • Decide whether to change the default
  • Add tests

@philippjfr
Copy link
Member Author

philippjfr commented Jul 31, 2019

It also seems like depth-first execution is what a regular user would naively expect, because naively you wouldn't expect an event queue to handle the breadth-first case for you.

@jbednar
Copy link
Member

jbednar commented Jul 31, 2019

I don't have any opinions about the name. It's worth working out which scenarios work better in eager=True, and which work better in eager=False, and try to estimate which of those are more common scenarios. If they are about equally common, I'd say to keep the current default behavior. If one of the categories is way more common and typical than the other, favor that one. I don't have a good idea myself which way that falls.

@philippjfr
Copy link
Member Author

I think depth-first is way more common, wanting some side-effect to take place while you run some callback is very common, e.g. change the style or color of a widget or disable it while it executes is a common usecase. Even in pure param usage you may want a callback to trigger some other side-effect on the same class. Comparing that to the breadth-first case I can't think of many cases where that would occur, you will note for example that I had to draw out a whole diagram for #322 to even visualize and understand the three way interaction where breadth-first execution is important.

@jbednar
Copy link
Member

jbednar commented Jul 31, 2019

What about a busy indicator or progress bar; sounds like easier in depth-first?

@philippjfr
Copy link
Member Author

philippjfr commented Jul 31, 2019

What about a busy indicator or progress bar; sounds like easier in depth-first?

If the busy indicator lives on the same class yes. One thing I didn't explain well enough that the depth vs breadth first execution model only matters for events happening on the same object, for everything else it's always depth-first. That inconsistency is, I believe another good reason to go for depth-first by default.

@jbednar
Copy link
Member

jbednar commented Jul 31, 2019

Ok, sounds good.

@jlstevens
Copy link
Contributor

Instead of eager (which makes me think of eager versus lazy evaluation) how about queued? By default (currently) it sounds like things go into a queue so queued=False would mean 'eager'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eager argument to watch Watcher may be called after newer event has been processed
3 participants