Skip to content

FIX: ensure switching the backend installs repl hook #23057

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
May 23, 2022

Conversation

tacaswell
Copy link
Member

@tacaswell tacaswell commented May 17, 2022

PR Summary

closes #23042

In #22005 we change pyplot so that at import time we do not force a switch of
the backend and install the repl displayhook.

However, this meant that in some cases (primarily through ipython --pylab) to
end up with a session where install_repl_displayhook had never been called.

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).

I'm still thinking about how to test this.

@tacaswell tacaswell added this to the v3.5.3 milestone May 17, 2022
@tacaswell tacaswell changed the title FIX: ensure switching the backend while interactive is interactive FIX: ensure switching the backend installs repl hook May 17, 2022
"IPython",
"--pylab",
"-c",
"import matplotlib.pyplot as plt; assert plt._REPL_DISPLAYHOOK == plt._ReplDisplayHook.IPYTHON", # noqa
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just implictly join the strings here to split this over two lines

        [
            sys.executable,
            "-mIPython",
            "--pylab",
            "-c",
            "import matplotlib.pyplot as plt; "
            "assert plt._REPL_DISPLAYHOOK == plt._ReplDisplayHook.IPYTHON",
        ],

Copy link
Member Author

Choose a reason for hiding this comment

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

The importance of the lack of the trailing comma is too subtle, went with a ';'.join(...) instead.

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Appears to fix using the example in #23042; not sure I want to dig into pylab to see why.

closes matplotlib#23042

In matplotlib#22005 we change `pyplot` so that at import time we do not force a switch of
the backend and install the repl displayhook.

However, this meant that in some cases (primarily through `ipython --pylab`) to
end up with a session where `install_repl_displayhook` had never been called.

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
timhoffm
timhoffm previously approved these changes May 19, 2022
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

This causes timeouts in the py39 and py310 azure builds.

@timhoffm timhoffm dismissed their stale review May 19, 2022 07:04

Timeouts

@tacaswell
Copy link
Member Author

This causes timeouts in the py39 and py310 azure builds.

We "just" need unreasonably long timeouts 🤷🏻 (was 5s, went to 60s).

"assert plt._REPL_DISPLAYHOOK == plt._ReplDisplayHook.IPYTHON",
)),
],
env={**os.environ, "SOURCE_DATE_EPOCH": "0"},
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't really need this, nor the stdout/stderr capture (pytest does it for you), nor universal_newlines, I think?

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.

Minor nit on test, feel free to selfmerge with or without fixing.

@anntzer
Copy link
Contributor

anntzer commented May 23, 2022

I'll put the cleanup in a separate PR.

@anntzer anntzer merged commit b31c5ae into matplotlib:main May 23, 2022
@lumberbot-app
Copy link

lumberbot-app bot commented May 23, 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 b31c5ae782876386006a544a5cc833ddddb4b877
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #23057: FIX: ensure switching the backend installs repl hook'
  1. Push to a named branch:
git push YOURFORK v3.5.x:auto-backport-of-pr-23057-on-v3.5.x
  1. Create a PR against branch v3.5.x, I would have named this PR:

"Backport PR #23057 on branch v3.5.x (FIX: ensure switching the backend installs repl hook)"

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.

@tacaswell tacaswell deleted the fix_pylab branch May 23, 2022 15:55
@tacaswell tacaswell mentioned this pull request May 23, 2022
7 tasks
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request May 23, 2022
…lls repl hook

Merge pull request matplotlib#23057 from tacaswell/fix_pylab

(cherry picked from commit b31c5ae)
jklymak pushed a commit that referenced this pull request Jun 2, 2022
* Backport PR #23057: FIX: ensure switching the backend installs repl hook

Merge pull request #23057 from tacaswell/fix_pylab

(cherry picked from commit b31c5ae)

* Merge pull request #23106 from anntzer/tpi

MNT: Reuse subprocess_run_helper in test_pylab_integration.
(cherry picked from commit 51624b6)

* TST: Adjust to state tracking on 3.5.x branch

The implementation of tracking if the repl displayhook is installed changed on
main.  Adjust the backported test to work with the old way.

Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
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.

[Bug]: Figures fail to redraw with IPython
4 participants