Add missing closed method to QtInProcessChannel
#575
Conversation
A new function, `closed()`, is needed for compatibility with the `frontend_widget`.
|
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. |
|
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. |
|
@ccordoba12 CQ-editor is also suffering from this issue. AFAICT one should parameterize some tests with different KernelManagers (i.e. include qtconsole/qtconsole/tests/test_comms.py Line 13 in 949c0ec What would be the simplest way to achieve this with the current code base? I only know how to do this nicely with pytest. |
|
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. |
|
@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 |
|
@adam-urbanczyk, I get errors unless I remove the initialization of |
|
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') == 1Maybe 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. |
|
The trimmed down version seems to work. I got the test to fail by removing the I was able to add the following to the 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 I will add this to the test suite in the current PR, assuming you have no objection. |
|
Great, @ccordoba12 would you be OK with merging now? |
ccordoba12
changed the title
closed() function to QtInProcessChannelclosed() function to QtInProcessChannel
ccordoba12
changed the title
closed() function to QtInProcessChannelclosed method to QtInProcessChannel
There was a problem hiding this comment.
Looks good to me now. Thanks @rayosborn and @adam-urbanczyk for your help with this!
|
I just released 5.4.3 with this change. |
In PR #574, a new function,
closed()was added to theQtZMQSocketChannelclass in order to check whether a channel was closed before flushing the stream. This must also be added to theQtInProcessChannelto ensure compatibility. Since in-process channels are never closed, this function should always return the valueFalse.This PR corrects the NeXpy issue #378 using the latest development version of qtconsole.