Skip to content
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.

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

[ZEPPELIN-2756] Support ansi escape code for colorizing output in browser #2510

Closed

Conversation

1ambda
Copy link
Member

@1ambda 1ambda commented Jul 31, 2017

What is this PR for?

Support ANSI escape code for colorizing output in browser using ansi-up

This feature is written based on #2474

What type of PR is it?

[Improvement]

What is the Jira issue?

ZEPPELIN-2756

How should this be tested?

Setup ipython interpreter (or use any interpreter can return ansi escape code)

%python.ipython

import pandas as pd
df = pd.DataFrame({'id':[1,2,3], 'name': ['a', 'b', 'c']})
z.show(df)

Screenshots (if appropriate)

Before

image

After

image

Questions:

  • Does the licenses files need update? - YES, updated
  • Is there breaking changes for older versions? - YES, all outputs will be escaped
  • Does this needs documentation? - NO

@cloverhearts
Copy link
Member

AWESOME!

@1ambda
Copy link
Member Author

1ambda commented Jul 31, 2017

CI failed, but irrelevant. The error is related with Ipython which is going on #2510

Results :

Failed tests: 
  SparkParagraphIT.testPySpark:140 Paragraph from SparkParagraphIT of testPySpark result: 
Expected: "test loop 0\ntest loop 1\ntest loop 2"
     but: was "IPython is available, use IPython for PySparkInterpreter"

Tests in error: 
  InterpreterModeActionsIT.testPerUserIsolatedAction:674->logoutUser:154 » Timeout
  InterpreterModeActionsIT.testPerUserScopedAction:336->authenticationUser:136->AbstractZeppelinIT.pollingWait:94 » Timeout
  InterpreterModeActionsIT.testGloballyAction:183->authenticationUser:136->AbstractZeppelinIT.pollingWait:94 » Timeout

Tests run: 30, Failures: 1, Errors: 3, Skipped: 0

@zjffdu
Copy link
Contributor

zjffdu commented Jul 31, 2017

Great, Thanks @1ambda

.travis.yml Outdated

# Test spark module for 2.2.0 with scala 2.11, livy
- jdk: "oraclejdk8"
dist: precise
env: SCALA_VER="2.11" SPARK_VER="2.2.0" HADOOP_VER="2.6" PROFILE="-Pweb-ci -Pspark-2.2 -Phadoop-2.6 -Pscala-2.11" SPARKR="true" BUILD_FLAG="package -DskipTests -DskipRat" TEST_FLAG="test -DskipRat" MODULES="-pl .,zeppelin-interpreter,zeppelin-zengine,zeppelin-server,zeppelin-display,spark-dependencies,spark,livy" TEST_PROJECTS="-Dtest=ZeppelinSparkClusterTest,org.apache.zeppelin.spark.*,org.apache.zeppelin.livy.* -DfailIfNoTests=false"
env: PYTHON="2" SCALA_VER="2.11" SPARK_VER="2.2.0" HADOOP_VER="2.6" PROFILE="-Pweb-ci -Pspark-2.2 -Phadoop-2.6 -Pscala-2.11" SPARKR="true" BUILD_FLAG="install -DskipTests -DskipRat" TEST_FLAG="test -DskipRat" MODULES="-pl .,zeppelin-interpreter,zeppelin-zengine,zeppelin-server,zeppelin-display,spark-dependencies,spark,python,livy" TEST_PROJECTS="-Dtest=ZeppelinSparkClusterTest,org.apache.zeppelin.spark.*,org.apache.zeppelin.livy.* -DfailIfNoTests=false"
Copy link
Member

Choose a reason for hiding this comment

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

change some of these to PYTHON="3"?

python/README.md Outdated

# IPython Architecture
Current interpreter delegate the whole work to ipython kernel via `jupyter_client`. Zeppelin would launch a python process which host the ipython kernel.
Zeppelin interpreter process will communicate with the python process via `grpc`. Ideally every feature works in IPython should work in Zeppelin as well.
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 we should scale back on the claim here, as commented before - not everything would work here :)

LOGGER.info("Wait for IPython Kernel to be started");
}
} catch (Exception e) {
// ignore the exception, because is may happen when grpc server has not started yet.
Copy link
Member

Choose a reason for hiding this comment

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

should still log the error/exception?

@Leemoonsoo
Copy link
Member

@1ambda This PR includes some commits from #2474. Is it intended?

@zjffdu
Copy link
Contributor

zjffdu commented Aug 4, 2017

I think @1ambda just want to verify this PR based my PR, @1ambda can rebase this PR after #2474 is merged.

@1ambda
Copy link
Member Author

1ambda commented Aug 5, 2017

@zjffdu is right. This is based on #2474 as described in the PR description. Please review last 2 commits except for @zjffdu's commits.

@felixcheung
Copy link
Member

LG, I guess we go back to 2474 then

@zjffdu
Copy link
Contributor

zjffdu commented Aug 28, 2017

@1ambda I have merged #2474, can you rebase this PR ? Thanks

@zjffdu
Copy link
Contributor

zjffdu commented Aug 30, 2017

ping @1ambda I can do the rebase if you are busy with other stuff and can merge it if you don't mind.

@1ambda
Copy link
Member Author

1ambda commented Aug 31, 2017

@zjffdu Sorry for the late response. I can do in few hours. Thanks.

@1ambda 1ambda force-pushed the ZEPPELIN-2756/escape-ansi-code-in-browser branch from dd97d00 to 143fbbb Compare August 31, 2017 05:53
@1ambda
Copy link
Member Author

1ambda commented Aug 31, 2017

rebased to resolve conflict.

@1ambda
Copy link
Member Author

1ambda commented Aug 31, 2017

Verified that this PR works on master with ipython interpreter. (See the timestamp in the paragraph)

image

@zjffdu
Copy link
Contributor

zjffdu commented Aug 31, 2017

Thanks @1ambda I also tested it, it works very well. LGTM

Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

LGTM.
I assume AnsiUpConverter.ansi_to_html handles text already in html format correctly?

@1ambda
Copy link
Member Author

1ambda commented Aug 31, 2017

@felixcheung It will because It converts ANSI codes whatever the content format is.

And I intentionally made a change to convert TEXT format (of Zeppelin output) only to minimize the change.

// ANSI handler is in this function

 const renderText = function (targetElemId, data) {   

@1ambda 1ambda closed this Aug 31, 2017
@1ambda 1ambda reopened this Aug 31, 2017
@1ambda 1ambda closed this Aug 31, 2017
@1ambda 1ambda reopened this Aug 31, 2017
@felixcheung
Copy link
Member

got it, thanks!

@1ambda
Copy link
Member Author

1ambda commented Aug 31, 2017

CI passed. Merge if no more discussion.

@asfgit asfgit closed this in 69d58f3 Sep 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants