Add image message handler in ZMQTerminalInteractiveShell #1946

tkf · 2012-06-13T14:54:55Z

This change introduces handler for messages which contain image in
ZMQTerminalInteractiveShell. This is useful, for example, when
connecting to the kernel in which pylab inline backend is activated.
Current image handler only supports PIL backend.

This PR will fix #1575.

takluyver · 2012-06-13T15:07:59Z

I don't think we should always import PIL at startup - that will delay startup for something that's probably not needed in many cases. We could either disable it by default, so that PIL is loaded if a particular config option is set, or delay the import until the first time it receives an image.

In terms of the API, checking against a list of strings seems awkward. I'd suggest simply setting handle_image to a callable object that does what's needed. If it needs to be configurable, there's a DottedObjectName trait which we could use.

takluyver · 2012-06-13T15:10:24Z

IPython/frontend/terminal/console/interactiveshell.py

@@ -21,13 +21,21 @@
 import signal
 import sys
 import time
+from cStringIO import StringIO


For Python 3 compatibility, this should be from io import BytesIO. io is available in all the Python versions IPython supports.

tkf · 2012-06-13T15:47:07Z

Thanks for the review. Regarding the API, I think it's better to have some kind of fallback mechanism to help sharing config file in different machines. In that case, traits something like ListOfDottedObjectName is needed. But if this fallback style is not the IPython style, I will just change to DottedObjectName.

I will apply other suggestions.

takluyver · 2012-06-13T15:56:51Z

That makes sense, but I'd want to hear more about possible use cases with a fallback system - I'm generally wary of building more complex architectures without a practical reason. Especially because handling lists in config ends up with this kind of awkward dance:

if hasattr(app, 'exec_lines'):
    app.exec_lines.append(lines)
else:
    app.exec_lines = [lines]

I'm sure others will chime in on this in a bit.

If we do decide to go down that route, there's no real need for a ListOfDottedObjectName trait. It can be done with a regular List - that just means that the names are checked when they're used, rather than when they're set.

tkf · 2012-06-13T17:01:45Z

Other handlers I have in mind are:

Temp file: it creates temporally file and calls external program.
So you can set your favorite viewer.
Stream: it calls configured program and put the image in the
stdin.
Python callable: to make it more customizable.

Come to think of it, I just remember that config is a python file so you can check if PIL is available in the config file. I guess there is no need for the fallback system.

takluyver · 2012-06-13T17:11:04Z

That's all reasonable, but the list could mean "try to do all of these", or "try to do each of these until one succeeds".

It's also possible to use a single callable that tries multiple things. In fact, we already have a class for "try until one succeeds" - it's IPython.core.hooks.CommandChainDispatcher.

tkf · 2012-06-13T18:20:07Z

So, if user want to have fallback system s/he must use CommandChainDispatcher and the hypothetical "Python callback" handler. Is this what you mean?

fperez · 2012-06-13T18:43:26Z

I'd thought also that having this would be very cool, but I think it has enough implementation fine points (PIL vs native platform tools, config, etc) that we should probably mark it for 0.14. @takluyver, thoughts??

takluyver · 2012-06-13T18:49:26Z

Using a CCD as the handler, your config file would do something like:

from IPython.core.hooks import CommandChainDispatcher
from my_img_handlers import show_in_pil, save_to_file

c.ZMQTerminalInteractiveShell.image_handler = CommandChainDispatcher([show_in_pil, save_to_file])

I think I agree with @fperez that this is best done for 0.14.

fperez · 2012-06-13T18:53:18Z

OK, tagging as such.

tkf · 2012-06-13T19:16:53Z

I am fine with marking it as 0.14!

@takluyver I thought I'd have Enum(['PIL', 'Stream', 'TempFile', 'Callable']) as trait for image_handler and other options such as file_image_handler_command. So your example will be like:

c.ZMQTerminalInteractiveShell.image_handler = 'Callable'
c.ZMQTerminalInteractiveShell.callable_image_handler = CommandChainDispatcher([show_in_pil, save_to_file])

Other configuration example will be like:

c.ZMQTerminalInteractiveShell.image_handler = 'TempFile'
c.ZMQTerminalInteractiveShell.tempfile_image_handler = ['my_viewer', '--file', '{0}']  # temp file goes into '{0}'

What do you think?

takluyver · 2012-06-13T19:35:07Z

Why not just have the callable thing, and offer PIL, stream and tempfile as functions which it can be set to? That way we only need one configurable value, not two.

That's still compatible with special casing some short names so they're quick to set - here's an example that takes two special cases or an importable name:
https://github.com/ipython/ipython/blob/master/IPython/zmq/session.py#L252

tkf · 2012-06-13T19:42:37Z

How do you configure external program when you have only one configurable? Something like this?

from IPython.somewhere import TempFile
c.ZMQTerminalInteractiveShell.image_handler = TempFile(['my_viewer', '--file', '{0}'])

takluyver · 2012-06-13T19:50:07Z

There's various ways you could set the arguments for the image viewer - a second config value may make sense there. I was more objecting to the split between image_handler and callable_image_handler.

tkf · 2012-06-13T20:28:34Z

Wait, putting a callable in image_handler is an error if the trait is DottedObjectName, right? How can you have a configuration that can be a string or a callable?

takluyver · 2012-06-13T20:37:26Z

Yeah, I've gone back on my idea about DottedObjectName. I think we should have a generic configurable, that can take either a callable object or a string, and uses that to set handle_image.

tkf · 2012-06-13T20:44:12Z

What trait I should use in that case? Any? It would be nice to have "trait" that can do Or(Callable(...), Enum(...)).

takluyver · 2012-06-13T20:51:34Z

I think Any is fine - let's not overcomplicate this. I'd rather not
have to debug traitlets code, because it's very meta.

tkf · 2012-06-13T21:09:36Z

I am awkward to use Any for configurable because I think whole point of using trait is to make things type safe... I'd like to stick to my Enum solution because it become little bit verbose only when using the callable and I think you'd accept little bit verbose config file when you configuring complex stuff like user defined callable. What do you think?

takluyver · 2012-06-13T21:50:15Z

I think 2 configurables is still more complexity than 1. Both in terms of code, and for the user - it might not be obvious that callable_image_handler only affects anything if you set image_handler to "callable".

Remember that you can write a function to be called when the trait is changed, so you could validate the input there (if I understand traits correctly).

Another way would be to create a new Callable trait, and offer a convenience method select_image_handler to use the short names.

minrk · 2012-06-13T23:01:41Z

IPython/frontend/terminal/console/interactiveshell.py

+    def handle_rich_data(self, data):
+        for mime in ['image/png', 'image/jpeg', 'image/svg+xml']:
+            if mime in data:
+                self.handle_image(data, mime)


This should return after handling, in case of something that has both PNG/SVG reprs, you don't want to display both.

Fixed. Thanks for the quick review!

Don't you want to directly loop over _imagemime.keys() ?

The list defines preference order. PNG is chose over JPEG and SVG, when they are sent together, for example. But I agree that it is confusing. It took me for a while to remember what is this. Probably I should use ordered dict? Although it is not in Python 2.6.

Make sense, I should have thought of that. I just saw the same order on both, sorry.

I moved it to a variable and made it configurable.

tkf · 2012-06-13T23:02:39Z

I just made a sample implementation using Enum. In terms of code, I think it is better to have homogeneous type to simplify code. If you allow callable to image_handler, you need to add more if block. For the user side, you already have different configurables for external program. Why not for callable? It's verbose, but symmetric.

BTW, configurations I put does not show up by the command %config? What is wrong?

minrk · 2012-06-13T23:10:46Z

BTW, configurations I put does not show up by the command %config? What is wrong?

It needs to be added to the shell's configurables list to show up in %config. MagicsManager.register should be doing this.

tkf · 2012-06-15T19:10:56Z

I implemented everything I want to add in this PR. I think the remaining work is config API tweaks.

tkf · 2012-07-01T14:41:54Z

Now that the 0.13 is out, should I rebase it so that the review is easier?

fperez · 2012-07-01T19:30:32Z

No need: as long as there are no conflicts (which in this case there aren't), it doesn't matter.

Just note that we may be a bit slow with review for a bit, the release blitz put us all behind on other 'real life' things :)

tkf · 2012-07-01T19:35:39Z

No problem. Thanks for the great release!

minrk · 2012-07-01T19:40:32Z

I'll be in Sweden for the next week or so, and won't be doing much review between now and our SciPy tutorial Tuesday July 17, but then I should be doing lots of IPython work for the rest of that week while at SciPy.

bfroehle · 2012-07-27T23:10:46Z

IPython/frontend/terminal/console/interactiveshell.py

+        args = [s.format(**fmt) for s in self.stream_image_handler]
+        proc = subprocess.Popen(
+            args, stdin=subprocess.PIPE,
+            stdout=subprocess.PIPE, stderr=subprocess.PIPE)


Pedantic, but since we aren't reading stdout or stderr best practices would suggest redirecting them to /dev/null. In Python >= 3.3 this would be as simple as stdout=subprocess.DEVNULL, but instead the code gets a bit uglier:

with open(os.devnull, 'w') as devnull: proc = subprocess.Popen(..., stdout=devnull, stderr=devnull) proc.communicate(raw)

travisbot · 2012-08-21T18:36:48Z

This pull request passes (merged 61e38b8 into 3c8d448).

PIL.Image is actually a module.

travisbot · 2012-08-21T18:56:15Z

This pull request passes (merged 37580bc into 3c8d448).

takluyver · 2012-08-21T22:05:41Z

IPython/frontend/terminal/console/tests/test_image_handler.py

+            assert False
+
+        shell = self.shell
+        shell.handle_image_PIL


Should these lines be like shell.handle_image_PIL = raise_if_called?

takluyver · 2012-08-21T22:17:03Z

Thanks, I'm more comfortable with this knowing that there are tests. Inevitably I had a few questions about the test code, but they should mostly be quick fixes (apart from maybe the 'does this work on Windows?' one).

raise_if_called was not used as intended

tkf · 2012-08-22T15:12:15Z

Thanks for the comment. I fixed them.

travisbot · 2012-08-22T15:13:04Z

This pull request passes (merged 235a478 into 3c8d448).

takluyver · 2012-08-22T16:57:49Z

Test results for commit 235a478 merged into master (5308c36)
Platform: linux2

python2.7: OK (libraries not available: azure oct2py pymongo wx wx.aui)
python3.2: OK (libraries not available: azure oct2py pymongo wx wx.aui)

Not available for testing: python2.6

takluyver · 2012-08-22T17:03:38Z

I'm happy with this, thanks @tkf for making the requested changes.

@bfroehle: Do you have time to have a quick scan of this before it lands? I'm getting tunnel vision with it now. I think we're making slow progress on the PR backlog. :)

tkf · 2012-08-22T17:15:25Z

BTW, these are sample settings, in case you want to try quickly:

PIL type handler:

c.ZMQTerminalInteractiveShell.image_handler = 'PIL'

Stream type handler:

c.ZMQTerminalInteractiveShell.image_handler = 'stream'
c.ZMQTerminalInteractiveShell.stream_image_handler = ['display']

Tempfile type handler:

c.ZMQTerminalInteractiveShell.image_handler = 'tempfile'
c.ZMQTerminalInteractiveShell.tempfile_image_handler = ['eog', '{file}']

Callable type handler:

c.ZMQTerminalInteractiveShell.image_handler = 'callable'
c.ZMQTerminalInteractiveShell.callable_image_handler = some_callable

I found stream+display most useful.

takluyver · 2012-08-22T20:06:49Z

I've tested stream and tempfile. One thought: execution is blocked until you close the image viewer. Would it make more sense to bring it up and immediately return to the prompt? In the tempfile case, I guess that means we'd have to leave the tempfile lying around, but I think /tmp is supposed to be cleaned with each restart anyway.

I don't have strong views on this either way (I'm not a big user of ipython console), but it seemed worth bringing up.

tkf · 2012-08-22T20:44:58Z

Well, easiest solution is to use image viewer which exits after opening window. For example, you can use web browser for that:

c.ZMQTerminalInteractiveShell.tempfile_image_handler = ['google-chrome', '{file}']

or Emacs:

c.ZMQTerminalInteractiveShell.tempfile_image_handler = ['emacsclient', '--no-wait', '{file}']

I don't know if leaving many temporary files is ok. As you can workaround the problem by choosing image viewer, I think the current version is good for a start.

I guess better solution is to create a temporary directory which persists until client ends its session. But it is out of this PR's scope. BTW, leaving process means leaving file object opening /dev/null around. Is it ok to do?

takluyver · 2012-08-22T20:52:54Z

OK, if that's going to get complicated, let's leave it for now. We can
always add it later if people want it.

tkf · 2012-08-28T17:17:17Z

Not ready for pull yet?

takluyver · 2012-09-03T14:18:54Z

Sorry, I was away on a conference & holiday for the last week or so. I think it's OK, but I'd like someone else to have one more glance over it before we merge it.

Carreau · 2012-09-04T15:38:49Z

Looks ok to me.
Did not try it though

tkf · 2012-09-27T00:16:08Z

ping :)

Carreau · 2012-09-29T12:50:38Z

Thank for your patience @tkf I'm going to merge this one.

Add image message handler in ZMQTerminalInteractiveShell This change introduces several handlers for messages which contain image in ZMQTerminalInteractiveShell. This is useful, for example, when connecting to the kernel in which pylab inline backend is activated. This PR will fix #1575.

tkf · 2012-09-29T14:05:10Z

Thank you very much!

Add image message handler in ZMQTerminalInteractiveShell This change introduces several handlers for messages which contain image in ZMQTerminalInteractiveShell. This is useful, for example, when connecting to the kernel in which pylab inline backend is activated. This PR will fix ipython#1575.

takluyver reviewed Jun 13, 2012
View reviewed changes

minrk reviewed Jun 13, 2012
View reviewed changes

bfroehle reviewed Jul 27, 2012
View reviewed changes

tkf added 2 commits August 21, 2012 20:43

Simplify check_handler_with_file

2e36d88

"import PIL.Image" instead of "import PIL"

37580bc

PIL.Image is actually a module.

takluyver reviewed Aug 21, 2012
View reviewed changes

tkf added 4 commits August 22, 2012 16:46

Fix test_no_call_by_default

449f64a

raise_if_called was not used as intended

Use assertEqual when possible

e5c748e

Use sys.executable instead of find_cmd('python')

e9b4631

Open file after it is written

235a478

Carreau merged commit ee83d92 into ipython:master Sep 29, 2012

Add image message handler in ZMQTerminalInteractiveShell #1946

Add image message handler in ZMQTerminalInteractiveShell #1946

Conversation

tkf commented Jun 13, 2012

takluyver commented Jun 13, 2012

Choose a reason for hiding this comment

tkf commented Jun 13, 2012

takluyver commented Jun 13, 2012

tkf commented Jun 13, 2012

takluyver commented Jun 13, 2012

tkf commented Jun 13, 2012

fperez commented Jun 13, 2012

takluyver commented Jun 13, 2012

fperez commented Jun 13, 2012

tkf commented Jun 13, 2012

takluyver commented Jun 13, 2012

tkf commented Jun 13, 2012

takluyver commented Jun 13, 2012

tkf commented Jun 13, 2012

takluyver commented Jun 13, 2012

tkf commented Jun 13, 2012

takluyver commented Jun 13, 2012

tkf commented Jun 13, 2012

takluyver commented Jun 13, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkf commented Jun 13, 2012

minrk commented Jun 13, 2012

tkf commented Jun 15, 2012

tkf commented Jul 1, 2012

fperez commented Jul 1, 2012

tkf commented Jul 1, 2012

minrk commented Jul 1, 2012

Choose a reason for hiding this comment

travisbot commented Aug 21, 2012

travisbot commented Aug 21, 2012

Choose a reason for hiding this comment

takluyver commented Aug 21, 2012

tkf commented Aug 22, 2012

travisbot commented Aug 22, 2012

takluyver commented Aug 22, 2012

takluyver commented Aug 22, 2012

tkf commented Aug 22, 2012

takluyver commented Aug 22, 2012

tkf commented Aug 22, 2012

takluyver commented Aug 22, 2012

tkf commented Aug 28, 2012

takluyver commented Sep 3, 2012

Carreau commented Sep 4, 2012

tkf commented Sep 27, 2012

Carreau commented Sep 29, 2012

tkf commented Sep 29, 2012