Simplify protocol for reporting execution errors #24
Conversation
|
We need to proceed with caution here. It seems to me that without an error message type sent over IOPub, there is no reliable (machine readable) way for clients that did not send off code for execution to know that an error occurred (since only the client that started the execution will get the execute_reply). I agree that the situation could be improved here, but I'm not sure this is better. Switching on message types simplifies logic in clients, whereas now we'll need to parse display message on clients to get error status. |
My guess is that most of them are - see the motivation section. At least IPython, IRkernel and bash_kernel send both error indications. With careful use of protocol versioning, this shouldn't be an obstacle.
I thought about this, but I don't know of any actual use cases where a client that didn't send off the code needs to know that there was an error. I'm guessing you have some in mind? Eliminating the duplication is what I'm after - I can design that the other way, by only reporting errors in IOPub messages, but I think that way would involve more disruptive changes to the protocol. |
|
We have a few kernels written in metakernel that do it one way, or the other. It would be good to clean this up. |
|
@ivanov makes a good point about the IOPub message being useful for other clients. I do think it makes sense to have a single visual representation of the error and that probably makes sense to be on the IOPub channel so other clients can represent it to the user. Are there reasons to not put it on that channel? Would that include the traceback info that is currently returned in the If the visual representation is on the IOPub message, I think it does make sense to remove the traceback info from the |
|
In this proposal, the visual representation would be a standard I'm happy to modify it so that the visual representation is also the machine-readable indication, but I'd like to see a concrete use case for other clients knowing that an error has occurred first. In all the cases I know of where a client is listening to execution it didn't request, it just displays input and output. |
|
I think display data makes sense as long as we can add metadata to indicate it encodes an error
…Sent from my iPhone
On Oct 17, 2017, at 8:10 AM, Thomas Kluyver ***@***.***> wrote:
In this proposal, the visual representation would be a standard display_data message containing the traceback, separate from the machine-readable indication that an error occurred.
I'm happy to modify it so that the visual representation is also the machine-readable indication, but I'd like to see a concrete use case for other clients knowing that an error has occurred first. In all the cases I know of where a client is listening to execution it didn't request, it just displays input and output.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
If we're going to require metadata indicating that it's an error, I'd like to remove the status field from I think doing it that way is a more disruptive change - that's why I'm trying to establish if there's a real use case that necessitates it. |
|
I think hijacking display data messages for the purposes of error status reporting is the wrong direction. An error message type makes it simple to reason about errors. We have an implementation where we're machine reading the error messages to provide some UI functionality. In short, for a certain error type, we show a specific UI workflow to overcome that error. I can see how the "status" field of execute reply can be seen as redundant - since it's possible to infer wether the execution was OK or produced an error from the other keys (ename, evalue, and traceback), - though the way the messaging spec is written now, those keys "should" be there on error, not that they "must" be there. The presence of the "status" field just makes it easy to reason about the end result. I don't like having drift in the specifics of our implementation that doesn't buy us anything - I think the cost of changing the protocol for this redundancy is greater than the benefit it would result in, in this case. |
|
I completely agree with Paul that the cost of changing the protocol for this redundancy is much greater than the benefit. Handling this with asynchronous frontends is pretty simple, since we can assume either will occur on the stream of messages. Adapting messages is quite a bit more work, as is getting all kernels to adapt to a major bump. |
|
Do we have a way in the current spec to pass a mime bundle as the
representation of an error on either of those two error messages (IOPub or
execute_reply)?
…On Wed, Oct 18, 2017 at 9:10 AM, Kyle Kelley ***@***.***> wrote:
I completely agree with Paul that the cost of changing the protocol for
this redundancy is much greater than the benefit. Handling this with
asynchronous frontends is pretty simple, since we can assume either will
occur on the stream of messages. Adapting messages is quite a bit more
work, as is getting all kernels to adapt to a major bump.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#24 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABr0GEW448cHNxIRMV0DOICmLS5MdNZks5stiLbgaJpZM4P6xZy>
.
--
Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com
|
|
@ellisonbg - no, we currently do not, but I would be in favor of extending the protocol along that dimension. |
|
Yes, I think that would be helpful. I think the textual representations of
tracebacks we are currently getting are less desirable than having a mime
bundle somewhere.
…On Wed, Oct 18, 2017 at 2:25 PM, Paul Ivanov ***@***.***> wrote:
@ellisonbg <https://github.com/ellisonbg> - no, we currently do not, but
I would be in favor of extending the protocol along that dimension.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABr0E-tsUGgNJZ7IZvvRZddW223ihP0ks5stmzmgaJpZM4P6xZy>
.
--
Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com
|
|
Hi @takluyver 👋—Zach from the @jupyter/software-steering-council here. We're working through old JEPs and closing proposals that are no longer active or may not be relevant anymore. Under Jupyter's new governance model, we have an active Software Steering Council who reviews JEPs weekly. We are catching up on the backlog now. Since there has been no active discussion on this JEP in awhile, I'd propose we close it here (we'll leave it open for two more weeks in case you'd like to revive the conversation). If you would like to re-open the discussion after we close it, you are welcome to do that too. |
There are currently two ways for kernels to report an error during execution. Many frontends and kernels have code to use both, to deal with the other end having only used one way.
This proposal is to simplify how errors are reported by deprecating some parts of the message protocol, and then removing them in protocol version 6.