Add missing closed method to QtInProcessChannel

[rayosborn]

In PR #574, a new function, closed() was added to the QtZMQSocketChannel class in order to check whether a channel was closed before flushing the stream. This must also be added to the QtInProcessChannel to ensure compatibility. Since in-process channels are never closed, this function should always return the value False.

This PR corrects the NeXpy issue #378 using the latest development version of qtconsole.

[ccordoba12]

Thanks for your contribution @rayosborn! Perhaps this is too much to ask, but is there any chance you could add a test for the InProcess integration? That way we will be sure of not breaking it in the future.

[rayosborn]

Thanks for the suggestion. I will have a look at the current test functions to see if they could be adapted for use with the in-process kernel. I can't guarantee I have the right expertise, but I agree that it could be a useful addition to the test suite.

[adam-urbanczyk]

@ccordoba12 CQ-editor is also suffering from this issue. AFAICT one should parameterize some tests with different KernelManagers (i.e. include InProcessKernelManager), e.g. here:

qtconsole/qtconsole/tests/test_comms.py

Line 13 in 949c0ec

self.kernel_manager = QtKernelManager()

What would be the simplest way to achieve this with the current code base? I only know how to do this nicely with pytest.

[ccordoba12]

A simple test that checks that the console can run code with the in-process kernel manager would do for now. No need to try to make our entire test suite run with it.

[adam-urbanczyk]

@rayosborn AFAICT something like this should be enough. Could you try?

class InProcessTests(unittest.TestCase):

    def setUp(self):
        """Open a kernel."""
        self.kernel_manager = QtInProcessKernelManager()
        self.kernel_manager.start_kernel()
        self.kernel_client = self.kernel_manager.client()
        self.kernel_client.start_channels(shell=True, iopub=True)
        self.blocking_client = self.kernel_client.blocking_client()
        self.blocking_client.start_channels(shell=True, iopub=True)
        self.comm_manager = self.kernel_client.comm_manager

        # Check if client is working
        self.blocking_client.execute('print(0)')
        try:
            self._get_next_msg()
            self._get_next_msg()
        except TimeoutError:
            # Maybe it works now?
            self.blocking_client.execute('print(0)')
            self._get_next_msg()
            self._get_next_msg()


    def tearDown(self):
        """Close the kernel."""
        if self.kernel_manager:
            self.kernel_manager.shutdown_kernel(now=True)
        if self.kernel_client:
            self.kernel_client.shutdown()
            
    def _get_next_msg(self, timeout=10):
        # Get status messages
        timeout_time = time.time() + timeout
        msg_type = 'status'
        while msg_type == 'status':
            if timeout_time < time.time():
                raise TimeoutError
            try:
                msg = self.blocking_client.get_iopub_msg(timeout=3)
                msg_type = msg['header']['msg_type']
            except Empty:
                pass
        return msg

    def test_execute(self):
        self.kernel_client.execute('a=1')
        assert self.kernel_manager.kernel.shell.user_ns.get('a') == 1

[rayosborn]

@adam-urbanczyk, I get errors unless I remove the initialization of blocking_client and the calls to shutdown the kernel and client. Unfortunately, it is then not sensitive to whether the new closed function is present or not. I'm not so familiar with using the unittest framework so there may be things I need to initialize first. Does it work for you?

[adam-urbanczyk]

This seems to be better:

    def setUp(self):
        """Open a kernel."""
        self.kernel_manager = QtInProcessKernelManager()
        self.kernel_manager.start_kernel()
        self.kernel_client = self.kernel_manager.client()

    def tearDown(self):
        pass

    def test_execute(self):
        
        # check that closed works as expected
        assert not self.kernel_client.iopub_channel.closed()
        
        # check that running code works
        self.kernel_client.execute('a=1')
        assert self.kernel_manager.kernel.shell.user_ns.get('a') == 1

Maybe commit just commit some test and ask the owners for feedback? I'm not familiar with the codebase enough to be able to tell what is sufficient.

[rayosborn]

The trimmed down version seems to work. I got the test to fail by removing the closed function from in process.py, so it at least catches the reason for this PR. @adam-urbanczyk, thanks for doing the work.

I was able to add the following to the tearDown function.

def tearDown(self):
    self.kernel_client.stop_channels()
    self.kernel_manager.shutdown_kernel()

I am not sure why that failed before but I have these lines in the NeXpy shutdown code. I presumably copied them from another package, but it was a long time ago and I'm not sure why they are needed. They emit some stopped signals and 'join' the ioPub thread. It's possible that other applications need this, so I will leave them in for now.

I will add this to the test suite in the current PR, assuming you have no objection.

[adam-urbanczyk]

Great, @ccordoba12 would you be OK with merging now?

[ccordoba12]

Looks good to me now. Thanks @rayosborn and @adam-urbanczyk for your help with this!

[ccordoba12]

I just released 5.4.3 with this change.