display images, HTML... and MIME rendering #105

cosmos72 · 2018-04-07T14:43:08Z

show as image each expression result that implements Go interface image.Image

…rash on import of complicates packages as "gonum.org/v1/plot/plotter", and other improvements

…Go interface 'image.Image'

dwhitena · 2018-04-20T21:14:46Z

This is great @cosmos72. Thank you so much! I have been swamped, but I'm hoping to test this soon. @SpencerPark, have you been able to test this?

…omacro/fast/Interp.RunExpr()

…es/tree/mime-rendering to implement rendering of HTML, JSON, JPEG, PNG, SVG, LaTeX... New functions: display.PNG([]byte), display.JPEG([]byte), display.HTML(string), display.SVG(string) ...

cosmos72 · 2018-06-02T16:23:21Z

I just updated my display_image branch (and thus this pull request) by merging and simplifying @SpencerPark contribution #110

See #17 for discussion

From SpencerPark <spinnr95@gmail.com>: Lock writes to the sockets to fix a bug with interleaved publishes.

SpencerPark · 2018-06-04T02:32:34Z

display.go

+// See http://ipython.readthedocs.io/en/stable/api/generated/IPython.display.html#IPython.display.display
+// for a good overview of the support types. Note: This is missing _repr_markdown_ and _repr_javascript_.
+
+var globalReceipt *msgReceipt // ugly global variable. any alternative?


I think restricting this to just rendering (returning a mime bundle) and pulling the actual display portion will clean this up a lot.

SpencerPark · 2018-06-04T02:34:12Z

display.go

+			mimeType:     data,
+		}, make(bundledMIMEData))
+}
+


render2 and render3 would then return map[string]interface{} instead of publishing.

SpencerPark · 2018-06-04T02:36:14Z

image.go

@@ -27,7 +27,7 @@ func publishImages(vals []interface{}, receipt msgReceipt) []interface{} {
 }

 // publishImages sends a "display_data" broadcast message for given image.
-func publishImage(img image.Image, receipt msgReceipt) error {
+func publishImage(img image.Image, receipt *msgReceipt) error {
 	bytes, mime, err := encodePng(img)
 	if err != nil {
 		return err


These functions can move into the same package for rendering and ideally keep them more or less pure. This separation will be really helpful both for testing and maintaining.

SpencerPark · 2018-06-04T02:40:23Z

kernel.go

+	if err != nil {
+		log.Print(err)
+	}
+


Something like in f696d76 that replaces the display function with a closure that knows about the eval context removes the global. Not sure of any repercussions gomacro will have as a result but taking advantage of the more dynamic interpreter seemed like a great use for gomacro.

SpencerPark · 2018-06-04T02:44:08Z

kernel.go

@@ -359,7 +372,7 @@ func handleExecuteRequest(ir *interp.Interp, receipt msgReceipt) error {

 	if executionErr == nil {
 		// if one or more value is image.Image, display it instead
-		vals = publishImages(vals, receipt)
+		vals = publishImages(vals, &receipt)


If all of the functions simply render instead of display this is the place we get to take advantage of that. We need a type assertion but I think checking for a struct is fine. If we are trying to return a mimebundle we can get the rich output in the Out[_] in addition to the display calls.

SpencerPark · 2018-06-04T02:55:48Z

I might have reviewed at the wrong commit... anyways I like the changes but I think the separation of rendering and displaying is really important. I also had some unit tests in the works including the helpers for it which I would like to bring over here.

This is such a popular feature that we should really make sure to get something stable right off the bat.

cosmos72 · 2018-06-04T08:16:35Z

You reviewed the right commit, don't worry.

Yes, I saw you injected into the interpreter closures that kept a reference to the current msgReceipt. At least syntactically, it's much cleaner than my global variable.

But the real question is: what's the lifetime of the msgReceipt passed to handleExecuteRequest() ?
Can it still be used after handleExecuteRequest() returns or not?

Closures on the msgReceipt will remain available to the interpreter even after Interp.RunExpr() returns - for example, one may attempt to display from a long-running interpreted goroutine. I have no idea what would happen in such case.
On the other hand, my patch sets globalReceipt to nil when handleExecuteRequest() returns. This prevents long-running interpreted goroutines from sending display requests to gophernotes and Jupyter. Clearly, a similar technique can be used also for closures if needed.

About splitting the rendering (returning a mime bundle) from the actual display portion:

I am not fond of exposing very complex API to interpreted code - that's also why I tried to simplify your implementation. The most complex function I exposed to the interpreter is display.MIME(data, metadata map[string]interface{}) error and, while needed for the fully general case, it already feels complicated to use for me.
In any case, some code will have to send the Jupiter message containing the mime bundle, and such code will have the same problem of accessing the correct msgReceipt

In summary, I think the best approach depends on msgReceipt lifetime, i.e. whether it can be used or not after handleExecuteRequest() returns

SpencerPark · 2018-06-04T15:50:59Z

But the real question is: what's the lifetime of the msgReceipt passed to handleExecuteRequest() ?
Can it still be used after handleExecuteRequest() returns or not?

Good point. I believe the behavior is undefined in the spec and so we can't rely on the server to do anything with it but I don't think there is any harm in using it. In either case it might be worth setting it to nil as you have done but maybe still keep it local (and hidden).

I am not fond of exposing very complex API to interpreted code - that's also why I tried to simplify your implementation. The most complex function I exposed to the interpreter is display.MIME(data, metadata map[string]interface{}) error and, while needed for the fully general case, it already feels complicated to use for me.

I do agree with this in most cases but the reason I am so strong on it is because of the very long running precedent that Jupyter has set with IPython's display mechanics. The display function and it's behavior should essentially give access to some print stream for richer output in the same way that returning something (an expression at the end of a cell) should. This is why the rendering/display separation is required as something can be rendered and used as the ExecuteResult instead of DisplayData.

I'm fine with it being simplified to always return something like DisplayData and keep interfaces out of it but the user code needs access to these mechanics even if the rendering always needs to be explicit.

In any case, some code will have to send the Jupiter message containing the mime bundle, and such code will have the same problem of accessing the correct msgReceipt.

Hopefully we can hide as much of that as possible which I propose we do with a Display function taking some DisplayData.

cosmos72 · 2018-06-04T16:09:48Z

Thanks, it's clearer now. I guess I should read IPython API...
If I understand correctly, you want to expose to interpreted code an API like

type DisplayData struct { Data, Metadata, Transient map[string]interface{} } // or possibly something else to be defined
func Display(data DisplayData) error

plus some other utility functions that create such DisplayData objects,
plus extending gophernotes to detect any DisplayData object returned by the interpreter and send them to Jupyter for displaying?

That makes sense.

…layData) error' function that sends multimedia to Jupyter for displaying from the various factory functions 'func display.*(...) DisplayData' that create and return DisplayData values. Also remove the global variable globalReceipt and use a closure instead.

cosmos72 · 2018-06-04T20:02:57Z

I just updated my branch (and as a consequence this pull request) to better follow your suggestion and IPython API, including the separation between the func Display(data DisplayData) error that sends multimedia data to Jupyter for displaying,
and the the various factory functions func display.*(...) DisplayData that create and return DisplayData values.

I also replaced the global variable with a closure, and updated publishImages to detect (and show) DisplayData values returned at prompt.

SpencerPark · 2018-06-05T16:04:06Z

kernel.go

-		// if one or more value is image.Image, display it instead
-		vals = publishImages(vals, &receipt)
+		// if one or more value is image.Image or DisplayData, display it instead
+		vals = publishImagesAndDisplayData(vals, &receipt)


See the Execution Result message format which is display data plus a bit extra. This is a minor detail but if you notice the Out[_] is missing when a value is returned because here it is being displayed rather than put in an execution result.

SpencerPark · 2018-06-05T16:05:48Z

@cosmos72 that image is beautiful! I have the one minor comment above but that is looking really great.

cosmos72 · 2018-06-05T19:34:21Z

Actually I intentionally replaced all DisplayData and image.Image returned by Interp.RunExpr() with nil before sending the results to ExecutionResult, because such types are intercepted by publishImagesAndDisplayData().

I guess ExecutionResult supports images and multimedia too, but since it has a single data map[string]interface{} it cannot display more than one value per mime type - no way to show two PNGs, for example.

Which alternative do you propose?

SpencerPark · 2018-06-05T21:20:25Z

Yes this was another thing Dan and I talked a lot about back when trying to design an API for this.

The IPython semantics are such that when a single result is returned by the eval, that result is rendered nicely. If multiple then a string representation is returned. We opted for fmt.Sprint. Now to better suit the Go environment a nil result is considered non-existent and therefor situations that, for example, return an image and an error don't break the nice rendering by including error in their signature because when the error is nil we just render the image.

Thanks for putting up with all these requests :)

cosmos72 · 2018-06-06T07:54:28Z

You mean that you propose the following algorithm for values returned by the REPL ?

"if a single non-nil value is returned by REPL (other values, if present, are nil), and such value is a DisplayData or an image.Image, it is displayed graphically. If multiple non-nil values are returned, they are displayed as string"

If that's what you propose, I politely disagree, because I find it confusing for users and unnecessarily limiting.

Python does not have multiple return values, so any "multiple values" returned by an expression are actually wrapped in a tuple, list, dictionary or object - IMHO in such case it makes much more sense not to extract them for graphical displaying. Go multiple return values are thus a language feature with no Python counterpart, and should be considered when designing an API.

My proposal, which is implemented in the current pull request, is:

"if one or more values returned by REPL is a DisplayData or an image.Image, it is displayed graphically and replaced with a placeholder (currently 'nil') in the returned values. Then the returned values are also passed as string to ExecutionResult, unless they are all nil or placeholders"

This has the advantage of not special-casing the single-value results, and it can display multiple images / html / ... returned by a single expression. If a user wants to see the string representation of a DisplayData or image.Image (and I guess such need will be rare beyond debugging/studying gophernotes or the package 'image'), IMHO it's quite straightforward for him/her to come up with the idea to call fmt.Sprint(val)

SpencerPark · 2018-06-07T17:12:56Z

"if a single non-nil value is returned by REPL (other values, if present, are nil), and such value is a DisplayData or an image.Image, it is displayed graphically. If multiple non-nil values are returned, they are displayed as string"

Correct, that is what I propose.

Python does not have multiple return values, so any "multiple values" returned by an expression are actually wrapped in a tuple, list, dictionary or object - IMHO in such case it makes much more sense not to extract them for graphical displaying. Go multiple return values are thus a language feature with no Python counterpart, and should be considered when designing an API.

Sure the implementation is with tuples, but with syntax for building that tuple without parentheses it's really not fair to say Python doesn't have multiple return values. You can write code that assumes multiple return values without ever needing to know there was a tuple conversion in the middle. The fact that it is a tuple just makes it play nicer with Python.

The part that makes this approach a bit more Go friendly is considering nil as a non-returned value because return values are given a default value and with the Go pattern of returning an error in addition to a return value, you would never be able to display anything. The intent when returning an image and a nil error is that you are returning an image. This is captured in the semantics of the single non-nil return value giving the richer representation.

"if one or more values returned by REPL is a DisplayData or an image.Image, it is displayed graphically and replaced with a placeholder (currently 'nil') in the returned values. Then the returned values are also passed as string to ExecutionResult, unless they are all nil or placeholders"

I disagree with this approach. There is a message in the protocol dedicated for cell results and this is the place it needs to be used. If multiple images (for example) need to be displayed then those need to be captured in variables and displayed separately. Since images can't be displayed side-by-side anyways the implicit transposition is also (in my opinion) not very natural.

The default behavior should always be a string representation. In some cases where we are sure the intent is something nicer then we can display that. If a string is a return value there is no doubt that it would be returned (in an execution_result) and not displayed on stdout for example. The display stream should be treated the same way. It needs to be used explicitly.

…ly if it's the only non-nil result

cosmos72 · 2018-06-10T21:10:45Z

I am not convinced, but for the sake of finding an agreement I modified the pull request as you asked:
automatically display image.Image or display.Data returned by REPL only if it's the only non-nil result

SpencerPark · 2018-06-10T21:27:04Z

I don't want a disagreement to hold up this feature. You know Go very well and so if you are strong on this then I think we should bring it into gophernotes.

automatically display image.Image or display.Data returned by REPL only if it's the only non-nil result

One last note on this last commit: if we are going with the single output approach it should be put into an execute_result as otherwise the restriction for 1 output is unjustified. Or we can keep sending display_data messages and then might as well go back to your approach of supporting multiple returns.

On an unrelated note, thanks for all the work lately. You are bringing life back into this project and it's really great to see.

cosmos72 · 2018-06-10T23:11:22Z

Ah, ok. I read your comment previously about using execute_result instead of display_data but I did not understand what you meant. Now it's clear, thanks.

Well, I did not want to get stuck in a discussion "A: let's do my way. B: No, let's do my way", and I also don't want to get stuck in the opposite "A: let's do your way. B: No, let's do your way"

The single-result functions are almost ready, let's use them. In case, we can extend to multiple results later.

…lay.Data, publish it with "execute_result" Jupyter message, not with "display_data"

cosmos72 · 2018-06-10T23:54:45Z

modified to use execute_result instead of display_data if there's a single image.Image or Data returned by REPL

cosmos72 · 2018-06-14T20:07:54Z

merged

cosmos72 added 5 commits April 7, 2018 14:05

update vendored copy of gomacro: adds interpreted interfaces, fixes c…

7c1987e

…rash on import of complicates packages as "gonum.org/v1/plot/plotter", and other improvements

add vendored copy of github.com/peterh/liner needed by gomacro

6adaaa8

simplify doEval()

404fc47

doEval(): preallocate a slice of values with required length

3ddbe1f

display images: show as image each expression result that implements …

e7a9663

…Go interface 'image.Image'

cosmos72 mentioned this pull request Apr 7, 2018

Return image data to Jupyter front end? #17

Closed

cosmos72 added 3 commits June 2, 2018 16:04

update vendored copy of gomacro; update kernel.go for API change in g…

720be35

…omacro/fast/Interp.RunExpr()

Merge branch 'master' into display_image

3c56c86

simplify @SpencerPark branch https://github.com/SpencerPark/gophernot…

c75e6bc

…es/tree/mime-rendering to implement rendering of HTML, JSON, JPEG, PNG, SVG, LaTeX... New functions: display.PNG([]byte), display.JPEG([]byte), display.HTML(string), display.SVG(string) ...

cosmos72 changed the title ~~display images~~ display images, HTML... and MIME rendering Jun 2, 2018

cosmos72 mentioned this pull request Jun 2, 2018

MIME rendering #110

Closed

3 tasks

cosmos72 added 2 commits June 2, 2018 18:49

add .gitignore

633fa16

merge @SpencerPark commit 710233f

be43b70

From SpencerPark <spinnr95@gmail.com>: Lock writes to the sockets to fix a bug with interleaved publishes.

cosmos72 mentioned this pull request Jun 2, 2018

update vendored copy of gomacro: adds interpreted interfaces and more... #104

Closed

SpencerPark reviewed Jun 4, 2018

View reviewed changes

SpencerPark reviewed Jun 5, 2018

View reviewed changes

SpencerPark mentioned this pull request Jun 5, 2018

Unable to create Linked struct in go repl #113

Closed

cosmos72 mentioned this pull request Jun 6, 2018

update vendored gomacro #114

Closed

cosmos72 closed this Jun 9, 2018

cosmos72 deleted the display_image branch June 9, 2018 09:30

cosmos72 restored the display_image branch June 9, 2018 09:30

cosmos72 reopened this Jun 9, 2018

cosmos72 mentioned this pull request Jun 9, 2018

Plans for fully functional MacOS and Windows implementation for compatibility #102

Open

automatically display image.Image or display.Data returned by REPL on…

568d6d7

…ly if it's the only non-nil result

take updated README.md from gopherdata/gophernotes master branch

bd0dab5

if the only non-nil result returned by REPL is an image.Image or disp…

9e18fed

…lay.Data, publish it with "execute_result" Jupyter message, not with "display_data"

cosmos72 merged commit 9e18fed into gopherdata:master Jun 14, 2018

cosmos72 deleted the display_image branch June 14, 2018 20:07

cosmos72 mentioned this pull request Apr 4, 2019

Need more integrated HTML support #166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

display images, HTML... and MIME rendering #105

display images, HTML... and MIME rendering #105

cosmos72 commented Apr 7, 2018

dwhitena commented Apr 20, 2018

cosmos72 commented Jun 2, 2018

SpencerPark Jun 4, 2018

SpencerPark Jun 4, 2018

SpencerPark Jun 4, 2018

SpencerPark Jun 4, 2018

SpencerPark Jun 4, 2018

SpencerPark commented Jun 4, 2018

cosmos72 commented Jun 4, 2018 •

edited

Loading

SpencerPark commented Jun 4, 2018

cosmos72 commented Jun 4, 2018 •

edited

Loading

cosmos72 commented Jun 4, 2018 •

edited

Loading

SpencerPark Jun 5, 2018

SpencerPark commented Jun 5, 2018

cosmos72 commented Jun 5, 2018 •

edited

Loading

SpencerPark commented Jun 5, 2018

cosmos72 commented Jun 6, 2018 •

edited

Loading

SpencerPark commented Jun 7, 2018

cosmos72 commented Jun 10, 2018

SpencerPark commented Jun 10, 2018

cosmos72 commented Jun 10, 2018

cosmos72 commented Jun 10, 2018

cosmos72 commented Jun 14, 2018

display images, HTML... and MIME rendering #105

display images, HTML... and MIME rendering #105

Conversation

cosmos72 commented Apr 7, 2018

dwhitena commented Apr 20, 2018

cosmos72 commented Jun 2, 2018

SpencerPark Jun 4, 2018

Choose a reason for hiding this comment

SpencerPark Jun 4, 2018

Choose a reason for hiding this comment

SpencerPark Jun 4, 2018

Choose a reason for hiding this comment

SpencerPark Jun 4, 2018

Choose a reason for hiding this comment

SpencerPark Jun 4, 2018

Choose a reason for hiding this comment

SpencerPark commented Jun 4, 2018

cosmos72 commented Jun 4, 2018 • edited Loading

SpencerPark commented Jun 4, 2018

cosmos72 commented Jun 4, 2018 • edited Loading

cosmos72 commented Jun 4, 2018 • edited Loading

SpencerPark Jun 5, 2018

Choose a reason for hiding this comment

SpencerPark commented Jun 5, 2018

cosmos72 commented Jun 5, 2018 • edited Loading

SpencerPark commented Jun 5, 2018

cosmos72 commented Jun 6, 2018 • edited Loading

SpencerPark commented Jun 7, 2018

cosmos72 commented Jun 10, 2018

SpencerPark commented Jun 10, 2018

cosmos72 commented Jun 10, 2018

cosmos72 commented Jun 10, 2018

cosmos72 commented Jun 14, 2018

cosmos72 commented Jun 4, 2018 •

edited

Loading

cosmos72 commented Jun 4, 2018 •

edited

Loading

cosmos72 commented Jun 4, 2018 •

edited

Loading

cosmos72 commented Jun 5, 2018 •

edited

Loading

cosmos72 commented Jun 6, 2018 •

edited

Loading