-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Use cached kernel for faster kernel restart (IPython console) #19092
PR: Use cached kernel for faster kernel restart (IPython console) #19092
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
d2c8aa7
to
11e092b
Compare
77594f2
to
2b6dd56
Compare
b672574
to
0dd53df
Compare
0dd53df
to
fb26208
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small suggestions for you @impact27, otherwise looks good to me.
restart_kernel = self.config_shortcut( | ||
self.ipyclient.restart_kernel, | ||
context='ipython_console', | ||
name='Restart kernel', | ||
parent=self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctrl + . is hardcoded in qtconsole, so I skipped this. It should work now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still doesn't work for me @impact27. Although now Ctrl+.
doesn't work either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is perplexing. I will investigate but because on Mac ctrl + . And cmd + . Are distinct, it is not trivial for me to debug this.
@impact27, please also take care of the |
@ccordoba12 the |
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
c167b59
to
9d886ad
Compare
I tested this manually and found a small regression: the Perhaps you introduced that regression in commit 52532ad (not sure about that though). |
This is strange as well, I see: Which is what I expect from the line: |
I understood what happens here: if you go to the Options menu and click on the However, if you press You can see that the dialog is not translated, which means that comes from Qtconsole. We prevent that in Spyder 5 by overriding the 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
|
I understand why it is different on mac: cmd + . works but ctrl + . has the behaviour you described! |
My understanding so far is: Now that 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 |
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). |
/Show binder |
No luck either, as you can check as well. |
I restored it, but I think you can't change the shortcut that way. However it appears to be already the case with master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now. Thanks @impact27 for your hard work and patience with this one.
This is a really cool improvement!
Description of Changes
restart kernel instantaneously by using the cached kernel
depends on #19411
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: