BUG: fix IPython's %pylab mode detection

[dnicolodi]

There is no pyplot module in the system registry, as the module is imported as matplotlib.pyplot.

This code tries to detect when matplotlib.pyplot.show() is invoked in a script run via the %run IPython's magic while in interactive mode as enabled by the %pylab or %matplotlib IPyhton's magic commands. In these modes, matplotlib.pyplot.show() should not block.

Failing do detect these modes, a simple test.py script as this

import matplotlib.pyplot as plt
plt.figure()
plt.show()

run in IPython as follows

%matplotlib
%run test.py

is not expected to block on matplotlib.pyplot.show(). Without this patch it does. Additionally, the redraw implemented by IPython at the end of %run caused an empty figure to be displayed when the script terminates.

Fixing the module lookup in sys.modules fixes the issue.

Fixes: 86f26a0.
Fixes: #25485.

[dnicolodi]

I've absolutely no idea why CircleCI does not want to run on this PR

[dnicolodi]

Funny, it is expected that I need to authorize CircleCI on my GitHub account to have it run on PRs for matplotlib?

[greglucas]

It helps to provide a little bit of context in the PR as well and not just a reference to the issue.

I can confirm that this works as expected now. There is no module pyplot in the system registry, it is matplotlib.pyplot as updated here. This looks like it was overlooked in the previous commit that used a from matplotlib import pyplot and then checked the pyplot attribute. 86f26a0

For other reviewers the reproducer is:

1. Start ipython
2. %matplotlib
3. %run test.py
4. You should be in interactive mode, not blocked

test.py file:

import matplotlib.pyplot as plt
plt.figure()
plt.show()

[dnicolodi]

Thanks @greglucas for the review. I haven't contributed to matplotlib much and different projects have different customs regarding the amount of detail to put into PR descriptions and commit message. Should I add more context to the commit message?

[greglucas]

Should I add more context to the commit message?

It is nice for viewing git blames going back in history. For example, the one that this traced back through made it pretty easy for me to read what was going on: 86f26a0

Funny, it is expected that I need to authorize CircleCI on my GitHub account to have it run on PRs for matplotlib?

I don't think you should have to authorize anything to get CircleCI to run on a PR, maybe to get it to run on your own fork though... It looks like it did run eventually, so hopefully, it is good now.

[jklymak]

Should I add more context to the commit message?

If you want reviewer attention it's a good idea to make it as easy as possible on them.

[dnicolodi]

I don't think you should have to authorize anything to get CircleCI to run on a PR, maybe to get it to run on your own fork though... It looks like it did run eventually, so hopefully, it is good now.

It started running only after I authorized it. I think it may be a bug on the CircleCI side: a while back I tried CircleCI on a test project and when I understood that I was not going to use it after all I revoked the authorization for CircleCI from my GitHub projects.

[dnicolodi]

Should I add more context to the commit message?

If you want reviewer attention it's a good idea to make it as easy as possible on them.

Do you realize that this comment is very dismissive of the time I spent in submitting a detailed bug report, provide feedback when other maintainers commented on such issue, track down the issue, and submit a patch to fix it. If you do realize that this attitude is not respectful of contributors, and this is the only contribution you want to give to this PR, please go play the burnt-out maintainer somewhere else.

[dnicolodi]

@greglucas I updated the commit message.

[jklymak]

My apologies - I wasn't trying to be dismissive at all. Just suggesting that a summary of the issue and why this is the correct fix would get it reviewed faster. Issues with lots of back and forth get confusing fast, and it's often not clear what the final proposed solution is.

[tacaswell]

Thanks for your work to debug and fix this @dnicolodi !