[SPARK-32408][BUILD] Disable crossPaths only in GitHub Actions to prevent side effects #29205

HyukjinKwon · 2020-07-23T13:49:42Z

What changes were proposed in this pull request?

This PR is a kind of a followup of #29057 which disabled crossPaths in order to run JUnit tests together properly when an SBT project is explicitly selected. For example, if you run the tests via sbt kvstore/test, it didn't run relevant JUnit tests before disabling crossPaths, see also sbt/junit-interface#35.

Disabling crossPaths removes a Scala version directory to the build output, for example,

from:

target/scala-2.12/classes
target/scala-2.12/test-classes

to:

target/classes
target/test-classes

The previous PR disabled it only for the main SBT projects that relates to the main codes only, and did not disable for other SBT projects such as Unidoc, assembly and examples to minimise the side effects. For example, the jars from the assembly output are still placed under target/scala-2.12 properly.

However, I realised that later there are some codes dependent on this scala-2.12 paths in the tests and main codes, for example, SPARK_PREPEND_CLASSES or some PySpark tests that require some classes present in JVM (you can search the instances via git grep -r "target/scala-"). Although these are mostly for dev purposes, I would like to minimise such side effects by only disabling crossPaths in GitHub Actions as a temporary workaround for now.

In the long run, I think we should disable crossPaths for all projects including Unidoc, assembly and etc.

Roughly speaking, the only advantage of enabling crossPaths is to support SBT's cross building, for example,sbt +package to build multiple Scala versions at once (which Maven build doesn't as far as I know). Up to my knowledge, we're not supporting this at all, and will be very difficult to support considering that we're currently relying on the conversion from poms to SBT projects. More importantly, we're using Maven to make the releases.

There was already a try at SPARK-14574 before which was resolved as a Later. Probably, we should revisit it soon to solve JUnit testing problem together.

Why are the changes needed?

To minimise the side effects from crossPaths such as SPARK_PREPEND_CLASSES or tests that run conditionally if the test classes are present in PySpark.

Does this PR introduce any user-facing change?

No, dev-only.

How was this patch tested?

Manually tested by:

build/sbt -Phadoop-2.7 -Phive -Phive-2.3 -Phive-thriftserver -DskipTests clean test:package
./python/run-tests --python-executable=python3 --testname="pyspark.sql.tests.test_dataframe QueryExecutionListenerTests"

GITHUB_ACTIONS=1 build/sbt -Phadoop-2.7 -Phive -Phive-2.3 -Phive-thriftserver -DskipTests clean test:package
GITHUB_ACTIONS=1 ./python/run-tests --python-executable=python3 --testname="pyspark.sql.tests.test_dataframe QueryExecutionListenerTests"

Before:

python3 version is: Python 3.7.7
Starting test(python3): pyspark.sql.tests.test_dataframe QueryExecutionListenerTests
Finished test(python3): pyspark.sql.tests.test_dataframe QueryExecutionListenerTests (0s) ... 2 tests were skipped
Tests passed in 0 seconds

Skipped tests in pyspark.sql.tests.test_dataframe QueryExecutionListenerTests with python3:
      test_query_execution_listener_on_collect (pyspark.sql.tests.test_dataframe.QueryExecutionListenerTests) ... SKIP (0.000s)
      test_query_execution_listener_on_collect_with_arrow (pyspark.sql.tests.test_dataframe.QueryExecutionListenerTests) ... SKIP (0.000s)

After:

python3 version is: Python 3.7.7
Starting test(python3): pyspark.sql.tests.test_dataframe QueryExecutionListenerTests
Finished test(python3): pyspark.sql.tests.test_dataframe QueryExecutionListenerTests (10s)
Tests passed in 10 seconds

but it should be tested on GitHub Actions in this PR.

HyukjinKwon · 2020-07-23T13:55:55Z

cc @JoshRosen, @vanzin and @srowen FYI

project/SparkBuild.scala

dongjoon-hyun · 2020-07-23T16:01:00Z

Oh.

dongjoon-hyun · 2020-07-23T16:02:03Z

launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java

@@ -181,9 +181,16 @@ void addOptionString(List<String> cmd, String options) {
        }
      }
      if (isTesting) {
+        boolean isGitHubActionsBuild = System.getenv("GITHUB_ACTIONS") != null;


Is it inevitable? It looks a little weird to me to have GITHUB_ACTIONS in this part.

It is weird .. I will take a fresh look tomorrow and see if there's a better way. To avoid such bandaid fix, we should basically make a fix like #12334 .. #12334 wasn't even a complete fix if I am not mistaken. At least I think It will need a big change ..

SparkQA · 2020-07-23T16:48:57Z

Test build #126414 has finished for PR 29205 at commit 64fc499.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-23T16:53:34Z

Test build #126413 has finished for PR 29205 at commit 9bbdb6c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-23T19:20:17Z

Test build #126423 has finished for PR 29205 at commit c9e54bf.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-07-24T04:21:39Z

Okay, I think this is somehow indeterministic. I could reproduce in my local sometimes. Sometimes I can't. I believe it happens in certain conditions: refs: that akka/akka-http#1191 and databricks/spark-xml#371 (comment)

HyukjinKwon · 2020-07-24T04:22:28Z

I am going to simply turn it on again, and monitor GitHub Actions jobs. This is still an issue even in Jenkins jobs just for clarification.

HyukjinKwon · 2020-07-24T04:23:29Z

I will close this PR and create another one to file the analysis I made here.

### What changes were proposed in this pull request? This PR proposes to enable `corssPaths` back for now to match with the build as it was. It still indeterministically doesn't run JUnit tests given my observation, and this PR basically reverts the partial fix from #29057. See also #29205 for the full context. ### Why are the changes needed? To prevent the side effects from crossPaths such as SPARK_PREPEND_CLASSES or tests that run conditionally if the test classes are present in PySpark. ### Does this PR introduce _any_ user-facing change? No, dev-only. ### How was this patch tested? Manually tested: ```bash build/sbt -Phadoop-2.7 -Phive -Phive-2.3 -Phive-thriftserver -DskipTests clean test:package ./python/run-tests --python-executable=python3 --testname="pyspark.sql.tests.test_dataframe QueryExecutionListenerTests" ``` Closes #29218 from HyukjinKwon/SPARK-32408-1. Authored-by: HyukjinKwon <gurwls223@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

### What changes were proposed in this pull request? This PR proposes to enable `corssPaths` back for now to match with the build as it was. It still indeterministically doesn't run JUnit tests given my observation, and this PR basically reverts the partial fix from apache#29057. See also apache#29205 for the full context. ### Why are the changes needed? To prevent the side effects from crossPaths such as SPARK_PREPEND_CLASSES or tests that run conditionally if the test classes are present in PySpark. ### Does this PR introduce _any_ user-facing change? No, dev-only. ### How was this patch tested? Manually tested: ```bash build/sbt -Phadoop-2.7 -Phive -Phive-2.3 -Phive-thriftserver -DskipTests clean test:package ./python/run-tests --python-executable=python3 --testname="pyspark.sql.tests.test_dataframe QueryExecutionListenerTests" ``` Closes apache#29218 from HyukjinKwon/SPARK-32408-1. Authored-by: HyukjinKwon <gurwls223@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

Disable crossPaths only in GitHub Actions to prevent side effects

9bbdb6c

probot-autolabeler bot added BUILD PYTHON SQL labels Jul 23, 2020

HyukjinKwon requested review from dongjoon-hyun, cloud-fan and gatorsmile July 23, 2020 13:50

srowen reviewed Jul 23, 2020

View reviewed changes

project/SparkBuild.scala Outdated Show resolved Hide resolved

HyukjinKwon added 2 commits July 23, 2020 23:08

Typo

64fc499

Use set to avoid jar path duplication

c9e54bf

dongjoon-hyun reviewed Jul 23, 2020

View reviewed changes

HyukjinKwon closed this Jul 24, 2020

HyukjinKwon mentioned this pull request Jul 24, 2020

[SPARK-32408][BUILD] Enable crossPaths back to prevent side effects #29218

Closed

HyukjinKwon deleted the SPARK-32408 branch July 27, 2020 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-32408][BUILD] Disable crossPaths only in GitHub Actions to prevent side effects #29205

[SPARK-32408][BUILD] Disable crossPaths only in GitHub Actions to prevent side effects #29205

HyukjinKwon commented Jul 23, 2020 •

edited

HyukjinKwon commented Jul 23, 2020

dongjoon-hyun commented Jul 23, 2020

dongjoon-hyun Jul 23, 2020

HyukjinKwon Jul 23, 2020

SparkQA commented Jul 23, 2020

SparkQA commented Jul 23, 2020

SparkQA commented Jul 23, 2020

HyukjinKwon commented Jul 24, 2020

HyukjinKwon commented Jul 24, 2020

HyukjinKwon commented Jul 24, 2020

[SPARK-32408][BUILD] Disable crossPaths only in GitHub Actions to prevent side effects #29205

[SPARK-32408][BUILD] Disable crossPaths only in GitHub Actions to prevent side effects #29205

Conversation

HyukjinKwon commented Jul 23, 2020 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

HyukjinKwon commented Jul 23, 2020

dongjoon-hyun commented Jul 23, 2020

dongjoon-hyun Jul 23, 2020

Choose a reason for hiding this comment

HyukjinKwon Jul 23, 2020

Choose a reason for hiding this comment

SparkQA commented Jul 23, 2020

SparkQA commented Jul 23, 2020

SparkQA commented Jul 23, 2020

HyukjinKwon commented Jul 24, 2020

HyukjinKwon commented Jul 24, 2020

HyukjinKwon commented Jul 24, 2020

HyukjinKwon commented Jul 23, 2020 •

edited