You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by 1ambda <gi...@git.apache.org> on 2017/01/25 01:09:31 UTC
[GitHub] zeppelin pull request #1940: [ZEPPELIN-2008] Introduce Spell
GitHub user 1ambda opened a pull request:
https://github.com/apache/zeppelin/pull/1940
[ZEPPELIN-2008] Introduce Spell
### What is this PR for?
Implemented **Spell** to Helium categories. *Technically, it's the frontend interpreter* runs on browser not backend.
Spell can provide many benefits.
1. Anyone can install, remove using helium package registry by #1936
2. Implementing spell is extremely easier rather than adding backend interpreter
3. Can use existing javsacript libraries. (e.g [flowchart.js](http://flowchart.js.org/), [sequence diagram js](https://github.com/bramp/js-sequence-diagrams), ...). This enable us to add many visualization tools. Assume that you can implement whatever you want to visualize in few lines of code like [flowchart-spell-example](https://github.com/apache/zeppelin/compare/master...1ambda:ZEPPELIN-2008/introduce-spell?expand=1#diff-364845b20d68e4d94688e44fef03da98)
4. The most important thing is, spell can be not only interpreter but also display system. Because it runs on browser. This enable us to combining existing backend interpreter with spell display like
```
// if we have markdown spell
%spark
val calculated = doSomething()
println(s"%markdown _${calculated})
```
I added some examples. Checkout `echo`, `markdown`, `translator`, `flowchart` spells.
### What type of PR is it?
[Feature]
### Todos
* [x] - Add `SPELL` as one of Helium categories.
* [x] - Implement framework code (`zeppelin-spell`)
* [x] - Make some examples (flowchart, google translator, markdown, echo)
* [x] - Support custom display system
* [x] - Fix some bugs in `HeliumBundleFactory`
* [x] - Save spell rendering result into `note.json` while broadcasting to other websocket clients
### What is the Jira issue?
[ZEPPELIN-2008](https://issues.apache.org/jira/browse/ZEPPELIN-2008)
### How should this be tested?
See the screenshots below.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update - NO
* Is there breaking changes for older versions? - NO
* Does this needs documentation? - YES, but framework can be enhanced so i would like to defer to write document right now.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/1ambda/zeppelin ZEPPELIN-2008/introduce-spell
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/1940.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 #1940
----
commit de220b36d39a535f097f6d49820973738be079cc
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-17T10:40:12Z
feat: Support FRONTEND_INTERPRETER type in backend
commit 1c57cb626d9006cdbd59722022de0a798b345a03
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-17T10:41:10Z
feat: Support FRONTEND_INTERPRETER type in frontend
commit 62b2e880ddb02157b25236f1cd3afaad2ba3ac15
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-17T10:41:32Z
feat: Add frontend interpreter framework
commit 4a1e42d662cebbcf87242fb743b9bf6c8d5e8b8f
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-17T10:41:59Z
feat: Add flowchart, translator examples
commit 150ab44b2710f34b93d938038d1063ec10aecfd3
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-17T10:42:26Z
feat: Apply frontend interpreter to paragraph
commit 921b1774137f4cb7c58e90e4a609a7f85e9c400e
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-20T23:22:22Z
feat: Automated display type checking in result
commit 07d944209a26e9f375d5f16d8956cb54f0dc86b5
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-21T00:18:26Z
feat: Update examples to use single FrontIntpRes
commit 1cd8910be6dd48024e9845f50c3e2cec7f0e7ab2
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-23T17:52:40Z
feat: Support custom display
commit ccd57a60ab5b1b7fac30b7e23b4eb5e61bd664e0
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-23T17:54:36Z
example: Add echo, markdown
commit fc2780cd62ece92e18f3305a9fefaca3ca1da62c
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-23T20:09:27Z
style: Rename to Spell
commit ed328ce74fd9e60b9732d89259cc7d1af34293e5
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-23T20:22:26Z
style: Rename generator -> data
commit bf62f4af21d634a7eb7d6f77003e750cc4cdb1f2
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-23T20:48:39Z
feat: Enhance translator spell
commit 7814d7882462e9c52c1758d8e591ee222e2fd263
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-23T22:01:34Z
fix: NPM installation error
commit c270bfd5c3aa20d1a4b0fe9873d2a27e8195c0d9
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-24T13:13:16Z
feat: Save spell result and propagate
commit 8294bcd82d5a1fd5c65f6e7c60ddf0e0a27790f4
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-24T23:25:20Z
refactor: result controller retry
commit 1fa4df863a2724c34216bdcf69e7277ecd48c711
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-25T00:26:03Z
refactor: NotebookServer.java
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/1940
Merge to master if no further comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/1940
@1ambda Awesome new feature! LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #1940: [ZEPPELIN-2008] Introduce Spell
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/1940
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/1940
Thanks @1ambda for contributing very interesting new feature.
I've play around with it and have couple of feedbacks.
### Couldn't make Spell as display system work
> The most important thing is, spell is not only interpreter but also display system. Because it runs on browser. This enable us to combine existing backend interpreters with spell like
So i tried following but couldn't make it work. Am i missing something?
### Configuring Display order of Spell doesn't make sense
Enabled spells are displayed in 'Helium' menu like below.
Which is designed to change display order of visualization package when they displayed in chart selector inside of the paragraph.
As far as i understand, Spell does not have to be listed in the chart selector and does not have to be listed there and change order. So i think reordering should be only for 'Visualization' type package.
I think Spell can create another section just below enabled visualization buttons, to list enabled spells.
### Display keyword of the spell
Each spell has keyword to invoke. For example `markdown-spell`'s keyword is `%markdown`, `flowchart-spell`'s keyword is `%flowchart`. However, i couldn't find these keyword anywhere but the source code.
So it'll be nice if 'Helium' menu displays keyword of the Spell type packages.
### Example for example spells
If there's some informations or example that how to use (syntax, etc) included example Spells, (e.g. in description section or somewhere) it'll be more convenient.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/1940
Thanks addressing the comments. Tested and it works well as expected.
Agree, 1) can be handled in separate issue while this PR already quite big.
CI fails on RAT check profile
```
/home/travis/build/apache/zeppelin/zeppelin-interpreter/src/main/java/org/apache/zeppelin/helium/HeliumType.java
/home/travis/build/apache/zeppelin/zeppelin-interpreter/src/main/java/org/apache/zeppelin/helium/SpellPackageInfo.java
/home/travis/build/apache/zeppelin/zeppelin-interpreter/src/test/java/org/apache/zeppelin/helium/HeliumPackageTest.java
```
Looks like above 3 file need license header. @1ambda could you check?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/1940
rebased to resolve conflict with master.
@Leemoonsoo Thanks for detailed review! I updated code according to your comment.
(1) **Couldn't make Spell as display system work:** Let's handle in following PRs.
originally, i meant "we can use spell display with backend interpreter **if we modify some backend code**". Sorry for confusing. I tried to fix it in this PR, but it's not that easy as i expected since current `InterpreterResult.Type` is enum, not string. IMO, it can be handled in following PRs. (I modified this PR description too)
- https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOutput.java#L225
(2) **Configuring Display order of Spell doesn't make sense**: Fixed.
(3, 4) **Display keyword of the spell**, **Example for example spells**
Now `#/helium` page displays `spell.magic`, `spell.usage` which are saved helium package json.
<img width="707" alt="screen shot 2017-01-30 at 9 32 42 am" src="https://cloud.githubusercontent.com/assets/4968473/22409610/b00f928c-e6cf-11e6-9035-8692cc32e345.png">
```
{
"type" : "SPELL",
"name" : "translator-spell",
...
"spell": {
"magic": "%translator",
"usage": "%translator <source>-<target> <access-key> <TEXT>"
}
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the issue:
https://github.com/apache/zeppelin/pull/1940
From now, `SPELL` type pkg info fetched from [Helium registry](https://s3.amazonaws.com/helium-package/helium.json) is also shown in GUI :)
<img width="1274" alt="screen shot 2017-02-01 at 6 24 38 pm" src="https://cloud.githubusercontent.com/assets/10060731/22501386/e56b8336-e8ab-11e6-94ee-e0bed54fe2a1.png">
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/1940
finally, CI is green. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---