Adds jupyter-run to jupyter_client #184
Conversation
| stream = getattr(sys, content['name']) | ||
| stream.write(content['text']) | ||
| elif msg_type in ('display_data', 'execute_result', 'error'): | ||
| if msg_type == 'error': |
There was a problem hiding this comment.
This nesting is a bit odd - why not have elif msg_type == 'error' broken out separately before display_data and execute_result.
There was a problem hiding this comment.
I think this is cut and paste from Min's link:
https://github.com/dask/distributed/pull/370/files#diff-b7f744240c50427e50e9c844a13a00fdR43
so @minrk can comment on that.
|
If I do If I do |
|
Agreed regarding stopping-on-error. I added two checks for error conditions, as I found some kernels had not yet used the error message type, so printing on stderr is also detected as an error code here. |
|
Mentioning #183 as I forgot before. This should take care of that issue. |
|
Sorry, I don't think printing to stderr should count as an error - it's common to write logging and things to stderr even when there's no exception condition. We should, however, get the |
|
@takluyver I agree about counting as an error. However, there some kernels that need to be update to be able to handle this. I'll add "aborted" too. |
| return_code = 1 | ||
| else: | ||
| sys.stdout.write(content['data'].get('text/plain', '')) | ||
| elif msg_type == 'aborted': |
There was a problem hiding this comment.
This is not a message type. We need to get a message from the shell channel with self.kernel_client.get_shell_msg, check that it's an execute_reply message, and check its 'status' field.
http://jupyter-client.readthedocs.io/en/latest/messaging.html#execution-results
|
Exit code should only consider the |
|
Removed the break condition when printing to stderr, and removed the "aborted" msg_type handler. Somehow, this needs to poll handle iopub and shell, and print output from both. But I don't think @minrk 's dask link was doing this, right? |
|
Oh, I think I see my issue: not all kernels publish the execute_reply also on the iopub channel. If they do, I think that this will work. What would probably also make this more clear is:
|
|
No kernels should publish When the kernel gets an So the error information can be duplicated: in an There should be no need to poll shell or print output from it. We can poll iopub until we get the status:idle message telling us that we've seen all outputs. And then we can fetch the execute_reply message from shell. |
|
@takluyver Ok, thanks. That is confusing, and isn't reflected in the code that @minrk linked to, that I can see. I don't see how the code would keep the error message from printing out twice, if it can come in two forms (one on iopub and one on shell). I think I'll wait for someone to write the ultimate function for handling this. Then I'll call it from this code. |
|
Updated PR to use reply=True. Very nice! |
|
I think that this can be tagged 4.5 like #185 |
|
Neat. |
|
@dsblank sorry for the delay on this one. Thanks! Marked as 5.0, since that's what the next release will be. |
No description provided.