-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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
[SPARK-13808][test-maven] Don't build assembly in dev/run-tests #11701
Conversation
/cc @vanzin, this patch removes the need to build an assembly before running tests. |
@@ -323,7 +323,7 @@ def get_hadoop_profiles(hadoop_version): | |||
def build_spark_maven(hadoop_version): | |||
# Enable all of the profiles for the build: | |||
build_profiles = get_hadoop_profiles(hadoop_version) + modules.root.build_profile_flags | |||
mvn_goals = ["clean", "package", "-DskipTests"] | |||
mvn_goals = ["clean", "package", "-DskipTests", "-pl", "!assembly"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're looking at speeding up the build, building and testing in one shot might be a good thing to do at least for maven. Using -fae
would allow the build to go as far as it can when something fails.
No need to do that in this change, though.
LGTM if tests pass; although a have a comment about the pyspark tests workaround. |
Test build #53094 has finished for PR 11701 at commit
|
Test build #53120 has finished for PR 11701 at commit
|
Test build #53110 has finished for PR 11701 at commit
|
Jenkins, retest this please. |
Test build #53133 has finished for PR 11701 at commit
|
Jenkins, retest this please. |
Test build #53148 has finished for PR 11701 at commit
|
Test build #53158 has finished for PR 11701 at commit
|
Jenkins, retest this please. |
Test build #53209 has finished for PR 11701 at commit
|
Jenkins, retest this please. |
(Pretty sure that something is wrong here, but hoping that maybe it's just flaky; I'll dig in more when I have time.) |
Test build #53285 has finished for PR 11701 at commit
|
Jenkins, retest this please. |
(Going to quickly make sure this also works with Maven tests, just to sanity-check) |
Test build #53332 has finished for PR 11701 at commit
|
retest this please |
Test build #53333 has finished for PR 11701 at commit
|
Looks like some adjustment is needed in my previous change? |
Perhaps changing this in
|
Test build #53336 has finished for PR 11701 at commit
|
fi | ||
|
||
LAUNCH_CLASSPATH="$SPARK_JARS_DIR/*" | ||
|
||
# Add the launcher build dir to the classpath if requested. | ||
if [ -n "$SPARK_PREPEND_CLASSES" ]; then | ||
LAUNCH_CLASSPATH="${SPARK_HOME}/launcher/target/scala-$SPARK_SCALA_VERSION/classes:$LAUNCH_CLASSPATH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow this is not being picked up (test is failing because the Main
class is not found). Is this path correct for maven too?
Hmmm. I think this is because this code in
Since the assembly build is being skipped, it's defaulting to scala 2.10 and failing to find the classes. |
BTW, I'm just waiting for this patch before working on SPARK-13579; so it's probably ok to keep building the assembly in the maven build, because it will become a lot cheaper once I implement that change (basically just copy files around). |
Yeah, I literally just found the same issue with the defaults in |
Since all of this code is going to be changed heavily / removed after your final patch, I'm going to go ahead and just leave the Maven test path unchanged so that we can get this merged. |
original_working_dir = os.getcwd() | ||
os.chdir(SPARK_HOME) | ||
cp = subprocess_check_output( | ||
["./build/sbt", "-Phive", "export assembly/managedClasspath"], universal_newlines=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vanzin, as part of your next patch will SPARK_DIST_CLASSPATH
just point to the libs
directory? If so, we can remove this as part of that patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially. I'll make a note to take a look at this.
Jenkins, retest this please. |
Test build #53345 has finished for PR 11701 at commit
|
Test build #53364 has finished for PR 11701 at commit
|
Okay, so this passes all Scala tests but is failing PySpark Maven tests... |
Are the failures legitimate? When I looked before it seemed only one of the python interpreters failed, which generally screams "flaky test" to me... |
Jenkins, retest this please. |
Test build #53443 has finished for PR 11701 at commit
|
Jenkins, retest this please. |
Test build #53453 has finished for PR 11701 at commit
|
As of SPARK-9284 we should no longer need to build the full Spark assembly JAR in order to run tests. Therefore, we should remove the assembly step from dev/run-tests in order to reduce build + test time.
Most of the changes in this PR were originally part of #11178.