-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
91e2fd8
to
297cf79
Compare
There was a problem hiding this 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).
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. |
We basically do per #17159. |
Will add helper function. |
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. |
8698d15
to
7a412cd
Compare
There was a problem hiding this 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.
7a412cd
to
f777177
Compare
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
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. |
… 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
… 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
…-v3.2.x Backport PR #17560: FIX: do not let no-op monkey patches to renderer …
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