8000 Fix AttributeError for pickle load of Figure class by wsykala · Pull Request #23462 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Fix AttributeError for pickle load of Figure class #23462

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 26, 2022
Merged

Fix AttributeError for pickle load of Figure class #23462

merged 2 commits into from
Jul 26, 2022

Conversation

wsykala
Copy link
Contributor
@wsykala wsykala commented Jul 21, 2022

PR Summary

When saving a figure with the pickle module in one python process and then loading it in a different process a following error appears:

/figure.py", line 2911, in __setstate__
    mgr = plt._backend_mod.new_figure_manager_given_figure(num, self)
AttributeError: 'NoneType' object has no attribute 'new_figure_manager_given_figure'

This PR replaces the plt._backend_mod with the call to plt._get_backend_mod() in the Figure.__setstate__ method. This change ensures that the _backend_mod is not None during pickle load.

A reproducible example is provided in the comments

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@wsykala
Copy link
Contributor Author
wsykala commented Jul 21, 2022 •
edited
Loading

Reproducible example:

# save.py
import numpy as np
import matplotlib.pyplot as plt
import pickle

fig_handle = plt.figure()
x = np.linspace(0,2*np.pi)
y = np.sin(x)

with open('sinus.pickle','wb') as file:
    pickle.dump(fig_handle, file)

# read.py
import matplotlib.pyplot as plt
import pickle
import numpy as np

with open('sinus.pickle', 'rb') as blob:
    fig = pickle.load(blob)

Outcome:

$ python save.py
$ python read.py
Traceback (most recent call last):
  File "/Users/wsykala/projects/playground/read.py", line 6, in <module>
    fig_handle = pl.load(open('sinus.pickle','rb'))
  File "/Users/wsykala/miniconda3/envs/matplotlib/lib/python3.9/site-packages/matplotlib/figure.py", line 2911, in __setstate__
    mgr = plt._backend_mod.new_figure_manager_given_figure(num, self)
AttributeError: 'NoneType' object has no attribute 'new_figure_manager_given_figure'

@wsykala wsykala force-pushed the figure-pickle-load-fix branch from ea0363d to 30da817 Compare July 21, 2022 20:03
github-actions[bot]
github-actions bot reviewed Jul 21, 2022
View reviewed changes
Copy link
@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@QuLogic
Copy link
Member
QuLogic commented Jul 21, 2022

We already have tests for pickling; I assume they didn't catch this because it's all in the same process.

We definitely need a test to be sure that this is working.

@tacaswell tacaswell added this to the v3.5.3 milestone Jul 21, 2022
@tacaswell
Copy link
Member

This is another fallout from making backend setup lazier in pyplot (previously we would call switch_backend on import which prevented this).

I suspect https://docs.pytest.org/en/7.1.x/how-to/tmp_path.html#the-tmp-path-fixture +

matplotlib/lib/matplotlib/testing/__init__.py

Lines 53 to 66 in 4455dc9

def subprocess_run_helper(func, *args, timeout, extra_env=None):
"""
Run a function in a sub-process.
Parameters
----------
func : function
The function to be run. It must be in a module that is importable.
*args : str
Any additional command line arguments to be passed in
the first argument to ``subprocess.run``.
extra_env : dict[str, str]
Any additional environment variables to be set for the subprocess.
"""
is what we need to test this.

@wsykala wsykala force-pushed the figure-pickle-load-fix branch from 7d5f53a to 2a3de64 Compare July 22, 2022 12:55
@wsykala wsykala force-pushed the figure-pickle-load-fix branch from 2a3de64 to 3c5c2f8 Compare July 22, 2022 13:39
@wsykala
Copy link
Contributor Author
wsykala commented Jul 22, 2022 •
edited
Loading

@tacaswell @QuLogic
Added a test for this behaviour and extracted the code that was duplicated. The test scenario is as follows:

We create and pickle the figure in the main process. Next we run the pickle.load and the dump it to stdout in the subprocess. After that the binary representation of the figure is loaded from stdout using ast, and we compare it the same way as in test_complete.

I am not sure why the circleci job for building docs failed though.

Also one more point. When creating the test I came across a strange bug. I described it in this issue #23471

@jklymak
Copy link
Member
jklymak commented Jul 22, 2022

The CI errors are

/home/circleci/project/lib/matplotlib/axes/_base.py:docstring of matplotlib.axes._base._AxesBase.set_prop_cycle:14: WARNING: py:obj reference target not found: cycler.Cycler
/home/circleci/project/lib/matplotlib/axes/_base.py:docstring of matplotlib.axes._base._AxesBase.set_prop_cycle:16: WARNING: py:obj reference target not found: cycler.Cycler
/home/circleci/project/lib/matplotlib/axes/_base.py:docstring of matplotlib.axes._base._AxesBase.set_prop_cycle:23: WARNING: py:obj reference target not found: cycler.Cycler
/home/circleci/project/lib/matplotlib/axes/_base.py:docstring of matplotlib.axes._base._AxesBase.set_prop_cycle:23: WARNING: py:obj reference target not found: cycler.cycler
/home/circleci/project/lib/matplotlib/axes/_base.py:docstring of matplotlib.axes._base._AxesBase.set_prop_cycle:55: WARNING: py:obj reference target not found: cycler.cycler
/home/circleci/project/lib/matplotlib/rcsetup.py:docstring of matplotlib.rcsetup.cycler:2: WARNING: py:obj reference target not found: cycler.Cycler
/home/circleci/project/lib/matplotlib/rcsetup.py:docstring of matplotlib.rcsetup.cycler:2: WARNING: py:func reference target not found: cycler.cycler
/home/circleci/project/lib/matplotlib/rcsetup.py:docstring of matplotlib.rcsetup.cycler:11: WARNING: py:obj reference target not found: cycler.Cycler
/home/circleci/project/lib/matplotlib/rcsetup.py:docstring of matplotlib.rcsetup.cycler:13: WARNING: py:obj reference target not found: cycler.Cycler
/home/circleci/project/lib/matplotlib/rcsetup.py:docstring of matplotlib.rcsetup.cycler:17: WARNING: py:obj reference target not found: cycler.Cycler
/home/circleci/project/lib/matplotlib/rcsetup.py:docstring of matplotlib.rcsetup.cycler:38: WARNING: py:class reference target not found: cycler.Cycler

But I don't see that this PR should have caused that....

@tacaswell
Copy link
Member

I think the docs failure is related to hosting issues (which were resolved by our providers), re-restarted docs build.

@QuLogic QuLogic merged commit 7117a16 into matplotlib:main Jul 26, 2022
@lumberbot-app
Copy link
lumberbot-app bot commented Jul 26, 2022

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.5.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 7117a16eabe67c374fde085f17e0eb905149cadc
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #23462: Fix AttributeError for pickle load of Figure class'
  1. Push to a named branch:
git push YOURFORK v3.5.x:auto-backport-of-pr-23462-on-v3.5.x
  1. Create a PR against branch v3.5.x, I would have named this PR:

"Backport PR #23462 on branch v3.5.x (Fix AttributeError for pickle load of Figure class)"

And apply the correct labels and milestones.

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

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

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

@QuLogic
Copy link
Member
QuLogic commented Jul 26, 2022

Thanks @wsykala! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

@tacaswell
Copy link
Member

There are same pretty big changes in the test file on main that are not an 3.5.x (for example the pickle test went to an figures equal test from an image comparison test!).

I'm inclined to backport this without backporting the tests. It is not ideal, but I think it is less bad than re-writing the tests to not confilct (and dealing with a messy merge up) or implicitly backporting a bunch of other changes to the tests.

@tacaswell
Copy link
Member

Discussed on the call. Plan: copy the tests back and accept the code duplication and clean it up on merge up.

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jul 29, 2022
@tacaswell
Backport PR matplotlib#23462: Fix AttributeError for pickle load of F…
bd0df1f 
…igure class

There were a number of complications with the backport:

 - the new test generalized an existing test that had picked up
   additional changes on the main branch
 - the signature of the subproccess helper has changed on the main branch
QuLogic added a commit that referenced this pull request Jul 29, 2022
@QuLogic
Merge pull request #23515 from tacaswell/auto-backport-of-pr-23462-on…
88e7680 
…-v3.5.x

Backport PR #23462: Fix AttributeError for pickle load of Figure class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@tacaswell tacaswell tacaswell approved these changes

@QuLogic QuLogic QuLogic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.5.3
Development

Successfully merging this pull request may close these issues.
4 participants
@wsykala @QuLogic @tacaswell @jklymak
0