This is an automated email from the ASF dual-hosted git repository. zjffdu pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push: new acf1cbd [ZEPPELIN-4562]. Googlevis doesn't work in IRInterpreter acf1cbd is described below commit acf1cbde2ca12ab85b740c3e94e543af3954281e Author: Jeff Zhang <zjf...@apache.org> AuthorDate: Thu Jan 16 16:08:01 2020 +0800 [ZEPPELIN-4562]. Googlevis doesn't work in IRInterpreter ### What is this PR for? Googlevis doesn't work in IRInterpreter, I fix it by always output html by default for IRInterpreter. ### What type of PR is it? [Bug Fix] ### Todos * [ ] - Task ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-4562 ### How should this be tested? * CI pass, unit test is added ### Screenshots (if appropriate) ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: Jeff Zhang <zjf...@apache.org> Closes #3608 from zjffdu/ZEPPELIN-4562 and squashes the following commits: 23f00cda4 [Jeff Zhang] [ZEPPELIN-4562]. Googlevis doesn't work in IRInterpreter --- .../java/org/apache/zeppelin/r/IRInterpreter.java | 2 +- spark/interpreter/figure/unnamed-chunk-1-1.png | Bin 0 -> 403630 bytes .../spark/PySparkInterpreterMatplotlibTest.java | 2 +- testing/install_external_dependencies.sh | 4 ++-- .../zeppelin/interpreter/InterpreterOutput.java | 6 ++++-- .../apache/zeppelin/jupyter/JupyterKernelClient.java | 9 +++++++-- .../zeppelin/jupyter/JupyterKernelInterpreter.java | 5 +++++ .../org/apache/zeppelin/jupyter/IRKernelTest.java | 19 ++++++++++++++++++- 8 files changed, 38 insertions(+), 9 deletions(-) diff --git a/rlang/src/main/java/org/apache/zeppelin/r/IRInterpreter.java b/rlang/src/main/java/org/apache/zeppelin/r/IRInterpreter.java index 949b1b1..f460e84 100644 --- a/rlang/src/main/java/org/apache/zeppelin/r/IRInterpreter.java +++ b/rlang/src/main/java/org/apache/zeppelin/r/IRInterpreter.java @@ -133,7 +133,7 @@ public class IRInterpreter extends JupyterKernelInterpreter { .replace("${timeout}", timeout) .replace("${isSparkSupported}", "\"" + isSparkSupported() + "\"") .replace("${authSecret}", "\"" + sparkRBackend.socketSecret() + "\""); - LOGGER.info("Init IRKernel via script:\n" + code); + LOGGER.debug("Init IRKernel via script:\n" + code); ExecuteResponse response = jupyterKernelClient.block_execute(ExecuteRequest.newBuilder() .setCode(code).build()); if (response.getStatus() != ExecuteStatus.SUCCESS) { diff --git a/spark/interpreter/figure/unnamed-chunk-1-1.png b/spark/interpreter/figure/unnamed-chunk-1-1.png new file mode 100644 index 0000000..e3ca4d1 Binary files /dev/null and b/spark/interpreter/figure/unnamed-chunk-1-1.png differ diff --git a/spark/interpreter/src/test/java/org/apache/zeppelin/spark/PySparkInterpreterMatplotlibTest.java b/spark/interpreter/src/test/java/org/apache/zeppelin/spark/PySparkInterpreterMatplotlibTest.java index 305839a..4f5c020 100644 --- a/spark/interpreter/src/test/java/org/apache/zeppelin/spark/PySparkInterpreterMatplotlibTest.java +++ b/spark/interpreter/src/test/java/org/apache/zeppelin/spark/PySparkInterpreterMatplotlibTest.java @@ -227,7 +227,7 @@ public class PySparkInterpreterMatplotlibTest { // again but in a different color. ret = pyspark.interpret("plt.plot([1, 2, 3])", context); ret2 = pyspark.interpret("plt.show()", context); - assertNotSame(ret1.message().get(0).getData(), ret2.message().get(0).getData()); + assertEquals(0, ret2.message().size()); } @Test diff --git a/testing/install_external_dependencies.sh b/testing/install_external_dependencies.sh index 492f37e..b9e1b48 100755 --- a/testing/install_external_dependencies.sh +++ b/testing/install_external_dependencies.sh @@ -67,6 +67,6 @@ if [[ "$R" == "true" ]] ; then R -e "install.packages('knitr', repos = 'http://cran.us.r-project.org', lib='~/R')" > /dev/null 2>&1 R -e "install.packages('ggplot2', repos = 'http://cran.us.r-project.org', lib='~/R')" > /dev/null 2>&1 R -e "install.packages('IRkernel', repos = 'http://cran.us.r-project.org', lib='~/R');IRkernel::installspec()" > /dev/null 2>&1 - R -e "install.packages('shiny', repos = 'http://cran.us.r-project.org', lib='~/R');IRkernel::installspec()" > /dev/null 2>&1 - + R -e "install.packages('shiny', repos = 'http://cran.us.r-project.org', lib='~/R')" > /dev/null 2>&1 + R -e "install.packages('googleVis', repos = 'http://cran.us.r-project.org', lib='~/R')" > /dev/null 2>&1 fi diff --git a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOutput.java b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOutput.java index f85e535..3641222 100644 --- a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOutput.java +++ b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOutput.java @@ -329,9 +329,11 @@ public class InterpreterOutput extends OutputStream { List<InterpreterResultMessage> list = new LinkedList<>(); synchronized (resultMessageOutputs) { for (InterpreterResultMessageOutput out : resultMessageOutputs) { - if (out.toInterpreterResultMessage().getType() == InterpreterResult.Type.TEXT && + InterpreterResultMessage msg = out.toInterpreterResultMessage(); + if ((msg.getType() == InterpreterResult.Type.TEXT || + msg.getType() == InterpreterResult.Type.HTML) && StringUtils.isBlank(out.toInterpreterResultMessage().getData())) { - // skip blank text, because when print table data we usually need to print '%text \n' + // skip blank text/html, because when print table data we usually need to print '%text \n' // first to separate it from previous other kind of data. e.g. z.show(df) continue; } diff --git a/zeppelin-jupyter-interpreter/src/main/java/org/apache/zeppelin/jupyter/JupyterKernelClient.java b/zeppelin-jupyter-interpreter/src/main/java/org/apache/zeppelin/jupyter/JupyterKernelClient.java index 1f4e200..0e0902b 100644 --- a/zeppelin-jupyter-interpreter/src/main/java/org/apache/zeppelin/jupyter/JupyterKernelClient.java +++ b/zeppelin-jupyter-interpreter/src/main/java/org/apache/zeppelin/jupyter/JupyterKernelClient.java @@ -22,6 +22,8 @@ import io.grpc.ManagedChannelBuilder; import io.grpc.stub.StreamObserver; import org.apache.commons.lang.exception.ExceptionUtils; import org.apache.zeppelin.interpreter.InterpreterContext; +import org.apache.zeppelin.interpreter.InterpreterResult; +import org.apache.zeppelin.interpreter.InterpreterResultMessageOutput; import org.apache.zeppelin.interpreter.jupyter.proto.JupyterKernelGrpc; import org.apache.zeppelin.interpreter.util.InterpreterOutputStream; import org.apache.zeppelin.interpreter.jupyter.proto.CancelRequest; @@ -155,12 +157,15 @@ public class JupyterKernelClient { // the output from jupyter kernel maybe specify format already. interpreterOutput.write((executeResponse.getOutput()).getBytes()); } else { - // only add %text when the previous output type is not TEXT. + // only add %text when the previous output type is not TEXT & HTML. // Reason : // 1. if no `%text`, it will be treated as previous output type. // 2. Always prepend `%text `, there will be an extra line separator, // because `%text ` appends line separator first. - if (lastOutputType != OutputType.TEXT) { + InterpreterResultMessageOutput curOutput = + interpreterOutput.getInterpreterOutput().getCurrentOutput(); + if (curOutput != null && curOutput.getType() != InterpreterResult.Type.HTML && + curOutput.getType() != InterpreterResult.Type.TEXT) { interpreterOutput.write("%text ".getBytes()); } interpreterOutput.write(executeResponse.getOutput().getBytes()); diff --git a/zeppelin-jupyter-interpreter/src/main/java/org/apache/zeppelin/jupyter/JupyterKernelInterpreter.java b/zeppelin-jupyter-interpreter/src/main/java/org/apache/zeppelin/jupyter/JupyterKernelInterpreter.java index d1dc351..d0e9348 100644 --- a/zeppelin-jupyter-interpreter/src/main/java/org/apache/zeppelin/jupyter/JupyterKernelInterpreter.java +++ b/zeppelin-jupyter-interpreter/src/main/java/org/apache/zeppelin/jupyter/JupyterKernelInterpreter.java @@ -245,6 +245,11 @@ public class JupyterKernelInterpreter extends AbstractInterpreter { interpreterOutput.setInterpreterOutput(context.out); jupyterKernelClient.setInterpreterContext(context); try { + // always use html output for ir kernel. otherwise some R packages doesn't work. + // e.g. googlevis + if (getKernelName().equals("ir")) { + context.out.write("%html\n"); + } ExecuteResponse response = jupyterKernelClient.stream_execute(ExecuteRequest.newBuilder().setCode(st).build(), interpreterOutput); diff --git a/zeppelin-jupyter-interpreter/src/test/java/org/apache/zeppelin/jupyter/IRKernelTest.java b/zeppelin-jupyter-interpreter/src/test/java/org/apache/zeppelin/jupyter/IRKernelTest.java index 002e97b..f3fbffe 100644 --- a/zeppelin-jupyter-interpreter/src/test/java/org/apache/zeppelin/jupyter/IRKernelTest.java +++ b/zeppelin-jupyter-interpreter/src/test/java/org/apache/zeppelin/jupyter/IRKernelTest.java @@ -86,7 +86,7 @@ public class IRKernelTest { assertEquals(InterpreterResult.Code.ERROR, result.code()); resultMessages = context.out.toInterpreterResultMessage(); assertEquals(1, resultMessages.size()); - assertEquals(result.toString(), InterpreterResult.Type.TEXT, resultMessages.get(0).getType()); + assertEquals(result.toString(), InterpreterResult.Type.HTML, resultMessages.get(0).getType()); assertTrue(resultMessages.toString(), resultMessages.get(0).getData().contains("object 'unknown_var' not found")); @@ -99,6 +99,7 @@ public class IRKernelTest { assertEquals(resultMessages.toString(), InterpreterResult.Type.IMG, resultMessages.get(0).getType()); + // ggplot2 result = interpreter.interpret("library(ggplot2)\n" + "ggplot(diamonds, aes(x=carat, y=price, color=cut)) + geom_point()", getInterpreterContext()); @@ -107,6 +108,22 @@ public class IRKernelTest { assertEquals(1, resultMessages.size()); assertEquals(resultMessages.toString(), InterpreterResult.Type.IMG, resultMessages.get(0).getType()); + + // googlevis + context = getInterpreterContext(); + result = interpreter.interpret("library(googleVis)\n" + + "df=data.frame(country=c(\"US\", \"GB\", \"BR\"), \n" + + " val1=c(10,13,14), \n" + + " val2=c(23,12,32))\n" + + "Bar <- gvisBarChart(df)\n" + + "print(Bar, tag = 'chart')", context); + assertEquals(InterpreterResult.Code.SUCCESS, result.code()); + resultMessages = context.out.toInterpreterResultMessage(); + assertEquals(1, resultMessages.size()); + assertEquals(resultMessages.toString(), + InterpreterResult.Type.HTML, resultMessages.get(0).getType()); + assertTrue(resultMessages.get(0).getData(), + resultMessages.get(0).getData().contains("javascript")); } protected InterpreterContext getInterpreterContext() {