PR: Use cached kernel for faster kernel restart (IPython console) #19092

impact27 · 2022-08-17T12:40:49Z

Description of Changes

restart kernel instantaneously by using the cached kernel

depends on #19411

Wrote at least one-line docstrings (for any new functions)
Added unit test(s) covering the changes (if testable)

Included a screenshot or animation (if affecting the UI, see Licecap)

Issue(s) Resolved

Fixes #

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:

ccordoba12

Some small suggestions for you @impact27, otherwise looks good to me.

spyder/plugins/ipythonconsole/utils/kernel_handler.py

spyder/plugins/ipythonconsole/widgets/client.py

spyder/plugins/ipythonconsole/widgets/shell.py

ccordoba12 · 2022-11-08T01:37:30Z

spyder/plugins/ipythonconsole/widgets/shell.py

-        restart_kernel = self.config_shortcut(
-            self.ipyclient.restart_kernel,
-            context='ipython_console',
-            name='Restart kernel',
-            parent=self)


This shortcut is important, so please find a way to restore it. I know many users (and also I) use it all the time to quickly restart the kernel.

but it still work, doesn't it? The same shortcut is created in main_widget.py:

self.restart_action = self.create_action( IPythonConsoleWidgetActions.Restart, text=_("Restart kernel"), icon=self.create_icon('restart'), triggered=self.restart_kernel, register_shortcut=True )

but it still work, doesn't it?

Nop, it doesn't. I changed it to other shortcuts and they do nothing when pressed on the console. Probably it's necessary to have it here too so that it actually has effect.

That is strange. When launching this PR, I can:

1 - click on the console
2 - "Cmd + ."
3 - A pop up appears asking if I want to restart the kernel
4 - Open settings and change the shortcut to "Cmd + o"
5 - click on the console
6 - "Cmd + o"
7 - A pop up appears asking if I want to restart the kernel

That is not what you are observing? I mean create_action(register_shortcut=True) should work and does work in my case, could it be a linux-only bug?

That is not what you are observing?

Nop. After I change the shortcut in Preferences, it doesn't have any effect in the console.

could it be a linux-only bug?

Possibly. @dalthviz, could you check if you're able to change the "restart kernel" shortcut on Windows?

A possible cause for this is that the shortcut is not registered anymore in ShellWidget. That's how I understand Qt triggers it when the focus widget is precisely that one and the reason why we had this piece of code you removed here (actually, I don't understand how a different shortcut is working for you on Mac).

Checking on Windows I'm unable to change the shortcut. Tested setting the shortcut to be Ctrl + o but the console still uses the Ctrl + . :

Maybe on MacOS there is something that changes the way widgets manage their shortcut or shortcut context that enables not having the code on the shellwidget?

ctrl + . is hardcoded in qtconsole, so I skipped this. It should work now

It still doesn't work for me @impact27. Although now Ctrl+. doesn't work either.

This is perplexing. I will investigate but because on Mac ctrl + . And cmd + . Are distinct, it is not trivial for me to debug this.

spyder/plugins/ipythonconsole/widgets/shell.py

ccordoba12 · 2022-11-08T01:49:17Z

@impact27, please also take care of the pep8speaks warnings above, which are very simple.

impact27 · 2022-11-08T19:19:23Z

@ccordoba12 the pep8speaks are not applicable, the comment has not been updated since august and doesn't corresponds to the current state of the PR.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

ccordoba12 · 2022-11-09T21:38:11Z

I tested this manually and found a small regression: the hr painted after the Restarting kernel message doesn't adjust to the current interface theme, i.e. it's black when using a dark theme:

Perhaps you introduced that regression in commit 52532ad (not sure about that though).

impact27 · 2022-11-11T05:38:39Z

I tested this manually and found a small regression: the hr painted after the Restarting kernel message doesn't adjust to the current interface theme, i.e. it's black when using a dark theme:

This is strange as well, I see:

Which is what I expect from the line:
ruler.setBackground(QColor(QStylePalette.COLOR_TEXT_1))

ccordoba12 · 2022-11-12T17:45:12Z

This is strange as well, I see

I understood what happens here: if you go to the Options menu and click on the Restart kernel action, then the line is painted according to the interface theme (i.e. as in your screenshots above).

However, if you press Ctrl+. in the console, then you get the black line (as in my screenshot). I think this problem happens because in this case the shortcut is calling the method to restart kernels available in Qtconsole, not the Spyder one. As evidence of that is the following screenshot, where I set my interface language to Spanish and press Ctrl+.:

You can see that the dialog is not translated, which means that comes from Qtconsole. We prevent that in Spyder 5 by overriding the request_restart_kernel method in ShellWidget:

https://github.com/jupyter/qtconsole/blob/90ffe335cf9c82f7814c9a945553584d6665a62c/qtconsole/frontend_widget.py#L344-L346

but you removed it on this PR. So, that's another regression that you need to address here.

However, I think it should be relatively simple to resolve both this problem and the shortcut one by

Adding a new signal to ShellWidget called sig_kernel_restart_requested.
Connecting that signal to the new restart_kernel method in IPythonConsoleWidget.
Restoring request_restart_kernel and make it a emit that signal.
Restoring the shortcut declaration and make it emit that signal.

impact27 · 2022-11-14T22:18:30Z

I understand why it is different on mac: cmd + . works but ctrl + . has the behaviour you described!

…rnel

impact27 · 2022-11-15T21:57:45Z

My understanding so far is:
in qtconsole.frontend_widget.FrontendWidget._event_filter_console_keypress: The console catches ctrl + . events and calls request_restart_kernel. We want to avoid that because the shortcut can not be changed. This was the original reason this PR didn't work in Linux and Window because on mac ctrl and cmd are distinct. So the code was messing with ctrl + . but I didn't see it because I used cmd + ..

Now that ctrl + . is not caught by the console, it is free to propagate up the chain to main_widget.IPythonConsoleWidget.restart_action. On my mac, I can change my shortcut from cmd + . to ctrl + . and it still works. It did not work before my latest change.

This explain everything that you saw, but I am perplexed by the fact that it doesn't work for you @ccordoba12. Could you check that the shortcut for restart kernel was not changed in your settings?

ccordoba12 · 2022-11-16T00:25:32Z

This explain everything that you saw, but I am perplexed by the fact that it doesn't work for you @ccordoba12. Could you check that the shortcut for restart kernel was not changed in your settings?

I'm running Spyder with clean settings and it still doesn't work for me.

But let's try this PR on Binder. A link for it will be generated below by calling the following command (we have an action that detects it and automatically creates the link).

ccordoba12 · 2022-11-16T00:26:41Z

/Show binder

github-actions · 2022-11-16T00:26:51Z

👈 Launch a Binder instance on this branch

ccordoba12 · 2022-11-16T00:41:14Z

No luck either, as you can check as well.

…rnel

impact27 · 2022-11-16T06:23:04Z

I restored it, but I think you can't change the shortcut that way. However it appears to be already the case with master.

ccordoba12

Thanks @impact27 for your last commit! I can confirm it fixes the restart issue on Linux, although you're right in the sense that changing the shortcut doesn't have any effect.

I left an additional comment for you, then this should be ready.

spyder/plugins/ipythonconsole/widgets/shell.py

ccordoba12

Looks good to me now. Thanks @impact27 for your hard work and patience with this one.

This is a really cool improvement!

This comment was marked as outdated.

Sign in to view

impact27 force-pushed the faster_restart_kernel branch 16 times, most recently from d2c8aa7 to 11e092b Compare August 24, 2022 07:08

impact27 force-pushed the faster_restart_kernel branch 8 times, most recently from 77594f2 to 2b6dd56 Compare August 26, 2022 12:59

impact27 mentioned this pull request Aug 28, 2022

PR: Refactor the way clients are created (IPython console) #19062

Merged

3 tasks

impact27 force-pushed the faster_restart_kernel branch 3 times, most recently from b672574 to 0dd53df Compare September 4, 2022 06:00

impact27 force-pushed the faster_restart_kernel branch from 0dd53df to fb26208 Compare September 12, 2022 15:40

impact27 changed the title ~~[WIP] PR: Used cached kernel for faster kernel restart~~ PR: Used cached kernel for faster kernel restart Nov 5, 2022

ccordoba12 added this to the v6.0alpha1 milestone Nov 5, 2022

git merge

5c37d65

ccordoba12 reviewed Nov 8, 2022

View reviewed changes

ccordoba12 changed the title ~~PR: Used cached kernel for faster kernel restart~~ PR: Use cached kernel for faster kernel restart (IPython console) Nov 8, 2022

ccordoba12 assigned impact27 Nov 8, 2022

ccordoba12 added type:Enhancement component:IPython Console labels Nov 8, 2022

Apply suggestions from code review

9d886ad

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

impact27 force-pushed the faster_restart_kernel branch from c167b59 to 9d886ad Compare November 8, 2022 19:32

Quentin Peter added 3 commits November 14, 2022 23:20

Merge remote-tracking branch 'upstream/master' into faster_restart_ke…

5cd6dc5

…rnel

restore request_restart_kernel

834be50

update comments

2b2f411

Quentin Peter added 2 commits November 16, 2022 07:08

Merge remote-tracking branch 'upstream/master' into faster_restart_ke…

0571e03

…rnel

restre restart kernel

23c2ff1

ccordoba12 reviewed Nov 16, 2022

View reviewed changes

spyder/plugins/ipythonconsole/widgets/shell.py Show resolved Hide resolved

ccordoba12 approved these changes Nov 17, 2022

View reviewed changes

ccordoba12 merged commit d2de59c into spyder-ide:master Nov 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Use cached kernel for faster kernel restart (IPython console) #19092

PR: Use cached kernel for faster kernel restart (IPython console) #19092

impact27 commented Aug 17, 2022 •

edited

Loading

This comment was marked as outdated.

ccordoba12 left a comment

ccordoba12 Nov 8, 2022

impact27 Nov 8, 2022

ccordoba12 Nov 9, 2022

impact27 Nov 11, 2022 •

edited

Loading

ccordoba12 Nov 12, 2022

ccordoba12 Nov 12, 2022

dalthviz Nov 14, 2022

impact27 Nov 14, 2022

ccordoba12 Nov 15, 2022 •

edited

Loading

impact27 Nov 15, 2022

ccordoba12 commented Nov 8, 2022

impact27 commented Nov 8, 2022

ccordoba12 commented Nov 9, 2022

impact27 commented Nov 11, 2022

ccordoba12 commented Nov 12, 2022 •

edited

Loading

impact27 commented Nov 14, 2022

impact27 commented Nov 15, 2022 •

edited

Loading

ccordoba12 commented Nov 16, 2022

ccordoba12 commented Nov 16, 2022

github-actions bot commented Nov 16, 2022

ccordoba12 commented Nov 16, 2022

impact27 commented Nov 16, 2022

ccordoba12 left a comment

ccordoba12 left a comment

PR: Use cached kernel for faster kernel restart (IPython console) #19092

PR: Use cached kernel for faster kernel restart (IPython console) #19092

Conversation

impact27 commented Aug 17, 2022 • edited Loading

Description of Changes

Issue(s) Resolved

Affirmation

This comment was marked as outdated.

ccordoba12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

impact27 Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Nov 8, 2022

impact27 commented Nov 8, 2022

ccordoba12 commented Nov 9, 2022

impact27 commented Nov 11, 2022

ccordoba12 commented Nov 12, 2022 • edited Loading

impact27 commented Nov 14, 2022

impact27 commented Nov 15, 2022 • edited Loading

ccordoba12 commented Nov 16, 2022

ccordoba12 commented Nov 16, 2022

github-actions bot commented Nov 16, 2022

ccordoba12 commented Nov 16, 2022

impact27 commented Nov 16, 2022

ccordoba12 left a comment

Choose a reason for hiding this comment

ccordoba12 left a comment

Choose a reason for hiding this comment

impact27 commented Aug 17, 2022 •

edited

Loading

impact27 Nov 11, 2022 •

edited

Loading

ccordoba12 Nov 15, 2022 •

edited

Loading

ccordoba12 commented Nov 12, 2022 •

edited

Loading

impact27 commented Nov 15, 2022 •

edited

Loading