ZEPPELIN-1105: Python - add paragraph ERROR status #1124
Conversation
bzz
changed the title
bzz
force-pushed
the
ZEPPELIN-1105/python/add-paragraph-error-statu
branch
from
e68fae8 to
a7bf8f3
Compare
|
reabsed on latest master |
|
Tested basic functionality. Looks good to me. CI failure is irrelevant and being taken care of under #1129. |
|
Thank you @minahlee for prompt review and notice on CI fix, I really appreciate! Merging if there is no more discussion |
| private Boolean py4J = false; | ||
| private Boolean py4JisInstalled = false; | ||
| private InterpreterContext context; | ||
| private Pattern errorInLastLine = Pattern.compile(".*(Error|Exception): .*$"); |
There was a problem hiding this comment.
this seems rather fragile, it would match something like:
there is no Error: ok
There was a problem hiding this comment.
Maybe we should put user code under try ... except or re-direct sys.stderr and check for non-empty value, as an alternative?
There was a problem hiding this comment.
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!
|
I think this is a good improvement - left a comment. |
|
CI is green now |
### What is this PR for? Implement paragraph ERROR status for Python interpreter in case of Error or Exception in the output. ### What type of PR is it? Improvement ### What is the Jira issue? [ZEPPELIN-1105](https://issues.apache.org/jira/browse/ZEPPELIN-1105) ### How should this be tested? CI should pass, or ``` mvn "-Dtest=org.apache.zeppelin.python.PythonInterpreterWithPythonInstalledTest" test -pl python ``` should pass, or paragraph status should be ERROR for something like ``` import some-thing ``` ### 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: Alexander Bezzubov <bzz@apache.org> Closes apache#1124 from bzz/ZEPPELIN-1105/python/add-paragraph-error-statu and squashes the following commits: a7bf8f3 [Alexander Bezzubov] Python: add missing license header b585982 [Alexander Bezzubov] Python: include Python-dependant tests to 1 CI profile e7d5371 [Alexander Bezzubov] Python: add ERROR paragraph status on Error and Exception in output 4c1107b [Alexander Bezzubov] Refactoring: rename and extract var assignment
3 participants
What is this PR for?
Implement paragraph ERROR status for Python interpreter in case of Error or Exception in the output.
What type of PR is it?
Improvement
What is the Jira issue?
ZEPPELIN-1105
How should this be tested?
CI should pass, or
should pass, or paragraph status should be ERROR for something like
Screenshots (if appropriate)
Questions: