-
-
Notifications
You must be signed in to change notification settings - Fork 8k
FIX: pyplot auto-backend detection case-sensitivity fixup #29721
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
FIX: pyplot auto-backend detection case-sensitivity fixup #29721
Conversation
Thinking about how to add test coverage here: I think that modifying/extending the test_backend_fallback_headless
and test_backend_fallback_headful
test cases might be a reasonable way to demonstrate the problem and/or fix:
matplotlib/lib/matplotlib/tests/test_rcparams.py
Lines 523 to 563 in b37dbe9
It's possible to re-order some of the import
and get_backend
code lines in test_backend_fallback_headful
to get the test to fail without this fix and pass with it in place -- but I'm not completely certain what the expected logic should be, so I'll open this PR for review/feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I was wondering whether this is the right case to handle the case. Alternatives could be
validate_backend()
coercing to lower-case. Seematplotlib/lib/matplotlib/rcsetup.py
Line 280 in b37dbe9
def validate_backend(s):
While we could do this, the BackendRegistry generally accepts arbitrary casing and coerces internally. Therefore, it may be slightly more consistent to keep the name as is in rcParams, so that the user always sees the name as given: They may usematplotlib.use("TkAgg")
and set "TkAgg" in theirmatplotlibrc
file. Then it's slightly awkward to return the lower-case variant forrcParams["backend_fallback"]
- though one could still argue that eager normalization is still the way to go. I have no strong opinion here.- Push everything down to the BackendRegistry. While this would be nice in principle, we have a very specific request here: "Is the given backend name an available interactive backend, but not webagg or nbagg" and that's not easy to express as a high-level BackendRegistry API that could support arbitrary casing.
I therefore think the present solution is right.
@nileshpatra can you confirm that this fixup (f45707d) resolves the problems you encountered during unit tests for matplotlib
-dependent packages? I think it should do, but it would be good to make sure.
Thanks @jayaddison, this is a bug that I introduced and the fix here looks good to me. I think you are looking in the right place for a test, but it might be a little awkward due to the subprocess approach.
Thank you @ianthomas23 @timhoffm for the code reviews. Also, I think I've figured out what could be some viable test coverage:
Given that test_backend_fallback_headless
tests a negative/failure case (the ability to open a tkagg
output when headless mode is active is expected to cause a failure), I think that test coverage for this change should add a second headless test that presents a successful rendering case. And to do that, it should be possible I think to import matplotlib.pyplot
followed by creating a plot directly (no matplotlib.use
/ other imports required).
I've tested that theory in a shell with and without the fix in place and it seems to behave as expected; I'll try adding it to the test suite in a few moments.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Can we squash this to one or two commits?
Co-authored-by: Ian Thomas <ianthomas23@gmail.com>
b07d9c6
to
2ed08c1
Compare
nileshpatra
commented
Mar 13, 2025
via email
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jayaddison. CI failure is unrelated.
309d380
into
matplotlib:main
...-sensitivity fixup
Thank you @ianthomas23!
...721-on-v3.10.x Backport PR #29721 on branch v3.10.x (FIX: pyplot auto-backend detection case-sensitivity fixup)
Uh oh!
There was an error while loading. Please reload this page.
PR summary
There seems to be a case-sensitivity bug in the
matplotlib.pyplot
module code that decides whether to configure backend autodetection, and I think that this is the cause of the bug reported in #29713.The bug means that a current interactive environment value of
TkAgg
is not detected as one of the possible values from a set that includes the stringtkagg
-- and so an auto-backend is not configured, as it would have been previously in v3.8.3 of this library.A minimal repro example is provided below, for use in a container environment with no graphical/GUI environment available:
PR checklist
(削除) Plotting related features are demonstrated in an example (削除ここまで)N/A(削除) New Features and API Changes are noted with a directive and release note (削除ここまで)N/A(削除) Documentation complies with general and docstring guidelines (削除ここまで)N/ACloses #29713.