Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
May 27, 2022
Merged

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

merged 5 commits into from
May 27, 2022

Conversation

res-life
Copy link
Collaborator

@res-life res-life commented May 24, 2022 •
edited
Loading

Closes #5294

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

Unshim jni and ucx
99bf60a 
Signed-off-by: Chong Gao <res_life@163.com>
@res-life
Copy link
Collaborator Author

build

@res-life 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 res-life marked this pull request as draft May 24, 2022 02:36
@res-life res-life marked this pull request as ready for review May 24, 2022 04:10
@res-life res-life requested review from gerashegalov and pxLi May 24, 2022 04:12
@res-life 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
firestarman reviewed May 24, 2022
View reviewed changes
firestarman
firestarman reviewed May 24, 2022
View reviewed changes
NvTimLiu
NvTimLiu reviewed May 24, 2022
View reviewed changes
@res-life 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
@res-life
Copy link
Collaborator Author

build

@res-life
Copy link
Collaborator Author

build

firestarman
firestarman reviewed May 24, 2022
View reviewed changes
@res-life
Copy link
Collaborator Author

Should target branch-22.08

@res-life
Copy link
Collaborator Author

build

jlowe
jlowe reviewed May 24, 2022
View reviewed changes
Copy link
Contributor

@jlowe jlowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

aggregator/pom.xml Outdated
@@ -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 -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@res-life
Copy link
Collaborator Author

res-life commented May 25, 2022 •
edited
Loading

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
gerashegalov reviewed May 25, 2022
View reviewed changes
aggregator/pom.xml Outdated
@@ -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 -->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this one is more natural. Done.

dist/pom.xml
<goals>
<goal>unpack</goal>
</goals>
<configuration>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@gerashegalov gerashegalov May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

build

jlowe
jlowe previously approved these changes May 25, 2022
View reviewed changes
@gerashegalov gerashegalov added the build Related to CI / CD or cleanly building label May 25, 2022
gerashegalov
gerashegalov previously approved these changes May 25, 2022
View reviewed changes
Copy link
Collaborator

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@res-life res-life changed the base branch from branch-22.06 to branch-22.08 May 26, 2022 13:52
@res-life res-life dismissed stale reviews from gerashegalov and jlowe May 26, 2022 13:52

The base branch was changed.

@res-life
Copy link
Collaborator Author

build

1 similar comment
@gerashegalov
Copy link
Collaborator

build

@sperlingxx sperlingxx self-requested a review May 27, 2022 01:52
@sperlingxx
Copy link
Collaborator

LGTM

sperlingxx
sperlingxx approved these changes May 27, 2022
View reviewed changes
Copy link
Collaborator

@sperlingxx sperlingxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@res-life
Copy link
Collaborator Author

Re-target 22.08

@res-life res-life merged commit c3d21de into NVIDIA:branch-22.08 May 27, 2022
@res-life res-life deleted the unshim-jni-ucx branch May 27, 2022 02:16
HaoYang670 pushed a commit to HaoYang670/spark-rapids that referenced this pull request Jun 6, 2022
@res-life @HaoYang670
Skip unshim and dedup of external spark-rapids-jni and jucx [databric…
b62dd36 
…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>
@gerashegalov gerashegalov mentioned this pull request Jun 7, 2022
Closed
@res-life res-life mentioned this pull request Jun 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@firestarman firestarman firestarman left review comments

@NvTimLiu NvTimLiu NvTimLiu left review comments

@gerashegalov gerashegalov gerashegalov approved these changes

@sperlingxx sperlingxx sperlingxx approved these changes

@GaryShen2008 GaryShen2008 GaryShen2008 approved these changes

@jlowe jlowe jlowe left review comments

@revans2 revans2 Awaiting requested review from revans2 revans2 is a code owner

@tgravescs tgravescs Awaiting requested review from tgravescs tgravescs is a code owner

@pxLi pxLi Awaiting requested review from pxLi

Assignees

@res-life res-life

Labels
build Related to CI / CD or cleanly building
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dist jar build should not need to unshim and dedup external dependencies
7 participants
@res-life @gerashegalov @sperlingxx @jlowe @firestarman @GaryShen2008 @NvTimLiu