Skip to content

Warn when trying to start a GUI event loop out of the main thread. #15504

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

Merged
merged 3 commits into from
Feb 11, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 24, 2019

PR Summary

The first commit rearchitects a bit pyplot to make it possible to further wrap the backend-provided new_figure_manager().
The second commit uses that to emit a warning when new_figure_manager() (and thus figure()) is called for an GUI backend when not in the main loop. Note that the text of the warning is a placeholder, suggestions for the wording would be welcome.
Closes #12085, closes #14304.

For the record the main thread requirement is documented at
https://developer.gnome.org/gdk3/stable/gdk3-Threads.html
https://doc.qt.io/qt-5/thread-basics.html#gui-thread-and-worker-thread
http://effbot.org/zone/tkinter-threads.htm
https://docs.wxwidgets.org/3.0/overview_thread.html#overview_thread_notes

I chose to only emit a warning because 1) it looks like things actually work with gtk if you are extra careful? ("You should only use GTK+ and GDK from the thread gtk_init() and gtk_main() were called on. This is usually referred to as the “main thread”." -- I guess the "gtk main thread" is not necessarily the python main thread, and indeed things don't appear to crash immediately.), and 2) if things are going to crash anyways you'll have a traceback and it's not as if catching the exception would really help, as it's a program logic error.

Test e.g. with

from threading import Thread
from matplotlib.pyplot import show
Thread(target=lambda: show()).start()

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

@anntzer anntzer force-pushed the new_figure_in_main_thread branch from de4ad77 to b910442 Compare October 24, 2019 20:29
@anntzer anntzer force-pushed the new_figure_in_main_thread branch 2 times, most recently from 70c7b91 to c908015 Compare December 11, 2019 17:59
Backend = type(
"Backend", (matplotlib.backend_bases._Backend,), vars(backend_mod))

class _backend_mod(matplotlib.backend_bases._Backend):
Copy link
Member

Choose a reason for hiding this comment

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

maybe _backend_module for more clarity?

Also if you understand, what this really does, an explanatory comment would be helpful. Seems sort of a hybrid namespace for the module + class thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment to clarify.
I would prefer not changing _backend_mod, because in fact it should really be public: there should be a way to get a hold on "the canvas class of the current pyplot backend" without instantiating one. Right now if you look at e.g. the embedding_in_qt_sgskip.py example, you have something like

if is_pyqt5():
    from matplotlib.backends.backend_qt5agg import (
        FigureCanvas, NavigationToolbar2QT as NavigationToolbar)
else:
    from matplotlib.backends.backend_qt4agg import (
        FigureCanvas, NavigationToolbar2QT as NavigationToolbar)

and that won't even give you a qt5cairo/mplcairo.qt canvas even if your matplotlibrc is configured to do so; better would be something like

from matplotlib.pyplot import backend_mod  # name up to bikeshedding
FigureCanvas = backend_mod.FigureCanvas

Anyways, this can be a discussion for another PR.

Copy link
Member

Choose a reason for hiding this comment

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

The motivation here is to slowly move to using the _Backend sub-classes rather than the modules as the namespace?

It seems simpler to do _backend_mod = improtlib.import_module(...)?

If you are in the mode of embedding, you sholud be directly picking the backend, not relying on pyplot's settings. While it would be solve your mplcairo vs qt5agg vs qt5cairo problem, it would go very badly if the end-user was set up to use wxagg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I would ultimately like to make _Backend public, because there is really an inheritance-like relationship e.g. qt5agg inherits from qt5 + agg, before the introduction of _Backend this was implemented manually with a lot of redundant code whereas I would argue (well, I wrote _Backend so I'm perhaps biased here...) that things are now simpler by actually using real class inheritance.

_backend_mod = importlib.import_module(...) would not fill up missing methods in third-party backend modules (the pre-_Backend code did fill them), that's what inheritance does for you.

Re embedding: I can always start by creating a QApplication() early to prevent the other backends from being loaded (at worst I'll error out more or less cleanly). One motivation is that I want my GUIs to use mplcairo(.qt) when I can (because, well, I think mplcairo is awesome :-) but again I'm a bit biased...) but there's typically no need to actually have a dependency on mplcairo and I'm happy to fall back to qt5agg if that's all there is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case, this PR is fairly unrelated.

Copy link
Member

Choose a reason for hiding this comment

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

Having backends as classes instead of modules makes sense to me.

@anntzer anntzer force-pushed the new_figure_in_main_thread branch from c908015 to c25dc9d Compare December 13, 2019 13:34
@anntzer
Copy link
Contributor Author

anntzer commented Dec 13, 2019

test failures seem unrelated

@timhoffm
Copy link
Member

Indeed, test failures seem unrelated. Nevertheless we should make sure they are not a new persistent issue. Restarted travis to see if they are gone now. Newer PRs do not seem to suffer from the same issue.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

This PR does not change the module/class hybrid that backends are right now. It only reformulates how that's cobbled together.

While I'd be in favor of not hiding this reorganization under the innocent PR title "Warn when trying to start a GUI event loop out of the main thread", at least it's two separate commits with appropriate messages.

Backend = type(
"Backend", (matplotlib.backend_bases._Backend,), vars(backend_mod))

class _backend_mod(matplotlib.backend_bases._Backend):
Copy link
Member

Choose a reason for hiding this comment

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

Having backends as classes instead of modules makes sense to me.

@timhoffm timhoffm added this to the v3.3.0 milestone Dec 15, 2019
@anntzer
Copy link
Contributor Author

anntzer commented Dec 15, 2019

The problem is that I don't think the first commit really stands by itself -- it is only motivated by the needs of the second commit (if I PR'd the first commit by itself you could legitimately wonder what's its point).

@timhoffm
Copy link
Member

CI problem persists. Can you try to rebase?

@anntzer anntzer force-pushed the new_figure_in_main_thread branch from c25dc9d to 3c16d20 Compare December 15, 2019 13:48
@anntzer
Copy link
Contributor Author

anntzer commented Dec 15, 2019

Rebased, pushed a temporary commit to try to debug the failure remotely.

Edit: failure looks real... investigating.

@anntzer anntzer force-pushed the new_figure_in_main_thread branch 2 times, most recently from 390529c to cd4be8c Compare December 15, 2019 16:01
@anntzer anntzer force-pushed the new_figure_in_main_thread branch 3 times, most recently from 1cf8ddc to 266c143 Compare December 15, 2019 22:30
@anntzer
Copy link
Contributor Author

anntzer commented Dec 15, 2019

Fixed. The tricky point was that I should not modify the global _backend_mod instance before having checked that the new backend's interactive framework is indeed consistent with the currently running one.

@anntzer anntzer force-pushed the new_figure_in_main_thread branch from 266c143 to e460321 Compare February 11, 2020 10:00
# This function's signature is rewritten upon backend-load by switch_backend.
def new_figure_manager(*args, **kwargs):
"""Create a new figure manager instance."""
global _backend_mod
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the global here and in then next 2 functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought yes to placate flake8 similarly to the global _show in the old show() (see #10689), but apparently pyflakes got smarter since then, so removing them.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

modulo my question about the (extra?) globals.

@anntzer anntzer force-pushed the new_figure_in_main_thread branch from e460321 to ad90166 Compare February 11, 2020 14:49
@timhoffm timhoffm merged commit aaf140e into matplotlib:master Feb 11, 2020
@anntzer anntzer deleted the new_figure_in_main_thread branch February 11, 2020 20:42
@timhoffm timhoffm mentioned this pull request Jul 14, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants