PR: Add detection of external Spyder kernels #4670

dalthviz · 2017-06-28T19:52:55Z

ccordoba12 · 2017-06-28T20:11:07Z

spyder/widgets/ipythonconsole/shell.py

@@ -45,6 +45,7 @@ class ShellWidget(NamepaceBrowserWidget, HelpWidget, DebuggingWidget):
    focus_changed = Signal()
    new_client = Signal()
    sig_got_reply = Signal()
+    sig_spyder_kernel = Signal()


Let's call this sig_is_spykernel

ccordoba12 · 2017-06-28T20:11:14Z

spyder/widgets/ipythonconsole/shell.py

@@ -77,6 +78,14 @@ def is_running(self):
        else:
            return False

+    def is_spyder_kernel(self):
+        """."""


Missing docstring

ccordoba12 · 2017-06-28T20:13:17Z

spyder/widgets/ipythonconsole/client.py

+        # attach it to other plugins
+        self.shellwidget.sig_spyder_kernel.connect(
+                lambda : self.sig_spyder_kernel.emit(self))
+


There's no need for these changes in client.py. We can directly connect to the signal emitted by shellwidget in ipythonconsole.py.

So please remove the signal above and this code here.

ccordoba12 · 2017-06-28T20:15:33Z

spyder/plugins/ipythonconsole.py

@@ -1390,6 +1390,13 @@ def process_finished(self, client):
        if self.variableexplorer is not None:
            self.variableexplorer.remove_shellwidget(id(client.shellwidget))

+    def connect_shellwidget(self, client):


connect_shellwidget -> connect_external_kernel_to_varexp

ccordoba12 · 2017-06-28T20:17:36Z

spyder/plugins/ipythonconsole.py

+        kc = client.shellwidget.kernel_client
+        self.process_started(client)
+        client.shellwidget.set_namespace_view_settings()
+        kc.stopped_channels.connect(lambda c=client: self.process_finished(c))


Let's rewrite this as

sw = client.shellwidget kc = sw.kernel_client self.process_started(client) sw.set_namespace_view_settings() kc.stopped_channels.connect(lambda c=client: self.process_finished(c))

ccordoba12 · 2017-06-28T20:20:29Z

spyder/plugins/ipythonconsole.py


        # Assign kernel manager and client to shellwidget
        client.shellwidget.kernel_client = kernel_client
        client.shellwidget.kernel_manager = kernel_manager
+        client.sig_spyder_kernel.connect(self.connect_shellwidget)
+        kernel_client.start_channels()
+        client.shellwidget.is_spyder_kernel()


These three lines only apply when master_name is None because we don't want to do it for clients connected to kernels already created by Spyder.

Maybe instead of master_name is None should we create a validation with the external_kernel variable?

The validation is precisely done here. We iterate over all open clients in the IPython console to see if we're trying to connect to the kernel of a current client. If that's the case, we don't need to connect that kernel to the Variable Explorer because it's already connected.

If that fails, then it means we're trying to connect to an external kernel, in which case we want to connect it to the Variable Explorer (if it's a Spyder kernel, of course).

What you've done was the right thing to do :-)

ccordoba12 · 2017-06-28T20:21:53Z

spyder/plugins/ipythonconsole.py


        # Assign kernel manager and client to shellwidget
        client.shellwidget.kernel_client = kernel_client
        client.shellwidget.kernel_manager = kernel_manager
+        client.sig_spyder_kernel.connect(self.connect_shellwidget)


This needs to be

client.shellwidget.sig_is_spkernel.connect( self.connect_external_kernel_to_varexp)

ccordoba12 · 2017-06-29T15:12:18Z

spyder/plugins/ipythonconsole.py

@@ -1390,6 +1390,18 @@ def process_finished(self, client):
        if self.variableexplorer is not None:
            self.variableexplorer.remove_shellwidget(id(client.shellwidget))

+    def connect_external_kernel_to_varexp(self, shellwidget):


I forgot about connecting to our Help, so let's rename this to connect_to_external_kernel

ccordoba12 · 2017-06-29T15:13:54Z

spyder/plugins/ipythonconsole.py

@@ -1390,6 +1390,18 @@ def process_finished(self, client):
        if self.variableexplorer is not None:
            self.variableexplorer.remove_shellwidget(id(client.shellwidget))

+    def connect_external_kernel_to_varexp(self, shellwidget):
+        """Connect a shellwidget to the variable explorer."""


This needs to be

Connect an external kernel to the Variable Explorer and Help.

Sorry, we need to be more specific

""" Connect an external kernel to the Variable Explorer and Help, if it is a Spyder kernel. """

ccordoba12 · 2017-06-29T16:17:07Z

This needs tests. For that, please copy the function called start_new_kernel, from

https://github.com/jupyter/jupyter_client/blob/master/jupyter_client/manager.py#L445

to app/tests/test_mainwindow.py, and add to it a new parameter called spykernel=False, and inside it this content:

...
km = KernelManager(kernel_name=kernel_name)
if spykernel:
    km._kernel_spec = SpyderKernelSpec()
...

Using this function, you need to create two kernels (inside the same test): a regular kernel and a Spyder kernel. Then you connect a client to each one of them, run something in them (e.g. a = 10) and finally verify that for the regular kernel there's nothing shown in the Variable Explorer and for the Spyder kernel there's one row present.

To get the connection file of those kernels, you need this

km, kc = start_new_kernel()
kc.connection_file

And to verify how many rows there are in the Variable Explorer, please use this (inside the test, of course :-):

main_window.variableexplorer.visibility_changed(True)
nsb = main_window.variableexplorer.get_focus_widget()
assert nsb.editor.model.rowCount() == 1

ccordoba12 · 2017-06-29T16:22:01Z

And at end of test you need to shutdown those kernels with

km.shutdown_kernel(now=True)

ccordoba12 · 2017-06-29T22:09:56Z

spyder/app/tests/test_mainwindow.py

@@ -105,6 +126,42 @@ def close_window():
 # Tests
 #==============================================================================
 @flaky(max_runs=3)
+def test_kernel_connection(main_window, qtbot):


test_kernel_connection -> test_connection_to_external_kernel

ccordoba12 · 2017-06-29T22:10:40Z

spyder/app/tests/test_mainwindow.py

@@ -105,6 +126,42 @@ def close_window():
 # Tests
 #==============================================================================
 @flaky(max_runs=3)
+def test_kernel_connection(main_window, qtbot):
+    """Test that a kernel from Spyder is detected and attached to variable explorer."""


Change this to

Test that only Spyder kernels are connected to the Variable Explorer

ccordoba12 · 2017-06-29T22:15:08Z

spyder/app/tests/test_mainwindow.py

+    shell = main_window.ipyconsole.get_current_shellwidget()
+    qtbot.waitUntil(lambda: shell._prompt_html is not None, timeout=SHELL_TIMEOUT)
+    with qtbot.waitSignal(shell.executed):
+        shell.execute('a = 0')


ccordoba12 · 2017-06-29T22:15:27Z

spyder/app/tests/test_mainwindow.py

+
+    # Assert that there are no variables in the variable explorer
+    main_window.variableexplorer.visibility_changed(True)
+    nsb = main_window.variableexplorer.get_focus_widget()


Add qtbot.wait(500) below this line to wait a little bit until the result appears in the Variable Explorer

Remember that all these operations are asynchronous

ccordoba12 · 2017-06-29T22:18:10Z

spyder/app/tests/test_mainwindow.py

+    shell = main_window.ipyconsole.get_current_shellwidget()
+    qtbot.waitUntil(lambda: shell._prompt_html is not None, timeout=SHELL_TIMEOUT)
+    with qtbot.waitSignal(shell.executed):
+        shell.execute('a = 0')


ccordoba12 · 2017-06-29T22:18:46Z

spyder/app/tests/test_mainwindow.py

+    # Assert that a variable is visible in the variable explorer
+    main_window.variableexplorer.visibility_changed(True)
+    nsb = main_window.variableexplorer.get_focus_widget()
+    qtbot.waitUntil(lambda: nsb.editor.model.rowCount() == 1, timeout=500)


Change this line simply to

qtbot.wait(500)

ccordoba12 · 2017-06-29T22:18:53Z

spyder/app/tests/test_mainwindow.py

+    main_window.variableexplorer.visibility_changed(True)
+    nsb = main_window.variableexplorer.get_focus_widget()
+    qtbot.waitUntil(lambda: nsb.editor.model.rowCount() == 1, timeout=500)
+


Remove blank

ccordoba12 · 2017-06-29T22:22:06Z

spyder/plugins/ipythonconsole.py

    def _create_client_for_kernel(self, connection_file, hostname, sshkey,
                                  password):
        # Verifying if the connection file exists
        try:
            cf_path = osp.dirname(connection_file)
            cf_filename = osp.basename(connection_file)
+            # To change a possible empty string to None
+            falsy_to_none = lambda arg: arg if arg else None
+            cf_path = falsy_to_none(cf_path)


There's no need of that falsy_to_none function. You can simply write

cf_path = cf_path if cf_path else None

ccordoba12 · 2017-06-30T00:09:20Z

spyder/app/tests/test_mainwindow.py

+    main_window.ipyconsole._create_client_for_kernel(kc.connection_file, None,
+                                                     None, None)
+    shell = main_window.ipyconsole.get_current_shellwidget()
+    qtbot.waitUntil(lambda: shell._prompt_html is not None, timeout=SHELL_TIMEOUT)


You forgot to remove them from here :-)

I think this is needed to update the shell variable and get the current shellwidget (to execute the code in the new console with the kernel spyder), no?

Yes, you're right! Then please remove the same lines at the beginning of the test because it makes no sense to have them there.

ccordoba12 · 2017-06-30T01:21:03Z

spyder/app/tests/test_mainwindow.py

+    main_window.variableexplorer.visibility_changed(True)
+    nsb = main_window.variableexplorer.get_focus_widget()
+    qtbot.wait(500)
+


Remove this blank line.

ccordoba12 · 2017-06-30T01:23:39Z

@dalthviz, you committed some garbage files and commented some tests too in your last commit, so please redo it and make a forced push to not have that commit as part of this PR.

dalthviz · 2017-06-30T02:27:44Z

@ccordoba12 you are right, sorry for that, now it is ok 👍

ccordoba12 · 2017-06-30T02:48:49Z

@dalthviz, please move test_calltip to be the first one on test_mainwindow.py because it's timing out in Appveyor if run after test_connection_to_external_kernel.

ccordoba12 · 2017-06-30T03:16:49Z

@dalthviz, after you solve my last (simple) comments, this is ready to be merged!

ccordoba12

Thanks @dalthviz for your hard work on this one.

Fixes #4664

Adds detection of Spyder kernels.

512d674

dalthviz added component:IPython Console type:Enhancement labels Jun 28, 2017

dalthviz added this to the v3.2 milestone Jun 28, 2017

dalthviz self-assigned this Jun 28, 2017

dalthviz requested a review from ccordoba12 June 28, 2017 19:52

Sets settings for the namespace.

4088e6e

ccordoba12 reviewed Jun 28, 2017

View reviewed changes

Improvements.

9ac890f

dalthviz mentioned this pull request Jun 29, 2017

PR: Add option to open an IPython console connected to the current notebook spyder-ide/spyder-notebook#79

Merged

dalthviz added 2 commits June 29, 2017 08:20

Fix for the tests.

48d56e6

Deletes print.

3428fcc

ccordoba12 reviewed Jun 29, 2017

View reviewed changes

ccordoba12 changed the title ~~PR: Add detection of Spyder kernels~~ PR: Add detection of external Spyder kernels Jun 29, 2017

dalthviz added 2 commits June 29, 2017 16:35

Adds a test, fix to use the kernel_id.

dfd86b9

Fixes in the test.

c71ffa7

ccordoba12 reviewed Jun 29, 2017

View reviewed changes

Fixes in the test.

8c40090

ccordoba12 reviewed Jun 30, 2017

View reviewed changes

Improvements.

d60775b

dalthviz force-pushed the fixes_issue_4664 branch from f47a817 to d60775b Compare June 30, 2017 02:13

Changes in the order of the tests.

2d42e24

Adds comment in the tests.

807c0f0

ccordoba12 approved these changes Jun 30, 2017

View reviewed changes

ccordoba12 merged commit 9a92ca0 into spyder-ide:3.x Jun 30, 2017

ccordoba12 added a commit that referenced this pull request Jun 30, 2017

Merge from 3.x: PR #4670

d2593ba

Fixes #4664

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Add detection of external Spyder kernels #4670

PR: Add detection of external Spyder kernels #4670

dalthviz commented Jun 28, 2017

ccordoba12 Jun 28, 2017

ccordoba12 Jun 28, 2017

ccordoba12 Jun 28, 2017

ccordoba12 Jun 28, 2017

ccordoba12 Jun 28, 2017

ccordoba12 Jun 28, 2017 •

edited

Loading

dalthviz Jun 29, 2017

ccordoba12 Jun 29, 2017

ccordoba12 Jun 28, 2017

ccordoba12 Jun 29, 2017

ccordoba12 Jun 29, 2017

ccordoba12 Jun 29, 2017

ccordoba12 commented Jun 29, 2017 •

edited

Loading

ccordoba12 commented Jun 29, 2017

ccordoba12 Jun 29, 2017

ccordoba12 Jun 29, 2017

ccordoba12 Jun 29, 2017

ccordoba12 Jun 29, 2017

ccordoba12 Jun 29, 2017 •

edited

Loading

ccordoba12 Jun 29, 2017

ccordoba12 Jun 29, 2017

ccordoba12 Jun 29, 2017

ccordoba12 Jun 29, 2017

ccordoba12 Jun 30, 2017

dalthviz Jun 30, 2017

ccordoba12 Jun 30, 2017

ccordoba12 Jun 30, 2017

ccordoba12 commented Jun 30, 2017

dalthviz commented Jun 30, 2017

ccordoba12 commented Jun 30, 2017

ccordoba12 commented Jun 30, 2017

ccordoba12 left a comment

PR: Add detection of external Spyder kernels #4670

PR: Add detection of external Spyder kernels #4670

Conversation

dalthviz commented Jun 28, 2017

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 Jun 28, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Jun 29, 2017 • edited Loading

ccordoba12 commented Jun 29, 2017

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 Jun 29, 2017 • 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

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 commented Jun 30, 2017

dalthviz commented Jun 30, 2017

ccordoba12 commented Jun 30, 2017

ccordoba12 commented Jun 30, 2017

ccordoba12 left a comment

Choose a reason for hiding this comment

ccordoba12 Jun 28, 2017 •

edited

Loading

ccordoba12 commented Jun 29, 2017 •

edited

Loading

ccordoba12 Jun 29, 2017 •

edited

Loading