Uploaded image for project: 'Zeppelin'
  1. Zeppelin
  2. ZEPPELIN-212

Interpreter should support returning a list of InterpreterResults

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.6.0
    • Fix Version/s: 0.7.0
    • Component/s: None
    • Labels:
      None

      Issue Links

        Activity

        Show
        githubbot ASF GitHub Bot added a comment - Github user felixcheung commented on the pull request: https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-129220667 Created https://issues.apache.org/jira/browse/ZEPPELIN-212
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user eranwitkon commented on the pull request:

        https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-129221434

        I hope we are going to merge this one before moving to the list
        implementation. Eran

        בתאריך יום א׳, 9 באוג׳ 2015, 20:36 מאת Felix Cheung <
        notifications@github.com>:

        > Created https://issues.apache.org/jira/browse/ZEPPELIN-212
        >
        > —
        > Reply to this email directly or view it on GitHub
        > <https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-129220667>
        > .
        >

        Show
        githubbot ASF GitHub Bot added a comment - Github user eranwitkon commented on the pull request: https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-129221434 I hope we are going to merge this one before moving to the list implementation. Eran בתאריך יום א׳, 9 באוג׳ 2015, 20:36 מאת Felix Cheung < notifications@github.com>: > Created https://issues.apache.org/jira/browse/ZEPPELIN-212 > > — > Reply to this email directly or view it on GitHub > < https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-129220667 > > . >
        Hide
        moon Lee moon soo added a comment -

        Following items need to be considered

        • Compatibility of note.json (reading previous version of Zeppelin generated note.json. Let previous version of Zeppelin read note.json with multiple interpreter result)
        • %table display system allows %html inside of cell. So parsing multiple display systems in the output need to consider that.
        • Helium application and it's launcher also need to support multiple interpreter results.
        Show
        moon Lee moon soo added a comment - Following items need to be considered Compatibility of note.json (reading previous version of Zeppelin generated note.json. Let previous version of Zeppelin read note.json with multiple interpreter result) %table display system allows %html inside of cell. So parsing multiple display systems in the output need to consider that. Helium application and it's launcher also need to support multiple interpreter results.
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user Leemoonsoo opened a pull request:

        https://github.com/apache/zeppelin/pull/1658

        [WIP] ZEPPELIN-212 Multiple paragraph results

            1. 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 : [
        {
        result : {
        {
        code: "",
        msg: [

        { type: "", data: "" }

        ,

        { type: "", data: "" }

        ]
        },
        config : {
        result : [
        {
        graph : {},
        },
        {
        graph : {},
        },
        ...
        ]
        },
        ...
        },
        ...
        ],
        ...
        ```

            1. What type of PR is it?
              Improvement
            1. Todos
        • [x] - Make InterpreterResult and InterpreterOutput support multiple display system
        • [x] - 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
        • [ ] - Save note in old format in some notebook repo (zeppelinhubrepo)
            1. What is the Jira issue?
              https://issues.apache.org/jira/browse/ZEPPELIN-212
            1. How should this be tested?
            1. Screenshots (if appropriate)
            1. Questions:
        • Does the licenses files need update? no
        • Is there breaking changes for older versions? yes
        • Does this needs documentation? yes

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/Leemoonsoo/zeppelin ZEPPELIN-212

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/zeppelin/pull/1658.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #1658



        Show
        githubbot ASF GitHub Bot added a comment - GitHub user Leemoonsoo opened a pull request: https://github.com/apache/zeppelin/pull/1658 [WIP] ZEPPELIN-212 Multiple paragraph results 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 : [ { result : { { code: "", msg: [ { type: "", data: "" } , { type: "", data: "" } ] }, config : { result : [ { graph : {}, }, { graph : {}, }, ... ] }, ... }, ... ], ... ``` What type of PR is it? Improvement Todos [x] - Make InterpreterResult and InterpreterOutput support multiple display system [x] - 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 [ ] - Save note in old format in some notebook repo (zeppelinhubrepo) What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-212 How should this be tested? Screenshots (if appropriate) Questions: Does the licenses files need update? no Is there breaking changes for older versions? yes Does this needs documentation? yes You can merge this pull request into a Git repository by running: $ git pull https://github.com/Leemoonsoo/zeppelin ZEPPELIN-212 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zeppelin/pull/1658.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1658
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user Leemoonsoo commented on the issue:

        https://github.com/apache/zeppelin/pull/1658

        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()](https://github.com/apache/zeppelin/blob/7ab6679a87ed75f176ffa61d8cf6c73f4241c528/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java#L385) 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 api changes.

        Show
        githubbot ASF GitHub Bot added a comment - Github user Leemoonsoo commented on the issue: https://github.com/apache/zeppelin/pull/1658 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()] ( https://github.com/apache/zeppelin/blob/7ab6679a87ed75f176ffa61d8cf6c73f4241c528/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java#L385 ) 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 api changes.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user agoodm commented on the issue:

        https://github.com/apache/zeppelin/pull/1658

        Very nice @Leemoonsoo 👍

        I have tested this with the matplotlib tutorial notebook just to see if ZEPPELIN-1638(https://issues.apache.org/jira/browse/ZEPPELIN-1638) is also resolved by this, and everything checks out!

        Show
        githubbot ASF GitHub Bot added a comment - Github user agoodm commented on the issue: https://github.com/apache/zeppelin/pull/1658 Very nice @Leemoonsoo 👍 I have tested this with the matplotlib tutorial notebook just to see if ZEPPELIN-1638 ( https://issues.apache.org/jira/browse/ZEPPELIN-1638 ) is also resolved by this, and everything checks out!
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user khalidhuseynov commented on the issue:

        https://github.com/apache/zeppelin/pull/1658

        tried locally and works as expected, LGTM!

        Show
        githubbot ASF GitHub Bot added a comment - Github user khalidhuseynov commented on the issue: https://github.com/apache/zeppelin/pull/1658 tried locally and works as expected, LGTM!
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user Leemoonsoo commented on the issue:

        https://github.com/apache/zeppelin/pull/1658

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

        Show
        githubbot ASF GitHub Bot added a comment - Github user Leemoonsoo commented on the issue: https://github.com/apache/zeppelin/pull/1658 Thanks @agoodm @khalidhuseynov for the review! Merge to master if there're no more comments.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user agoodm commented on the issue:

        https://github.com/apache/zeppelin/pull/1658

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

        Show
        githubbot ASF GitHub Bot added a comment - Github user agoodm commented on the issue: https://github.com/apache/zeppelin/pull/1658 @Leemoonsoo FYI, #1632 added another set of tests. The interpreter result format for these should be changed as well.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user Leemoonsoo commented on the issue:

        https://github.com/apache/zeppelin/pull/1658

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

        CI failure is not related. Merge to master.
        ![image](https://cloud.githubusercontent.com/assets/1540981/20757762/7087d81e-b6cc-11e6-8eb9-f3689903ec83.png)

        Show
        githubbot ASF GitHub Bot added a comment - Github user Leemoonsoo commented on the issue: https://github.com/apache/zeppelin/pull/1658 @agoodm merged master and addressed tests added from #1632. CI failure is not related. Merge to master. ! [image] ( https://cloud.githubusercontent.com/assets/1540981/20757762/7087d81e-b6cc-11e6-8eb9-f3689903ec83.png )
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/zeppelin/pull/1658

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/zeppelin/pull/1658

          People

          • Assignee:
            moon Lee moon soo
            Reporter:
            felixcheung Felix Cheung
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development