-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
MNT: let bad rcParam keys pass #19588
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
Conversation
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.
I'm undecided if a configuration MPLBISECT_MODE=1
as proposed here is needed or if the warning is sufficient.
The source if rcParamsDefault
is cbook._get_data_path("matplotlibrc")
. Am I right that we control that file and it's not user configurable. Then, erroring out with undefined keys serves the purpose that we don't carry around inconsistent state in the repo. Warnings can be overlooked. Otherwise, just warning would be ok.
lib/matplotlib/__init__.py
Outdated
if key == 'backend': | ||
defaultParams['backend'] = rcsetup._auto_backend_sentinel | ||
elif key in rcParamsDefault.keys(): | ||
defaultParams[key] = validator |
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.
I'm a bit confused that before defaultParams
was a mapping key: [default_value, validator]
and now it's key: validator
.
Both don't make sense to me. I'd expect key: default_value
. But maybe that's just my before-first-coffee brain.
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.
Ooops, well, it is certainly a mystery to me what this is even supposed to do if this big error didn't kill the test suite. A search in the repo turns up defaultParams
only gets used here, and nowhere else. Removing this whole block of code leads to no changes so far as I can tell. So I am completely mystified why this code even exists.
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.
Just pushed a new commit that cuts it all. So far tests are fine. So again, what is this supposed to do (he sheepishly asks of a PR change he approved #15029)
I think I would rather have something like: if loading the default matplotlibrc fails, then check whether matplotlibrc.template exists in the directory twice above |
I'm marking release critical because #15029 has not been released yet, and I think this would be good to sort out. But apologies in advance if I'm just missing something obvious. |
So this only checks consistency between the template, and the rcParams (and spits out I'd argue this should not be done every time the matplotlib starts up, but rather should be a test, since inconsistency between |
So all this is doing is testing, every time we load matplotlib, that the installed version of the template in |
The check went in a few years ago when someone noticed that the template
wasn't getting updated appropriately, and we got complaints from users who
took the template and produced incorrect matplotlibrc files. I think you
are right that this should be moved to a unit test instead of living as an
import-time check.
…On Fri, Feb 26, 2021 at 11:33 AM Jody Klymak ***@***.***> wrote:
So all this is doing is testing, every time we load matplotlib, that the
installed version of the template in mpl-data is the same as what is
hardcoded in rcsetup.py? a) are we protecting against the user manually
mucking with this? Why? b) if we want to give the user a way to make a
matplotlibrc.template shouldn't we just give them a function to do that?
Why do we have mpl-data/matplotlibrc at all?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#19588 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACHF6G3QUPI6OMLIKQAFL3TA7EOLANCNFSM4YH2JYIA>
.
|
OK, that makes sense. But I think we now make the template during Edit: confirmed, |
43f16ae
to
2fdb7df
Compare
The pastIf you look at #15029, you can see what is going on:
The present
The problemThis is a messy but carefully tuned setup that works for production. The bisect problem #19578 comes from the fact that the two carefully synced sources have different update mechanisms. Validators are immediately updated as you walk through the history. But defaults are only updated when you run The futureis left as an exercise to the able reader. 😄 Hint: It's probably not feasible/desired to unite the two sources. If that's the case, you need to be able to get consistent defaults at startup, which may mean we need a flag that one has to enable for bisecting, which generates matplotlibrc at startup. |
Thanks @timhoffm I think I understand now: We have
I still feel, however, that there is no need to check the consistency of |
1abe137
to
e2a21aa
Compare
Added a test that |
fd5996a
to
88b2d1f
Compare
88b2d1f
to
1a46563
Compare
Thanks @timhoffm for the long writeup :-) diff --git i/lib/matplotlib/__init__.py w/lib/matplotlib/__init__.py
index c3d4aaf62..5cf679ad5 100644
--- i/lib/matplotlib/__init__.py
+++ w/lib/matplotlib/__init__.py
@@ -823,6 +823,17 @@ rcParamsDefault = _rc_params_in_file(
transform=lambda line: line[1:] if line.startswith("#") else line,
fail_on_error=True)
dict.update(rcParamsDefault, rcsetup._hardcoded_defaults)
+if {*rcParamsDefault} ^ {*rcsetup._validators}:
+ # This should only occur e.g. when bisecting on a dev install when setup.py
+ # hasn't been run to update mpl-data/matplotlibrc.
+ _repo_template = (Path(__file__).parents[2] / "matplotlibrc.template")
+ if _repo_template.exists(): # Do we want to update the template here?
+ rcParamsDefault = _rc_params_in_file(
+ _repo_template,
+ # Strip leading comment.
+ transform=lambda line: line[1:] if line.startswith("#") else line,
+ fail_on_error=True)
+ dict.update(rcParamsDefault, rcsetup._hardcoded_defaults)
rcParams = RcParams() # The global instance.
dict.update(rcParams, dict.items(rcParamsDefault))
dict.update(rcParams, _rc_params_in_file(matplotlib_fname())) |
@anntzer, I'm not following the point of this. If the user has broken their Maybe an idea is to hide a copy of The other idea would be to wrap the config template in a python file, and or make |
Here I am specifically talking about the case of bisecting on editable installs, because that's a case that occurs relatively often for devs (I have noticed the problem too). If a plain end-user install is broken, then we should just error out -- mpl-data/matplotlibrc being messed up on a plain install should be treated just as if an end-user edits and breaks any .py file in their matplotlib install, we shouldn't try to handle that. For editable installs, mpl-data/matplotlibrc can (as you noticed) easily get broken nowadays if you change your local repo HEAD and don't run setup.py to regen it from the toplevel matplotlibrc.template. On the other hand, the toplevel matplotlibrc.template should always be in sync with the rest of the repo state (because any change to e.g. rcParams.validators should be commited simultaneously with a change to matplotlibrc.template in order to make the tests pass -- if they are not in sync, then CI will not pass). So matplotlibrc.template should always be correct. |
Right, so why don't we load the defaults from there for everyone? Why does |
Because matplotlibrc.template doesn't exist for user installs? (where would it be for a matplotlib installed in someone's site-packages?) |
Ok, then conversely why don't we version control |
I would be in favor of killing matplotlibrc.template and having just mpl-data/matplotlibrc. However, I think in #14929 there were some concerns about people relying on the template as documentation (see e.g. #14929 (comment)). Also, we allow redistributors to change the default backend (via setup.cfg, see #11844) which means that the file needs to be modified to support that -- although that's admittedly fixable by doing slightly more nasty things in setup.py to change the file in the wheel. |
For documentation consistency we could easily just softlink I don't understand wheel building well enough - is the idea that
and it seems that could easily be done in-place. |
See #19602 for that approach |
soft-links will be a problem on Windows (they can be made to work but ugh); updating matplotlibrc in place directly means your repo will be in a dirty state which may or may not confuse versioneer. See #19603. |
I don't think the softlink is for windows in particular, but so anyone who was grabbing |
Closing for #19603. |
PR Summary
Close #19578
I don't see why we would fail to import if the rcdefault and the rcParam dicts don't exactly match. It definitely is a pain for developers who need to bisect. But I frankly don't follow why we have all this machinery at all, so no doubt I am missing a subtlety...EDIT: As pointed out by @timhoffm the issue is that
/matplotlibrc.template
can get out of sync withrcdefaults._validators
.This is confusing because we were not checking against
/matplotlibrc.template
, but rather/lib/matplotlib.mpl-data/matplotlibrc
which gets copied from/matplotlibrc.template
whensetup.py
is called.This PR moves the check from being done on
import matplotlib
to a test that is run by CI.PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).