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

Revert nvcomp #2483

Merged
merged 8 commits into from
Oct 11, 2024
Merged

Revert nvcomp #2483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6528393
Revert "Update JNI build to support nvcomp4 (#2217)"
gerashegalov Oct 2, 2024
0c49214
Version
gerashegalov Oct 2, 2024
a4fd224
Apply and remove patches to CUDF as part of the build
revans2 Oct 8, 2024
77f7446
Merge branch 'branch-24.10' into revert_nvcomp_fully
revans2 Oct 9, 2024
d875207
Better checking if git is dirty
revans2 Oct 9, 2024
e848145
Merge branch 'branch-24.10' into revert_nvcomp_fully
revans2 Oct 9, 2024
0ee3f79
Some review comments
revans2 Oct 10, 2024
dec9ca7
Get submodule sync to work
revans2 Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions build/apply-patches
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#!/bin/bash

#
# Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

# Run a command in a Docker container with devtoolset

set -e

BASE_DIR=$( git rev-parse --show-toplevel )

PATCH_DIR=${PATCH_DIR:-$(realpath "$BASE_DIR/patches/")}

CUDF_DIR=${CUDF_DIR:-$(realpath "$BASE_DIR/thirdparty/cudf/")}

pushd "$CUDF_DIR"
gerashegalov marked this conversation as resolved.
Show resolved Hide resolved
if [ -n "$(git status --porcelain --untracked-files=no)" ] ; then
echo "Error: CUDF repository has uncommitted changes. No patches will be applied..."
exit 1
fi

find "$PATCH_DIR" -maxdepth 1 -type f -print0 | sort -zV | while IFS= read -r -d '' file; do
echo "patching with: $file"
patch --no-backup-if-mismatch -f -t --reject-file=- -p1 -i "$file"
done
popd
44 changes: 44 additions & 0 deletions build/unapply-patches
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#!/bin/bash

#
# Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

# Run a command in a Docker container with devtoolset

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


PATCH_DIR=${PATCH_DIR:-$(realpath "$SCRIPT_DIR/../patches/")}

CUDF_DIR=${CUDF_DIR:-$(realpath "$SCRIPT_DIR/../thirdparty/cudf/")}


pushd "$CUDF_DIR"
if [ -n "$(git status --porcelain --untracked-files=no)" ] ; then
#only try to remove patches if it looks like something was changed
find "$PATCH_DIR" -maxdepth 1 -type f -print0 | sort -zV -r | while IFS= read -r -d '' file; do
echo "patching with: $file"
patch -R --no-backup-if-mismatch --reject-file=- -f -t -p1 -i "$file"
done
fi

# Check for modifications
if [ -n "$(git status --porcelain --untracked-files=no)" ] ; then
echo "Error: CUDF repository has uncommitted changes. You might want to clean in manually if you know that is expected"
exit 1
fi
popd
40 changes: 28 additions & 12 deletions ci/submodule-sync.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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>

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} \

-DCPP_PARALLEL_LEVEL=${PARALLEL_LEVEL} \
-Dlibcudf.build.configure=true \
-Dlibcudf.dependency.mode=latest \
-Dsubmodule.patch.skip \
-DUSE_GDS=ON -Dtest=*,!CuFileTest,!CudaFatalTest,!ColumnViewNonEmptyNullsTest \
-DBUILD_TESTS=ON \
-DUSE_SANITIZER=ON
verify_status=$?
validate_status=$?
set -e

test_pass="False"
if [[ "${verify_status}" == "0" ]]; then
echo "Test passed, will try merge the change"
test_pass="True"
else
echo "Test failed, will update the result"
fi

build_name=$(${MVN} help:evaluate -Dexpression=project.build.finalName -q -DforceStdout)
cuda_version=$(${MVN} help:evaluate -Dexpression=cuda.version -q -DforceStdout)
. ci/check-cuda-dependencies.sh "target/${build_name}-${cuda_version}.jar"
Expand All @@ -100,8 +94,11 @@ echo "${rapids_cmake_sha}" > thirdparty/cudf-pins/rapids-cmake.sha

# Bash the wrong nvcomp version to the correct version until
# nvcomp version mismatch is fixed. https://github.com/rapidsai/cudf/issues/16772.
echo "Workaround nvcomp 4.0.1 / 4.0.1.0 versioning issues"
sed -i 's/4\.0\.1\.0/4.0.1/' thirdparty/cudf-pins/versions.json
echo "Revert nvcomp to 3.0.6"
sed -i -e 's/4\.0\.1\.0/3.0.6/' \
-e 's|https://developer.download.nvidia.com/compute/nvcomp/${version}/local_installers/nvcomp-linux-sbsa-${version}-cuda${cuda-toolkit-version-mapping}.tar.gz|https://developer.download.nvidia.com/compute/nvcomp/${version}/local_installers/nvcomp_${version}_SBSA_${cuda-toolkit-version-mapping}.tgz|' \
-e 's|https://developer.download.nvidia.com/compute/nvcomp/${version}/local_installers/nvcomp-linux-x86_64-${version}-cuda${cuda-toolkit-version-mapping}.tar.gz|https://developer.download.nvidia.com/compute/nvcomp/${version}/local_installers/nvcomp_${version}_x86_64_${cuda-toolkit-version-mapping}.tgz|' \
thirdparty/cudf-pins/versions.json

# Do the git add after the build so that we get
# the updated versions.json generated by the build
Expand All @@ -110,6 +107,25 @@ git add .
git diff-index --quiet HEAD || git commit -s -m "Update submodule cudf to ${cudf_sha}"
sha=$(git rev-parse HEAD)

set +e
# now build and test everything with the patches in place
${MVN} clean verify ${MVN_MIRROR} \
-DCPP_PARALLEL_LEVEL=${PARALLEL_LEVEL} \
-Dlibcudf.build.configure=true \
-DUSE_GDS=ON -Dtest=*,!CuFileTest,!CudaFatalTest,!ColumnViewNonEmptyNullsTest \
-DBUILD_TESTS=ON \
-DUSE_SANITIZER=ON
verify_status=$?
set -e

test_pass="False"
if [[ ( "${verify_status}" == "0" ) && ( "${validate_status}" == "0" ) ]]; then
echo "Test passed, will try merge the change"
test_pass="True"
else
echo "Test failed, will update the result"
fi

# push the intermediate branch and create PR against REF
# if test passed, it will try auto-merge the PR
# if test failed, it will only comment the test result in the PR
Expand Down
Loading
Loading