Skip to content

Fix logging level type #10734

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 9, 2018
Merged

Fix logging level type #10734

merged 1 commit into from
Mar 9, 2018

Conversation

gregorybchris
Copy link
Contributor

@gregorybchris gregorybchris commented Mar 9, 2018 •
edited
Loading

PR Summary

When importing matplotlib version 2.2.0 a TypeError is raised (see trace below) upon initialization. This is caused by a string being passed as the level parameter to the standard python logger function log() when it should be an integer.

The fix was to change the contract of the _wrap() function so that the default value of the level parameter is logger.debug and not 'DEBUG'.

Traceback (most recent call last):
  File "run.py", line 1, in <module>
    import matplotlib.pyplot as plt
  File "/dir/venv/lib/python3.4/site-packages/matplotlib/__init__.py", line 1153, in <module>
    rcParams = rc_params()
  File "/dir/venv/lib/python3.4/site-packages/matplotlib/__init__.py", line 987, in rc_params
    fname = matplotlib_fname()
  File "/dir/venv/lib/python3.4/site-packages/matplotlib/__init__.py", line 831, in matplotlib_fname
    for fname in gen_candidates():
  File "/dir/venv/lib/python3.4/site-packages/matplotlib/__init__.py", line 828, in gen_candidates
    yield os.path.join(_get_configdir(), 'matplotlibrc')
  File "/dir/venv/lib/python3.4/site-packages/matplotlib/__init__.py", line 696, in _get_configdir
    return _get_config_or_cache_dir(_get_xdg_config_dir())
  File "/dir/venv/lib/python3.4/site-packages/matplotlib/__init__.py", line 620, in _get_xdg_config_dir
    path = get_home()
  File "/dir/venv/lib/python3.4/site-packages/matplotlib/__init__.py", line 400, in wrapper
    _log.log(lvl, fmt % ret)
  File "/Library/Frameworks/Python.framework/Versions/3.4/lib/python3.4/logging/__init__.py", line 1330, in log
    raise TypeError("level must be an integer")
TypeError: level must be an integer

PR Checklist

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

@jklymak
Copy link
Member

jklymak commented Mar 9, 2018

What is the minimal code that triggers the error?

@jklymak
Copy link
Member

jklymak commented Mar 9, 2018

And what are the contents of your matplotlibrc file? I suspect your file is out of date and maybe you need to change the rcparam?

@gregorybchris
Copy link
Contributor Author

gregorybchris commented Mar 9, 2018 •
edited
Loading

The minimal code that triggers the error for me is import matplotlib. All I have in my matplotlibrc file is backend: TkAgg. I do not have this issue when I revert to matplotlib version 2.1.2 and this change stopped the TypeError from occurring.

@jklymak
Copy link
Member

jklymak commented Mar 9, 2018

OK, well, _wrap takes a string argument.

lvl = logging.getLevelName(level.upper())
print(type(lvl))

returns integers: <class 'int'>

and all works as expected. Is it possibly an issue with python 3.4? I can't find the python 3.4 logging documentation on-line, but its possible that old versions of getLevelName don't return the int if you pass the string.

Python 2.1.2 did logging an entirely different way.

EDIT: OK

https://docs.python.org/3.4/library/logging.html

and indeed the meaning of getLevelName has sloshed around some in python 3.4. What version of 3.4 are you on?

@gregorybchris
Copy link
Contributor Author

Python 3.4.1. Installed with brew install python --framework.

@jklymak
Copy link
Member

jklymak commented Mar 9, 2018

Changed in version 3.4: In Python versions earlier than 3.4, this function could also be passed a text level, and would return the corresponding numeric value of the level. This undocumented behaviour was considered a mistake, and was removed in Python 3.4, but reinstated in 3.4.2 due to retain backward compatibility.

You have the one version of python that this won't work on...

@tacaswell @dopplershift @efiring whats the policy here? We could fix _wrap to support this version of python. Should we? The proposed fix in the PR is not quite correct, but something similar could be done.

@efiring
Copy link
Member

efiring commented Mar 9, 2018

I'm evidently missing something, because I don't see why the PR isn't correct--it looks right to me. And using "logging.DEBUG" as the argument makes sense to me, especially since it looks like the kwarg is never used with other than its default value, and _wrap is a very local private function.

@jklymak
Copy link
Member

jklymak commented Mar 9, 2018

@efiring that'd fine if no one else is using the private API. Given the number of people who tend to use our private API, I was hesitant to change the call signature of _wrap

@tacaswell tacaswell added this to the v2.2.1 milestone Mar 9, 2018
@tacaswell
Copy link
Member

We could put in a check that it is not python 3.4.1 and try to do the log look if the input is a string, but as @efiring says this is a private method.

I think this fix is fine bit a bit moot on master where we have dropped support for 3.4.

@jklymak
Copy link
Member

jklymak commented Mar 9, 2018

I'm fine with the change. But for some reason suddenly leery about API changes, even ones considered private 😉

@jklymak
Copy link
Member

jklymak commented Mar 9, 2018

I did want to figure out why this was breaking, because its not like Matplotlib is so poorly tested no one had run import matplotlib on the codebase...

@tacaswell
Copy link
Member

👍 on running exactly what was going on to ground!

@jklymak jklymak merged commit fbcfe76 into matplotlib:master Mar 9, 2018
@jklymak
Copy link
Member

jklymak commented Mar 9, 2018

Thanks @gregorybchris !

lumberbot-app bot pushed a commit that referenced this pull request Mar 9, 2018
@jklymak
Backport PR #10734: Fix logging level type
ba6ddcb
@lumberbot-app lumberbot-app bot mentioned this pull request Mar 9, 2018
Merged
jklymak added a commit that referenced this pull request Mar 9, 2018
@jklymak
Merge pull request #10743 from matplotlib/auto-backport-of-pr-10734
8a900fc 
Backport PR #10734 on branch v2.2.x
@zachkirsch
Copy link

+1 This was a huge blockade for our team, really happy to have this integrated in. Thank you @gregorybchris!!

@jklymak
Copy link
Member

jklymak commented Mar 10, 2018

Fair warning: MPL 3.0 will no longer actively support python 3.4. I think anyone who got caught by this bug is on either on 3.4.0 or 3.4.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.2.1
Development

Successfully merging this pull request may close these issues.
6 participants
@gregorybchris @jklymak @efiring @tacaswell @zachkirsch @tekknolagi