Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

anntzer
Copy link
Contributor

@anntzeranntzer commentedOct 24, 2019
edited
Loading

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

fromthreadingimportThreadfrommatplotlib.pyplotimportshowThread(target=lambda:show()).start()

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 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

timforr reacted with thumbs up emoji
@anntzeranntzerforce-pushed thenew_figure_in_main_thread branch fromde4ad77 tob910442CompareOctober 24, 2019 20:29
@anntzeranntzerforce-pushed thenew_figure_in_main_thread branch 2 times, most recently from70c7b91 toc908015CompareDecember 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
ContributorAuthor

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

ifis_pyqt5():frommatplotlib.backends.backend_qt5aggimport (FigureCanvas,NavigationToolbar2QTasNavigationToolbar)else:frommatplotlib.backends.backend_qt4aggimport (FigureCanvas,NavigationToolbar2QTasNavigationToolbar)

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

frommatplotlib.pyplotimportbackend_mod# name up to bikesheddingFigureCanvas=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
ContributorAuthor

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
ContributorAuthor

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.

@anntzeranntzerforce-pushed thenew_figure_in_main_thread branch fromc908015 toc25dc9dCompareDecember 13, 2019 13:34
@anntzer
Copy link
ContributorAuthor

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

@timhoffmtimhoffm 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.

@timhoffmtimhoffm added this to thev3.3.0 milestoneDec 15, 2019
@anntzer
Copy link
ContributorAuthor

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?

@anntzeranntzerforce-pushed thenew_figure_in_main_thread branch fromc25dc9d to3c16d20CompareDecember 15, 2019 13:48
@anntzer
Copy link
ContributorAuthor

anntzer commentedDec 15, 2019
edited
Loading

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

Edit: failure looks real... investigating.

@anntzeranntzerforce-pushed thenew_figure_in_main_thread branch 2 times, most recently from390529c tocd4be8cCompareDecember 15, 2019 16:01
@anntzeranntzerforce-pushed thenew_figure_in_main_thread branch 3 times, most recently from1cf8ddc to266c143CompareDecember 15, 2019 22:30
@anntzer
Copy link
ContributorAuthor

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.

@anntzeranntzerforce-pushed thenew_figure_in_main_thread branch from266c143 toe460321CompareFebruary 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 theglobal here and in then next 2 functions?

Copy link
ContributorAuthor

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 theglobal _show in the old show() (see#10689), but apparently pyflakes got smarter since then, so removing them.

Copy link
Member

@tacaswelltacaswell 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.

@anntzeranntzerforce-pushed thenew_figure_in_main_thread branch frome460321 toad90166CompareFebruary 11, 2020 14:49
@timhoffmtimhoffm merged commitaaf140e intomatplotlib:masterFeb 11, 2020
@anntzeranntzer deleted the new_figure_in_main_thread branchFebruary 11, 2020 20:42
@timhoffmtimhoffm mentioned this pull requestJul 14, 2020
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

'NSWindow drag regions should only be invalidated on the Main Thread!' - macos/python Tcl_AsyncDelete: async handler deleted by the wrong thread
3 participants
@anntzer@timhoffm@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp