Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Timer consistency across backends#29062
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
With this PR all backends produce 7 events for this script: |
I added tests and they all pass locally for me, but I can't seem to get things working on the CI runners, so I'm going to put some thoughts/notes down here. I'm wondering if there are issues with trying to assert against timing on shared resources? Thinking it was just trying to run things too fast on limited resources, I increased the timer interval to 500ms and that didn't seem to help. Currently, I am asserting against the expected number of calls, is there possibly a delay involved in first timer spin-up so I should be asserting against the final timer interval somehow instead? If anyone has ideas for better tests here let me know. |
8596fb6 to7f40ba5CompareSince this is only failing on 3.13 (on macOS at least), one possibly suspicious change is in
But I thought you had used |
I can reproduce the failure on AppVeyor locally (Expected 8, got 5); given#28647 (comment), I think that means it is re-evaluating the timeout after the slow callback and not attempting to create a consistent time. |
That is actually a different failure than the one we are seeing here of course 😂https://github.com/matplotlib/matplotlib/actions/runs/11688137387/job/32547874573?pr=29062#step:15:200 These tests on CI systems seem flakey because I had a job right before this push where all the tests passed. I think this gets to the comment you left about system resources and scheduling because sometimes QT is the one failing in CI, others it is GTK. We are running pytest with as many processors as possible Running locally with this script: importtimeimportmatplotlib.pyplotaspltfig=plt.figure()last_time=orig_time=time.perf_counter()x=0defon_timer():globalxgloballast_timet=time.perf_counter()print(f"{x:02d}:{t-last_time:.3f} ({t-orig_time:.3f})")last_time=tifx==0:time.sleep(0.5)else:time.sleep(0.1)x+=1timer=fig.canvas.new_timer(interval=150)timer.add_callback(on_timer)timer.start()fig.canvas.start_event_loop(3) GTK has an ~8ms drift associated with it, whereas all the other backends hit within ~1ms. |
It looks like GTK has no plans on changing this:https://gitlab.gnome.org/GNOME/glib/-/issues/503 |
7f40ba5 to8bc2e95Compare8bc2e95 to54e07edCompareThe implementation of start_event_loop would previously just countthe number of sleeps that occurred. But this could lead to longerevent loop times if flush_events() added time into the loop. Wewant the condition to be dependent on the end-time so we don'trun our loop longer than necessary.
If we set interval in a loop, this could cause the timing to becomedependent on the callback processing time.
The Tk timer would reset itself after the callback had processedto add a new interval. This meant that if a long callback was beingadded we would get a drift in the timer. We need to manually trackthe original firing time and intervals based on that.
Make sure that the interactive timers don't drift when longcallbacks are associated with them.
Previously, the timers were dependent on the length of time it took forthe timer callback to execute. This dispatches the callback tothe task queue to avoid synchronously waiting on long-running callbacktasks.
This adds more robust interactive timer tests to assert against someof the discrepencies that were found in testing.- Run loop shouldn't depend on callback time- Slow callbacks shouldn't cause a timer to drift over time, it should continually fire at the requested cadence- When start() is called again it should invalidate the previous timer associated with that Timer object
Rebased and pushed again. All tests passing locally, going to see how this does on CI now... |
Uh oh!
There was an error while loading.Please reload this page.
PR summary
Currently, all of the backend timers have subtly different behavior relating to long-running callbacks. Meaning we don't make any guarantees about when the timers will fire currently and it is the wild-west of what will happen. This PR has quite a few things going on to try and bring these all into consistency and adds tests for these cases. I'll describe the main updates below:
default backend main loop
This is a really bad error IMO. We are currently running the loop for x number of sleeps(), but if the callback takes longer than the sleep duration this makes the runloop run longer than a user requested. I have updated this to be dependent on total duration within the loop.
interval updates
When setting the interval or singleshot attribute of a timer, we would always call the underlying
start()again. Meaning that this would trigger a timer "reset" in a sense, even if the interval was exactly the same as before (i.e. setting it constantly in a loop). I have updated this to only "reset"/"restart" the timer if the value has changed.Tk
We need to keep track of the expected firing time ourselves since there is no repeating timer. We keep track of the original requested firing time and the callback trigger to account for the case of a callback takinglonger than the repeat timer when we actually want the timer to fire immediately and not on the next interval.
Added singleshot and interval handler updates.
wx
Added singleshot update handler.
macos
Added an asynchronous dispatch to the main queue. This prevents timer drift with synchronous slow callbacks.
Added singleshot and interval update handling.
Removed previously started repeating timer
testing
Added a test for a slow callback to the interactive timers and refactored the tests to try to process in parallel and reduce some of the time spent in the tests.
closes#28647
closes#29029
closes#29076
PR checklist