Replace defaulted stream value for libcudf APIs that use NVCOMP #10877

jbrennan333 · 2022-05-17T18:28:24Z

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.

jbrennan333 · 2022-05-17T20:47:55Z

I have verified that this is working in spark by building along with #10851 and then building a spark-rapids-jni and spark-rapids. I then ran a query and verified that ptds is being used. I think this is ready for review.

jbrennan333 · 2022-05-17T20:50:33Z

This checkstyle failure does not appear to be due to these changes:

>>>> FAILED: libcudf header existence conda/recipes/libcudf/meta.yaml check; begin output
114d113
<         - test -f $PREFIX/include/cudf/lists/list_view.hpp
>>>> FAILED: libcudf header existence conda/recipes/libcudf/meta.yaml check; end output

Should I fix the meta.yaml file?

nvdbaranec

One small comment.

cpp/src/utilities/default_stream.cpp

cpp/tests/utilities_tests/default_stream_tests.cpp

nvdbaranec

cpp changes look good.

java/ci/build-in-docker.sh

codecov · 2022-05-17T23:43:36Z

Codecov Report

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

❗ Current head a0e4025 differs from pull request most recent head 8d070e3. Consider uploading reports for the commit 8d070e3 to get more accurate results

@@               Coverage Diff                @@
##           branch-22.06   #10877      +/-   ##
================================================
+ 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 1db83e3...8d070e3. Read the comment docs.

cpp/include/cudf/io/detail/avro.hpp

cpp/include/cudf/io/detail/parquet.hpp

harrism

Ignore me.

I misunderstood.

…writer calls that use nvcomp

jbrennan333 · 2022-05-19T03:09:54Z

I have updated based on review comments from @jrhemstad and @vuule. I really appreciate all the reviews/comments.

jbrennan333 · 2022-05-19T14:04:05Z

@jrhemstad, @vuule, this is ready for another review.

vuule

cuIO changes look good, thank you for addressing the feedback.

cpp/CMakeLists.txt

java/src/main/native/CMakeLists.txt

robertmaynard

CMake changes LGTM

jbrennan333 · 2022-05-24T20:46:24Z

Since we are going forward with including nvcomp-2.3 in 22.06, I think this can be merged now.

nvdbaranec · 2022-05-24T22:34:23Z

@gpucibot merge

…fault_stream_value (#11082) Closes #10864. Also closes #9614. This PR is a follow-up to #10877. It replaces all of the remaining instances of `rmm::cuda_stream_default` with `cudf::default_stream_value`. There are a lot of replacements and addition of includes, along with some reformatting due to clang-format, but like #10877, there should be no noticeable functional change here. Authors: - Jim Brennan (https://github.com/jbrennan333) Approvers: - Robert (Bobby) Evans (https://github.com/revans2) - https://github.com/nvdbaranec - Nghia Truong (https://github.com/ttnghia) - Karthikeyan (https://github.com/karthikeyann) URL: #11082

Closes #10862. The `PER_THREAD_DEFAULT_STREAM` build option was deprecated in branch-22.06 via #10877, and replaced with the new build option `CUDF_USE_PER_THREAD_DEFAULT_STREAM`. This PR removes `PER_THREAD_DEFAULT_STREAM`. I am putting this up as a draft because I am not certain if we want to do this in 22.08 or wait for a later release? Authors: - Jim Brennan (https://github.com/jbrennan333) Approvers: - Jason Lowe (https://github.com/jlowe) - Vyas Ramasubramani (https://github.com/vyasr) URL: #11134

Replace defaulted stream value for libcudf APIs that use NVCOMP

ca8c5d2

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

jbrennan333 self-assigned this May 17, 2022

jbrennan333 added 4 - Needs Review Waiting for reviewer to review or respond ! - Release Performance Performance related issue labels May 17, 2022

Update copyrights

f9d4a48

jbrennan333 added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 17, 2022

jbrennan333 marked this pull request as ready for review May 17, 2022 20:51

jbrennan333 requested review from a team as code owners May 17, 2022 20:51

jbrennan333 requested review from cwharris and nvdbaranec May 17, 2022 20:51

nvdbaranec reviewed May 17, 2022

View reviewed changes

cpp/src/utilities/default_stream.cpp Outdated Show resolved Hide resolved

cpp/tests/utilities_tests/default_stream_tests.cpp Outdated Show resolved Hide resolved

Address review comments and checkstyle issue

2da5119

jbrennan333 requested a review from a team as a code owner May 17, 2022 21:18

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

nvdbaranec approved these changes May 17, 2022

View reviewed changes

jlowe reviewed May 17, 2022

View reviewed changes

java/ci/build-in-docker.sh Show resolved Hide resolved

jbrennan333 added 2 commits May 17, 2022 16:37

fix cmake checkstyle issues

d5d950e

Update java native build to use new define and other review fixes

2d22e5a

Fix copyright and add new define to pom file

e4f97a3

more review comments - fixes related to is_ptds_enabled()

d2b63d0

vuule requested changes May 18, 2022

View reviewed changes

cpp/include/cudf/io/detail/avro.hpp Show resolved Hide resolved

cpp/include/cudf/io/detail/parquet.hpp Show resolved Hide resolved

harrism previously requested changes May 18, 2022

View reviewed changes

jbrennan333 mentioned this pull request May 19, 2022

Change PER_THREAD_DEFAULT_STREAM to CUDF_USE_PER_THREAD_DEFAULT_STREAM NVIDIA/spark-rapids-jni#258

Merged

Updates based on more review comments - updated orc reader calls and …

3a8d167

…writer calls that use nvcomp

Merge branch 'branch-22.06' into jtb-use-default-stream

358bcb5

vuule approved these changes May 19, 2022

View reviewed changes

vuule mentioned this pull request May 20, 2022

Parquet reader does not take a stream #10903

Closed

jrhemstad approved these changes May 20, 2022

View reviewed changes

vuule added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Needs Review Waiting for reviewer to review or respond labels May 20, 2022

robertmaynard requested changes May 23, 2022

View reviewed changes

cpp/CMakeLists.txt Outdated Show resolved Hide resolved

java/src/main/native/CMakeLists.txt Outdated Show resolved Hide resolved

Address review comments from Robert Maynard

8d070e3

robertmaynard approved these changes May 24, 2022

View reviewed changes

rapids-bot bot merged commit e096345 into rapidsai:branch-22.06 May 24, 2022

jbrennan333 mentioned this pull request Jun 8, 2022

Replace remaining instances of rmm::cuda_stream_default with cudf::default_stream_value #11082

Merged

jbrennan333 mentioned this pull request Jun 22, 2022

Remove deprecated PER_THREAD_DEFAULT_STREAM #11134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace defaulted stream value for libcudf APIs that use NVCOMP #10877

Replace defaulted stream value for libcudf APIs that use NVCOMP #10877

jbrennan333 commented May 17, 2022

jbrennan333 commented May 17, 2022

jbrennan333 commented May 17, 2022

nvdbaranec left a comment

nvdbaranec left a comment

codecov bot commented May 17, 2022 •

edited

Loading

harrism left a comment •

edited

Loading

jbrennan333 commented May 19, 2022

jbrennan333 commented May 19, 2022

vuule left a comment

robertmaynard left a comment

jbrennan333 commented May 24, 2022

nvdbaranec commented May 24, 2022

Replace defaulted stream value for libcudf APIs that use NVCOMP #10877

Replace defaulted stream value for libcudf APIs that use NVCOMP #10877

Conversation

jbrennan333 commented May 17, 2022

jbrennan333 commented May 17, 2022

jbrennan333 commented May 17, 2022

nvdbaranec left a comment

Choose a reason for hiding this comment

nvdbaranec left a comment

Choose a reason for hiding this comment

codecov bot commented May 17, 2022 • edited Loading

Codecov Report

harrism left a comment • edited Loading

Choose a reason for hiding this comment

jbrennan333 commented May 19, 2022

jbrennan333 commented May 19, 2022

vuule left a comment

Choose a reason for hiding this comment

robertmaynard left a comment

Choose a reason for hiding this comment

jbrennan333 commented May 24, 2022

nvdbaranec commented May 24, 2022

codecov bot commented May 17, 2022 •

edited

Loading

harrism left a comment •

edited

Loading