Make new build default and combine into dist package #3411

tgravescs · 2021-09-08T16:23:15Z

contributes to #3223

This pr is based off of #3381 and includes:

makes the new build targets the defaults. it builds a single version of spark by default and the dist jar only includes that single version unless other profiles specified.
removes many of the old ones
Combines the spark version packages into a single dist package - this requires installing all other versions into .m2. This adds some profiles to combine snapshots (-Pallwithsnapshots), snapshots with db (-PallwithsnapshotsDB), no snapshots (-Pallnosnapshots) and no snapshots with DB (-Pallnosnapshotswithdb).
Supports building databricks 301db and 311db with PR 3381 changes
tries to update the jenkins build scripts
adds a scripts build/buildall that just installs all the apache and CDH versions and create a dist package with -Pallwithsnapshots. This is very basic and we can make it better with more options later.
checked jar contents against older jars and it seems like we have everything for dependencies and properties files and license, notice, etc.
removed shims/aggregator module since no longer needed
Added an aggregator module that is responsible for shading each of the spark versions. This combines the sql-plugin, shuffle, udf, etc and then shades the dependencies. Those aggregated modules are what end up being combined into the file uber jar in the dist directory where they are put into a spark specific directory.

The jar is much larger right now until we can dedup the code. it was at 115M

Default build with no targest builds spark301. to build another version you specify -Dbuildver=spark3XX.

I tested databricks builds and one of the apache spark version integration tests. all versions ran unit tests.
I can't test on cloudera right now because the override shim in pr 3381 isn't working and it needs these changes to test so I think we can fix this after they are merged.

There is more to do but I think the release jar is working for everything I've tested so would be good to switch to it and get a base.

I have not tested UCX or the udf examples.

In progress of righting a nightly build to see if that script works.

…ument Signed-off-by: Jason Lowe <jlowe@nvidia.com>

Signed-off-by: Thomas Graves <tgraves@nvidia.com>

Signed-off-by: Gera Shegalov <gera@apache.org>

Signed-off-by: Thomas Graves <tgraves@nvidia.com>

…apids into builddistnewwithclass2

Signed-off-by: Thomas Graves <tgraves@nvidia.com>

dist/unshimmed-base.txt

docs/configs.md

jenkins/spark-nightly-build.sh

tgravescs · 2021-09-10T03:08:05Z

build

gerashegalov · 2021-09-10T03:34:19Z

dist/pom.xml

+                <artifactId>apache-rat-plugin</artifactId>
+                <configuration>
+                    <excludes>
+                        <exclude>com.nvidia.spark.rapids.SparkShimServiceProvider.*</exclude>


the shade plugin outputs dependency-reduced-pom.xml under the pom's basedir by default since we were not using dependencyReducedPomLocation config. As a consequence many developers who have used the old build in their local repo prior to this PR will have a stale file under dist that is not cleaned up by maven

I suggest to add it to the exclude list in this PR to minimize reported build failures.

In a follow up we can add dependencyReducedPomLocation to the shade plugin configs

tgravescs · 2021-09-10T03:42:45Z

there are other shuffle related classes I need to handle to make those non-shimmed, working through them.

tgravescs · 2021-09-10T04:43:50Z

the shuffle stuff got very ugly, started requiring a lot of things to be unshimmed which then eventually broke using it without rapids shuffle, need to investigate a better way to handle shuffle

gerashegalov · 2021-09-10T05:37:28Z

dist/pom.xml

+                                <id>create-parallel-world</id>
+                                <configuration>
+                                    <target>
+


TODO explore ant-contrib for loops and try to have a single execution configured where the missing input for unzip is skipped

- created profile representative shims for each FEATURE version and a vendor shim - Use VisibleShuffleManager mostly for tagging, so stop inheriting ShuffleManager - prevent ClassCastExceptions when casting to VisibleShuffleManager by not using it in the ShimLoader method signatures Signed-off-by: Gera Shegalov <gera@apache.org>

gerashegalov · 2021-09-10T09:14:25Z

shuffle should be fixed if you merge tgravescs#3 into your branch

Fix classloading via ProxyRapidsShuffleManager

tgravescs · 2021-09-10T12:06:34Z

build

revans2 · 2021-09-10T13:33:06Z

aggregator/pom.xml

+                            <goal>shade</goal>
+                        </goals>
+                        <configuration>
+                        <artifactSet>


nit: The indentation under configuration appears to be off, only filters appears to be indented correctly.

revans2 · 2021-09-10T13:34:21Z

aggregator/pom.xml

+                            <goal>shade</goal>
+                        </goals>
+                        <configuration>
+                    <artifactSet>


nit: Indention appears to be off here.

revans2 · 2021-09-10T13:42:07Z

aggregator/pom.xml

+                        <goals>
+                            <goal>shade</goal>
+                        </goals>
+                        <configuration>


Have you tried to put the common configuration under the plugin, and only the differences in the execution itself? That should hopefully make this much more readable, with less duplication.

will fix in #3440

revans2 · 2021-09-10T13:44:36Z

build/buildall

+set -ex
+
+# Install all the versions we support
+mvn -U -Dbuildver=302 clean install -Drat.skip=true -DskipTests -Dmaven.javadoc.skip=true -Dskip -pl aggregator -am


I'm not sure if this was discussed before, but if we don't want rat, or tests or javadocs/etc, why do we have them on by default in the maven build at all? I would much rather see a way to pass parameters to the shell script on to the maven build so I can decide what I want and what I don't instead.

yeah the build script is very basic right now just to get started, lots of improvements to it need to be done

revans2 · 2021-09-10T13:45:30Z

build/buildall

+#
+
+set -ex
+


Should we do something to make sure that the directory is the correct one? This assumes that you are in the root directory calling build/buildall.

revans2 · 2021-09-10T13:50:18Z

integration_tests/run_pyspark_from_build.sh

@@ -29,11 +29,13 @@ else
    if [ -d "$LOCAL_JAR_PATH" ]; then
        CUDF_JARS=$(echo "$LOCAL_JAR_PATH"/cudf-*.jar)
        PLUGIN_JARS=$(echo "$LOCAL_JAR_PATH"/rapids-4-spark_*.jar)
+        # TODO - this should really only pick up the specific version being tested


Is there a follow on issue for this?

its subitem in #3223

revans2 · 2021-09-10T13:51:44Z

jenkins/spark-nightly-build.sh

-# Disabled until Spark 3.2 source incompatibility fixed, see https://github.com/NVIDIA/spark-rapids/issues/2052
-#mvn -U -B -Pspark320tests,snapshot-shims test $MVN_URM_MIRROR -Dmaven.repo.local=$M2DIR -Dcuda.version=$CUDA_CLASSIFIER
+# Install all the versions we support
+mvn -U -B -Dbuildver=302 clean install $MVN_URM_MIRROR -Dmaven.repo.local=$M2DIR -Dcuda.version=$CUDA_CLASSIFIER


If we made buildall configurable then we could reuse it here instead of copying everything

tgravescs · 2021-09-10T13:58:33Z

@revans2 thanks for the review, if you are ok, I'd like to do nits and things in followup since ci passed here and would like to get it as base?

revans2

All of the issues I pointed out before can be done as follow on work.

jlowe and others added 30 commits August 25, 2021 14:24

Update to cudf conditional join change that removes null equality arg…

53f665b

…ument Signed-off-by: Jason Lowe <jlowe@nvidia.com>

dist initial working

b80cb31

Signed-off-by: Thomas Graves <tgraves@nvidia.com>

add classes files

5e1cacb

Parallel world ServiceLoader

07526a3

Signed-off-by: Gera Shegalov <gera@apache.org>

Implement per-shim parallel world jar classloader

a8f5949

Signed-off-by: Gera Shegalov <gera@apache.org>

working

8ffc90e

checkpoint

8cb180e

Merge branch '3381' into builddistnewwithclass

d2453b7

working

9ee5eeb

review and test fix

0d5b11c

Adding shuffle manager

fbacdbe

Signed-off-by: Gera Shegalov <gera@apache.org>

Merge remote-tracking branch 'origin/branch-21.10' into shimLoader320

72dd879

wip

59503cd

test fix

aca6e8e

test fix

9fbe630

Merge branch '3381' into builddistnewwithclass2

fb0399a

working

3331515

really working

4e24306

cleanup old profiles

34dd324

cdh building again

6ab79ad

format

4d8df72

Working on 311db

9ffe27d

Signed-off-by: Thomas Graves <tgraves@nvidia.com>

fix int tests

b8513e5

fix individual

df4d48e

rename databricks modules to conform

662a76f

add snapshot and no snapshot dist

120d359

Merge branch 'builddistnewwithclass2' of github.com:tgravescs/spark-r…

ca832e7

…apids into builddistnewwithclass2

add file

c6611ff

301 databricks building

64bf62d

Signed-off-by: Thomas Graves <tgraves@nvidia.com>

remove aggregator

1d7abb3

gerashegalov reviewed Sep 10, 2021

View reviewed changes

dist/unshimmed-base.txt Outdated Show resolved Hide resolved

Add RapidsShuffleHeartbeatHandler to list to not shim

2297962

tgravescs commented Sep 10, 2021

View reviewed changes

docs/configs.md Outdated Show resolved Hide resolved

fix configs.md doc

b42c158

gerashegalov reviewed Sep 10, 2021

View reviewed changes

jenkins/spark-nightly-build.sh Outdated Show resolved Hide resolved

tgravescs added 2 commits September 9, 2021 22:03

add clean to nightly

339d10f

fix inclusion

895216b

gerashegalov reviewed Sep 10, 2021

View reviewed changes

tgravescs mentioned this pull request Sep 10, 2021

Qualification tool: Update expected result for test #3436

Merged

gerashegalov mentioned this pull request Sep 10, 2021

Remove shim-specific copies of ParquetCachedBatchSerializer #3390

Closed

gerashegalov reviewed Sep 10, 2021

View reviewed changes

gerashegalov added 2 commits September 9, 2021 23:22

Merge branch 'ShuffleManager-fix' into builddistnewwithclass2+gerafixes

f20c79d

Merge pull request #3 from gerashegalov/builddistnewwithclass2+gerafixes

b13a61c

Fix classloading via ProxyRapidsShuffleManager

revans2 reviewed Sep 10, 2021

View reviewed changes

Release 21.10 automation moved this from Review in progress to Reviewer approved Sep 10, 2021

revans2 approved these changes Sep 10, 2021

View reviewed changes

tgravescs merged commit 960d52d into NVIDIA:branch-21.10 Sep 10, 2021

Release 21.10 automation moved this from Reviewer approved to Done Sep 10, 2021

tgravescs mentioned this pull request Sep 10, 2021

[BUG] Followup comments from PR3411 #3440

Closed

jlowe mentioned this pull request Sep 10, 2021

[FEA] create the plugin package capable of storing conflicting multiple versions of same named classes #3232

Closed

gerashegalov mentioned this pull request Sep 10, 2021

[FEA] Rework the shim layer to robustly handle ABI and API incompatibilities across Spark releases #3223

Closed

21 tasks

gerashegalov linked an issue Sep 13, 2021 that may be closed by this pull request

[FEA] create the plugin package capable of storing conflicting multiple versions of same named classes #3232

Closed

gerashegalov mentioned this pull request Sep 28, 2023

Building different Cuda versions section profile does not take effect [skip ci] #9328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make new build default and combine into dist package #3411

Make new build default and combine into dist package #3411

tgravescs commented Sep 8, 2021 •

edited

tgravescs commented Sep 10, 2021

gerashegalov Sep 10, 2021

tgravescs commented Sep 10, 2021

tgravescs commented Sep 10, 2021

gerashegalov Sep 10, 2021

gerashegalov commented Sep 10, 2021

tgravescs commented Sep 10, 2021

revans2 Sep 10, 2021

revans2 Sep 10, 2021

revans2 Sep 10, 2021

tgravescs Sep 10, 2021

revans2 Sep 10, 2021

tgravescs Sep 10, 2021

revans2 Sep 10, 2021

revans2 Sep 10, 2021

tgravescs Sep 10, 2021

revans2 Sep 10, 2021

tgravescs commented Sep 10, 2021

revans2 left a comment

		#

		set -ex

Make new build default and combine into dist package #3411

Make new build default and combine into dist package #3411

Conversation

tgravescs commented Sep 8, 2021 • edited

tgravescs commented Sep 10, 2021

Choose a reason for hiding this comment

tgravescs commented Sep 10, 2021

tgravescs commented Sep 10, 2021

Choose a reason for hiding this comment

gerashegalov commented Sep 10, 2021

tgravescs commented Sep 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgravescs commented Sep 10, 2021

revans2 left a comment

Choose a reason for hiding this comment

tgravescs commented Sep 8, 2021 •

edited