[ZEPPELIN-1639] Add tests with external python dependencies to CI build by agoodm · Pull Request #1632 · apache/zeppelin

agoodm · 2016-11-14T21:34:09Z

What is this PR for?

Take 2 of #1618 because I had some earlier problems with rebasing. Since, then I have added some new features, namely:

Matplotlib integration tests for pyspark
install_external_dependencies.sh which conditionally installs the R or python dependencies based on the specified build profile in .travis.yml. This saves several minutes of time for a few of the build profiles since the R dependencies are compiled from source and therefore take quite a bit of time to install.
The extra python unit tests which require external dependencies (matplotlib and pandas) are now relegated to two separate build profiles. This is done primarily to efficiently test both Python 2 and 3.
Some minor bugs in the python and pyspark interpreters (mostly with respect to python 3 compatibility) were caught as a result of these tests, and are also fixed in this PR.

What type of PR is it?

Improvement and Bugfix

What is the Jira issue?

ZEPPELIN-1639

How should this be tested?

CI tests should be green!

Questions:

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

agoodm · 2016-11-14T21:36:19Z

ping @bzz @felixcheung @Leemoonsoo

felixcheung · 2016-11-15T00:29:39Z

.travis.yml


+    # Test python/pyspark with python2
+    - jdk: "oraclejdk7"
+      env: PYTHON="2.7" SCALA_VER="2.10" SPARK_VER="1.6.1" HADOOP_VER="2.3" PROFILE="-Pspark-1.6 -Phadoop-2.3 -Ppyspark" BUILD_FLAG="package -pl spark,python -am -DskipTests -DskipRat" TEST_FLAG="verify -DskipRat" TEST_PROJECTS="-pl zeppelin-interpreter,zeppelin-display,spark-dependencies,spark,python -Dtest=org.apache.zeppelin.spark.PySpark*Test,org.apache.zeppelin.python.* -Dpyspark.test.exclude='' -DfailIfNoTests=false"


I think maybe we could run only a small subset of this list
-pl zeppelin-interpreter,zeppelin-display,spark-dependencies,spark,python

or perhaps we could have one of python in the test mix of the spark one above?

Though I listed all of the projects which the spark and python depend on, the only actual tests that are being run in this build profile are Pyspark as well as all of the Python interpreter tests.

Essentially the purpose of this profile is to run all python specific tests only, and do so for both python 2 and 3. Since we have two pure spark testing profiles, we would need to double that if we bundle these python tests as well if we want to cover both python 2 and 3, which would lead to waste since that would mean running the non-python tests multiple times for no benefit. Am I misunderstanding and did you mean something else?

I was thinking the existing spark profile for spark 2.0.0 (or the latest) could also run the Python 3 tests too?

Basically I think we are finding that additional test profile in the matrix could increase test run time and decrease overall stability, so if we could avoid adding more profile the better

Ah, I see. I think that's a bit of a tough call. On one hand, I think it makes sense because we originally designated that profile for "testing all modules", so it would be logical to include these additional tests too, and it saves us a little bit of time in terms of reducing the number of builds by one. On the other hand, it also means that we can't guarantee an "apples to apples comparison" with the python 2 test so to speak in case a test failure happens prior to the python tests in the build. I think given the the general flakiness of the recent builds though, your suggestion seems like the right way to go. Do you have any thoughts on this, @bzz or @Leemoonsoo ?

felixcheung · 2016-11-15T00:32:58Z

testing/install_external_dependencies.sh

+  conda update -q conda
+  conda info -a
+  conda config --add channels conda-forge
+  conda install -q matplotlib pandasql


where do they go in the install - is this something we could leverage Travis's container base run caching to avoid redownloading it everytime?

In this case, conda installs packages to $HOME/miniconda/lib/pythonX.Y/site-packages. Note that these dependencies are binary packages, so the total download plus installation time is very short. I think this would be worth looking into for the R packages though as those take several minutes to download, compile, and install.

even so, if there are opportunities to optimize or to avoid download, we should try to cache it? I think we are finding any download from travis could be a source of fragility, as it could be related network issue in travis, or the target host being temporarily unavailable etc

Fair enough. It might be slightly trickier to do this here than with native python projects since we would need to cache two different installation environments, one for each python version. I'll see what I can do.

felixcheung · 2016-11-15T00:34:21Z

.travis.yml

+
+    # Test python/pyspark with python3
+    - jdk: "oraclejdk7"
+      env: PYTHON="3.5" SCALA_VER="2.10" SPARK_VER="1.6.1" HADOOP_VER="2.3" PROFILE="-Pspark-1.6 -Phadoop-2.3 -Ppyspark" BUILD_FLAG="package -pl spark,python -am -DskipTests -DskipRat" TEST_FLAG="verify -DskipRat" TEST_PROJECTS="-pl zeppelin-interpreter,zeppelin-display,spark-dependencies,spark,python -Dtest=org.apache.zeppelin.spark.PySpark*Test,org.apache.zeppelin.python.* -Dpyspark.test.exclude='' -DfailIfNoTests=false"


below it looks like we are always downloading the latest conda, so if Python 3.6 is release this PYTHON="3.5" would not match, is that going to be an issue?

No, the PYTHON variable is simply used to determine whether or not to use Miniconda2 or 3. I merely labeled it as 3.5 for the purpose of being specific, since that is the actual version of python being used in this case. I should probably just set this to 2 or 3 so it won't confuse anyone.

agoodm · 2016-11-15T09:10:10Z

Thank you for the review @felixcheung , see my responses. Although this is beyond the scope of the original intent of the PR, I have added the R dependencies to the cache, which could potentially save several minutes of time for most of the build profile.

agoodm · 2016-11-18T08:51:14Z

@felixcheung so I have tested caching the miniconda directory and found that it actually slows down the build by 1-2 minutes on average. From what I can tell the other big python projects are not fully caching their conda installations either, at the very least not the dependencies themselves. I think the only compelling reason to cache the miniconda installation at this point is to prevent the build from failing due to download errors. In my own experience this has been very rare until recently, and the errors are always happening after conda gets updated. This is making me think that the problems are most likely some bug in the latest version of conda, so I think I will test this some more with an older version of conda and see how stable the build is.

felixcheung · 2016-11-18T17:27:23Z

That's great. Thanks for looking into it.
I guess we will let Travis run this through a few times at different time of the day (where load varies)

bzz · 2016-11-23T22:46:29Z

@agoodm thank you, looks great! There has been some changes in .travis.yaml, could you please rebase?

agoodm · 2016-11-24T06:20:41Z

@bzz done.

bzz · 2016-11-24T09:45:04Z

@agoodm as CI is failing - could you please rebase on latest master, it has some CI improvements and then open\close the PR to trigger a new CI here again?

agoodm · 2016-11-24T17:40:51Z

I think the CI failures were due to me somehow inadvertently removing the -Ppyspark from the build profile. Let's see how it goes now.

agoodm · 2016-11-24T20:46:49Z

@felixcheung @bzz

So the pyspark cluster test has two failures. One is a result of matplotlib always printing a warning message when it is first loaded, and the other is how python 3 handles unicode. For these reason I think we should either move the python 3 tests to another profile which doesn't perform this test, or have it in a separate profile as I had already done once before. What do you think?

bzz · 2016-11-25T03:52:31Z

.travis.yml


+    # Test python/pyspark with python 2
+    - jdk: "oraclejdk7"
+      env: PYTHON="2" SCALA_VER="2.10" SPARK_VER="1.6.1" HADOOP_VER="2.3" PROFILE="-Pspark-1.6 -Phadoop-2.3 -Ppyspark" BUILD_FLAG="package -pl spark,python -am -DskipTests -DskipRat" TEST_FLAG="verify -DskipRat" TEST_PROJECTS="-pl zeppelin-interpreter,zeppelin-display,spark-dependencies,spark,python -Dtest=org.apache.zeppelin.spark.PySpark*Test,org.apache.zeppelin.python.* -Dpyspark.test.exclude='' -DfailIfNoTests=false"


Did not know that -Dtest= overrides -DskipTests

bzz · 2016-11-25T04:02:09Z

Thank you for CI failure assessment @agoodm !

How do you think, how crucial are those tests? May be just documenting on how to run them manually could be an option?

Generally, we tried to avoid exploding number of CI profiles to keep the CI build time minimum, but if more profiles is not going cost substantial build time then it definitely could be an option. In that case any of the suggested solutions sounds good to me.

agoodm · 2016-11-25T06:14:23Z

To be honest, I don't see it being that big of a deal. Both profiles take about 5 minutes each which I think is fairly short, since up until recently we were spending 4 minutes just installing the R dependencies for all of the other profiles. Additionally, this would make it much easier to test everything for both python 2 and 3, which can be quite a pain to do manually with conda / virtualenv (eg, fiddling with the PATH before running tests for each case). Also, matplotlib and pandas are most likely used for a large percentage of python / pyspark use cases in Zeppelin, so I think having that integration tested would be really useful.

felixcheung · 2016-11-25T07:51:12Z

Thanks for spending more time investigating this. I'd agree with your assessment.

felixcheung · 2016-11-25T19:03:49Z

.travis.yml

      env: SCALA_VER="2.11" PROFILE="-Prat" BUILD_FLAG="clean" TEST_FLAG="org.apache.rat:apache-rat-plugin:check" TEST_PROJECTS=""

-    # Test all modules with spark 2.0.0 and scala 2.11
+    # Test all modules with spark 2.0.0 and scala 2.11 and python 3


should revert this now

felixcheung · 2016-11-25T19:04:48Z

.travis.yml

+
+    # Test python/pyspark with python 3
+    - jdk: "oraclejdk7"
+      env: PYTHON="3" SCALA_VER="2.10" SPARK_VER="1.6.1" HADOOP_VER="2.3" PROFILE="-Pspark-1.6 -Phadoop-2.3 -Ppyspark" BUILD_FLAG="package -pl spark,python -am -DskipTests -DskipRat" TEST_FLAG="verify -DskipRat" TEST_PROJECTS="-pl zeppelin-interpreter,zeppelin-display,spark-dependencies,spark,python -Dtest=org.apache.zeppelin.spark.PySpark*Test,org.apache.zeppelin.python.* -Dpyspark.test.exclude='' -DfailIfNoTests=false"


was there a reason Python tests are building with Spark 1.6.1 instead of 2.0.0? Perhaps it is better to stay with the latest?

There is no particular reason, since any spark version should work. I simply just copied / pasted another profile when I began writing these ones.

I see. Perhaps one for stay at 1.6.1 and another (like Python3) we could change to 2.0.0? what do you think?

felixcheung · 2016-11-25T19:11:57Z

great! a few minor comments (like like you addressed one already!)
CI passed the last time. I think this is looking good.

So to recap, we are able to cache R but not miniconda, correct?

Any other comment - merging when CI complete.

agoodm · 2016-11-25T19:17:49Z

Correct, the main reason being that caching it actually increases the build time. I still have one slight nitpick myself, but I'll leave that up to others if that is worth pursuing right now (the newly added docker and conda tests have increased the build time for the python profiles by a good bit, so perhaps they should be excluded somehow since testing them again here has no benefit)

felixcheung · 2016-11-25T19:35:37Z

I see, I think generally if we could isolate test sets so that they don't overlap (ie. python tests not running docker tests), we would likely have more stable CI runs. I think that's really our main concerns - CI stability is kinda high priority since it affects everyone.

But if CI passes for these last 2 runs then I don't mind either way :)

agoodm · 2016-11-25T23:30:57Z

Oh for the love of... looks like I will need to update the cache again. Let me run this one more time.

felixcheung · 2016-11-26T19:36:20Z

looks like transient network error but python 3 test failed. could you rebase and kick this off again?
https://api.travis-ci.org/jobs/178966601/log.txt?deansi=true

agoodm · 2016-11-27T07:04:34Z

@felixcheung so I looked at the travis log more carefully, and I am pretty sure the main thing I was missing was an additional "-Pscala-2.11" to the maven build profile. Hopefully it will finally be green this time.

agoodm · 2016-11-28T07:42:50Z

@felixcheung @bzz

The final remaining travis error is unrelated, it seems to be occurring with other recent PRs (https://travis-ci.org/apache/zeppelin/builds/179341529). Perhaps we should focus on making the logs less verbose, since this profile already produces a log file that's bigger 3 MB on master.

In any case, is this good to finally merge?

bzz · 2016-11-28T10:43:51Z

I think it is mostly the test output that pollutes the CI logs...

So, if I read TravisCI logs right - with 2 new profiles Elapsed time 31 min, and on master i.e yesterday Elapsed time 31 min 14 sec

Looks great to me, merging to master if there is no further discussion!

felixcheung reviewed Nov 15, 2016

View reviewed changes

agoodm force-pushed the ZEPPELIN-1639 branch from 8f88213 to ad71f5a Compare November 21, 2016 23:06

agoodm force-pushed the ZEPPELIN-1639 branch from 2ed2305 to a308e7e Compare November 24, 2016 06:19

agoodm force-pushed the ZEPPELIN-1639 branch from a308e7e to 9ce982a Compare November 24, 2016 17:38

bzz reviewed Nov 25, 2016

View reviewed changes

bzz mentioned this pull request Nov 25, 2016

[ZEPPELIN-1386] Docker images for running Apache Zeppelin releases #1538

Closed

felixcheung reviewed Nov 25, 2016

View reviewed changes

agoodm added 3 commits November 26, 2016 16:52

Add python dependencies to .travis.yml

b6b88be

Shorten python version

42da31c

Travis

3a76958

agoodm added 6 commits November 26, 2016 16:52

Move python 3 tests to all modules with spark 2.0

ee95d67

Re-add pyspark to build profile

5709c5d

Update tests for new InterpreterContext constructor

73663f6

Isolate python tests

5a60181

Update comments in .travis.yml

a4f43af

Use spark 2.0 with python 3

363019e

agoodm force-pushed the ZEPPELIN-1639 branch from 833dbf5 to 363019e Compare November 27, 2016 00:57

Make sure python 3 profile uses scala 2.11

01380c2

agoodm closed this Nov 28, 2016

agoodm reopened this Nov 28, 2016

asfgit closed this in 4ac577f Nov 29, 2016

agoodm mentioned this pull request Nov 29, 2016

[ZEPPELIN-212] Multiple paragraph results #1658

Closed

5 tasks

Conversation

agoodm commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

Questions:

Uh oh!

agoodm commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agoodm Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agoodm commented Nov 15, 2016

Uh oh!

agoodm commented Nov 18, 2016

Uh oh!

felixcheung commented Nov 18, 2016

Uh oh!

bzz commented Nov 23, 2016

Uh oh!

agoodm commented Nov 24, 2016

Uh oh!

bzz commented Nov 24, 2016

Uh oh!

agoodm commented Nov 24, 2016

Uh oh!

agoodm commented Nov 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzz commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agoodm commented Nov 25, 2016

Uh oh!

felixcheung commented Nov 25, 2016 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Nov 25, 2016

Uh oh!

agoodm commented Nov 25, 2016

Uh oh!

felixcheung commented Nov 25, 2016

Uh oh!

agoodm commented Nov 25, 2016

Uh oh!

felixcheung commented Nov 26, 2016

Uh oh!

agoodm commented Nov 14, 2016 •

edited

Loading

agoodm Nov 15, 2016 •

edited

Loading

felixcheung Nov 15, 2016 •

edited

Loading

bzz commented Nov 25, 2016 •

edited

Loading