Skip to content
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

PR: Run GUI eventloop while waiting for input #438

Closed
wants to merge 27 commits into from

Conversation

impact27
Copy link
Contributor

@impact27 impact27 commented Sep 17, 2019

While waiting for the PY2 raw_input / PY3 input function, ipykernel is now completely blocked.
This is a problem for example while debugging. (Pdb uses input to get a command).
This PR would allow two new things to happen while waiting an input:

  • self.do_one_iteration() would give a chance to process comms messages.
  • manager.canvas.flush_events() would give a change to any matplotlib backend to update.

@blink1073
Copy link
Member

We can't depend on matplotlib. Can we make that import optional?

@blink1073
Copy link
Member

@impact27 @ccordoba12 do we expect more PRs leading up to the Spyder release?

@impact27
Copy link
Contributor Author

We can't depend on matplotlib. Can we make that import optional?

Sure, I am not sure how you would prefer to do that?
Alternatively we can just call self.do_one_iteration() and subclass it in spyder-kernels.

@impact27 @ccordoba12 do we expect more PRs leading up to the Spyder release?

I don't expect more PRs but I was not really expecting this one.

@impact27
Copy link
Contributor Author

I just added a try: except ImportError

@impact27
Copy link
Contributor Author

@impact27 @ccordoba12 do we expect more PRs leading up to the Spyder release?

We could port some of the improvments from spyder-kernels into ipykernel. We don't need to do that before the Spyder release. These improvments are mostly ipdb related. I don't know if you would be willing to have a ipdb subclass for ipykernel but the advantages that this could bring would be:

  • Autocomplete (technically already there but doesn't work in qtconsole)
  • ipython magic in ipdb
  • history in ipdb
  • multiline support in ipdb
    (Maybe some of these should be ported in IPython instead?)

@ccordoba12
Copy link
Member

@impact27, I don't how these improvements would work in the notebook, which also has support for input and hence ipdb.

That's the main issue you'll probably face when trying to move them upstream.

@ccordoba12
Copy link
Member

@impact27, did you test this with the inline backend? If so, could you post a screenshot about it?

@impact27
Copy link
Contributor Author

Inline:
Screenshot 2019-09-18 at 00 26 43
Qt5:
Screenshot 2019-09-18 at 00 28 06

@impact27
Copy link
Contributor Author

(The screenshots are from qtconsole)

@ccordoba12
Copy link
Member

Nice!! Thanks for posting them!

Pinging @tacaswell about this one because he'll probably be very interested about this new development.

@ccordoba12
Copy link
Member

And @SylvainCorlay could also be interested about this because I think the comms fix should allow using bqplot in debugging mode.

@@ -861,6 +862,13 @@ def raw_input(self, prompt=''):
)

def _input_request(self, prompt, ident, parent, password=False):
"""Send an input request to the frontend and wait for the reply."""
# matplotlib needs to be imported after app.launch_new_instance()
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to do if 'matplotlib' in sys.modules: ... as this will import matplotlib if it is installed, but not yet imported and it is not the quickest import.

I am not stoked about importing a private module here, I would do something like

if 'matplotlib.pyplot' in sys.modules:
    def flush():
        import matplotlib.pyplot as plt
        if plt.get_fignums():
            plt.gcf().canvas.flush_events()
else:
    flush = None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I imported that as this is the same import made here:

from matplotlib._pylab_helpers import Gcf

and here in plt.pause:
https://github.com/matplotlib/matplotlib/blob/5e9c54e7c4feb296b387fadb56394ef67ac2927f/lib/matplotlib/pyplot.py#L307
I am not sure if there is a subtle difference between Gcf.get_active().canvas.flush_events() and plt.gcf().canvas.flush_events()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Gcf is not part of the public API (not withstanding the other usage in IPython).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

plt.gcf().canvas.flush_events() creates a new figure if no figures are open. Is there any way to avoid that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

plt.get_current_fig_manager() has the same problem. It creates an active figure as a side effect.

Copy link
Contributor Author

@impact27 impact27 Sep 19, 2019

Choose a reason for hiding this comment

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

Sorry I missed the plt.get_fignums()

@SylvainCorlay
Copy link
Member

I am not really sure about this.

  • ipykernel should really not do anythings specific with respect to matplotlib (it currently does but we would like it not to)
  • Also, this may break the contract that messages should be processed in order...

One thing we may want to add to the protocol though is the ability to have a timeout option to the input request so that it does not hang forever...

@impact27
Copy link
Contributor Author

* ipykernel should really not do anythings specific with respect to matplotlib (it currently does but we would like it not to)

Alternatively, this could just call do_one_iteration which can be modified to flush the matplotlib events somewhere else. (In ipykernel/pylab ?)

* Also, this may break the contract that messages should be processed in order...

The problem I am trying to solve is to process messages while waiting for a Pdb prompt (ideally I would process all events that are normally processed while waiting for a regular prompt. My understanding is that an alternative way to do that is to run the asyncio loop somehow. I think that would require a large part of pdb, bdb, cmd, and IPython.core.debugger to be rewritten with coroutines. We could also modify just pdb.cmdloop so this processing of messages only happens when called from pdb.

What message would actually be problematic to process here? Surely e.g. a completion request would be fine. I am trying to understand the potential problems as this is what we already do in spyder.

In spyder-kernels we currently call do_one_iteration explicitly after sending a request while debugging (such as a comms message or a completion request). It would probably be more elegant to have the kernel processing messages as it does while not debugging. This approach does not work for matplotlib plots, as they need to be updated continuously, which is the reason why we need to modify this function.

Now that I think about it, maybe a better alternative would be to run the debugger (or even files) in another process. Threads wouldn't work because matplotlib and other libraries want to be on the main thread. This would also solve the problem of asyncio not running (see jupyter/notebook#3397) but I think I am getting off topic...

One thing we may want to add to the protocol though is the ability to have a timeout option to the input request so that it does not hang forever...

Well if I am debugging something and go for lunch, I would expect to be able to resume debugging when I get back. Therefore I don't really see any reasonable timeout time.

@impact27
Copy link
Contributor Author

Also, this may break the contract that messages should be processed in order...

Is it possible to send a message with low priority? Like saying explicitely: It is ok to process other messages while this message is being processed?

@ccordoba12
Copy link
Member

ccordoba12 commented Sep 19, 2019

Also, this may break the contract that messages should be processed in order

If comm messages are queued, then do_one_iteration should process them in order, shouldn't it?

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Sep 19, 2019

Also, this may break the contract that messages should be processed in order...

Is it possible to send a message with low priority? Like saying explicitely: It is ok to process other messages while this message is being processed?

We need to think about possible race conditions. It may as well be ok.

@impact27
Copy link
Contributor Author

Also, this may break the contract that messages should be processed in order

If comm messages are queued, then do_one_iteration should process them in order, isn't it?

I guess the problem is that they will start processing in order, but might not finish processing in order.

@SylvainCorlay
Copy link
Member

If comm messages are queued, then do_one_iteration should process them in order, shouldn't it?

comm messages are just shell messages and are queued with other shell messages. If code is written with the assumption the input() is blocking, I presume this may as well break.

@impact27
Copy link
Contributor Author

As an alternative, I created #439 that would allow to change this behaviour easily.

@tacaswell
Copy link
Contributor

The problem at the bottom here is that for the GUI to "look alive" you need to spin the input hook or is someother way give the GUI event loop some time to process it's events (user input, redrawing, etc). Calling flush_events is a framework agnostic way that Matplotlib has to do that. Another option is to sort out what GUI is running and spin it directly.

Way more details than anyone really wants at matplotlib/matplotlib#4779

@tacaswell
Copy link
Contributor

ipython/ipython#5026 this popped up in my feed today and seems at least a bit relevant.

@impact27
Copy link
Contributor Author

I think ipython/ipython#5026 is a front end problem: the kernel issues a input request but the frontend doesn’t know what to do with it. This PR is not really related as running the GUI event loop doesn’t really help here.

@tacaswell
Copy link
Contributor

Mostly race conditions / failure modes in raw_input.

@impact27
Copy link
Contributor Author

impact27 commented Oct 27, 2019 via email

except Exception:
self.log.error("Invalid Message", exc_info=True)
return
self._stdin_msg = msg
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to lock here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The messages are processed by a single thread so a lock would not be useful here.

threading._MainThread)
if is_main_thread and self.eventloop:
self.eventloop(self)
return self._stdin_msg
Copy link
Contributor

Choose a reason for hiding this comment

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

and possibly here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_input_request_loop_step is called from _wait_input_request_reply which is protected by a lock in _input_request, so I don't think a lock here would do anything.

@impact27
Copy link
Contributor Author

impact27 commented Dec 4, 2019

I have added an option to enable this behaviour. I think it should be off by default for the following reason:

  • If I am debugging an application that uses the same eventloop as matplotlib, the debugger won't stop the program, as the eventloop will still be running!

Should matplotlib be called explicitly to avoid this problem as was the case earlier in this PR?

@impact27
Copy link
Contributor Author

impact27 commented Dec 5, 2019

I opened #465 as an alternative that only updates matplotlib.

@impact27
Copy link
Contributor Author

impact27 commented Dec 5, 2019

I tried to add a %input_eventloop magic to enable / disable this. What do you think of that?
The idea is that if you want matplotlib plots while debugging, you must explicitly activate it. If this is activated by default it might be very counter-intuitive for a user trying to debug a gui application.

@jkablan
Copy link

jkablan commented Apr 3, 2020

Hi, I'm trying to use your updates to plot while debugging. I've installed it by doing the following.

  1. Cloning the update Update_comms_matplotlib branch
  2. running pip install -e .

I then start ipython and attempt to run your magic command

%input_eventloop

which returns the following.

UsageError: Line magic function '%input_eventloop' not found.

If I try plot anyways. I have to run plt.pause(10) for figures to show up.

Can you give me an idea of what I'm doing wrong? Also wondering if there's a timeline for integrating this back to master? Lack of this functionality is a serious productivity killer for those of us who are coming from MATLAB.

@impact27
Copy link
Contributor Author

impact27 commented Apr 3, 2020

Can you give me an idea of what I'm doing wrong? Also wondering if there's a timeline for integrating this back to master? Lack of this functionality is a serious productivity killer for those of us who are coming from MATLAB.

%input_eventloop should work. If it doesn't, it might mean the installation failed somehow. You can try:

import ipykernel
print(ipykernel.__file__)

And check if the file is indeed the one you are expecting.

About the timeline, the problem is that there is a trade-off here: If this is implemented, any app that uses an event loop (such as matplotlib) can keep running while the debugger is stopped. This is good because you can plot things but bad if you are trying to debug a gui app.

This trade-off means it is not trivial to merge this PR. My idea was to leave this behaviour disabled by default but give an option to enable it (with the %input_eventloop magic), which might make it acceptable.

Another solution for this problem would be to modify Pdb so it uses a custom method for blocking the code. This would be quite a big change and a lot of work.

@jkablan
Copy link

jkablan commented Apr 3, 2020

Thanks for getting back to me so quickly.
print(ipykernel.__file__)
returns
C:\git\projects\temp\ipykernel\ipykernel\__init__.py

This is indeed the directory I cloned your respo into. The head of my clone is pointed to 'd16d6e4e40f68f384d57a1a4458d4ef848e4ceb5' which is from about a week ago. Should I be using this commit?

Introducing the magic command to allow the gui event loop to keep running seems like a very reasonable solution to me given the amount of work it would be to modify pdb. I guess I can't see what about this solution would prevent it from being merged.

@impact27
Copy link
Contributor Author

impact27 commented Apr 6, 2020

I am not convinced this is the right approach. This might introduce unwanted side effects.
In Spyder I solve this problem by:

import cmd
cmd.input = patched_input

and then running eventloop from this function. This means the eventloop is only run when called from pdb instead of any input calls. The main thing that would be nice to have is a way of interrupting the eventloop. The workaround I found is to send a dummy message on the shell channel, but having a kernel.eventloop.stop() function would be nice.

@impact27
Copy link
Contributor Author

impact27 commented Jun 3, 2021

I am going to close this for now as I am not convinced this is the right approach. What I am afraid of is that someone will use input() to try and block the execution. If the eventloop is still running, some code could be executed by the eventloop. This could make it hard to debug qt apps for example. Of course this is only an edge case but there should be at least an easy way of disabling this.

@impact27 impact27 closed this Jun 3, 2021
@Carreau Carreau added this to the no action milestone Jun 14, 2021
@felipeespic
Copy link

I recall that this problem wasn't happening with IPython 7.7. Actually, I also remember that I didn't update IPython for a very long time just to avoid braking the integration between ipdb and Matplotlib.

@felipeespic
Copy link

I just want to add that the issue with Matplotlib is not happening with pdb, only with ipdb.
For reference:
pdb in Python 3.8.10
ipdb 0.13.9 in IPython 8.6.0

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.

None yet

9 participants