Adding clear_output to kernel and HTML notebook. #789

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@ellisonbg
Member

This enables the clearing of output during the execution of a cell. It can be used for simple forms of animation in the notebook.

@fperez
Member
fperez commented Sep 14, 2011

I like the idea of the clear_output message, because I think it does have a lot of valid use cases. A few comments though on this PR before merging:

  • this is a new message type, so it needs to be documented in the spec.
  • there should be at least a test about the message

We were terrible in adding the entire messaging spec without a simple test suite for all the message types that would check compliance and should match our docs. While we may not have time right now to retrofit the whole spec with full test coverage, let's not compound the problem by adding more messages without any tests. This is an easy one to start with, and our test_messages can for now just consist of a test for this one. Over time, we can extend it to complete coverage of the entire specification.

We're starting to have other users of the messaging spec and more than one client, so it's becoming very important to have that specification really validated.

Once these two points are addressed, this one can go in as far as I'm concerned. Thanks for doing it quickly, I think it's a very good idea and will be super-useful in practice.

@minrk
Member
minrk commented Sep 14, 2011

Since this is a new message type, it should probably be added to the message spec doc.

(quote from #788 comment):

Yep, that is basically what I implemented.

No, it isn't. You implemented a new message type, for which the behavior is really quite different. I'm not saying what you did is better or worse, but it's certainly not adding hold-like function to existing display messages, since it's a new message altogether. If you did want to avoid adding a new message to the spec, this could certainly have been added tot he regular display-message spec, which would also result in a reduction in the number of messages to accomplish a 'display this and this alone' action from two to one.

@ellisonbg
Member

On Tue, Sep 13, 2011 at 9:50 PM, Min RK
reply@reply.github.com
wrote:

Since this is a new message type, it should probably be added to the message spec doc.

(quote from #788 comment):

Yep, that is basically what I implemented.

No, it isn't.  You implemented a new message type, for which the behavior is really quite different. I'm not saying what you did is better or worse, but it's certainly not adding hold-like function to existing display messages, since it's a new message altogether.  If you did want to avoid adding a new message to the spec, this could certainly have been added tot he regular display-message spec, which would also result in a reduction in the number of messages to accomplish a 'display this and this alone' action from two to one.

Sorry, I misunderstood your original idea. What I have implemented is
different. Are you thinking that clear_output would still be the
public API for this or would clear become a keyword arg for the
existing display methods/functions? If clear_output is used, how
would the calling to clear_output lead to the clear field being set on
the next iopub message that is sent?

Reply to this email directly or view it on GitHub:
#789 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg
Member

On Tue, Sep 13, 2011 at 9:43 PM, Fernando Perez
reply@reply.github.com
wrote:

I like the idea of the clear_output message, because I think it does have a lot of valid use cases.  A few comments though on this PR before merging:

  • this is a new message type, so it needs to be documented in the spec.

Yep, once we settle on the design I will do this.

  • there should be at least a test about the message

We were terrible in adding the entire messaging spec without a simple test suite for all the message types that would check compliance and should match our docs.  While we may not have time right now to retrofit the whole spec with full test coverage, let's not compound the problem by adding more messages without any tests.  This is an easy one to start with, and our test_messages can for now just consist of a test for this one.  Over time, we can extend it to complete coverage of the entire specification.

The testing of the message spec will require implementing a complex
testing machinery that can start a kernel and then use zmq sockets to
send appropriate messages and test the behavior of the kernel. This
approach would test both the kernel behavior as well as the message
spec. I don't see how to test the message spec alone. Do you have
ideas on how we can test the message spec itself? Writing the more
complex testing machinery for testing the kernel and kernel clients is
beyond my availability at this point.

But the most important part is to make sure we agree on the structure
of the new message type/field.

We're starting to have other users of the messaging spec and more than one client, so it's becoming very important to have that specification really validated.

Once these two points are addressed, this one can go in as far as I'm concerned.  Thanks for doing it quickly, I think it's a very good idea and will be super-useful in practice.

Reply to this email directly or view it on GitHub:
#789 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Member
minrk commented Sep 14, 2011

I think the clear_message is perfectly fine, and probably the cleanest implementation. The only drawback is the added message class, but I'm not sure there's a better alternative. Just trying to make sure we are thorough I. our messages.

The only reason I was thinking of adding this functionality to the display message itself is that it's really part of the same system, and there is little
to no reason that a clear call would ever not be followed by a display call.

We could also add it in a manner that would allow extensible commands issued to the display frontend, rather than having an entire slot that only clears everything, and if we ever want to do anything else that would mean another message type. For instance, clear_request could optionally only clear stdout, stderr, or a particular mime-type.

Are there any other commands we might want to issue in the frontends from the kernel that would suggest that this message should be abstracted a bit?

@ellisonbg
Member

On Wed, Sep 14, 2011 at 12:06 AM, Min RK
reply@reply.github.com
wrote:

I think the clear_message is perfectly fine, and probably the cleanest implementation.  The only drawback is the added message class, but I'm not sure there's a better alternative.  Just trying to make sure we are thorough I. our messages.

OK I think it does make sense to think through these things.

The only reason I was thinking of adding this functionality to the display message itself is that it's really part of the same system, and there is little
to no reason that a clear call would ever not be followed by a display call.

We could also add it in a manner that would allow extensible commands issued to the display frontend, rather than having an entire slot that only clears everything, and if we ever want to do anything else that would mean another message type.  For instance, clear_request could optionally only clear stdout, stderr, or a particular mime-type.

If we want to add clearing of only stdout, stderr or other output
types I would extend the clear_output message type with additional
fields.

Are there any other commands we might want to issue in the frontends from the kernel that would suggest that this message should be abstracted a bit?

At this point, I can't think of any other messages that would fit into
this category, but some may show up in the future. I think the new
message is fine for now and that we can see how things evolve.

Reply to this email directly or view it on GitHub:
#789 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@fperez
Member
fperez commented Sep 15, 2011

On Wed, Sep 14, 2011 at 1:11 PM, Brian E. Granger
reply@reply.github.com
wrote:

If we want to add clearing of only stdout, stderr or other output
types I would extend the clear_output message type with additional
fields.

Yes, since the display protocol has a concept of pyout/pyerr,
stdout/stderr, and generic mimetypes, the clear message should match
these, with a default of 'clear all' but the option for users to
specify which display 'channels' to clear.

Re. the testing issues, @minrk and I talked a bit about it over lunch.
I'd like to take a stab at building some basic testing for the spec
in the near term, but I'm not sure how much time I'll have for that.
So at the very least let's make sure that all of this is very
thoroughly documented in the spec, and whoever can take a stab at
testing the messaging machinery would be our hero. I'm really getting
a little worried about the fact that we don't have an isolated set of
tests just for the messaging part itself (they are implicitly tested
in many ways, but it would be good to have messaging-only tests to
validate conformance to our spec).

@ellisonbg
Member

I agree we do need to test the kernel and associated messaging. While
it will take a bit of work to get the inrastructure into place,
writing tests should be pretty straightforward after that is done.

Cheers,

Brian

Reply to this email directly or view it on GitHub:
#789 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@fperez
Member
fperez commented Sep 15, 2011

On Thu, Sep 15, 2011 at 3:35 PM, Brian E. Granger
reply@reply.github.com
wrote:

The clearing of particular output types will take a bit more work in
the notebook that I don't have time to work on right now.  But as long
as it is OK, I will update the message docs and merge into master.

Sounds good. I would put in the docs language about this idea, even
if it needs a

.. warning::  the per-channel clearing machinery isn't actually

implemented yet.

so that at least people know what the design intent is. The system
will behave anyways as equivalent to a python api like

def clear(channels=None)

where if channels is None then all are cleared, and otherwise a list
of one or more channels can be provided. We should probably for now
settle on, and list right away in the docs, the channel names. That
will help us frame the problem correctly now, even if the actual
implementation is missing. It seems to me we have:

  • pyout: what goes in the Out[] prompts, calls to sys.displayhook.
  • pyerr: tracebacks.
  • stdout
  • stderr
  • payloads from the display channels. Do we want to break these by
    mimetype? Or do we use 'payload' as a generic name indicating that all
    should be cleared, with the option to specify individual mimetypes
    only if the user desires? I don't really know yet...

So once we agree on these ideas and document them, I think it can go in.

I agree we do need to test the kernel and associated messaging.  While
it will take a bit of work to get the inrastructure into place,
writing tests should be pretty straightforward after that is done.

If I can find the time for it I'll jump on it.

@minrk
Member
minrk commented Oct 14, 2011

I like the channels idea. I wouldn't breakdown by mimetype, because that doesn't make a lot of sense to me. For instance, if you are doing repeated plots of a value inplace, changing the figure type from png to svg shouldn't change the behavior.

The way we have channel options elsewhere is in the KernelManager, which has:

KM.start_channels(self, shell=True, stdin=True, iopub=True, hb=True)

so, should we have:

clear_output(stdout=True, stderr=True, pyout=True, pyerr=True, payloads=True)

?

Note that I don't think pyerr and pyout can get output at any point other than the end of execution, which is necessarily after any call to clear_output(), so they may not make sense to include. I'm pretty sure about that on pyerr, but not pyout.

@minrk
Member
minrk commented Oct 18, 2011

I made the channel support changes described in my branch.

@fperez
Member
fperez commented Oct 18, 2011

Did you make a PR for @ellisonbg to merge those in here, or do you want to move this PR over to your branch?

@minrk
Member
minrk commented Oct 18, 2011

I can do a new PR if Brian doesn't have time to pull from mine.

@fperez
Member
fperez commented Oct 19, 2011

Closing here since it will be done in #893.

@fperez fperez closed this Oct 19, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment