Skip to content

Do not rely on external stack frame to exist #12771

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 2 commits into from
Nov 8, 2018
Merged

Conversation

tkf
Copy link
Contributor

@tkf tkf commented Nov 8, 2018

PR Summary

When using matplotlib in an embedded Python interpreter (such as in Julia package PyCall.jl), stack frame may not exist outside matplotlib. Therefore, it is better to not assume frame.f_back to be an existing call frame.

See: JuliaPy/PyPlot.jl#409

PR Checklist

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

When using matplotlib in an embedded Python interpreter (such as in
Julia package PyCall.jl), stack frame may not exist outside
matplotlib.  Therefore, it is better to not assume `frame.f_back` to
be an existing call frame.

See: JuliaPy/PyPlot.jl#409
@@ -1990,6 +1990,8 @@ def _warn_external(message, category=None):
"""
frame = sys._getframe()
for stacklevel in itertools.count(1): # lgtm[py/unused-loop-variable]
if frame is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would suggest leaving a comment (as just below) so that people realize when this can happen.

Copy link
Member

Choose a reason for hiding this comment

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

I took the liberty of adding such a comment

@tacaswell tacaswell added this to the v3.0.x milestone Nov 8, 2018
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

modulo comment as to why

@anntzer anntzer merged commit c62080b into matplotlib:master Nov 8, 2018
@anntzer
Copy link
Contributor

anntzer commented Nov 8, 2018

thanks all!

@lumberbot-app
Copy link

lumberbot-app bot commented Nov 8, 2018

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout v3.0.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 c62080b46e18d8776747898ec715305126d79d04
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #12771: Do not rely on external stack frame to exist'
  1. Push to a named branch :
git push YOURFORK v3.0.x:auto-backport-of-pr-12771-on-v3.0.x
  1. Create a PR against branch v3.0.x, I would have named this PR:

"Backport PR #12771 on branch v3.0.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@tkf
Copy link
Contributor Author

tkf commented Nov 8, 2018

Thanks for the prompt review and merge!

tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Nov 8, 2018
…exist

Conflicts:
	lib/matplotlib/cbook/__init__.py
           - also backported lgtm noqa flag
timhoffm pushed a commit that referenced this pull request Nov 9, 2018
)

Conflicts:
	lib/matplotlib/cbook/__init__.py
           - also backported lgtm noqa flag
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.

4 participants