FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

FIX: do not let no-op monkey patches to renderer leak out by tacaswell · Pull Request #17560 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

FIX: do not let no-op monkey patches to renderer leak out - #17560

Merged
QuLogic merged 2 commits into
matplotlib:masterfrom
tacaswell:fix_noop_tight_bbox
Jun 9, 2020
ConversationCommits (2)ChecksFiles changed
Merged

FIX: do not let no-op monkey patches to renderer leak out#17560
QuLogic merged 2 commits into
matplotlib:masterfrom
tacaswell:fix_noop_tight_bbox

Conversation

tacaswell commented Jun 3, 2020

Copy link
Copy Markdown
Member

PR Summary

closes #17542

This issue is that in some cases the no-op monkey patched renderer was getting cached deep in the backend code and the no-op functions were getting used for the actual save which resulted in empty figures. This moves the monkey patching to the call sites so we can use the _setattr_cm to manage this.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • [NA] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way (internal api only)

tacaswell added this to the v3.2.2 milestone Jun 3, 2020
tacaswell requested a review from anntzer June 3, 2020 20:32
tacaswell force-pushed the fix_noop_tight_bbox branch from 91e2fd8 to 297cf79 Compare June 3, 2020 20:58
anntzer reviewed Jun 3, 2020
View reviewed changes

anntzer left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks fine, but perhaps this should instead be a method (private or public) on the RendererBase class?

def _draw_disabled(self):
    return _setattr_cm(**{...})

so that you can just write

with renderer._draw_disabled(): ...

saving on the duplication? Also this way, at least in theory, Renderer subclasses could customize that behavior as needed (effectively bringing back a dryrun-like functionality (#14134), but on a strictly optional basis).

tacaswell mentioned this pull request Jun 4, 2020
Closed
2 tasks done

tacaswell commented Jun 4, 2020

Copy link
Copy Markdown
Member Author

I'm 50/50 on moving it to the base class.

On one hand I agree it is much neater.

On the other hand, how strongly do we require the renderers to actually be sub-classes? I would say that adding a new method to the semi-public API of the renderer is something we should not do in a patch release and we probably should backport this to 3.2.2 to fix the empty figure bug.

anntzer commented Jun 4, 2020

Copy link
Copy Markdown
Contributor

We basically do per #17159.
If you really want to be super safe we can add an isinstance check and if the renderer subclass is not a RendererBase, then patch the method onto it, or even just make that a free private function in backend_bases (but then subclasses can't override it).

tacaswell commented Jun 8, 2020

Copy link
Copy Markdown
Member Author

Will add helper function.

tacaswell commented Jun 8, 2020

Copy link
Copy Markdown
Member Author

I made it forgiving for now (to backport to 3.2.x) but I'll open a follow-on PR to make it warn for 3.3.

tacaswell force-pushed the fix_noop_tight_bbox branch from 8698d15 to 7a412cd Compare June 9, 2020 02:43
anntzer reviewed Jun 9, 2020
View reviewed changes
Comment thread lib/matplotlib/backend_bases.py Outdated
anntzer approved these changes Jun 9, 2020
View reviewed changes

anntzer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

doc needs update (so approval conditional on that) but otherwise looks good

MNT: consolidate the no-op logic into a RenderBase method
f777177 
Be forgiving about renderer instances that do not inherit from
RendereBase.
tacaswell force-pushed the fix_noop_tight_bbox branch from 7a412cd to f777177 Compare June 9, 2020 16:08
QuLogic merged commit f820c27 into matplotlib:master Jun 9, 2020

lumberbot-app Bot commented Jun 9, 2020

Copy link
Copy Markdown

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.2.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 f820c27323146c81e29b1c5e12fb506f5405497d
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #17560: FIX: do not let no-op monkey patches to renderer leak out'
  1. Push to a named branch :
git push YOURFORK v3.2.x:auto-backport-of-pr-17560-on-v3.2.x
  1. Create a PR against branch v3.2.x, I would have named this PR:

"Backport PR #17560 on branch v3.2.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.

tacaswell deleted the fix_noop_tight_bbox branch June 10, 2020 02:21
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Jun 10, 2020
Backport PR matplotlib#17560: FIX: do not let no-op monkey patches to…
e40d9af 
… renderer leak out

Merge pull request matplotlib#17560 from tacaswell/fix_noop_tight_bbox

FIX: do not let no-op monkey patches to renderer leak out

 Conflicts:
	lib/matplotlib/backend_bases.py
          - trivial conflicting imports
          - did not back port other changes to tight box
	lib/matplotlib/tight_layout.py
          - ended up not backporting any of the changes
tacaswell mentioned this pull request Jun 10, 2020
Merged
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Jun 10, 2020
Backport PR matplotlib#17560: FIX: do not let no-op monkey patches to…
5a91618 
… renderer leak out

Merge pull request matplotlib#17560 from tacaswell/fix_noop_tight_bbox

FIX: do not let no-op monkey patches to renderer leak out

 Conflicts:
	lib/matplotlib/backend_bases.py
          - trivial conflicting imports
          - did not back port other changes to tight box
	lib/matplotlib/tight_layout.py
          - ended up not backporting any of the changes
QuLogic added a commit that referenced this pull request Jun 10, 2020
Merge pull request #17605 from tacaswell/auto-backport-of-pr-17560-on…
889c73f 
…-v3.2.x

Backport PR #17560: FIX: do not let no-op monkey patches to renderer …
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

QuLogic QuLogic approved these changes
anntzer anntzer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.2.2

Development

Successfully merging this pull request may close these issues.

Matplotlib 3.2.1 savefig empty image when fig size matches data size exactly

3 participants

Back | FazBrowse Home | New Git URL