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

Implement update_display #10048

Merged
merged 7 commits into from
Nov 18, 2016
Merged

Implement update_display #10048

merged 7 commits into from
Nov 18, 2016

Conversation

minrk
Copy link
Member

@minrk minrk commented Nov 8, 2016

display(obj, display_id='xxx')

sets a display id.

update_display(obj, display_id='xxx')

updates the display in-place.

This is implemented through adding the transient dict to display-data, where display_id resides.

Still todo:

  • handle ipykernel not supporting the extra arguments (current stable)
  • handle-API for setting a display-id automatically and updating it
  • test exercise
  • example notebook

and display(display_id='...')

for updating display_data outputs in-place.
"""
from IPython.core.interactiveshell import InteractiveShell
InteractiveShell.instance().display_pub.publish(
data=data,
metadata=metadata,
transient=transient,
**kwargs
Copy link
Member

Choose a reason for hiding this comment

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

Leaving room for later I see. 😄

metadata = kwargs.pop('metadata', None)
transient = kwargs.setdefault('transient', {})
if 'display_id' in kwargs:
transient['display_id'] = kwargs.pop('display_id')
Copy link
Member

Choose a reason for hiding this comment

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

👍

def update_display(*objs, **kwargs):
"""Update an existing display"""
kwargs['update'] = True
return display(*objs, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

👍 I like that I can use display with update = True and not have to use update_display directly as a function. 😄 /cc @ivanov

avoids unnecessary errors with older ipykernel

passing display_id or transient will trigger TypeError: unsupported arg on ipykernel that doesn't support transient data
- display(display_id=True) generates new display_id
- display(display_id=anything) returns DisplayHandle
@minrk
Copy link
Member Author

minrk commented Nov 18, 2016

Here's an example demonstrating this with current master of ipykernel, notebook now that those PRs have been merged.

This is my interpretation of proposals from @fperez and @jasongrout about creating a handle and auto-generating a display_id with a boolean flag (in this case, I used display_id=True rather than a separate track=True flag):

  • display(obj, display_id=True) generates a new display_id and returns a DisplayHandle
  • display(obj, display_id='given-id') returns a DisplayHandle with given display_id
  • display(obj) returns None
  • update_display(obj, display_id='id') display_id is required
  • DisplayHandle.display(obj) is equivalent to display(obj, display_id=handle.display_id)
  • DisplayHandle.update(obj) is equivalent to update_display(obj, display_id=handle.display_id)

So the quickest way to create and update a display without needing to generate an id yourself:

handle = display(x, display_id=True)
handle.update(y)
handle.update(z)

Possible functionality that is currently excluded:

  1. the ability for items to define and record their own display_id, such as:
    • display(x) being equivalent to display(x, display_id=x._ip_display_id_())
    • update_display(x) discovering display_id from x itself (same as above, really)
    • allowing ExecuteResults to have display_id (again, ultimately boils down to getting display_id from the object)

@minrk minrk changed the title [WIP] Implement update_display Implement update_display Nov 18, 2016
@minrk minrk added this to the 6.0 milestone Nov 18, 2016
@@ -78,7 +79,7 @@ def _display_mimetype(mimetype, objs, raw=False, metadata=None):
# Main functions
#-----------------------------------------------------------------------------

def publish_display_data(data, metadata=None, source=None, transient=None, **kwargs):
def publish_display_data(data, metadata=None, source=None, *, transient=None, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

@minrk What does the * represent? Or typo?


def update_display(obj, *, display_id, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to explain * in docstring here and above. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

It is in the docstring, in that * is Python's syntax to denote the end of positional args and display_id is labeled as keyword-only in the docstring. What would you recommend for comments regarding adopting new Python syntax?

Copy link
Member

Choose a reason for hiding this comment

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

Wow, didn't realize that was available as a feature in Python. 😎

Copy link
Member Author

Choose a reason for hiding this comment

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

I added some comments on the methods to clarify these. Thanks!

@rgbkrk rgbkrk merged commit 94d6f5b into ipython:master Nov 18, 2016
@minrk
Copy link
Member Author

minrk commented Nov 18, 2016

For @Carreau, @takluyver: I made use of the fact that we are Python 3-only now by making some new args explicitly keyword-only but still in the signature. Hooray for progress!

We can refine/revise the APIs in subsequent PRs, but this one is complete enough for people to play with and build opinions about, I think. Thanks, @rgbkrk!

@minrk minrk deleted the update-display branch November 18, 2016 16:43
@willingc
Copy link
Member

@minrk Thanks for the clarification re: '*' and arguments. I hadn't seen it used in the wild yet ;-)

@Carreau
Copy link
Member

Carreau commented Nov 18, 2016

Sweet !

@willingc , you will also see / in docstrings:

In [2]: input?
Signature: input(prompt=None, /)
Docstring:
Read a string from standard input.  The trailing newline is stripped.

The prompt string, if given, is printed to standard output without a
trailing newline before reading input.

If the user hits EOF (*nix: Ctrl-D, Windows: Ctrl-Z+Return), raise EOFError.
On *nix systems, readline is used if available.

It means "end of positional only arguments". Above it means that prompt default to None, but that you can't say input(prompt='foo'), you have to use input('foo'). It's an even more rare beast than * :-)

@rgbkrk
Copy link
Member

rgbkrk commented Nov 19, 2016

Oh does this mean no update display in Python 2 for now? I misconstrued earlier sentiment that we'd keep doing a python 2 kernel for a bit and enforcing python 3 for services (notebook, hub).

@takluyver
Copy link
Member

We're planning to keep supporting IPython 5 for Python 2 users for some time, but make IPython 6 require Python 3. As this is a new feature in IPython, Min has targeted it for IPython 6. We could maybe make a distinction between user-facing features and infrastructure features, and allow backporting some of the latter. I'd be -0.25 on doing that.

Separately, at some point the applications will require Python 3. Hub already does. For those, we don't plan to make an LTS version with Python 2 support as we have for IPython.

@minrk
Copy link
Member Author

minrk commented Nov 21, 2016

I'm okay targeting this for 5.x as well, if people have that preference. I just wanted to note that my experience was slightly improved by the fact that IPython 6 has dropped py2 support. The differences are small, so it wouldn't be a big deal to backport.

It is only ipython/ipython that's dropped py2 so far, so the ipykernel, notebook changes will still propagate to py2 users. A library that wants to support this in py2 could do ip.display_pub.publish(data=,metadata=,transient=).

@rgbkrk
Copy link
Member

rgbkrk commented Nov 21, 2016

Ok, great. I'll use ip.display_pub.publish for now within the lib I'm working with that supports both.

@Carreau
Copy link
Member

Carreau commented Nov 21, 2016

I would be ok backporting. My Backporting bot that you can just @-mention is not ready yet though.

@Carreau
Copy link
Member

Carreau commented Nov 21, 2016

TIL @mention is a reserved name on github so you don't need to take extra-care when writing it.

@rgbkrk
Copy link
Member

rgbkrk commented May 4, 2017

@Carreau would you be willing to backport this one? It's the feature I want to use and support across PySpark installations for updating job statuses.

@Carreau Carreau modified the milestones: 5.4, 6.0 May 4, 2017
@Carreau
Copy link
Member

Carreau commented May 4, 2017

I think that is reasonable enough to make API used by many package like TQDM consistant between stable version. Let's see if it applies cleanly to 5.x

@meeseeksdev backport

@lumberbot-app
Copy link
Contributor

lumberbot-app bot commented May 4, 2017

Oops, something went wrong applying the patch... Please have a look at my logs.

@Carreau Carreau added the Still Needs Manual Backport Added My MrMeeseeks when a backport fails. Help by backporting it, solving conflicts, send PR. label May 4, 2017
@Carreau
Copy link
Member

Carreau commented May 4, 2017

Ok, may need manual backport.

Carreau pushed a commit to Carreau/ipython that referenced this pull request May 5, 2017
Carreau added a commit that referenced this pull request May 10, 2017
@Carreau Carreau added backported PR that have been backported by MrMeeseeks and removed Still Needs Manual Backport Added My MrMeeseeks when a backport fails. Help by backporting it, solving conflicts, send PR. labels May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported PR that have been backported by MrMeeseeks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants