psygnal icon indicating copy to clipboard operation
psygnal copied to clipboard

feat: support coroutine functions

Open tlambert03 opened this issue 1 year ago • 6 comments

fixes #345

tlambert03 avatar Dec 20 '24 02:12 tlambert03

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 99.83%. Comparing base (8bf2d82) to head (35dde45). :warning: Report is 15 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main     #346    +/-   ##
========================================
  Coverage   99.82%   99.83%            
========================================
  Files          22       23     +1     
  Lines        2351     2480   +129     
========================================
+ Hits         2347     2476   +129     
  Misses          4        4            

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Dec 20 '24 03:12 codecov[bot]

Thanks for the quick PR @tlambert03! In #345 you mentioned that:

psygnal itself has no concept of an event loop (it's a "bring-your-own-event-loop" library)

But this PR currently assumes that the event loop is asyncio. However Trio and AnyIO are very popular, so I'm wondering how we could support them and really be event-loop agnostic.

davidbrochart avatar Dec 20 '24 08:12 davidbrochart

Because structured concurrency always needs a task group in order to launch tasks, I think it's better to not support async callbacks in psygnal. One can just use a sync callback, and have the async logic in the user code where the new value is put synchronously in a stream, and a background task gets the values from the stream asynchronously. That way the user can launch this background task in the appropriate task group.

davidbrochart avatar Dec 20 '24 08:12 davidbrochart

Thanks for your input!

I think it's better to not support async callbacks in psygnal. One can just use a sync callback, and have the async logic in the user code where the new value is put synchronously in a stream, and a background task gets the values from the stream asynchronously.

Are you sure? I'm definitely happy to do whatever we can here to make using coroutines in psygnal easier. That could include supporting auto detection of asyncio, trio, or anyio.

I don't have a ton of personal experience with async/await, so would happily take advice and implement whatever folks suggest. At the very least we could add some better documentation explaining/showing the strategy you mentioned for using a sync function to put a task in a stream.

And if there are conveniences we could add around that pattern, that's fine too

tlambert03 avatar Dec 20 '24 20:12 tlambert03

one additional note, I'm skipping the _async.py module from the mypyc compilation targets for now. When I include it, it's giving me odd, seemingly-unrelated errors in other modules. it might have to do with trio/anyio missing from the environment during mypyc compilation. But I think it's probably just asking for problems anyway. The key bits (signal emissions and weak-callbacks) are still compiled

tlambert03 avatar Feb 17 '25 15:02 tlambert03

CodSpeed Performance Report

Merging #346 will not alter performance

Comparing tlambert03:support-async (35dde45) with main (8bf2d82)

Summary

✅ 67 untouched benchmarks

codspeed-hq[bot] avatar Feb 17 '25 17:02 codspeed-hq[bot]

Hey @davidbrochart, sorry again for the delay on this. I cleaned it up a bit and added tests for all three backends. I'm sure the details are all a little fuzzy for you at this point; but I wrote up some user-facing documentation that describes how we expect people to use this new feature:

https://psygnal--346.org.readthedocs.build/en/346/guides/async/

It would be great if you could just take a quick look at that page, to make sure it's accurate and consistent with how you envisioned this feature being used, and feel free to suggest changes on wording. If the basic API pattern is correct, we can merge and I'll get this in the next upcoming release

tlambert03 avatar Jun 29 '25 15:06 tlambert03

thanks @davidbrochart!

tlambert03 avatar Jul 01 '25 13:07 tlambert03

Oh, then internal Trio errors on Windows don't look good.

davidbrochart avatar Jul 01 '25 13:07 davidbrochart

Oh, then internal Trio errors on Windows don't look good.

Was just looking into that. I actually think it's just a side effect of me greedily trying to test all three backends in the same runtime process. I think https://github.com/pyapp-kit/psygnal/pull/346/commits/982e4aad2c266453b59e2d5c844fdbc701975ddf may have fixed it

tlambert03 avatar Jul 01 '25 13:07 tlambert03

the failing benchmarks are these:

| Change   | Before [8bf2d823] <main>   | After [c8af7ac1]    |   Ratio | Benchmark (Parameter)                         |
|----------|----------------------------|---------------------|---------|-----------------------------------------------|
| +        | 1.77±0.01μs                | 2.90±0.01μs         |    1.64 | benchmarks.ConnectSuite.time_connect_nochecks |
| +        | 19.8±0.1μs                 | 23.1±0.9μs          |    1.16 | benchmarks.ConnectSuite.time_connect_method   |

they are slight enough, and in things that don't get called frequently enough (unlike like emit), and already around a microseconds, that I'm not worried about them

tlambert03 avatar Jul 01 '25 15:07 tlambert03