Skip unshim and dedup of external spark-rapids-jni and jucx [databricks] #5603

res-life · 2022-05-24T02:30:19Z

Changes:

Stop pulling external dependencies such as jucx and spark-rapids-jni into every aggregator jar by marking them as provided dependency scope. Now the aggregator jar is less than 10M.

ls -lh aggregator/target/spark330/
... 7.2M  ... rapids-4-spark-aggregator_2.12-22.06.0-SNAPSHOT.jar

Copy one and only one version of them into dist jar after the deduping aggregator jar is done.

Note:

Related PR: Build against specified spark-rapids-jni snapshot jar [skip ci] #5501
This PR supports building against specified spark-rapids-jni snapshot jar by specify '-Dspark-rapids-jni.version=xx'
If depends on spark-rapids-jni snapshot version, the building may download multiple spark-rapids-jni snapthot jars for different Shims, but in the end, the packaging only picks one version of them. This is the potential risk, but I think we accept this risk for the snapshot build.

Signed-off-by: Chong Gao res_life@163.com

Signed-off-by: Chong Gao <res_life@163.com>

res-life · 2022-05-24T02:30:39Z

build

dist/pom.xml

res-life · 2022-05-24T07:59:07Z

build

res-life · 2022-05-24T08:05:27Z

build

aggregator/pom.xml

res-life · 2022-05-24T09:24:17Z

Should target branch-22.08

res-life · 2022-05-24T10:48:39Z

build

jlowe

Has this been tested with the -DallowConventionalDistJar feature added in #5528?

jlowe · 2022-05-24T13:14:21Z

aggregator/pom.xml

@@ -55,6 +55,18 @@
            <version>${project.version}</version>
            <classifier>${spark.version.classifier}</classifier>
        </dependency>
+        <!-- set provided scope for JNI and ucx to avoid pulling into aggregator jar  -->


If we cannot find a way to automatically pull in immediate provided dependencies, there needs to be a comment here explaining that any changes here likely impact the corresponding dependency list maintained in dist/pom.xml.

dist/pom.xml

res-life · 2022-05-25T02:11:57Z

Has this been tested with the -DallowConventionalDistJar feature added in #5528?

Tested, and it works.
unpack occurs after the targets build-conventional-jar and build-parallel-worlds.

mvn clean install -DskipTests -DallowConventionalDistJar
cd tests
mvn test

Also tested buildall

mvn clean
./build/buildall -p=320,330
# run 330 IT
./integration_tests/run_pyspark_from_build.sh -k test_if_else

gerashegalov · 2022-05-25T03:06:28Z

aggregator/pom.xml

@@ -55,6 +55,18 @@
            <version>${project.version}</version>
            <classifier>${spark.version.classifier}</classifier>
        </dependency>
+        <!-- set provided scope for JNI and ucx to avoid pulling into aggregator jar  -->


Alternatively we could simply add these artifacts to the exclude list of the shade artifactSet

<artifactSet> <excludes> <exclude>org.slf4j:*</exclude> <exclude>com.nvidia:spark-rapids-jni:*</exclude> <exclude>org.openucx:jucx:*</exclude> </excludes> </artifactSet>

Yes, this one is more natural. Done.

gerashegalov · 2022-05-25T03:18:31Z

dist/pom.xml

+                        <goals>
+                            <goal>unpack</goal>
+                        </goals>
+                        <configuration>


To have a more obvious way of reasoning about the order of the dependency processing I would move this unpacks inside ant build-parallel-worlds.xml to copy-dependencies target either before or after the for loop.

Not found a way to invoke the unpack goal of maven-dependency-plugin in the maven-antrun-plugin.
Just coping the jars in the local ~/.m2 path is not so good, because we can specify the local repository path when executing mvn commands. The maven-dependency-plugin is responsible to copy the right jars.

Now the unpack works and the order is right, let's keep it?

it can be done similar to the dependency:get code there.

+ <target name="unpack-ucx-and-jni"> + <resources id="unpackArray"> + <string>com.nvidia:spark-rapids-jni:${project.version}:jar:${cuda.version}</string> + <string>org.openucx:jucx:${ucx.version}</string> + </resources> + <pathconvert property="unpackList" refid="unpackArray" pathsep=","/> + <ac:for list="${unpackList}" param="unpackArtifact" trim="true"> + <sequential> + <echo level="info">dependency:unpack @{unpackArtifact}</echo> + <exec executable="${maven.home}/bin/mvn" failonerror="true"> + <arg value="dependency:unpack"/> + <arg value="-s" if:set="env.URM_URL"/> + <arg value="${project.basedir}/../jenkins/settings.xml" if:set="env.URM_URL"/> + <arg value="-B"/> + <arg value="-Dmaven.repo.local=${maven.repo.local}" if:set="maven.repo.local"/> + <arg value="-Dartifact=@{unpackArtifact}"/> + <arg value="-DoutputDirectory=${project.build.directory}/parallel-world"/> + </exec> + </sequential> + </ac:for> + </target>

Your suggestion is fine with me, however, I am biased to keeping related code in one place even if it's a little hacky.

res-life · 2022-05-25T10:34:33Z

build

gerashegalov

LGTM

The base branch was changed.

res-life · 2022-05-26T13:53:44Z

build

gerashegalov · 2022-05-26T15:58:32Z

build

sperlingxx · 2022-05-27T01:52:35Z

LGTM

sperlingxx

LGTM

res-life · 2022-05-27T01:57:45Z

Re-target 22.08

…ks] (NVIDIA#5603) * Unshim spark-rapids-jni and ucx * Manage versions in the dependency management section * Use the shade plugin to exclude external jars in the aggregator submodule Signed-off-by: Chong Gao <res_life@163.com>

Unshim jni and ucx

99bf60a

Signed-off-by: Chong Gao <res_life@163.com>

res-life requested review from jlowe, revans2, tgravescs, GaryShen2008 and NvTimLiu as code owners May 24, 2022 02:30

res-life changed the title ~~Skip unshim and dedup of external spark-rapids-jni and jucx~~ [WIP] Skip unshim and dedup of external spark-rapids-jni and jucx May 24, 2022

res-life marked this pull request as draft May 24, 2022 02:36

res-life marked this pull request as ready for review May 24, 2022 04:10

res-life requested review from gerashegalov and pxLi May 24, 2022 04:12

res-life changed the title ~~[WIP] Skip unshim and dedup of external spark-rapids-jni and jucx~~ Skip unshim and dedup of external spark-rapids-jni and jucx May 24, 2022

firestarman reviewed May 24, 2022

View reviewed changes

dist/pom.xml Outdated Show resolved Hide resolved

firestarman reviewed May 24, 2022

View reviewed changes

dist/pom.xml Outdated Show resolved Hide resolved

NvTimLiu reviewed May 24, 2022

View reviewed changes

dist/pom.xml Show resolved Hide resolved

Update

35c75dd

res-life changed the title ~~Skip unshim and dedup of external spark-rapids-jni and jucx~~ Skip unshim and dedup of external spark-rapids-jni and jucx [databricks] May 24, 2022

Fix unintended change

0e5192c

firestarman reviewed May 24, 2022

View reviewed changes

aggregator/pom.xml Outdated Show resolved Hide resolved

Manage versions in dependency management section

ea15cd8

jlowe reviewed May 24, 2022

View reviewed changes

gerashegalov reviewed May 25, 2022

View reviewed changes

Use shade plugin to excludes external jars in the aggregator submodule

0c7047f

jlowe previously approved these changes May 25, 2022

View reviewed changes

gerashegalov assigned res-life May 25, 2022

gerashegalov added the build Related to CI / CD or cleanly building label May 25, 2022

gerashegalov previously approved these changes May 25, 2022

View reviewed changes

res-life changed the base branch from branch-22.06 to branch-22.08 May 26, 2022 13:52

sperlingxx self-requested a review May 27, 2022 01:52

sperlingxx approved these changes May 27, 2022

View reviewed changes

gerashegalov approved these changes May 27, 2022

View reviewed changes

GaryShen2008 approved these changes May 27, 2022

View reviewed changes

res-life merged commit c3d21de into NVIDIA:branch-22.08 May 27, 2022

res-life deleted the unshim-jni-ucx branch May 27, 2022 02:16

gerashegalov mentioned this pull request Jun 7, 2022

[BUG] repeated runs of mvn package without clean lead to missing spark-rapids-jni-version-info.properties in dist jar #5777

Closed

res-life mentioned this pull request Jun 8, 2022

Fix repeated runs mvn package without clean lead to missing spark-rapids spark-rapids-jni-version-info.properties in dist jar #5779

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip unshim and dedup of external spark-rapids-jni and jucx [databricks] #5603

Skip unshim and dedup of external spark-rapids-jni and jucx [databricks] #5603

res-life commented May 24, 2022 •

edited

Loading

res-life commented May 24, 2022

res-life commented May 24, 2022

res-life commented May 24, 2022

res-life commented May 24, 2022

res-life commented May 24, 2022

jlowe left a comment

jlowe May 24, 2022

res-life commented May 25, 2022 •

edited

Loading

gerashegalov May 25, 2022

res-life May 25, 2022

gerashegalov May 25, 2022

res-life May 25, 2022

gerashegalov May 25, 2022 •

edited

Loading

res-life commented May 25, 2022

gerashegalov left a comment

res-life commented May 26, 2022

gerashegalov commented May 26, 2022

sperlingxx commented May 27, 2022

sperlingxx left a comment

res-life commented May 27, 2022

Skip unshim and dedup of external spark-rapids-jni and jucx [databricks] #5603

Skip unshim and dedup of external spark-rapids-jni and jucx [databricks] #5603

Conversation

res-life commented May 24, 2022 • edited Loading

res-life commented May 24, 2022

res-life commented May 24, 2022

res-life commented May 24, 2022

res-life commented May 24, 2022

res-life commented May 24, 2022

jlowe left a comment

Choose a reason for hiding this comment

jlowe May 24, 2022

Choose a reason for hiding this comment

res-life commented May 25, 2022 • edited Loading

gerashegalov May 25, 2022

Choose a reason for hiding this comment

res-life May 25, 2022

Choose a reason for hiding this comment

gerashegalov May 25, 2022

Choose a reason for hiding this comment

res-life May 25, 2022

Choose a reason for hiding this comment

gerashegalov May 25, 2022 • edited Loading

Choose a reason for hiding this comment

res-life commented May 25, 2022

gerashegalov left a comment

Choose a reason for hiding this comment

res-life commented May 26, 2022

gerashegalov commented May 26, 2022

sperlingxx commented May 27, 2022

sperlingxx left a comment

Choose a reason for hiding this comment

res-life commented May 27, 2022

res-life commented May 24, 2022 •

edited

Loading

res-life commented May 25, 2022 •

edited

Loading

gerashegalov May 25, 2022 •

edited

Loading