Skip to content

ZEPPELIN-1105: Python - add paragraph ERROR status #1124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bzz wants to merge 4 commits into from
Closed

ZEPPELIN-1105: Python - add paragraph ERROR status #1124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c1107b
Refactoring: rename and extract var assignment
bzz Jul 4, 2016
e7d5371
Python: add ERROR paragraph status on Error and Exception in output
bzz Jul 4, 2016
b585982
Python: include Python-dependant tests to 1 CI profile
bzz Jul 4, 2016
a7bf8f3
Python: add missing license header
bzz Jul 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ matrix:
include:
# Test all modules
- jdk: "oraclejdk7"
env: SPARK_VER="1.6.1" HADOOP_VER="2.3" PROFILE="-Pspark-1.6 -Pr -Phadoop-2.3 -Ppyspark -Psparkr -Pscalding -Pexamples" BUILD_FLAG="package -Pbuild-distr" TEST_FLAG="verify -Pusing-packaged-distr" TEST_PROJECTS=""
env: SPARK_VER="1.6.1" HADOOP_VER="2.3" PROFILE="-Pspark-1.6 -Pr -Phadoop-2.3 -Ppyspark -Psparkr -Pscalding -Pexamples" BUILD_FLAG="package -Pbuild-distr" TEST_FLAG="verify -Pusing-packaged-distr" TEST_PROJECTS="-Dpython.test.exclude=''"

# Test spark module for 1.5.2
- jdk: "oraclejdk7"
Expand Down
26 changes: 19 additions & 7 deletions python/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

<properties>
<py4j.version>0.9.2</py4j.version>
<python.test.exclude>**/PythonInterpreterWithPythonInstalledTest.java</python.test.exclude>
</properties>

<dependencies>
Expand Down Expand Up @@ -67,7 +68,7 @@
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
</dependency>

<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
Expand All @@ -89,15 +90,25 @@
<plugins>
<plugin>
<artifactId>maven-enforcer-plugin</artifactId>
<version>1.3.1</version>
<executions>
<execution>
<id>enforce</id>
<phase>none</phase>
<version>1.3.1</version>
<executions>
<execution>
<id>enforce</id>
<phase>none</phase>
</execution>
</executions>
</plugin>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<excludes>
<exclude>${python.test.exclude}</exclude>
</excludes>
</configuration>
</plugin>

<plugin>
<artifactId>maven-dependency-plugin</artifactId>
<version>2.8</version>
Expand Down Expand Up @@ -135,11 +146,12 @@
<version>${project.version}</version>
<type>${project.packaging}</type>
</artifactItem>
</artifactItems>
</artifactItems>
</configuration>
</execution>
</executions>
</plugin>

</plugins>
</build>

Expand Down
37 changes: 29 additions & 8 deletions python/src/main/java/org/apache/zeppelin/python/PythonInterpreter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import java.util.Collection;
import java.util.List;
import java.util.Properties;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.apache.zeppelin.display.GUI;
import org.apache.zeppelin.interpreter.Interpreter;
Expand All @@ -34,7 +36,6 @@
import org.apache.zeppelin.scheduler.Job;
import org.apache.zeppelin.scheduler.Scheduler;
import org.apache.zeppelin.scheduler.SchedulerFactory;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -54,8 +55,9 @@ public class PythonInterpreter extends Interpreter {

private Integer port;
private GatewayServer gatewayServer;
private Boolean py4J = false;
private Boolean py4JisInstalled = false;
private InterpreterContext context;
private Pattern errorInLastLine = Pattern.compile(".*(Error|Exception): .*$");
Copy link
Member

@felixcheung felixcheung Jul 5, 2016

Choose a reason for hiding this comment

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

this seems rather fragile, it would match something like:

there is no Error: ok

Copy link
Member

@felixcheung felixcheung Jul 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we should put user code under try ... except or re-direct sys.stderr and check for non-empty value, as an alternative?

Copy link
Member Author

@bzz bzz Jul 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Good points, thank you @felixcheung !

There is definitely room for improvement - this is more like a foundation of this feature, as previous impl always returned SUCCESS.

Your example of print("there is no Error: ok") will indeed trigger the the false positive ERROR status.

I have filed ZEPPELIN-1114 and logged this feedback there. I plan to take care of it in a subsequent PRs.

Please let me know what you think!

private int maxResult;

PythonProcess process = null;
Expand Down Expand Up @@ -91,7 +93,8 @@ public void open() {
LOG.error("Can't execute " + BOOTSTRAP_PY + " to initiate python process", e);
}

if (py4J = isPy4jInstalled()) {
py4JisInstalled = isPy4jInstalled();
if (py4JisInstalled) {
port = findRandomOpenPortOnAllLocalInterfaces();
LOG.info("Py4j gateway port : " + port);
try {
Expand Down Expand Up @@ -123,13 +126,31 @@ public void close() {

@Override
public InterpreterResult interpret(String cmd, InterpreterContext contextInterpreter) {
this.context = contextInterpreter;
if (cmd == null || cmd.isEmpty()) {
return new InterpreterResult(Code.SUCCESS, "");
}
this.context = contextInterpreter;
String output = sendCommandToPython(cmd);
return new InterpreterResult(Code.SUCCESS, output.replaceAll(">>>", "")
.replaceAll("\\.\\.\\.", "").trim());

InterpreterResult result;
if (pythonErrorIn(output)) {
result = new InterpreterResult(Code.ERROR, output.replaceAll(">>>", "").trim());
} else {
result = new InterpreterResult(Code.SUCCESS, output.replaceAll(">>>", "")
.replaceAll("\\.\\.\\.", "").trim());
}
return result;
}

/**
* Checks if there is a syntax error or an exception
*
* @param output Python interpreter output
* @return true if syntax error or exception has happened
*/
private boolean pythonErrorIn(String output) {
Matcher errorMatcher = errorInLastLine.matcher(output);
return errorMatcher.find();
}

@Override
Expand Down Expand Up @@ -195,7 +216,7 @@ private String sendCommandToPython(String cmd) {
return output;
}

private void bootStrapInterpreter(String file) throws IOException {
void bootStrapInterpreter(String file) throws IOException {
BufferedReader bootstrapReader = new BufferedReader(
new InputStreamReader(
PythonInterpreter.class.getResourceAsStream(file)));
Expand All @@ -205,7 +226,7 @@ private void bootStrapInterpreter(String file) throws IOException {
while ((line = bootstrapReader.readLine()) != null) {
bootstrapCode += line + "\n";
}
if (py4J && port != null && port != -1) {
if (py4JisInstalled && port != null && port != -1) {
bootstrapCode = bootstrapCode.replaceAll("\\%PORT\\%", port.toString());
}
LOG.info("Bootstrap python interpreter with code from \n " + file);
Expand Down
14 changes: 11 additions & 3 deletions python/src/test/java/org/apache/zeppelin/python/PythonInterpreterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@

package org.apache.zeppelin.python;

import static org.apache.zeppelin.python.PythonInterpreter.*;
import static org.apache.zeppelin.python.PythonInterpreter.DEFAULT_ZEPPELIN_PYTHON;
import static org.apache.zeppelin.python.PythonInterpreter.MAX_RESULT;
import static org.apache.zeppelin.python.PythonInterpreter.ZEPPELIN_PYTHON;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
Expand Down Expand Up @@ -45,8 +47,12 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;


/**
* Python interpreter unit test
*
* Important: ALL tests here DO NOT REQUIRE Python to be installed
* If Python dependency is required, please look at PythonInterpreterWithPythonInstalledTest
*/
public class PythonInterpreterTest {
private static final Logger LOG = LoggerFactory.getLogger(PythonProcess.class);
Expand Down Expand Up @@ -91,7 +97,8 @@ public void testOpenInterpreter() {
* If Py4J is not installed, bootstrap_input.py
* is not sent to Python process and py4j JavaGateway is not running
*/
@Test public void testPy4jIsNotInstalled() {
@Test
public void testPy4jIsNotInstalled() {
pythonInterpreter.open();
assertNull(pythonInterpreter.getPy4jPort());
assertTrue(cmdHistory.contains("def help()"));
Expand All @@ -108,7 +115,8 @@ public void testOpenInterpreter() {
*
* @throws IOException
*/
@Test public void testPy4jInstalled() throws IOException {
@Test
public void testPy4jInstalled() throws IOException {
when(mockPythonProcess.sendAndGetResult(eq("\n\nimport py4j\n"))).thenReturn(">>>");

pythonInterpreter.open();
Expand Down
61 changes: 61 additions & 0 deletions ...on/src/test/java/org/apache/zeppelin/python/PythonInterpreterWithPythonInstalledTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.zeppelin.python;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

import org.apache.zeppelin.interpreter.InterpreterResult;
import org.junit.Test;

/**
* Python interpreter unit test that user real Python
*
* Important: ALL tests here REQUIRE Python to be installed
* They are excluded from default build, to run them manually do:
*
* <code>
* mvn "-Dtest=org.apache.zeppelin.python.PythonInterpreterWithPythonInstalledTest" test -pl python
* </code>
*
* or
* <code>
* mvn -Dpython.test.exclude='' test -pl python
* </code>
*/
public class PythonInterpreterWithPythonInstalledTest {

@Test
public void badSqlSyntaxFails() {
//given
PythonInterpreter realPython = new PythonInterpreter(
PythonInterpreterTest.getPythonTestProperties());
realPython.open();

//when
InterpreterResult ret = realPython.interpret("select wrong syntax", null);

//then
assertNotNull("Interpreter returned 'null'", ret);
//System.out.println("\nInterpreter response: \n" + ret.message());
assertEquals(InterpreterResult.Code.ERROR, ret.code());
assertTrue(ret.message().length() > 0);
}

}