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.

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

Revert nvcomp #2483

Merged
merged 8 commits into from
Oct 11, 2024
Merged

Revert nvcomp #2483

merged 8 commits into from
Oct 11, 2024

Conversation

revans2
Copy link
Collaborator

@revans2 revans2 commented Oct 9, 2024

This is based off of the work done by @gerashegalov, but applies the patch at build time.

I have not tested this with the code that tries to upmerge to a new version of CUDF. I still need to test that.

Also note that this is not intended to be something that coexists with developers trying to work on CUDF or spark-rapids-jni. That said I would like as much feedback on anything I missed as possible.

@revans2
Copy link
Collaborator Author

revans2 commented Oct 9, 2024

build

@revans2 revans2 changed the base branch from branch-24.12 to branch-24.10 October 9, 2024 17:49
@revans2
Copy link
Collaborator Author

revans2 commented Oct 9, 2024

build


set -e

SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: since we use git later we can use git rev-parse --show-toplevel to get the local repo dir,. and build paths from that instead of using .. realpath "$SCRIPT_DIR/.." later

build/apply-patches Show resolved Hide resolved
@revans2 revans2 marked this pull request as ready for review October 10, 2024 14:19
@revans2
Copy link
Collaborator Author

revans2 commented Oct 10, 2024

I have decided not to try and fix the auto-upmerge of CUDF as it is going to be difficult to fix and this is intended to only be a fix for 24.10, at least until the regression in nvcomp can be diagnosed and fixed. Because this is temporary I think it is better to just upmerge manually.

@revans2 revans2 changed the title WIP: Revert nvcomp Revert nvcomp Oct 10, 2024
@revans2
Copy link
Collaborator Author

revans2 commented Oct 10, 2024

build

@revans2
Copy link
Collaborator Author

revans2 commented Oct 10, 2024

@gerashegalov @pxLi please take a look.


set -e

SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: the BASE_DIR changes from the apply script apply here as well

@@ -70,24 +70,18 @@ echo "Test against ${cudf_sha}..."

MVN="mvn -Dmaven.wagon.http.retryHandler.count=3 -B"
set +e
${MVN} verify ${MVN_MIRROR} \
# Don't do a full build. Just try to update/build CUDF with no patches on top of it.
${MVN} validate ${MVN_MIRROR} \
Copy link
Collaborator

@gerashegalov gerashegalov Oct 10, 2024

Choose a reason for hiding this comment

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

We currently lump together cmake build generation and the actual build step. This is an additional change that would mitigate this ~/gits/NVIDIA/spark-rapids-jni.reviews$ git diff pom.xml

diff --git a/pom.xml b/pom.xml
index dc55a89..e9d2efc 100644
--- a/pom.xml
+++ b/pom.xml
@@ -429,7 +429,7 @@
             <id>build-libcudf</id>
             <phase>validate</phase>
             <configuration>
-              <target xmlns:if="ant:if">
+              <target xmlns:if="ant:if" xmlns:unless="ant:unless">
                 <condition property="needConfigure">
                   <or>
                     <istrue value="${libcudf.build.configure}"/>
@@ -465,7 +465,8 @@
                 </exec>
                 <exec dir="${libcudf.build.path}"
                       failonerror="true"
-                      executable="cmake">
+                      executable="cmake"
+                      unless:true="${submodule.patch.skip}">
                   <arg value="--build"/>
                   <arg value="${libcudf.build.path}"/>
                   <arg value="--target"/>
@@ -482,6 +483,7 @@
             <id>build-libcudfjni</id>
             <phase>validate</phase>
             <configuration>
+              <skip>${submodule.patch.skip}</skip>
               <target>
                 <mkdir dir="${libcudfjni.build.path}"/>
                 <exec dir="${libcudfjni.build.path}"
@@ -517,6 +519,7 @@
             <id>build-sparkrapidsjni</id>
             <phase>validate</phase>
             <configuration>
+              <skip>${submodule.patch.skip}</skip>
               <target>
                 <mkdir dir="${native.build.path}"/>
                 <exec dir="${native.build.path}"
@@ -555,6 +558,7 @@
             <id>build-info</id>
             <phase>generate-resources</phase>
             <configuration>
+              <skip>${submodule.patch.skip}</skip>

@@ -70,24 +70,18 @@ echo "Test against ${cudf_sha}..."

MVN="mvn -Dmaven.wagon.http.retryHandler.count=3 -B"
set +e
${MVN} verify ${MVN_MIRROR} \
# Don't do a full build. Just try to update/build CUDF with no patches on top of it.
${MVN} validate ${MVN_MIRROR} \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we care only about one execution in the validate phase. So we could

Suggested change
${MVN} validate ${MVN_MIRROR} \
${MVN} antrun:run@build-libcudf ${MVN_MIRROR} \

Copy link
Collaborator

@pxLi pxLi left a comment

Choose a reason for hiding this comment

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

LGTM for the patch parts.

Let's try address comments for submodule-sync in 24.12, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants