Skip to content

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

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
Jun 9, 2020

Conversation

tacaswell
Copy link
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 tacaswell added this to the v3.2.2 milestone Jun 3, 2020
@tacaswell tacaswell requested a review from anntzer June 3, 2020 20:32
@tacaswell tacaswell force-pushed the fix_noop_tight_bbox branch from 91e2fd8 to 297cf79 Compare June 3, 2020 20:58
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

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
Copy link
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
Copy link
Contributor

anntzer commented Jun 4, 2020

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
Copy link
Member Author

Will add helper function.

@tacaswell
Copy link
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 tacaswell force-pushed the fix_noop_tight_bbox branch from 8698d15 to 7a412cd Compare June 9, 2020 02:43
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

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

Be forgiving about renderer instances that do not inherit from
RendereBase.
@tacaswell tacaswell force-pushed the fix_noop_tight_bbox branch from 7a412cd to f777177 Compare June 9, 2020 16:08
@QuLogic QuLogic merged commit f820c27 into matplotlib:master Jun 9, 2020
@lumberbot-app
Copy link

lumberbot-app bot commented Jun 9, 2020

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 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
… 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 pushed a commit to tacaswell/matplotlib that referenced this pull request Jun 10, 2020
… 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
…-v3.2.x

Backport PR #17560: FIX: do not let no-op monkey patches to renderer …
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.

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