Skip to content

Conversation

@rerngvit
Copy link
Contributor

What is this PR for?

This PR applies the new interpreter registration mechanism to KnitR and RRepl.

What type of PR is it?

Improvement

Todos

  • Move interpreter registration properties from static block to interpreter-setting.json

What is the Jira issue?

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

How should this be tested?

  1. apply patch
  2. rm -r interpreter/r
  3. rm conf/interpreter.json
  4. mvn clean package -DskipTests -Pspark-1.6 -Psparkr
  5. bin/zeppelin-daemon.sh start
  6. run some paragraph with simple R queries

Questions:

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

},
{
"group": "r",
"name": "spark",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should exchange the name and the group from this to the above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jongyoul Thank you for your review. I addressed it in the latest patch. Please have a look.

@jongyoul
Copy link
Member

@rerngvit In a description, I think -Psparkr is not related to this module. we should use -Pr, right?

@rerngvit rerngvit force-pushed the ZEPPELIN-921 branch 2 times, most recently from a7055d4 to fd7fa13 Compare June 24, 2016 19:26
@rerngvit
Copy link
Contributor Author

@jongyoul regarding the description text, according to the build documentation, both options should enable R support. Please let me know, if I miss something.

Note that I also tried building with "-Pr" instead of "-Psparkr". However, even on fresh master branch (i.e., not this patch at all) will result in a runtime error when trying to run an R paragraph ("org.apache.thrift.TApplicationException: Internal error processing createInterpreter"). I think this also should be resolved, but it is unrelated to this PR.

@jongyoul
Copy link
Member

@rerngvit I'm not good at R, if you, however, built with -Pr and got an error while creating interpreter, AFAIK, you should have a R installed by default. Could you please check this first?

@elbamos
Copy link
Contributor

elbamos commented Jun 27, 2016

@rerngvit Thanks for looking at this!

There were conflicts introduced with the addition of the livy interpreter (or around that time) because sparkr was hard-coded into some of the configuration files. I've been waiting for all of that code to stabilize before attempting a fix.

@rerngvit
Copy link
Contributor Author

@jongyoul I already installed R on my machine. I showed an excerpt of R command from my shell terminal below. As I mentioned above, trying to build with -Psparkr is working fine, which indicates that a problem is not the R installation (but rather likely a building issue, which is not related to this PR).

$ R --version
R version 3.3.0 (2016-05-03) -- "Supposedly Educational"
Copyright (C) 2016 The R Foundation for Statistical Computing
Platform: x86_64-apple-darwin15.5.0 (64-bit)

@elbamos Thanks for letting me know. That might be the reason for the problem.

@jongyoul
Copy link
Member

@rerngvit Could you please try to build with -Pr after rebase it from current master?

This PR applies the new interpreter registration mechanism to KnitR and RRepl.

### What type of PR is it?
Improvement

### Todos
- Move interpreter registration properties from static block to interpreter-setting.json

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-921

### How should this be tested?
1. apply patch
2. rm -r interpreter/r
3. rm conf/interpreter.json
4. mvn clean package -DskipTests -Pspark-1.6 -Psparkr
5. bin/zeppelin-daemon.sh start
6. run some paragraph with simple R queries

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
@rerngvit
Copy link
Contributor Author

rerngvit commented Jul 4, 2016

@jongyoul I have tried to rebuild with -Pr after rebase to master. I did not get the error "org.apache.thrift.TApplicationException: Internal error processing createInterpreter" anymore. Instead, I got the error "Prefix not found" when I try to run an R paragraph at runtime. This error does not occur on the master branch. This means that the problem is related to how the dynamic registration is done (this PR). Do you have an idea of what likely the root cause of the problem? Thanks.

@jongyoul
Copy link
Member

jongyoul commented Jul 4, 2016

@rerngvit How about stopping Zeppelin, removing conf/interpreter.json, starting Zeppelin and testing it again?

@rerngvit
Copy link
Contributor Author

rerngvit commented Jul 5, 2016

@jongyoul Thanks. However, I tried that before and still observed the same behaviour. It seems to be related to something else: i.e., the fact interpreter-setting.json for r interpreter is not registered from InterpreterFactory.java for some reason. Any other suggestions are welcome.

@rerngvit
Copy link
Contributor Author

rerngvit commented Jul 5, 2016

I think I found the root cause of the problem. The key issue is that currently the build configuration packages "zeppelin-zrinterpreter-0.6.0-SNAPSHOT.jar" under "interpreter/spark" instead of under "interpreter/r" (as similar to other interpreter like "flink" or "sh"). Then, when the registration process is done at "InterpreterFactory.java" method "registerInterpreterFromResource" takes the "interpreter-setting.json" from "zeppelin-spark-0.6.0-SNAPSHOT.jar". I will try to change this build configuration and see whether it addressed the issue.

@elbamos
Copy link
Contributor

elbamos commented Jul 5, 2016

I'm sorry I have not had a chance to finish addressing this. One issue is that a PR to interpreter.sh removed the rinterpreter jar from the classpath. It needs to be put back in. That will cause knitr to work. The second issue is that the other spark interpreter is hardcoded into the configuration files I believe related to livy - so it ignores the registration for rrepl as spark.r.

On Jul 5, 2016, at 5:38 PM, Rerngvit Yanggratoke [email protected] wrote:

I think I found the root cause of the problem. The key issue is that currently the build configuration packages "zeppelin-zrinterpreter-0.6.0-SNAPSHOT.jar" under "interpreter/spark" instead of under "interpreter/r" (as similar to other interpreter like "flink" or "sh"). Then, when the registration process is done at "InterpreterFactory.java" method "registerInterpreterFromResource" takes the "interpreter-setting.json" from "zeppelin-spark-0.6.0-SNAPSHOT.jar". I will try change this configuration and see whether it addressed the issue.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@rerngvit
Copy link
Contributor Author

@elbamos Thanks for your information. I found that moving "zeppelin-zrinterpreter-0.6.0-SNAPSHOT.jar" to under "interpreter/r" will resolve the issue and enable the registration process for Knitr and RRepl to take place. However, it would result in a different runtime error when trying to run an R paragraph. I think this issue should be waited until that dependant problem is resolved.

@rerngvit
Copy link
Contributor Author

rerngvit commented Aug 5, 2016

I have not got any replies on this. Let me close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants