Skip to content
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

Fixing #659 (output cutoff and iopub timeout) #994

Merged
merged 16 commits into from May 17, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Upgraded timeout tests for run_cell improvements

  • Loading branch information
MSeal committed May 5, 2019
commit f8c48013f5c7acb76a97921e01c6e2ad9984e961
@@ -537,7 +537,6 @@ def run_cell(self, cell, cell_index=0):
polling_exec_reply = True

while more_output or polling_exec_reply:

This comment has been minimized.

Copy link
@mpacer

mpacer May 15, 2019

Member

more_output being used as a signaling mechanism feels like a pretty significant piece of internal logic. Could you add a comment explaining why we're taking this approach?

I would think a description about the intention of the separate roles of more_output and polling_exec_reply would be enough to address the curious individual.

This comment has been minimized.

Copy link
@djherbis

djherbis May 17, 2019

Author Contributor

Great point, I added some detailed comments around this section to explain why it's written this way. Let me know if it's still not clear for future readers.


if polling_exec_reply:
if self._passed_deadline(deadline):
polling_exec_reply = False
@@ -559,7 +558,7 @@ def run_cell(self, cell, cell_index=0):
continue

if self.raise_on_iopub_timeout:
raise RuntimeError("Timeout waiting for IOPub output")
raise TimeoutError("Timeout waiting for IOPub output")
else:
self.log.warning("Timeout waiting for IOPub output")
more_output = False
@@ -568,11 +567,11 @@ def run_cell(self, cell, cell_index=0):
# not an output from our execution
continue

# Will raise CellExecutionComplete when completed
try:
# Will raise CellExecutionComplete when completed
self.process_message(msg, cell, cell_index)
except CellExecutionComplete:
break
more_output = False

# Return cell.outputs still for backwards compatability
return exec_reply, cell.outputs
@@ -32,9 +32,9 @@
from ipython_genutils.py3compat import string_types

try:
from queue import Queue # Py 3
from queue import Empty # Py 3
except ImportError:
from Queue import Queue # Py2
from Queue import Empty # Py 2
try:
TimeoutError # Py 3
except NameError:
@@ -146,6 +146,7 @@ def test_mock_wrapper(self):
shell_channel=MagicMock(get_msg=shell_channel_message_mock()),
execute=MagicMock(return_value=parent_id)
)
preprocessor.parent_id = parent_id
return func(self, preprocessor, cell_mock, message_mock)
return test_mock_wrapper
return prepared_wrapper
@@ -342,12 +343,12 @@ def test_kernel_death(self):
except TimeoutError:
pass
km, kc = preprocessor.start_new_kernel()

with patch.object(km, "is_alive") as alive_mock:
alive_mock.return_value = False
with pytest.raises(DeadKernelError):
input_nb, output_nb = preprocessor.preprocess(input_nb, {}, km=km)


def test_allow_errors(self):
"""
@@ -511,28 +512,65 @@ def test_busy_message(self, preprocessor, cell_mock, message_mock):
# Ensure no outputs were generated
assert cell_mock.outputs == []

@ExecuteTestBase.prepare_cell_mocks()
@ExecuteTestBase.prepare_cell_mocks({
'msg_type': 'stream',
'header': {'msg_type': 'stream'},
'content': {'name': 'stdout', 'text': 'foo'},
}, {
'msg_type': 'stream',
'header': {'msg_type': 'stream'},
'content': {'name': 'stderr', 'text': 'bar'}
})
def test_deadline_exec_reply(self, preprocessor, cell_mock, message_mock):
q = Queue()
# Both channels will never receive, so we expect to hit the timeout.
preprocessor.kc.shell_channel.get_msg = lambda timeout=0 : q.get(timeout=timeout)
preprocessor.kc.iopub_channel.get_msg = lambda timeout=0 : q.get(timeout=timeout)
# exec_reply is never received, so we expect to hit the timeout.
preprocessor.kc.shell_channel.get_msg = MagicMock(side_effect=Empty())
preprocessor.timeout = 1

with pytest.raises(TimeoutError):
preprocessor.run_cell(cell_mock)

assert message_mock.call_count == 3
# Ensure the output was captured
self.assertListEqual(cell_mock.outputs, [
{'output_type': 'stream', 'name': 'stdout', 'text': 'foo'},
{'output_type': 'stream', 'name': 'stderr', 'text': 'bar'}
])

@ExecuteTestBase.prepare_cell_mocks()
def test_deadline_iopub(self, preprocessor, cell_mock, message_mock):
q = Queue()
# The shell_channel will complete, so we expect only to hit the iopub timeout.
preprocessor.kc.iopub_channel.get_msg = lambda timeout=0 : q.get(timeout=timeout)
preprocessor.iopub_timeout = 1
message_mock.side_effect = Empty()
preprocessor.raise_on_iopub_timeout = True

with pytest.raises(RuntimeError):
with pytest.raises(TimeoutError):
preprocessor.run_cell(cell_mock)

@ExecuteTestBase.prepare_cell_mocks({
'msg_type': 'stream',
'header': {'msg_type': 'stream'},
'content': {'name': 'stdout', 'text': 'foo'},
}, {
'msg_type': 'stream',
'header': {'msg_type': 'stream'},
'content': {'name': 'stderr', 'text': 'bar'}
})
def test_eventual_deadline_iopub(self, preprocessor, cell_mock, message_mock):
# Process a few messages before raising a timeout from iopub
message_mock.side_effect = list(message_mock.side_effect)[:-1] + [Empty()]
preprocessor.kc.shell_channel.get_msg = MagicMock(
return_value={'parent_header': {'msg_id': preprocessor.parent_id}})
preprocessor.raise_on_iopub_timeout = True

with pytest.raises(TimeoutError):
preprocessor.run_cell(cell_mock)

assert message_mock.call_count == 3
# Ensure the output was captured
self.assertListEqual(cell_mock.outputs, [
{'output_type': 'stream', 'name': 'stdout', 'text': 'foo'},
{'output_type': 'stream', 'name': 'stderr', 'text': 'bar'}
])

@ExecuteTestBase.prepare_cell_mocks({
'msg_type': 'execute_input',
'header': {'msg_type': 'execute_input'},
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.