Use pre-built nvcomp 2.3 binaries by default #10851

robertmaynard · 2022-05-13T16:24:29Z

When building cudf on x86_64 hardware we can enable the new pre-built nvcomp 2.3 binaries to leverage new compression codec support.

Closes #10681
Closes #10837

When building cudf on x86_64 hardware we can enable the new pre-built nvcomp 2.3 binaries to leverage new compression codec support.

robertmaynard · 2022-05-13T16:24:46Z

Waiting on rapidsai/rapids-cmake#190

jbrennan333 · 2022-05-13T18:18:25Z

@robertmaynard do you want to pull the changes from #10837 into this PR?

robertmaynard · 2022-05-13T21:27:08Z

@robertmaynard do you want to pull the changes from #10837 into this PR?

Yeah I will do so

codecov · 2022-05-14T01:02:53Z

Codecov Report

Merging #10851 (b5f5c7c) into branch-22.06 (54789ee) will increase coverage by 0.02%.
The diff coverage is 93.75%.

❗ Current head b5f5c7c differs from pull request most recent head 7758d88. Consider uploading reports for the commit 7758d88 to get more accurate results

@@               Coverage Diff                @@
##           branch-22.06   #10851      +/-   ##
================================================
+ Coverage         86.30%   86.32%   +0.02%     
================================================
  Files               144      144              
  Lines             22665    22668       +3     
================================================
+ Hits              19560    19569       +9     
+ Misses             3105     3099       -6

Impacted Files	Coverage Δ
python/cudf/cudf/core/indexed_frame.py	`91.70% <ø> (ø)`
python/cudf/cudf/utils/ioutils.py	`79.47% <87.50%> (-0.13%)`	⬇️
python/cudf/cudf/io/avro.py	`78.57% <100.00%> (ø)`
python/cudf/cudf/io/csv.py	`91.80% <100.00%> (ø)`
python/cudf/cudf/io/json.py	`97.56% <100.00%> (ø)`
python/cudf/cudf/io/orc.py	`92.77% <100.00%> (ø)`
python/cudf/cudf/io/parquet.py	`90.83% <100.00%> (ø)`
python/cudf/cudf/io/text.py	`100.00% <100.00%> (ø)`
python/cudf/cudf/core/dataframe.py	`93.78% <0.00%> (+0.04%)`	⬆️
python/cudf/cudf/core/column/string.py	`88.78% <0.00%> (+0.12%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e096345...7758d88. Read the comment docs.

conda/recipes/libcudf/post-link.sh

vyasr · 2022-05-18T18:23:24Z

build.sh

@@ -144,6 +146,7 @@ function buildLibCudfJniInDocker {
                -DCMAKE_CUDA_ARCHITECTURES=${CUDF_CMAKE_CUDA_ARCHITECTURES} \
                -DCMAKE_INSTALL_PREFIX=/usr/local/rapids \
                -DUSE_NVTX=ON \
+                -DCMAKE_USE_PROPRIETARY_NVCOMP=ON \


I find it a bit disconcerting that all of the flags used for the JNI build do not respect the script's CLI. That seems like it's just waiting to bite some unsuspecting user of build.sh eventually. It looks like this isn't a new problem though since it affects all of the flags used in the JNI build, though, so we don't need to block this PR over this. It's probably worth factoring this build logic into a separate script thought since AFAICT it completely ignores all of the logic and variables in this build.sh script.

build.sh

conda/recipes/libcudf/post-link.sh

fetch_rapids.cmake

jbrennan333 · 2022-05-18T19:19:22Z

@vyasr I was able to build spark-rapids-jni.
@jlowe brought up a question off-line - I don't see anything in the code here that specifies which version of nvcomp we are downloading? Is it fixed to nvcomp-2.3? We don't really want it just pulling down the latest version, do we?
That said, we have found a problem in nvcomp-2.3 zstd codec, so there will likely be an updated version that we will need (2.3.1?).

jbrennan333 · 2022-05-18T19:52:09Z

To answer my own question, the version we download is specified here: https://github.com/rapidsai/rapids-cmake/blob/branch-22.06/rapids-cmake/cpm/versions.json

vyasr · 2022-05-18T20:21:51Z

Yep, sorry for the slow response. Versions for packages that we need to be standardized across all of rapids are stored in the rapids-cmake file that you pointed to. The rapids-cmake function is being called in the get_nvcomp.cmake file in this repository.

Regarding the nvcomp update, should we hold off on this PR until there is an nvcomp version with the necessary bug fix?

jbrennan333 · 2022-05-19T21:46:52Z

Closes #10681

vyasr · 2022-05-20T17:33:40Z

Just to give an update for those tracking this PR, we're holding off on merge while we wait to resolve issues with nvcomp 2.3.0

vyasr

@robertmaynard based on our meeting I think we are good to move forward with this, correct? I'll approve and let you merge when ready.

This PR addresses #10862, but does not completely remove `PER_THREAD_DEFAULT_STREAM`. I just adds the new define `CUDF_USE_PER_THREAD_DEFAULT_STREAM` and adds a deprecation warning for `PER_THREAD_DEFAULT_STREAM`. This PR also addresses #10864, but only changes the default parameter for `read_avro` and `read_parquet` to `cudf::default_stream_value`, which is defined as `rmm::cuda_stream_per_thread` when `CUDF_USE_PER_THREAD_DEFAULT_STREAM` is defined. These cover the current interfaces to nvcomp. The goal for this PR is to ensure that we pass `rmm::cuda_stream_per_thread` to the nvcomp apis when `CUDF_USE_PER_THREAD_DEFAULT_STREAM` is defined. This is needed for nvcomp-2.3, which is provided as dynamic libraries where we cannot recompile with PTDS enabled. nvcomp-2.3 is being enabled in #10851. I have not marked this PR as closing the above issues, because we will need to follow up in a future PR to remove `PER_THREAD_DEFAULT_STREAM` and in another to replace all of the rest of the cuDF API call sites to use the new `cudf::default_stream_value`. i am putting this up as a draft PR because I have not tested with nvcomp-2.3 yet. Authors: - Jim Brennan (https://github.com/jbrennan333) Approvers: - https://github.com/nvdbaranec - AJ Schmidt (https://github.com/ajschmidt8) - Jason Lowe (https://github.com/jlowe) - Vukasin Milovanovic (https://github.com/vuule) - Jake Hemstad (https://github.com/jrhemstad) - Robert Maynard (https://github.com/robertmaynard) URL: #10877

robertmaynard · 2022-05-25T00:25:03Z

@gpucibot merge

jbrennan333 and others added 5 commits May 12, 2022 08:31

Add support for nvcomp-2.3 in java build

168af05

Update copyrights.

f2f9ad9

Use $<TARGET_FILE:nvcomp::nvcomp> to get the full path

7f5d453

Add COMMENT to custom command to fix checkstyle issue

d8b8c89

Use pre-built nvcomp 2.3 binaries by default

eb43115

When building cudf on x86_64 hardware we can enable the new pre-built nvcomp 2.3 binaries to leverage new compression codec support.

robertmaynard added feature request New feature or request 5 - Merge After Dependencies non-breaking Non-breaking change labels May 13, 2022

github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels May 13, 2022

robertmaynard added 2 commits May 13, 2022 13:06

fix style issues found by ci

a5b4761

Update java side to use nvcomp from cudf

c1e5275

github-actions bot added the Java Affects Java cuDF API. label May 13, 2022

github-actions bot added the conda label May 13, 2022

Merge branch 'branch-22.06' into jtb-nvcomp-2.3

ff46fd3

robertmaynard force-pushed the fea/use_rapids_cpm_nvcomp branch from 2c4bc16 to c1e5275 Compare May 16, 2022 13:07

github-actions bot removed the conda label May 16, 2022

robertmaynard added 5 commits May 16, 2022 09:08

Merge commit 'jbrennan333:jtb-nvcomp-2.3' into fea/use_rapids_cpm_nvcomp

2e1b583

Merge branch 'branch-22.06' into fea/use_rapids_cpm_nvcomp

085de05

Correct style issues found by CI

d1ad96b

Add build.sh option to control using proprietary nvcomp binaries

f0667a8

Add conda license agreement

c64d4d9

github-actions bot added the conda label May 16, 2022

robertmaynard marked this pull request as ready for review May 16, 2022 17:49

robertmaynard requested a review from a team as a code owner May 16, 2022 17:49

gerashegalov reviewed May 18, 2022

View reviewed changes

conda/recipes/libcudf/post-link.sh Outdated Show resolved Hide resolved

vyasr requested changes May 18, 2022

View reviewed changes

Apply suggestions from code review

f531c11

jbrennan333 mentioned this pull request May 19, 2022

[FEA] Update Java bindings to handle separate nvcomp library #10681

Closed

jbrennan333 mentioned this pull request May 19, 2022

Add support for nvcomp-2.3 in java build #10837

Closed

jakirkham requested review from vyasr and gerashegalov May 20, 2022 07:15

vyasr added 5 - DO NOT MERGE Hold off on merging; see PR for details and removed 5 - Merge After Dependencies labels May 20, 2022

jbrennan333 mentioned this pull request May 20, 2022

[FEA] Enable zstd integration tests for parquet and orc NVIDIA/spark-rapids#5580

Closed

jbrennan333 approved these changes May 24, 2022

View reviewed changes

vyasr approved these changes May 24, 2022

View reviewed changes

robertmaynard added 3 - Ready for Review Ready for review by team and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels May 24, 2022

Merge branch 'branch-22.06' into fea/use_rapids_cpm_nvcomp

4827a64

Merge branch 'branch-22.06' into fea/use_rapids_cpm_nvcomp

7758d88

jakirkham removed the request for review from gerashegalov May 25, 2022 00:42

jbrennan333 mentioned this pull request May 25, 2022

Copy nvcomp-2.3 shared libs from the cudfjni build NVIDIA/spark-rapids-jni#239

Merged

rapids-bot bot merged commit f0d43e5 into rapidsai:branch-22.06 May 25, 2022

robertmaynard deleted the fea/use_rapids_cpm_nvcomp branch May 25, 2022 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pre-built nvcomp 2.3 binaries by default #10851

Use pre-built nvcomp 2.3 binaries by default #10851

robertmaynard commented May 13, 2022 •

edited by jakirkham

Loading

robertmaynard commented May 13, 2022

jbrennan333 commented May 13, 2022

robertmaynard commented May 13, 2022

codecov bot commented May 14, 2022 •

edited

Loading

vyasr May 18, 2022

jbrennan333 commented May 18, 2022

jbrennan333 commented May 18, 2022

vyasr commented May 18, 2022 •

edited

Loading

jbrennan333 commented May 19, 2022

vyasr commented May 20, 2022

vyasr left a comment

robertmaynard commented May 25, 2022

Use pre-built nvcomp 2.3 binaries by default #10851

Use pre-built nvcomp 2.3 binaries by default #10851

Conversation

robertmaynard commented May 13, 2022 • edited by jakirkham Loading

robertmaynard commented May 13, 2022

jbrennan333 commented May 13, 2022

robertmaynard commented May 13, 2022

codecov bot commented May 14, 2022 • edited Loading

Codecov Report

vyasr May 18, 2022

Choose a reason for hiding this comment

jbrennan333 commented May 18, 2022

jbrennan333 commented May 18, 2022

vyasr commented May 18, 2022 • edited Loading

jbrennan333 commented May 19, 2022

vyasr commented May 20, 2022

vyasr left a comment

Choose a reason for hiding this comment

robertmaynard commented May 25, 2022

robertmaynard commented May 13, 2022 •

edited by jakirkham

Loading

codecov bot commented May 14, 2022 •

edited

Loading

vyasr commented May 18, 2022 •

edited

Loading