Use cuFile direct device reads/writes by default in cuIO #9722

vuule · 2021-11-18T01:13:16Z

Making this change early in 22.02 to test through internal use + nightly builds before the release.

Modify the way cuFile integration is enabled to match the nvCOMP integration.
Change the default from OFF to GDS (GDS on, only for direct reads/writes, no compatibility mode).
cuFile JSON config file is now modified on first cuFile use (same time as the driver), instead of the first query that checks if GDS use is enabled.

codecov · 2021-11-18T02:45:16Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.02@32bacfa). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.02    #9722   +/-   ##
===============================================
  Coverage                ?   10.49%           
===============================================
  Files                   ?      119           
  Lines                   ?    20305           
  Branches                ?        0           
===============================================
  Hits                    ?     2130           
  Misses                  ?    18175           
  Partials                ?        0

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 32bacfa...8c38454. Read the comment docs.

davidwendt

Seems changing the default for LIBCUDF_CUFILE_POLICY would qualify this as a breaking change?

davidwendt · 2021-11-18T13:20:39Z

cpp/src/io/utilities/file_io_utilities.cpp

+  while (std::getline(user_config_file, line)) {
+    std::string const tag = "\"allow_compat_mode\"";
+    if (line.find(tag) != std::string::npos) {
+      // TODO: only replace the true/false value


I feel like the fact the file is overwritten and not actually modified should be documented in the .rst somehow.

cpp/src/io/utilities/config_utils.hpp

kkraus14 · 2021-11-18T16:20:53Z

FYI -- I recently upgraded my dev environment to CUDA 11.5 via the runfile and it seems like neither the runfile nor conda packages (either provided by NVIDIA or in conda-forge) currently include libcufile or its headers which will likely break quite a lot of users if we make this required and/or the default.

vuule · 2021-11-18T18:33:37Z

Seems changing the default for LIBCUDF_CUFILE_POLICY would qualify this as a breaking change?

I don't know. The change does not break any code. I'm okay with changing the label (whose description, btw, is not helpful here :D).

vuule · 2021-11-18T18:42:26Z

FYI -- I recently upgraded my dev environment to CUDA 11.5 via the runfile and it seems like neither the runfile nor conda packages (either provided by NVIDIA or in conda-forge) currently include libcufile or its headers which will likely break quite a lot of users if we make this required and/or the default.

I don't see how this would break cudf for users. This PR does not make cufile a requirement. If the headers are not present during compilation, we will compile without any cufile capability. If the library is not present at runtime, we will detect an issue when loading it and fall back to the host path. Even if something goes wrong when opening a file with cufile, we fall back to the host path.

Thank you for bringing this up, will look into whether cufile should be included in the packages.

Co-authored-by: David Wendt <[email protected]>

kkraus14 · 2021-11-18T21:01:06Z

I don't see how this would break cudf for users. This PR does not make cufile a requirement. If the headers are not present during compilation, we will compile without any cufile capability. If the library is not present at runtime, we will detect an issue when loading it and fall back to the host path. Even if something goes wrong when opening a file with cufile, we fall back to the host path.

Apologies, I misread the code. Awesome to see this moving forward 😄

davidwendt · 2021-11-19T12:18:55Z

Seems changing the default for LIBCUDF_CUFILE_POLICY would qualify this as a breaking change?

I don't know. The change does not break any code. I'm okay with changing the label (whose description, btw, is not helpful here :D).

I thought 'breaking' was a way to communicate that we are changing a default or a previously documented behavior.

…nto fea-use-gds-default

vyasr

Happy to see this happening! Mostly looks good, just a few minor suggestions before we push this out.

cpp/src/io/utilities/config_utils.hpp

cpp/src/io/utilities/file_io_utilities.cpp

docs/cudf/source/basics/io-gds-integration.rst

bdice

Overall this looks good! I have some comments on docs. 📜

cpp/src/io/utilities/config_utils.hpp

cpp/src/io/utilities/file_io_utilities.cpp

docs/cudf/source/basics/io-gds-integration.rst

Co-authored-by: Bradley Dice <[email protected]> Co-authored-by: Vyas Ramasubramani <[email protected]>

Co-authored-by: Bradley Dice <[email protected]>

…nto fea-use-gds-default

bdice

Two minor typos, otherwise LGTM. 👍

cpp/src/io/utilities/config_utils.hpp

docs/cudf/source/basics/io-gds-integration.rst

Co-authored-by: Bradley Dice <[email protected]>

vyasr · 2021-11-19T22:40:06Z

@gpucibot merge

Signed-off-by: Peixin Li <[email protected]> related to #9722 skip cufile test in JNI build while we have a separate pipeline for GDS testing Authors: - Peixin (https://github.com/pxLi) Approvers: - Tim Liu (https://github.com/NvTimLiu) - Gary Shen (https://github.com/GaryShen2008) URL: #9744

vuule added 2 commits November 17, 2021 17:10

replace cufile_config; change default

d4321ee

update docs

b83fe14

vuule added cuIO cuIO issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 18, 2021

vuule self-assigned this Nov 18, 2021

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Nov 18, 2021

vuule marked this pull request as ready for review November 18, 2021 06:04

vuule requested a review from a team as a code owner November 18, 2021 06:04

vuule requested review from vyasr and davidwendt November 18, 2021 06:04

davidwendt reviewed Nov 18, 2021

View reviewed changes

Update cpp/src/io/utilities/config_utils.hpp

ab3dbdd

Co-authored-by: David Wendt <[email protected]>

davidwendt approved these changes Nov 19, 2021

View reviewed changes

vuule added 2 commits November 19, 2021 11:28

expand a TODO comment

77c3929

Merge branch 'fea-use-gds-default' of https://github.com/vuule/cudf i…

cf4cf17

…nto fea-use-gds-default

vuule added breaking Breaking change and removed non-breaking Non-breaking change labels Nov 19, 2021

vyasr requested changes Nov 19, 2021

View reviewed changes

bdice requested changes Nov 19, 2021

View reviewed changes

docs suggestions

7c69c6e

Co-authored-by: Bradley Dice <[email protected]> Co-authored-by: Vyas Ramasubramani <[email protected]>

vuule and others added 3 commits November 19, 2021 12:07

typos

5d9a490

Co-authored-by: Bradley Dice <[email protected]>

move config impl to cpp

2b94a07

make modify_cufile_json const

0081f20

vuule requested a review from a team as a code owner November 19, 2021 20:23

github-actions bot added the CMake CMake build issue label Nov 19, 2021

vuule and others added 3 commits November 19, 2021 12:24

another typo

ab9e8a6

Co-authored-by: Bradley Dice <[email protected]>

EOF newline

8b4ff0a

Merge branch 'fea-use-gds-default' of https://github.com/vuule/cudf i…

b994728

…nto fea-use-gds-default

bdice approved these changes Nov 19, 2021

View reviewed changes

cpp/src/io/utilities/config_utils.hpp Outdated Show resolved Hide resolved

docs/cudf/source/basics/io-gds-integration.rst Outdated Show resolved Hide resolved

vuule and others added 3 commits November 19, 2021 13:12

change env var behavior to throw if an invalid value is set

0baa7e7

rst format

2afd721

yet another typo

8c38454

Co-authored-by: Bradley Dice <[email protected]>

vuule requested a review from vyasr November 19, 2021 21:15

vyasr approved these changes Nov 19, 2021

View reviewed changes

rapids-bot bot merged commit f0367c0 into rapidsai:branch-22.02 Nov 19, 2021

pxLi mentioned this pull request Nov 22, 2021

Skip cufile tests in JNI build script #9744

Merged

quasiben mentioned this pull request Nov 22, 2021

[QST] How to check if GDS is enabled #8838

Closed

vuule deleted the fea-use-gds-default branch April 20, 2022 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use cuFile direct device reads/writes by default in cuIO #9722

Use cuFile direct device reads/writes by default in cuIO #9722

vuule commented Nov 18, 2021

codecov bot commented Nov 18, 2021 •

edited

Loading

davidwendt left a comment

davidwendt Nov 18, 2021

kkraus14 commented Nov 18, 2021

vuule commented Nov 18, 2021

vuule commented Nov 18, 2021

kkraus14 commented Nov 18, 2021

davidwendt commented Nov 19, 2021

vyasr left a comment

bdice left a comment

bdice left a comment

vyasr commented Nov 19, 2021

Use cuFile direct device reads/writes by default in cuIO #9722

Use cuFile direct device reads/writes by default in cuIO #9722

Conversation

vuule commented Nov 18, 2021

codecov bot commented Nov 18, 2021 • edited Loading

Codecov Report

davidwendt left a comment

Choose a reason for hiding this comment

davidwendt Nov 18, 2021

Choose a reason for hiding this comment

kkraus14 commented Nov 18, 2021

vuule commented Nov 18, 2021

vuule commented Nov 18, 2021

kkraus14 commented Nov 18, 2021

davidwendt commented Nov 19, 2021

vyasr left a comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

vyasr commented Nov 19, 2021

codecov bot commented Nov 18, 2021 •

edited

Loading