Further defer backend selection #22005
Conversation
b57b94e to
95a2566
Compare
|
It helps to get the polarity of the test correct on filtering. |
|
My guess on the failing tk tests is that because the tk modules are no longer eagerly imported on pyplot import there is something missing with our internal module. However I do not understand why it works on linux and appveyor. |
729d021 to
e6a9e4d
Compare
|
Do we know how to get coverage data on the code we ship off to sub-processes this way? Is this too big for a bug-fix? |
|
In theory things should be automatically covered (the other tests contribute to coverage). |
|
I'm worried about the coverage on the test code (which @dopplershift has made the compelling case should always be ~100%). |
|
You can configure coverage.py to run coverage for subprocess. And while I never get matplotlib's suite to 100% lines of code for tests (because we have some non-deterministic behavior e.g. repeatedly running stuff until no error), it really is a nice check to have. There's no reason to have test code that's not run. |
|
But we are not running subprocces on a file, we are running it on a bunch of code that we (maybe too cleverly) read out of a function we define inline and then run with I guess the right thing to do there make a new directory and toss off the the inline stuff there. It is a pity to lose the readability win of the actual test-code being in-line for the tooling, but not having the computer tell us if we have coverage on these lines is a bit worrying (as it would be easy to break the subprocess tests in some way and never have them run and we would not know). |
|
I'm not following why running with
import coverage
coverage.process_startup() |
|
Doh, I get it now. The lines can't be matched up to the original lines of code. Hrmm... |
|
I see how to write the test for the warning, but out of time right now and want some feedback on if this is actually useful. The formatted warning looks like: |
|
At least locally, I am semi-magically seeing coverage from the sub-process without doing anything extra. I assume this is due to pytest-cov being helpful? This PR got a bit out of control and I ended up tweaking the tk tests, but we should be getting coverage on them now as well! I still need to finish writing the cross Qt major version test. |
|
I probably should squash this to a smaller number of commits, but I think this is otherwise good to go! |
ee7d467 to
4a2f554
Compare
lib/matplotlib/backends/qt_compat.py
Outdated
| if QT_API_ENV in ["pyqt5", "pyside2"]: | ||
| QT_API = _ETS[QT_API_ENV] | ||
| else: | ||
| _QT_MODE = 5 # noqa |
There was a problem hiding this comment.
It will otherwise complain about overwriting an unused import.
d3f83b3 to
bbbe8d2
Compare
ba24b8b to
3af4be1
Compare
This is to prevent the early importing of GUI bindings in the case where the user wants to use something later in our search list, but also has a toolkit higher in our list installed which we select. Thanks to @anntzer for the implementation suggestion.
Because the code in qt_compat tries qt6 bindings first, backend_qt supports both Qt5 and Qt6, and the qt5 named backends are shims to the generic Qt backend, if you imported matplotlib.backends.backend_qt5agg, matplotlib.backends.backend_qt5cairo, or matplotlib.backends.backend_qt5, and 1. had PyQt6 or pyside6 installed 2. had not previously imported a Qt5 binding Then you will end up with a backend that (by name) claims to be Qt5, but will be using Qt6 bindings. If you then subsequently import a Qt6 binding and try to embed the canvas it will fail (due to being Qt6 objects not Qt5 objects!). Additional changes to qt_compat that only matters if 1. rcparams['backend'] is set to qt5agg or qt5agg 2. QT_API env is not set 3. the user directly import matplotlib.backends.qt_compat This will likely only affect users who are using Matplotlib as an qt-shim implementation. closes matplotlib#21998 Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
66b03ad to
efc7f81
Compare
By putting the implementation in top-level functions and then importing the test module in the sub-process we are able to get accurate coverage on these tests. pytest-cov takes care of all of the coverage related magic implicitly. Also get coverage information out of isolated tk tests. Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
|
Sigh, I'll do the backport. |
Merge pull request matplotlib#22005 from tacaswell/further_defer_backend_selection Further defer backend selection
Merge pull request matplotlib#22005 from tacaswell/further_defer_backend_selection Further defer backend selection
…-v3.5.x Backport PR #22005: Further defer backend selection
closes matplotlib#23042 In matplotlib#22005 we change `pyplot` so that at import time we do not force a switch of the backend and install the repl displayhook. However, this meant that in some cases (primarily through `ipython --pylab`) to end up with a session where `install_repl_displayhook` had never been called.
closes matplotlib#23042 In matplotlib#22005 we change `pyplot` so that at import time we do not force a switch of the backend and install the repl displayhook. However, this meant that in some cases (primarily through `ipython --pylab`) to end up with a session where `install_repl_displayhook` had never been called.
closes matplotlib#23042 In matplotlib#22005 we change `pyplot` so that at import time we do not force a switch of the backend and install the repl displayhook. However, this meant that in some cases (primarily through `ipython --pylab`) to end up with a session where `install_repl_displayhook` had never been called.
closes matplotlib#23042 In matplotlib#22005 we change `pyplot` so that at import time we do not force a switch of the backend and install the repl displayhook. However, this meant that in some cases (primarily through `ipython --pylab`) to end up with a session where `install_repl_displayhook` had never been called. Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
closes matplotlib#23042 In matplotlib#22005 we change `pyplot` so that at import time we do not force a switch of the backend and install the repl displayhook. However, this meant that in some cases (primarily through `ipython --pylab`) to end up with a session where `install_repl_displayhook` had never been called. Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
PR Summary
This PR does two things (that maybe should be broken up)
In both cases the underlying bug is that if a Qt6 binding is installed there is no way (other than pre-importing it!) to force us to use the Qt5 bindings (in my case it was getting hit by the automatic backend resolution on pyplot import and in the linked issue due to a direct import of
backend_qt5!).PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
New features are documented, with examples if plot related.New features have an entry indoc/users/next_whats_new/(follow instructions in README.rst there).API changes documented indoc/api/next_api_changes/(follow instructions in README.rst there).Documentation is sphinx and numpydoc compliant (the docs should [build](https://matplotlib.org/devel/documenting_mpl.html#building-the-docs) without error).