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

Inject display into builtins #10596

Merged
merged 3 commits into from May 30, 2017
Merged

Inject display into builtins #10596

merged 3 commits into from May 30, 2017

Conversation

@Carreau
Copy link
Member

@Carreau Carreau commented May 24, 2017

display is an underutilized feature of IPython which is relatively hard
to access to. You need to explicitly import from IPython.display,
which not only is long to type, but users get confused if they should
use IPython.display or IPython.display.display also many tutorials
tell users to import from IPython.core.display which is not right.

So inject that directly in builtin module – so that user can overwrite
and delete without issues – and document display a lot more.

cc @rgbkrk, @ivanov

@Carreau Carreau requested review from ivanov and rgbkrk May 24, 2017
@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented May 24, 2017

🙇 Thank you!

@rgbkrk
rgbkrk approved these changes May 24, 2017
>>> del plain.type_printers[int]
>>> display(3+6)
9

This comment has been minimized.

@rgbkrk

rgbkrk May 24, 2017
Member

This is so great to have these examples.

This comment has been minimized.

@Carreau

Carreau May 24, 2017
Author Member

They are also in the Custom Display logic notebook.

This comment has been minimized.

@ivanov

ivanov May 24, 2017
Member

Totally minor nitpick, but it'd be even more clear if the display calls both used the same expression inside...
Make both display(3+6) for example, so the top one is even more clear about how it's doing the formatting...

@@ -618,6 +619,7 @@ def init_builtins(self):
# removing on exit or representing the existence of more than one
# IPython at a time.
builtin_mod.__dict__['__IPYTHON__'] = True
builtin_mod.__dict__['display'] = display

This comment has been minimized.

@rgbkrk

rgbkrk May 24, 2017
Member

away we go!

This comment has been minimized.

@DTAIEB

DTAIEB May 24, 2017

I think that pixiedust should fallback on ipython's display

Makes sense, I'll enter a Github issue in PixieDust repo

This comment has been minimized.

@Carreau

Carreau May 25, 2017
Author Member

Makes sense, I'll enter a Github issue in PixieDust repo

Thanks !

Or, if we can figure out a way to use IPython machinery for pixeldust (as we already have dispatch per object types) that would be great. Feel free to crosslink here or ping me.

@Carreau Carreau force-pushed the Carreau:display-builtin branch from c3041ad to bad9eea May 24, 2017
@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented May 24, 2017

I was recently alerted to the fact that pixiedust exposes display as a top level for use in the notebook, likely to match up with zeppelin/databricks/etc., which is also why I said we should expose display by default.

The extra thing most of these other displayers do though is hook the display for data frames (spark and pandas) and Spark GraphFrames.

@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented May 24, 2017

/cc @DTAIEB

@Carreau
Copy link
Member Author

@Carreau Carreau commented May 24, 2017

I was recently alerted to the fact that pixiedust exposes display as a top level for use in the notebook, likely to match up with zeppelin/databricks/etc., which is also why I said we should expose display by default.

Where is it injected ? in user_ns or builtins ? AFAICT that will lead to user having:

Pixiedust didn't find any vizualization plugins that can render this object: <__main__.Foo object at 0x7fd58600d810>

Or whatever object is given to it. I think that pixiedust should fallback on ipython's display.
It should even to hook into IPython formatter. Right now if you display(df) you get a nice rich representation. If you just return it as the last statemtn of the cell you get a DataFrame[Colors: string, %: bigint] output, which is less nice.

Registering formatter should allow to have rich representation in both case which should make the experience more uniform for users.

The other things is that with this PR IPython's display is restored as soon as you delete any things that shadows it:

$ ipython
Python 3.6.0 |Continuum Analytics, Inc.| (default, Dec 23 2016, 13:19:00)
Type 'copyright', 'credits' or 'license' for more information
IPython 6.1.0.dev -- An enhanced Interactive Python. Type '?' for help.

In [1]: display
Out[1]: <function IPython.core.display.display>

In [2]: display = lambda x:X

In [3]: display
Out[3]: <function __main__.<lambda>>

In [4]: del display

In [5]: display
Out[5]: <function IPython.core.display.display>
Copy link
Member

@ivanov ivanov left a comment

Looks sweet, Matthias! With the display_update stuff, I think display is an even more powerful feature now that should available more prominently.

>>> del plain.type_printers[int]
>>> display(3+6)
9

This comment has been minimized.

@ivanov

ivanov May 24, 2017
Member

Totally minor nitpick, but it'd be even more clear if the display calls both used the same expression inside...
Make both display(3+6) for example, so the top one is even more clear about how it's doing the formatting...

You can refer to the documentation on IPython display formatters in order to
register custom formatters for already existing types.
since IPython 5.4 and 6.1 display is automatically made availlable to the

This comment has been minimized.

@ivanov

ivanov May 24, 2017
Member

" Since IPython 5.4 and 6.1 , " (capital S, comma after 6.1)

since IPython 5.4 and 6.1 display is automatically made availlable to the
user without import. If you are using display in a document that might be
used in a pure python context or with older version of IPython use the

This comment has been minimized.

@ivanov

ivanov May 24, 2017
Member

comma after IPython

"""
raw = kwargs.pop('raw', False)
if transient is None:
transient = {}
if display_id:
if display_id == True:
if display_id is True:

This comment has been minimized.

@ivanov

ivanov May 24, 2017
Member

Nice bug fix! display_id=1 would have always generated a new ID for this.

@@ -16,6 +16,8 @@
from traitlets import Instance, Float
from warnings import warn

from IPython.display import display

This comment has been minimized.

@ivanov

ivanov May 24, 2017
Member

Not sure why this import is here. Could you document that if it needs to stay here?

@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented May 24, 2017

The other things is that with this PR IPython's display is restored as soon as you delete any things that shadows it:

Excellent!

@minrk
minrk approved these changes May 25, 2017
@minrk
Copy link
Member

@minrk minrk commented May 25, 2017

Great!

@ivanov since you mention the update display stuff, do you think update_display should be included, or is that too much? I realize builtins is a pretty precious namespace.

Minor technical detail: should this go in .user_ns_hidden instead of builtins, so it doesn't show up as available in module code?

@Carreau
Copy link
Member Author

@Carreau Carreau commented May 25, 2017

Minor technical detail: should this go in .user_ns_hidden instead of builtins, so it doesn't show up as available in module code?

It can, I did that originally, it's more complicated if we want to re-inject it when user delete it/shadow it, but doable.

I believe update_display is more a technical under the hood detail, while everyone should know about (and use) display.

This is still a relatively heavy usability change so I'm probing what other are thinking. I'd like @fperez input on this one as well (not on the code itself, but happy to get review) but wether it's ok to have display() available by default.

@Carreau Carreau force-pushed the Carreau:display-builtin branch 2 times, most recently from 4e5fd23 to b3c4dfc May 25, 2017
@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented May 25, 2017

I agree that we shouldn't surface update_display. We can still use update = True on display anyways.

@Carreau
Copy link
Member Author

@Carreau Carreau commented May 25, 2017

On the builtins vs user_ns_hidden:

When in user_ns_hidden it happily accept to delete it but the object will reapear:

$ ipython
Python 3.6.0 |Continuum Analytics, Inc.| (default, Dec 23 2016, 13:19:00)
Type 'copyright', 'credits' or 'license' for more information
IPython 6.1.0.dev -- An enhanced Interactive Python. Type '?' for help.

In [1]: display
Out[1]: <function IPython.core.display.display>

In [2]: display = 1

In [3]: del display

In [4]: display
Out[4]: <function IPython.core.display.display>

In [5]: del display

In [6]: display
Out[6]: <function IPython.core.display.display>

If in builtins, il will complain that object can't be found if trying to delete it:

$ ipython
Python 3.6.0 |Continuum Analytics, Inc.| (default, Dec 23 2016, 13:19:00)
Type 'copyright', 'credits' or 'license' for more information
IPython 6.1.0.dev -- An enhanced Interactive Python. Type '?' for help.

In [1]: display
Out[1]: <function IPython.core.display.display>

In [2]: display = 1

In [3]: display
Out[3]: 1

In [4]: del display

In [5]: display
Out[5]: <function IPython.core.display.display>

In [6]: del display
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-6-78e18c419bea> in <module>()
----> 1 del display

NameError: name 'display' is not defined

The user_ns_hidden also have the case that the object reapears only after each cell execution so cutting cell may change behavior. For example NameError vs NoError

In [1]: display=1
   ...: print(display)
   ...: del display
   ...: print(display)
   ...:
1
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-2-bb2e1216e38f> in <module>()
      2 print(display)
      3 del display
----> 4 print(display)

NameError: name 'display' is not defined

In [2]: display = 1
   ...: print(display)
   ...: del display
1

In [3]: print(display)
<function display at 0x102dfbd08>

We can also not reinject it, but I believe it is the wrong solution.

@Carreau
Copy link
Member Author

@Carreau Carreau commented May 25, 2017

Or, we can change the exec for exec(code_obj, self.user_global_ns, ChainMap(self.user_ns, hidden_ns))

(you can't ChainMap global, exec refuses), that should avoid a lot of the hidden_ns dance we do.

Carreau added 3 commits May 29, 2017
display is an underutilized feature of IPython which is relatively hard
to access to. You need to explicitly import from `IPython.display`,
which not only is long to type, but users get confused if they should
use `IPython.display` or `IPython.display.display` also many tutorials
tell users to import from IPython.core.display which is not right.

So inject that directly in builtin module – so that user can overwrite
and delete without issues – and document display a lot more.
@Carreau Carreau force-pushed the Carreau:display-builtin branch from b3c4dfc to 1dc063d May 29, 2017
@Carreau
Copy link
Member Author

@Carreau Carreau commented May 29, 2017

Rebased, updated, tested, documented.

@Carreau Carreau added this to the 5.4 milestone May 29, 2017
@rgbkrk
rgbkrk approved these changes May 29, 2017
@Carreau
Copy link
Member Author

@Carreau Carreau commented May 30, 2017

@fperez seem to +1,
@jasongrout was a good devil advocate and seem to agree this is a good idea
@willingc seem to thing for users.

There is one (longstanding) issue that calling display in a pure Python terminal break the repl by instantiating an InteractiveShell (which has crazy side effects like nuking your namespace and setting sys.ps1), I'm unsure it's a blocker for this PR.

@takluyver
Copy link
Member

@takluyver takluyver commented May 30, 2017

But that will only happen if you start a plain Python shell after starting IPython in the same process, right? I don't think that comes up very often.

@Carreau
Copy link
Member Author

@Carreau Carreau commented May 30, 2017

But that will only happen if you start a plain Python shell after starting IPython in the same process, right? I don't think that comes up very often.

Not even, it break if you just call display in a pure python repl. We can fix that, the only question if what to do. If Interactiveshell._instance is None fallback on print ? Even if raw=True ?

$ python
Python 3.6.0 |Continuum Analytics, Inc.| (default, Dec 23 2016, 13:19:00)
[GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from IPython.display import display
>>> a = 1
>>> display(a)
1
In : a
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'a' is not defined
In :

But that's another issue.

@rgbkrk rgbkrk merged commit 4f93bf1 into ipython:master May 30, 2017
4 checks passed
4 checks passed
codecov/patch 84.61% of diff hit (target 0%)
Details
codecov/project 66.82% (+0.01%) compared to 37eb85b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented May 30, 2017

@meeseeksdev backport

@meeseeksdev
Copy link
Contributor

@meeseeksdev meeseeksdev bot commented May 30, 2017

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

Carreau added a commit to Carreau/ipython that referenced this pull request May 30, 2017
Merge pull request ipython#10596 from Carreau/display-builtin

Inject display into builtins
@Carreau Carreau added the backported label May 30, 2017
Carreau added a commit that referenced this pull request May 30, 2017
Backport PR #10596 on branch 5.x
@takluyver
Copy link
Member

@takluyver takluyver commented May 31, 2017

Oh, I see, you mean if code explicitly imports it. That's not affected by injecting display into builtins when IPython runs, though.

@Carreau
Copy link
Member Author

@Carreau Carreau commented May 31, 2017

Oh, I see, you mean if code explicitly imports it. That's not affected by injecting display into builtins when IPython runs, though.

Yes, sorry the missing datapoint is:
If we inject it into builtins, we might want nbconvert --to script to add the import at the top.

@takluyver
Copy link
Member

@takluyver takluyver commented May 31, 2017

Ah, that makes sense. Yeah, that's tricky.

@Carreau Carreau deleted the Carreau:display-builtin branch May 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.