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

Conversation

Projects
None yet
3 participants
@minrk
Copy link
Member

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

This comment has been minimized.

Copy link
Member

takluyver commented Apr 3, 2017

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 = {}

This comment has been minimized.

@takluyver

takluyver Apr 3, 2017

Member

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

This comment has been minimized.

@minrk

minrk Apr 4, 2017

Author Member

Good call. Added.

minrk added some commits Nov 22, 2016

@minrk minrk force-pushed the minrk:display_id branch from 8cfd8ef to d38f56f Apr 4, 2017

@minrk

This comment has been minimized.

Copy link
Member Author

minrk commented Apr 4, 2017

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

@minrk minrk force-pushed the minrk:display_id branch from d38f56f to b027364 Apr 4, 2017

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 4, 2017

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 minrk force-pushed the minrk:display_id branch from d6f4f3d to 7d08eec Apr 4, 2017

@minrk

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

takluyver commented Apr 5, 2017

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

@mpacer

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

takluyver commented Apr 6, 2017

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

This comment has been minimized.

Copy link
Member

mpacer commented Apr 21, 2017

kk. LGTM.

@mpacer mpacer merged commit 76fa7f0 into jupyter:master Apr 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mpacer mpacer added to_changelog and removed to_changelog labels May 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment