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

feat: Add recursion_mode ('immediate' or 'deferred') to Signal and SignalInstance #293

Merged
merged 14 commits into from
Mar 11, 2024

Conversation

tlambert03
Copy link
Member

@tlambert03 tlambert03 commented Mar 8, 2024

fixes #292, and represents a partial reversion of #281 (the behavior of #281 is still possible, but must be opted into using recursion_mode = 'deferred')

This adds the possibility to control the order callbacks during recursive emission events (when one of the callbacks itself emits the same event):

  • "immediate": Nested emission events immediately begin a deeper emission loop:
    calling all callbacks with the arguments of the nested emitter before
    returning to continue the original emission loop. This is the default
    behavior.
  • "deferred": Nested emission events are deferred until the current emission
    loop is complete. This is the default behavior.

@Czaki, thoughts welcome on parameter naming and signature, etc... (i.e. should it be a boolean recurse_immediately instead of a set of literal strings? will we ever want additional modes?)

@tlambert03 tlambert03 requested a review from Czaki March 8, 2024 21:54
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (c643ad0) to head (1589cec).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #293   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         1933      1973   +40     
=========================================
+ Hits          1933      1973   +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

codspeed-hq bot commented Mar 8, 2024

CodSpeed Performance Report

Merging #293 will improve performances by 10.36%

Comparing tlambert03:recursion-mode (1589cec) with main (c643ad0)

Summary

⚡ 1 improvements
✅ 65 untouched benchmarks

Benchmarks breakdown

Benchmark main tlambert03:recursion-mode Change
test_emit_time[setattr-50] 225.7 µs 204.5 µs +10.36%

Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to use flag, instead of a boolean variable, as it will allow extending in future.

I would like to see in this PR update of EventedModel to use dereferred mode by default.

I thinks that it will be nice to document somewhere why dereferred mode is better in some cases.

@tlambert03
Copy link
Member Author

The failing benchmarks represent a slight increase in the time to create a signal instance:

Change Before [f4ac7c3] <v0.10.0^0> After [b17be85] Ratio Benchmark (Parameter)
+ 477±3ns 833±3ns 1.74 benchmarks.CreateSuite.time_create_signal_instance
+ 4.80±0.05μs 5.87±0.08μs 1.22 benchmarks.EventedSetSuite.time_create_set(10)
+ 4.85±0.03μs 5.92±0.02μs 1.22 benchmarks.EventedSetWithCallbackSuite.time_create_set(10)

which is not something I'm worried about here given that it's on the order of ns.

@Czaki, this is ready for final review. I'll wait for your review on this one before merging. And I intend to follow up in another PR reviewing the exception handling logic slightly

Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, looks ok, with one remark.

@tlambert03 tlambert03 merged commit f37c26f into pyapp-kit:main Mar 11, 2024
53 of 54 checks passed
@tlambert03 tlambert03 deleted the recursion-mode branch March 11, 2024 13:25
@tlambert03 tlambert03 added the enhancement New feature or request label Mar 11, 2024
@tlambert03 tlambert03 changed the title Add recursion_mode ('immediate' or 'deferred') to Signal and SignalInstance feat: Add recursion_mode ('immediate' or 'deferred') to Signal and SignalInstance Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

RecursionError with v0.10.0
2 participants