Tk backend improvements #17789

richardsheridan · 2020-06-28T18:54:03Z

PR Summary

This PR makes changes to the Tk backend to prevent unnecessary recursion into the Tcl event loop, REPL hangs on figure close, and unnecessary draws on resize plus some extras.

Background

I have a project to explore structured concurrency as applied to GUI design. Specifically, I have written a tkinter "app" that hosts trio in guest mode, and then uses callbacks exclusively to launch async functions, which themselves are eventually scheduled to run using Tk.after_idle.

Changes

Without going in to detail, it is easy to wind up in a situation where FigureCanvasTkAgg.draw is called from an async function in such a way that it deadlocks the event loop. Fortunately, there are absolutely no legitimate reasons to invoke Tk.update or tk.update_idletasks while an event loop is running, so this PR removes them entirely.

Other changes are less directly related to my primary motivation but became apparent as I was diagnosing the deadlock issue.

Resize events call FigureCanvasTkAgg.draw in FigureCanvasTkAgg.resize unnecessarily, as FigureCanvasBase.resize_event calls FigureCanvasTk.draw_idle. This PR removes that line, so resizes will be drawn eventually, without hogging the event loop.

NavigationToolbar2Tk has a special destroy method to delete it's tkinter.StringVar attribute named message. It will be destroyed via Tk object tree (master=self) and GCd by Python (only referred to by other self attributes) without this, so this PR removes it outright.

FigureManagerTk has a destroy method that is completely ineffective and also fails to stop the Tk mainloop. This PR rewrites the method entirely using protocol("WM_DELETE_WINDOW",...) rather than bind("<Destroy>",...). (Cherry picked and merged to master in #17802)

FigureCanvasTk is missing methods start_event_loop and stop_event_loop and so relies on FigureCanvasBase pure Python event loop implementations. This PR reimplements them following the example of the Qt and wx backends. However, I can't find any test code or Matplotlib API functions that actually exercise these methods.

Summary

These changes have minor or no user facing effect unless somewhere Tk.after_idle schedules a callback that contains Tk.update_idletasks. Writing this summary, it seems the fix to FigureManagerTk may actually the most user-noticeable (except most users are probably using Qt.) Not knowing why Tk.update_idletasks was sprinkled about in the first place, it is hard to say if this is backward-incompatable; it should be fine. A test to assert that update or update_idletasks is never called when using the Tk backend would be appropriate, as well as a test of the FigureCanvasTk event loop methods, but I can't fathom how the backend tests are constructed, so some help would be appreciated.

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

richardsheridan · 2020-06-29T21:53:28Z

One of the update_idletasks calls was added in #9956, fixing a bug without diagnosing the bug origin. The actual issue was failing to make use of protocol. git blame puts the other insertions at 15-16 years ago!

tacaswell · 2020-07-03T19:41:15Z

To check my understanding, in the mainloop function at the module level we are using the c-event loop (because we grab managers[0].window.mainloop()?

tacaswell · 2020-07-03T20:00:47Z

On linux the code from #9856

%matplotlib tk
import sys
import matplotlib
import matplotlib as mpl
import pylab as plt

print('OS: %s'%sys.platform)
print('matplotlib version: %s'%mpl.__version__)
print('Backend: %s'%matplotlib.get_backend())

for i in range(3):
    plt.figure()
    plt.close()

plt.figure()
plt.plot([0,1],[0,1])
plt.show()

does not crash, but gives me

OS: linux
matplotlib version: 3.3.0rc1.post102+g69e5b9e4f8
Backend: TkAgg

In [3]: can't invoke "event" command: application has been destroyed                                                                                                 
    while executing
"event generate $w <<ThemeChanged>>"
    (procedure "ttk::ThemeChanged" line 6)
    invoked from within
"ttk::ThemeChanged"
can't invoke "event" command: application has been destroyed
    while executing
"event generate $w <<ThemeChanged>>"
    (procedure "ttk::ThemeChanged" line 6)
    invoked from within
"ttk::ThemeChanged"
can't invoke "event" command: application has been destroyed
    while executing
"event generate $w <<ThemeChanged>>"
    (procedure "ttk::ThemeChanged" line 6)
    invoked from within
"ttk::ThemeChanged"

when run from inside of IPython, but no warnings when run as a script which makes me think that the problem is actually coming from the prompt toolkit inputhook (see https://github.com/ipython/ipython/blob/master/IPython/terminal/pt_inputhooks/tk.py to send you down all of the tk related rabbit holes...)? Are we sure that this will not bring back that crash on the mac? On v3.2.2 I get no warnings, but see the closed windows flicker in and out of existence.

richardsheridan · 2020-07-03T20:33:08Z

To check my understanding, in the mainloop function at the module level we are using the c-event loop (because we grab managers[0].window.mainloop()?

That is correct, to my understanding. for consistency "mainloop" should be everywhere and always the _tkinter.tkapp.mainloop method eventually.

Are we sure that this will not bring back that crash on the mac?

Unfortunately, I don't have a mac to test on. Can we get a test going on CI?

#9856

I get no messages like that on windows from python repl or ipython. I do see the blinky root windows on ipython though!

tacaswell · 2020-07-03T20:41:18Z

I have access to a mac that I can test on, but down a rabbit hole of figuring out why plt.ion() in the plain-python prompt is broken in 3.3.0rc1 :(

tacaswell · 2020-07-03T22:08:01Z

Fortunately, the bug I thought I found is fixed on both 3.3.x and master as of #17764 !

tacaswell · 2020-07-03T22:16:15Z

I can run the code and verify that it does not crash but am having issues getting VNC to work so ~95% sure this is OK. If you are feeling motivated, I think another subprocess test would work here, if not I'll try to take care of it.

jklymak · 2020-07-15T22:53:50Z

@tacaswell, is this still OK?

QuLogic · 2020-08-06T04:21:50Z

FigureCanvasTk is missing methods start_event_loop and stop_event_loop and so relies on FigureCanvasBase pure Python event loop implementations. This PR reimplements them following the example of the Qt and wx backends. However, I can't find any test code or Matplotlib API functions that actually exercise these methods.

These should be exercised with plt.pause or plt.ginput. The former is covered by test_backends_interactive.py, though the latter you'd have to check yourself interactively, I think.

lib/matplotlib/tests/test_backends_interactive.py

QuLogic · 2020-08-06T04:31:29Z

I don't really understand the destroy bit, so if tests are working, it's probably fine. Everything else looks reasonable, though.

richardsheridan · 2020-08-06T12:02:33Z

FYI plt.ginput checks out after c1cffde. Also, for more on the effects of fixing FigureManager destruction, see this comment.

No need to draw if event loop is responsive

message will be destroyed via tk object tree (master=self) and GCd by Python (only referred to by other self attributes)

tkinter has a perfectly good event loop written in C

timeout is given in float seconds according to FigureCanvasBase, but tk needs int milliseconds

start_event_loop semantics are equivalent to update with timeout < 0.001

richardsheridan · 2020-08-08T21:50:31Z

@QuLogic I believe I have addressed your comment with this last commit, my rebase on master might have made that unclear!

QuLogic

Small test improvements, but LGTM.

lib/matplotlib/tests/test_backends_interactive.py

anntzer added the GUI: tk label Jun 29, 2020

richardsheridan mentioned this pull request Jul 1, 2020

fix FigureManagerTk close behavior if embedded in Tk App #17802

Merged

6 tasks

tacaswell added this to the v3.4.0 milestone Jul 3, 2020

tacaswell approved these changes Jul 3, 2020

View reviewed changes

jklymak requested a review from tacaswell July 15, 2020 22:53

QuLogic mentioned this pull request Aug 5, 2020

Wait cursor may cause Tk to print _get_renderer.Done backtrace #18186

Closed

QuLogic reviewed Aug 6, 2020

View reviewed changes

lib/matplotlib/tests/test_backends_interactive.py Outdated Show resolved Hide resolved

richardsheridan added 7 commits August 8, 2020 15:36

Library code should never call update_idletasks

8a8786f

resize_event eventually calls draw_idle

23b0edf

No need to draw if event loop is responsive

Unnecessary method override for del

db31323

message will be destroyed via tk object tree (master=self) and GCd by Python (only referred to by other self attributes)

override FigureCanvasTk start_event_loop and stop_event_loop

630e806

tkinter has a perfectly good event loop written in C

Test update is never called

c94c5f9

Correct start_event_loop behavior

c53570d

timeout is given in float seconds according to FigureCanvasBase, but tk needs int milliseconds

pause for a short time rather than sneaky update

880c6c5

start_event_loop semantics are equivalent to update with timeout < 0.001

richardsheridan force-pushed the tkagg_improvements branch from c1cffde to 880c6c5 Compare August 8, 2020 20:29

QuLogic approved these changes Aug 11, 2020

View reviewed changes

lib/matplotlib/tests/test_backends_interactive.py Outdated Show resolved Hide resolved

lib/matplotlib/tests/test_backends_interactive.py Outdated Show resolved Hide resolved

richardsheridan added 2 commits August 11, 2020 11:20

tighten and comment test_never_update

9bc82fb

linting

502dd11

tacaswell approved these changes Aug 11, 2020

View reviewed changes

tacaswell merged commit f71ec63 into matplotlib:master Aug 11, 2020

richardsheridan deleted the tkagg_improvements branch August 11, 2020 21:30

richardsheridan mentioned this pull request Aug 12, 2020

invalid command name "..._on_timer" in FuncAnimation for (too) small interval #8107

Closed

kngwyu mentioned this pull request Apr 11, 2021

Windows are not shown with Tk + interactive mode, from matplotlib 3.4 #19935

Open

richardsheridan mentioned this pull request Apr 11, 2021

Tkagg event loop throws error on window close #19940

Closed

richardsheridan mentioned this pull request Jun 23, 2021

Memory leaks on matplotlib 3.4.2 (and 3.4.0) #20490

Closed

richardsheridan mentioned this pull request May 1, 2025

[Bug]: unstable tkagg small plot size window #28330

Closed

Uh oh!

Tk backend improvements #17789

Tk backend improvements #17789

Uh oh!

Conversation

richardsheridan commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Background

Changes

Summary

PR Checklist

Uh oh!

richardsheridan commented Jun 29, 2020

Uh oh!

tacaswell commented Jul 3, 2020

Uh oh!

tacaswell commented Jul 3, 2020

Uh oh!

richardsheridan commented Jul 3, 2020

Uh oh!

tacaswell commented Jul 3, 2020

Uh oh!

tacaswell commented Jul 3, 2020

Uh oh!

tacaswell commented Jul 3, 2020

Uh oh!

jklymak commented Jul 15, 2020

Uh oh!

QuLogic commented Aug 6, 2020

Uh oh!

Uh oh!

QuLogic commented Aug 6, 2020

Uh oh!

richardsheridan commented Aug 6, 2020

Uh oh!

richardsheridan commented Aug 8, 2020

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

richardsheridan commented Jun 28, 2020 •

edited

Loading