[ZEPPELIN-212] Multiple paragraph results #1658

Closed
wants to merge 29 commits into
from

Conversation

Projects
None yet
3 participants
@Leemoonsoo
Member

Leemoonsoo commented Nov 18, 2016

What is this PR for?

Currently a paragraph can display only a single type of result.
For example

print("""
%text textout
%html htmlout
""")

will display only last display system detected, "htmlout".

This pr implements multiple results supports, so not only the last, but also all the display systems in a paragraph outputs can be displayed.

To do that, Note.json format need to be changed. from

   paragraph : [
      {
          result : {
              code: "",
              type: "",
              msg: ""            
          },
          config : {
             graph : {},
             ...
          },
          ...
      },
      ...
   ],
   ...

to

   paragraph : [
      {
          results : {
             {
                code: "",
                msg: [
                   {
                        type: "",
                        data: ""
                    },
                    {
                        type: "",
                        data: ""
                     },
                     ...
                ]
          },
          config : {
             results : [
                {
                    graph : {},
                },
                {
                    graph : {},
                },
                ...
             ]
          },
          ...
      },
      ...
   ],
   ...

What type of PR is it?

Improvement

Todos

  • - Make InterpreterResult and InterpreterOutput support multiple display system
  • - Automatic migration old format to new format
  • - Render multiple results in front-end
  • - Take care Importing old notebook format
  • - Make helium framework support multiple results

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-212

How should this be tested?

run following code in a single spark paragraph.

%spark
// default output is text
(1 to 3).foreach{i=>
  println(new java.util.Date())
  Thread.sleep(1000)
}

// print something in html
println(s"""%html <h3><font style="color:blue">Some HTML</font></h3>""")

// create table
println(s"""%table key\tvalue
sun\t100
moon\t200

""")

// display text again
println("""%text""")
(1 to 3).foreach{i=>
  println(new java.util.Date())
  Thread.sleep(1000)
}

// another table
Thread.sleep(1000)
println(s"""%table key\tvalue
apple\t100
banana\t200
""")

Screenshots (if appropriate)

multipleout

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? yes
  • Does this needs documentation? yes

@Leemoonsoo Leemoonsoo changed the title from [WIP] [ZEPPELIN-212] Multiple paragraph results to [ZEPPELIN-212] Multiple paragraph results Nov 24, 2016

@Leemoonsoo

This comment has been minimized.

Show comment
Hide comment
@Leemoonsoo

Leemoonsoo Nov 25, 2016

Member

I think this PR is ready to be reviewed.
While this PR includes quite a lot of changes i'm highlighting some key changes.

  1. Display multiple types of output in a single paragraph.

    Previously, if output includes multiple display system directive (e.g. %html, %text, etc) only the last one was effective and all the previous output was ignored. After the change, a paragraph sequentially display all display system directive in the output result.

    To do that, on the front-end side,
    Created ResultCtrl controller under zeppelin-web/src/app/notebook/paragraph/result
    and moved paragraph result rendering related scripts/templates under this directory.

    /notebook/paragraph/paragraph-graph.html -> /notebook/paragraph/result/result-graph.html
    /notebook/paragraph/paragraph-pivot.html -> /notebook/paragraph/result/result-pivot.html
    /notebook/paragraph/paragraph-result.html -> /notebook/paragraph/result/result-results.html
    /notebook/paragraph/paragraph-chart-selector.html -> /notebook/paragraph/result/result-chart-selector.html

    on the back-end side,
    Created zeppelin-interpreter/interpreter/InterpreterResultMessageOutput.java to take care of single output stream and existing zeppelin-interpreter/interpreter/InterpreterResultMessageOutput.java changed to take care of multiple outputs through InterpreterResultMessageOutput

  2. Compatibility between old, new notebook format.

    note.json format is changed to store multiple outputs in the paragraph.

    To read note.json created by previous versions of Zeppelin, Notebook.convertFromSingleResultToMultipleResultsFormat() will automatically convert format when note.json is loaded in Zeppelin.

    If old version of Zeppelin reads notebook created by newer version, everything will be load normally except for output. Output will be empty.

    In detail, note.json is changed from

    paragraph : [
       {
           result : {
               code: "",
               type: "",
               msg: ""            
           },
           config : {
              graph : {},
              ...
           },
           ...
       },
       ...
    ],
    ...
    

    to

    paragraph : [
       {
           results : {
              {
                 code: "",
                 msg: [
                    {
                         type: "",
                         data: ""
                     },
                     {
                         type: "",
                         data: ""
                      },
                      ...
                 ]
           },
           config : {
              results : [
                 {
                     graph : {},
                 },
                 {
                     graph : {},
                 },
                 ...
              ]
           },
           ...
       },
       ...
    ],
    ...
    
  3. Every other changes are trivial

    Most changes are trivial change due to change of InterpreterOutput and InterpreterResult method changes.

Member

Leemoonsoo commented Nov 25, 2016

I think this PR is ready to be reviewed.
While this PR includes quite a lot of changes i'm highlighting some key changes.

  1. Display multiple types of output in a single paragraph.

    Previously, if output includes multiple display system directive (e.g. %html, %text, etc) only the last one was effective and all the previous output was ignored. After the change, a paragraph sequentially display all display system directive in the output result.

    To do that, on the front-end side,
    Created ResultCtrl controller under zeppelin-web/src/app/notebook/paragraph/result
    and moved paragraph result rendering related scripts/templates under this directory.

    /notebook/paragraph/paragraph-graph.html -> /notebook/paragraph/result/result-graph.html
    /notebook/paragraph/paragraph-pivot.html -> /notebook/paragraph/result/result-pivot.html
    /notebook/paragraph/paragraph-result.html -> /notebook/paragraph/result/result-results.html
    /notebook/paragraph/paragraph-chart-selector.html -> /notebook/paragraph/result/result-chart-selector.html

    on the back-end side,
    Created zeppelin-interpreter/interpreter/InterpreterResultMessageOutput.java to take care of single output stream and existing zeppelin-interpreter/interpreter/InterpreterResultMessageOutput.java changed to take care of multiple outputs through InterpreterResultMessageOutput

  2. Compatibility between old, new notebook format.

    note.json format is changed to store multiple outputs in the paragraph.

    To read note.json created by previous versions of Zeppelin, Notebook.convertFromSingleResultToMultipleResultsFormat() will automatically convert format when note.json is loaded in Zeppelin.

    If old version of Zeppelin reads notebook created by newer version, everything will be load normally except for output. Output will be empty.

    In detail, note.json is changed from

    paragraph : [
       {
           result : {
               code: "",
               type: "",
               msg: ""            
           },
           config : {
              graph : {},
              ...
           },
           ...
       },
       ...
    ],
    ...
    

    to

    paragraph : [
       {
           results : {
              {
                 code: "",
                 msg: [
                    {
                         type: "",
                         data: ""
                     },
                     {
                         type: "",
                         data: ""
                      },
                      ...
                 ]
           },
           config : {
              results : [
                 {
                     graph : {},
                 },
                 {
                     graph : {},
                 },
                 ...
              ]
           },
           ...
       },
       ...
    ],
    ...
    
  3. Every other changes are trivial

    Most changes are trivial change due to change of InterpreterOutput and InterpreterResult method changes.

@agoodm

This comment has been minimized.

Show comment
Hide comment
@agoodm

agoodm Nov 28, 2016

Member

Very nice @Leemoonsoo 👍

I have tested this with the matplotlib tutorial notebook just to see if ZEPPELIN-1638 is also resolved by this, and everything checks out!

Member

agoodm commented Nov 28, 2016

Very nice @Leemoonsoo 👍

I have tested this with the matplotlib tutorial notebook just to see if ZEPPELIN-1638 is also resolved by this, and everything checks out!

+ }
+ }
+ } catch (Exception e) {
+ logger.error("Conversion failure", e);

This comment has been minimized.

@khalidhuseynov

khalidhuseynov Nov 29, 2016

Contributor

would it be able to continue if the conversion has failed?

@khalidhuseynov

khalidhuseynov Nov 29, 2016

Contributor

would it be able to continue if the conversion has failed?

This comment has been minimized.

@Leemoonsoo

Leemoonsoo Nov 29, 2016

Member

It catches exception and just logging it, not propagating it. So i think note loading continues even if conversion fails.

@Leemoonsoo

Leemoonsoo Nov 29, 2016

Member

It catches exception and just logging it, not propagating it. So i think note loading continues even if conversion fails.

This comment has been minimized.

@khalidhuseynov

khalidhuseynov Nov 29, 2016

Contributor

I see, so basically it handles old result format notes on frontend as well

@khalidhuseynov

khalidhuseynov Nov 29, 2016

Contributor

I see, so basically it handles old result format notes on frontend as well

This comment has been minimized.

@Leemoonsoo

Leemoonsoo Nov 29, 2016

Member

Front end will display note without results in each paragraphs, incase of conversion failure.

@Leemoonsoo

Leemoonsoo Nov 29, 2016

Member

Front end will display note without results in each paragraphs, incase of conversion failure.

@khalidhuseynov

This comment has been minimized.

Show comment
Hide comment
@khalidhuseynov

khalidhuseynov Nov 29, 2016

Contributor

tried locally and works as expected, LGTM!

Contributor

khalidhuseynov commented Nov 29, 2016

tried locally and works as expected, LGTM!

@Leemoonsoo

This comment has been minimized.

Show comment
Hide comment
@Leemoonsoo

Leemoonsoo Nov 29, 2016

Member

Thanks @agoodm @khalidhuseynov for the review!
Merge to master if there're no more comments.

Member

Leemoonsoo commented Nov 29, 2016

Thanks @agoodm @khalidhuseynov for the review!
Merge to master if there're no more comments.

@agoodm

This comment has been minimized.

Show comment
Hide comment
@agoodm

agoodm Nov 29, 2016

Member

@Leemoonsoo FYI, #1632 added another set of tests. The interpreter result format for these should be changed as well.

Member

agoodm commented Nov 29, 2016

@Leemoonsoo FYI, #1632 added another set of tests. The interpreter result format for these should be changed as well.

@Leemoonsoo

This comment has been minimized.

Show comment
Hide comment
@Leemoonsoo

Leemoonsoo Nov 30, 2016

Member

@agoodm merged master and addressed tests added from #1632.

CI failure is not related. Merge to master.
image

Member

Leemoonsoo commented Nov 30, 2016

@agoodm merged master and addressed tests added from #1632.

CI failure is not related. Merge to master.
image

@asfgit asfgit closed this in 850fd81 Dec 1, 2016

asfgit pushed a commit that referenced this pull request Dec 23, 2016

[BugFix] Tutorial note json format.
### What is this PR for?
Since #1658, the output format had changed but note.json of Tutorials didn't changed.

### What type of PR is it?
Bug Fix

### How should this be tested?
Please make sure there is no `result` field in note.json file.

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: astroshim <hsshim@zepl.com>
Author: Hyungsung Shim <shim@Hyungsungui-MacBook-Pro.local>

Closes #1780 from astroshim/fix/tutorialNote and squashes the following commits:

eca8026 [astroshim] Merge branch 'master' into fix/tutorialNote
a957baa [Hyungsung Shim] fix tutorial note json format.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment