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

support display_id in execute preprocessor #563

Merged
merged 7 commits into from
Apr 21, 2017
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented Apr 3, 2017

Added in protocol v5.1, supported in notebook 5.0.

display_id allows updating existing outputs, so keep track of mapping of display_id to outputs.

@takluyver
Copy link
Member

You've got a merge conflict on this - looks like they were old commits but you just got round to making a PR?

@@ -154,6 +153,8 @@ class ExecutePreprocessor(Preprocessor):
config=True,
help='The kernel manager class to use.'
)

_display_id_map = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a comment here describing the structure of the data stored in this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Added.

@minrk
Copy link
Member Author

minrk commented Apr 4, 2017

Rebased. Not sure why Travis doesn't feel like running...

@takluyver
Copy link
Member

Travis has decided to run now, and there's a failure on Python 2, when it tries to start a Python 3 kernel for the new notebook. Have we done something to get round that for the other notebooks?

@minrk
Copy link
Member Author

minrk commented Apr 5, 2017

Have we done something to get round that for the other notebooks?

I looked around and only saw that the test notebooks lack kernel metadata, so launch implicitly with the default kernel. Seems like we should have a better mechanism for that, but I followed the rest in this PR.

@takluyver takluyver added this to the 5.2 milestone Apr 5, 2017
@takluyver
Copy link
Member

This looks good to me, but I'll give @mpacer a chance to look at it as well.

@mpacer
Copy link
Member

mpacer commented Apr 5, 2017

I had looked at this after the initial commits but didn't say anything because I didn't know the context of this. I don't see anything to take issue with at second glance, but what is this accomplishing in the larger picture (which will help me think about the code in greater depth)?

@takluyver
Copy link
Member

Recent versions of IPython and notebook added a mechanism for updating (replacing, really) an output that has already been displayed. When you display an output, you can give it an ID (or ask IPython to assign it a random ID and return that to you). That ID is sent in the metadata of the output message. If another output message comes later with the same display ID in its metadata, the frontend will replace the previous output with that ID, rather than adding a new one. This can be used to make a progress bar, for instance. (I've deliberately handwaved the details a bit because I don't remember them, but hopefully they're documented in the message spec.)

This PR adds support for this when nbconvert is executing a notebook.

@mpacer
Copy link
Member

mpacer commented Apr 21, 2017

kk. LGTM.

@mpacer mpacer merged commit 76fa7f0 into jupyter:master Apr 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants