Skip to content

Automagically set the stacklevel on warnings. #11298

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

Merged
merged 1 commit into from
May 26, 2018

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented May 24, 2018

There are many places in Matplotlib where it is impossible to set a
static stacklevel on warnings that works in all cases, because a same
function may either be called directly or via some wrapper (e.g.
pyplot).

Instead, compute the stacklevel by walking the stack. Given that
warnings refer to conditions that should, well, be avoided, I believe
we don't need to worry too much about the runtime cost.

As an example, use this mechanism for the "ambiguous second argument to
plot" warning. Now both

plt.gca().plot("x", "y", data={"x": 1, "y": 2})

and

plt.plot("x", "y", data={"x": 1, "y": 2})

emit a warning that refers to the relevant line in the user source,
whereas previously the second snippet would refer to the pyplot wrapper,
which is irrelevant to the user.

Note that this only works from source, not from IPython, as the latter
uses exec and AFAICT there is no value of stacklevel that correctly
refers to the string being exec'd.

Of course, the idea would be to ultimately use this throughout the
codebase.

PR Summary

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

@anntzer anntzer added this to the v3.0 milestone May 24, 2018
@jklymak
Copy link
Member

jklymak commented May 24, 2018

This looks like a good idea to me, particularly for pyplot....

@timhoffm
Copy link
Member

Great idea! 😄

While we're at it: Can we make that behavior configurable? While a user is not interested in the internals of matplotlib, it is really helpful for debugging purposes to see where a warning was really generated.

@anntzer
Copy link
Contributor Author

anntzer commented May 24, 2018

What kind of config do you propose?
The knob should (IMO) be a private one, e.g. a private rcparam ("dev only, dragons be here" (or do you want to expose it advanced users in general?)), but what value of stacklevel do you want? We could just always make it 2, which is slightly worse than the currently handcrafted stacklevels (because the plan again is to use this thoughout the codebase) but probably OK in general...

Note that you can already do (as of this PR)

cbook._warn.__defaults__ = (None, 2)

to achieve that :-)

Edit: Also, you can always set the warnings filter to error to throw the warning as an exception and get a traceback.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, but I didn't pull and test...

@timhoffm
Copy link
Member

@anntzer Stacklevel override:
Maybe we can just attach a parameter to cbook._warn:

cbook._warn.stacklevel_override = None
"""For debugging purposes. Set to a fixed integer value to override the used stacklevel.""" 

That's a bit more safe than messing with __defaults__.

@anntzer anntzer force-pushed the autostacklevel branch 2 times, most recently from 40369b5 to 371f801 Compare May 26, 2018 08:59
@anntzer
Copy link
Contributor Author

anntzer commented May 26, 2018

I got rid of the stacklevel argument as it literally goes against the point of the function, and renamed it to _warn_external, which is perhaps a bit more explicit (if you want to use stacklevel, just use warnings.warn, again there's no point of keeping around half-incorrect stacklevels at each call site).

Honestly, given that the thing is private, I think if one wants to know where the warning actually comes from, one should just do cbook._warn_external = warnings.warn (or -Werror) instead of going in roundabout ways. (Of course the argument would not apply if it was a public API.)

@timhoffm
Copy link
Member

+1 for _warn_external.

A final request: Can you add a note to the docstring:

"If you need the full stacktrace for debugging purposes, you can do do cbook._warn_external = warnings.warn."

Otherwise it may not be obvious how to get the additional info. Also it states the intent that the function signature should stay compatible with warnings.warn.

@anntzer
Copy link
Contributor Author

anntzer commented May 26, 2018

done

There are many places in Matplotlib where it is impossible to set a
static stacklevel on warnings that works in all cases, because a same
function may either be called directly or via some wrapper (e.g.
pyplot).

Instead, compute the stacklevel by walking the stack.  Given that
warnings refer to conditions that should, well, be avoided, I believe
we don't need to worry too much about the runtime cost.

As an example, use this mechanism for the "ambiguous second argument to
plot" warning.  Now both
```
plt.gca().plot("x", "y", data={"x": 1, "y": 2})
```
and
```
plt.plot("x", "y", data={"x": 1, "y": 2})
```
emit a warning that refers to the relevant line in the user source,
whereas previously the second snippet would refer to the pyplot wrapper,
which is irrelevant to the user.

Note that this only works from source, not from IPython, as the latter
uses `exec` and AFAICT there is no value of stacklevel that correctly
refers to the string being exec'd.

Of course, the idea would be to ultimately use this throughout the
codebase.
@timhoffm timhoffm merged commit f481a6e into matplotlib:master May 26, 2018
@anntzer anntzer deleted the autostacklevel branch May 26, 2018 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants