Improvements for synchronous and asychronous generators #908

philippjfr · 2024-02-08T22:26:47Z

This PR improves the support for synchronous and asychronous generators as references and dependencies. It also fixes a variety of bugs related to updating async generators. Specifically it implements the following:

Enhancements

Generators can now be used as references meaning that they are scheduled on a thread and then are applied asynchronously

class Test(param.Parameterized):

    value = param.Number(allow_refs=True)

def gen():
    i = 0
    while True:
        yield i
        time.sleep(0.1)
        i += 1

t = Test(value=gen)

t.param.value.rx()

Add the ability to construct expressions directly from sync and async generators

Add ability to Skip reference updates of all kinds
Asy 10000 nchronous generators can now be annotated with param.depends
In IPython contexts we now define a default async_executor meaning that async functionality can now be used by default without importing Panel or defining your own executor.

Bug fixes

Fixes an issue with asynchronous generators not being correctly cleaned up when overridden
Ensure replacing an async generator function does not try to update the parameter value with the function
Ensure replacing an async generator function does not delete the reference to a newer task making it immortal
Do not allow param.Number to accept generators since they are not actually supported for Dynamic parameters

ToDo

Implements Generator function not working panel#6295
Add tests
Update docs

param/ipython.py

tests/testrefs.py

param/_utils.py

doc/user_guide/Generators.ipynb

param/_utils.py

hoxbro · 2024-02-19T17:18:23Z

param/ipython.py

        from param.reactive import rx

+        handle = None


Suggested change

handle = None

handle: IPython.core.display_functions.DisplayHandle | None = None

Was a bit confused when reading the code. The import could be done by a TYPE_HINTING.

param/ipython.py

param/_utils.py

param/parameterized.py

param/_utils.py

philippjfr · 2024-02-25T09:57:39Z

@jbednar I'll leave final review and merging up to you if you get a chance, otherwise I'll ask for approval from @hoxbro and @maximlt before merging.

hoxbro · 2024-02-27T10:56:25Z

I'm still not a fan of the name Skip.

Another thing is, do we need it to be an Exception? It almost seems like the user is doing something wrong each time they raise it. If it is an exception because it is easier to propagate the error to the right place for us as maintainers and does not have a real effect on the user if we change it to an object, I think we should change it.

philippjfr · 2024-02-27T12:45:13Z

I still strongly prefer Skip and also think using it as an exception is the right choice. The only other option we have would be to allow the user to return the Skip sentinel value and there's a few reasons I don't like this:

The most important reason is that dependent functions can be nested in a number of ways and there is no obvious way to intercept a skipped sentinel value that is returned inside another dependent function. This means user code would have to know about the skip sentinel value and explicitly check for it to avoid performing invalid operations on it and causing actual exceptions. Exceptions allow you to bubble up through arbitrary layers of nesting that a user may have instrumented.
It does not clearly distinguish the skip case from a regular return/yield case.
Various forms of caching/memoization can be added to these functions and Skip should always bypass the cache.

philippjfr · 2024-02-27T13:01:38Z

Very happy to hear alternative naming suggestions.

hoxbro · 2024-02-27T13:44:40Z

Let's start from the back (I would say I'm playing devil's advocate for some of the arguments)

(3) A counterpoint would be if you do a heavy calculation in a function and then raise a Skip at the end. Having a sentinel value will actually remember that this function did return a Skip.

(2) Why is this important?

I think this code with sentinel is cleaner,

# with sentinel value
value = func()
if value is Skip:
    value = None

# with exception
try:
    value = func()
except Skip: 
    value = None

Another thing is that Skip could be wrongly caught in except RuntimeError statement, albeit highly unlikely.

(1) I have a hard time coming up with an example that does not end with a param.bind (or similar) getting the returned Skip. Or at least a hard time if we assume the user can't support such cases in their nested functions.

The problem you are describing is supporting suboptimal code. This could lead to a side effect function raising Skip, which is then raised to the top level even though this is not what you wanted, and then you are in a worse situation than with a sentinel value as you have no idea if the "correct" Skip was raised. All of this could be significantly harder if it is a watched function that raises Skip at a distance.

Another reason for having it as a sentinel value is also that it will make it possible to type a hint that a function can return it; this is not possible with exceptions in Python.

With that being said I generally don't have a problem with it being an exception. I just want to be sure that it is the best way to handle a skip, and if you feel that it is I have no objections to it.

For naming maybe SkipUpdate. (Note: I wouldn't be that afraid of the length of the name as most modern editors have autocomplete.)

philippjfr · 2024-02-27T21:06:30Z

(3) A counterpoint would be if you do a heavy calculation in a function and then raise a Skip at the end. Having a sentinel value will actually remember that this function did return a Skip.

Seems like an anti-pattern to run some long calculation only to then skip, but I'll accept there will be (I would consider rare) cases where you don't know you want to skip until you get to the end of some long-running computation.

(2) Why is this important?

It isn't super important but it's not our code I'm talking about but user code, i.e. I like the fact that raise Skip() stands out, while return Skip looks like any other return statement.

(1) I have a hard time coming up with an example that does not end with a param.bind (or similar) getting the returned Skip. Or at least a hard time if we assume the user can't support such cases in their nested functions.

I'm not really following your point here in particular the stuff about watch+Skip since that combination really doesn't make sense and isn't what Skip is intended for. In the side-effectey world you don't have to Skip, you can just return early. What I am thinking about is scenarios where you might have a method that is useful both on its own and may be invoked by another method, e.g. take my crappy example

class DatePlotter(param.Parameterized):

    date = param.CalendarDate()

    update = param.Event()

    ...

     @param.depends('date')
     def get_daily_data(self):
          if not self.update:
              raise Skip()
          return self.df[self.df.date==self.date]

     @param.depends('date')
     def plot_daily_data(self):
         daily_df = self.get_daily_data()
         return daily_df.hvplot()

dp = DailyPlotter()
pn.Row(dp.param, pn.widgets.Tabulator(dp), pn.pane.HoloViews(dp.plot_daily_data))

Here I can have a get_daily_data function that is both used directly and invoked by some other method for another purpose and I can write the Skip logic once. Both the consumer of get_daily_data and the consumer of plot_daily_data can rely on the fact that the data will only emit if the update parameter is triggered (i.e. when the button is clicked) without the user having to handle both cases. If we have a sentinel value returned by get_daily_data then plot_daily_data now also has to implement the Skip logic OR handle the Skip return value.

hoxbro · 2024-02-28T07:46:17Z

Seems like an anti-pattern to run some long calculation only to then skip, but I'll accept there will be (I would consider rare) cases where you don't know you want to skip until you get to the end of some long-running computation.

Then, let's say it is a lookup in a database, which is slow.

It isn't super important but it's not our code I'm talking about but user code, i.e. I like the fact that raise Skip() stands out, while return Skip looks like any other return statement.

I really don't think that it needs to stand out. As I see it, a raised exception is usual when something goes wrong, and that is not what Skip is; it is part of the API. This could lead to confusion for non-advanced users, maybe they will even try to avoid it because Exception == "bad".

I can write the Skip logic once

I don't necessarily see this as a good thing. I think this could lead to weird side effects, like why a widget isn't updating because some weird nested function is raising Skip. I think the main concern for me is also the advantage a raise brings: It breaks the control flow, you don't know what will catch it, and that could lead to hard-to-debug problems.
Having it as a sentinel value will, yes, introduce more code, but it will make users design their apps with this in mind upfront instead of having to figure it out in a debug session.

In your example, there is no way to typehint that the get_daily_data can raise a Skip, which is okay when you have it just above the method, but what if it is inherited from a class in another file? This means that potentially every method/function call can raise a Skip, and you would have no idea which one is causing it.

If we take your example and want to send a notification to the user that there is no data in plot_daily_data? This could quickly become ugly when having Skip as a raised exception. What if there is another function call inside plot_daily_data, which also raises a Skip but for a different reason, and you want to send a different notification to the user?

class DatePlotter(param.Parameterized):

    date = param.CalendarDate()

    update = param.Event()

    ...

     @param.depends('date')
     def get_daily_data(self) -> pd.DataFrame | Skip:
          if not self.update:
              return Skip
          return self.df[self.df.date==self.date]

     @param.depends('date')
     def plot_daily_data(self) -> hvplot | Skip:
         daily_df = self.get_daily_data()
         # We want to inform the user there is no daily data
         if daily_df is Skip:
              pn.state.notifications.info("No daily data")
              return Skip
         return daily_df.hvplot()

dp = DailyPlotter()
pn.Row(dp.param, pn.widgets.Tabulator(dp), pn.pane.HoloViews(dp.plot_daily_data))

An option could be to support both methods, e.g., both returning and raising Skip.

Another name for it could be NoUpdate.

jbednar · 2024-02-29T03:15:28Z

Exceptions usually are for error conditions, but the Skip exception seems analogous to the StopIteration exception, which is just for unusual control flows, not errors. The code examples above don't make a particularly strong case either way, so I'm inclined to support the way Philipp was originally motivated to write it (as an exception).

jbednar

Nice!

param/_utils.py

doc/user_guide/Generators.ipynb

doc/user_guide/References.ipynb

doc/user_guide/index.md

hoxbro · 2024-02-29T08:21:56Z

Exceptions usually are for error conditions, but the Skip exception seems analogous to the StopIteration exception, which is just for unusual control flows, not errors

The difference is that this is built into the language itself and is called each time you run a for loop without raising an exception yourself. I never think I have raised a StopIteration error by myself.

Another point is that people can catch Skip it with an except Exception by having it as an exception. And then they have to write code like this

try:
     # function which can both raise a ValueError and Skip
except Skip:
     raise
except Exception:
     # do something else

Which users likely won't do, or even if they do, they can miss one.

hoxbro · 2024-02-29T08:31:49Z

The last thing I want to say is that using Skip as an exception is not a golden bullet that will always just work. It should be seen as a half-poisoned apple; it tastes good until you get to the poison side of it, and when that happens, it is too late to do anything.

Co-authored-by: Simon Høxbro Hansen <simon.hansen@me.com>

Co-authored-by: James A. Bednar <jbednar@continuum.io>

philippjfr · 2024-03-04T12:30:19Z

Another point is that people can catch Skip it with an except Exception by having it as an exception. And then they have to write code like this

I can conceivably envision this being an issue but in practice I still consider this unlikely because:

Most functions will not be nested.
If the functions are nested they are generally unlikely to add exception handling like this.
If they do end up catching the Skip they are already considering the error condition which will effectively mean they either skip again, reraise (which is effectively the same as skip except it's less graceful) or add some special return value which will be visible to the user.

The last thing I want to say is that using Skip as an exception is not a golden bullet that will always just work. It should be seen as a half-poisoned apple; it tastes good until you get to the poison side of it, and when that happens, it is too late to do anything.

A little dramatic maybe, but sure 😄

In any case I have now also handled Skip return values and addressed all review comments.

philippjfr · 2024-03-04T12:36:32Z

Then, let's say it is a lookup in a database, which is slow.

If the database lookup fails that may well be because of a timeout error or other temporary issues in which case caching would be bad.

As for your example above, I actually see the exception as a benefit here, because I can attach a reason in a straightforward and canonical way:

class DatePlotter(param.Parameterized):

    date = param.CalendarDate()

    update = param.Event()

    ...

     @param.depends('date')
     def get_daily_data(self) -> pd.DataFrame | Skip:
          if not self.update:
              raise Skip("Not updating")
          elif not self.date in self.df.date:
              raise Skip('No daily data')
          return self.df[self.df.date==self.date]

     @param.depends('date')
     def plot_daily_data(self) -> hvplot | Skip:
         try:
             daily_df = self.get_daily_data()
         except Skip as e:
              pn.state.notifications.info(str(e))
              raise e
         return daily_df.hvplot()

dp = DailyPlotter()
pn.Row(dp.param, pn.widgets.Tabulator(dp), pn.pane.HoloViews(dp.plot_daily_data))

philippjfr · 2024-03-04T14:20:34Z

@hoxbro and I had another discussion. He still consider the Skip pattern dangerous since someone might raise it deep inside some code and you might never know. My opinion is that Skip should never be used deep in some code and have added a cautionary note to the docs saying that Skip should be primarily used when wrapping other code to be used as a reactive reference and it should never appear in business logic. I'll trust our users enough not to abuse it but I guess time will tell if that trust was misplaced.

For now I'd like to continue here and with one approval and having addressed the review comments I'm going to merge.

philippjfr force-pushed the generator_refs branch 2 times, most recently from 90d1483 to 2466c90 Compare February 9, 2024 04:03

hoxbro reviewed Feb 9, 2024

View reviewed changes

param/ipython.py Outdated Show resolved Hide resolved

philippjfr force-pushed the generator_refs branch from 0571f84 to 3e2734c Compare February 12, 2024 12:27

hoxbro reviewed Feb 13, 2024

View reviewed changes

tests/testrefs.py Show resolved Hide resolved

philippjfr force-pushed the generator_refs branch from ab9f9be to 8476be8 Compare February 15, 2024 16:03

hoxbro reviewed Feb 15, 2024

View reviewed changes

param/_utils.py Outdated Show resolved Hide resolved

ahuang11 reviewed Feb 19, 2024

View reviewed changes

doc/user_guide/Generators.ipynb Outdated Show resolved Hide resolved

ahuang11 reviewed Feb 19, 2024

View reviewed changes

doc/user_guide/Generators.ipynb Outdated Show resolved Hide resolved

hoxbro reviewed Feb 19, 2024

View reviewed changes

philippjfr commented Feb 20, 2024

View reviewed changes

param/_utils.py Outdated Show resolved Hide resolved

philippjfr mentioned this pull request Feb 26, 2024

Add ability to Skip Param<Ref|Function|Method> updates holoviz/panel#6396

Merged

jbednar approved these changes Feb 29, 2024

View reviewed changes

philippjfr added 7 commits March 4, 2024 13:22

Ensure async generator refs are cleaned up

41a28f5

Add default async_executor in IPython

0f19730

Support non-blocking synchronous generator functions

848570e

Support depends on async generator functions

1990b9d

Various fixes for async task cleanup and updates

ccbdc37

Allow depending on and binding generator functions

ba49c76

Do not allow generator functions on param.Number

716d894

philippjfr and others added 21 commits March 4, 2024 13:22

Correctly execute async tests

354deb4

Add larger timeout

90831e2

Add to_thread_polyfill

98017f0

Fix timeouts

f4877ef

Add test

cda396d

More skip handling and tests

00d6018

Improve timeouts

33afb75

Add generator documentation

9abd67a

Do not attempt to resolve dependency's dependency

b308929

Fix bug

0325c3c

Improve task cleanup

3e0c216

Add more timeouts

3328e85

Apply suggestions from code review

560c551

Co-authored-by: Simon Høxbro Hansen <simon.hansen@me.com>

Address comments

b33cdec

Increase timeout

d3d862c

Update param/_utils.py

7fe1f8b

Co-authored-by: James A. Bednar <jbednar@continuum.io>

Apply suggestions from code review

8af2e84

Co-authored-by: James A. Bednar <jbednar@continuum.io>

Add ability to return Skip

db7613f

Make Skip exception and do not instantiate in example

84014ff

Fix for Skip

9590d26

Update docs

2472467

philippjfr force-pushed the generator_refs branch from 036939b to 2472467 Compare March 4, 2024 12:22

Add cautionary note

d27f981

philippjfr force-pushed the generator_refs branch from 4d9dc9b to d27f981 Compare March 4, 2024 14:06

philippjfr merged commit fc75c96 into main Mar 4, 2024

philippjfr deleted the generator_refs branch March 4, 2024 14:21

philippjfr mentioned this pull request Mar 22, 2024

Add CHANGELOG 2.1.0 #925

Merged

	handle = None
	handle: IPython.core.display_functions.DisplayHandle \| None = None

Uh oh!

Improvements for synchronous and asychronous generators #908

Improvements for synchronous and asychronous generators #908

Uh oh!

Conversation

Uh oh!

Enhancements

Bug fixes

ToDo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!