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

Use KvikIO as the GDS backend #10468

Closed
wants to merge 12 commits into from
Closed

Conversation

madsbk
Copy link
Member

@madsbk madsbk commented Mar 21, 2022

Replacing the current GDS datasource with KvikIO.

Background

KvikIO is a new C++ and Python interface to cuFile that combines the current GDS implementation in cuDF and the current GDS implementation in cuCIM into a common library. The idea is to avoid double work and join forces to implement high performances IO for all of RAPIDS. In addition, KvikIO eliminates the needs for all projects to write a fallback implementation when GDS and/or cuFile isn't available. KvikIO should work in any circumstance even if cufile.h is available at compile time.

Policy

This PR reads LIBCUDF_CUFILE_POLICY to determent if KvikIO should be used. "OFF" disables KvikIO and "GDS" and "ALWAYS" enables KivkIO. It doesn't differentiate between "GDS" and "ALWAYS" and the question is if it should?
The current GDS backend rewrites the cuFile's configuration file in order to set cuFile's compatibility mode explicitly. Do we still want this feature? Isn't it better to just let users configure cuFile via. the dedicated configuration file?


Depend on rapidsai/kvikio#43

cc. @vuule

@madsbk madsbk added 2 - In Progress Currently a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 21, 2022
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Mar 21, 2022
@madsbk madsbk force-pushed the kvikio branch 2 times, most recently from c5712c6 to aeba830 Compare March 21, 2022 13:58
@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #10468 (3b40e48) into branch-22.06 (291fbcf) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 3b40e48 differs from pull request most recent head 274c44f. Consider uploading reports for the commit 274c44f to get more accurate results

@@               Coverage Diff                @@
##           branch-22.06   #10468      +/-   ##
================================================
+ Coverage         86.31%   86.33%   +0.01%     
================================================
  Files               140      140              
  Lines             22300    22300              
================================================
+ Hits              19249    19253       +4     
+ Misses             3051     3047       -4     
Impacted Files Coverage Δ
python/cudf/cudf/core/column/string.py 89.10% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.72% <0.00%> (+0.22%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 90.49% <0.00%> (+0.45%) ⬆️

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 291fbcf...274c44f. Read the comment docs.

@madsbk madsbk marked this pull request as ready for review March 23, 2022 16:03
@madsbk madsbk requested review from a team as code owners March 23, 2022 16:03
@madsbk madsbk added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Mar 23, 2022
@madsbk madsbk changed the title [WIP] Use KvikIO Use KvikIO as the GDS backend Mar 23, 2022
@devavret
Copy link
Contributor

Can we get a gbench on GDS before and after?

@devavret
Copy link
Contributor

Are file_io_utilities.hpp/cpp obsolete now?

@vuule
Copy link
Contributor

vuule commented Mar 23, 2022

Regarding the policy: I assume that KvikIO does not interact with the cuFile config?
If we remove the config override, the behavior would probably need to be "try KvikIO, if it fails, fall back to internal IO". This is different from the ALWAYS option, where we throw if anything goes wrong in cuFile. I feel like we need to get some form of logging before making the policy change, since cuFile/KvikIO use would be completely transparent otherwise and users wouldn't be able to tell if direct path is used (there's cuFile log, maybe that's fine to defer to).
Also, we'd want to evaluate compat code vs internal IO before making the change. Even without enforcing a default, we can recommend it in the documentation.
TL;DR: I'd like to keep the policy change separate from this PR, is possible.

@vuule
Copy link
Contributor

vuule commented Mar 23, 2022

Are file_io_utilities.hpp/cpp obsolete now?

For the most part, yes (if not completely). AFAIK, KvikIO includes all the best parts of this implementation :)

@robertmaynard
Copy link
Contributor

We should work out the dependency tree for CUDF and KvikIO in regards to nvcomp. Looking at rapidsai/kvikio#23 it seems that the current approach would be to use cudf installs for nvcomp, which would make this dependency tree pretty messy.

@vuule
Copy link
Contributor

vuule commented Mar 23, 2022

@madsbk I think this should be moved to 22.06, IMO we are too close to the code freeze for a PR that affects most of IO.

madsbk added 2 commits March 24, 2022 15:18
Do we want an option to enable the use of `direct_read_source`?
@madsbk
Copy link
Member Author

madsbk commented Mar 24, 2022

gbench on GDS

@devavret Is there a guide on how to run this?

@vuule I agree, we need an option to enable/disable GDS but KvikIO should work in all cases, we don't need a "try KvikIO, if it fails, fall back to internal IO". KvikIO implements its own fallback implementation that uses bounce buffers and doesn't require cufile.h or libcufile.so. In the future, I hope to completely replace the bounce buffer implementation in cuDF.

For now, I suggest that we have:

  • LIBCUDF_CUFILE_POLICY=OFF use cuDF's bounce buffer implementation
  • LIBCUDF_CUFILE_POLICY=ALWAYS use KvikIO and allow KvikIO to run in compatibility mode.
  • LIBCUDF_CUFILE_POLICY=GDS use KvikIO and do not allow KvikIO to run in compatibility mode. However, cuFile might still run in compatibility mode if cufile.json allows it.

@madsbk
Copy link
Member Author

madsbk commented Mar 24, 2022

@robertmaynard

We should work out the dependency tree for CUDF and KvikIO in regards to nvcomp. Looking at rapidsai/kvikio#23 it seems that the current approach would be to use cudf installs for nvcomp, which would make this dependency tree pretty messy.

Regarding rapidsai/kvikio#23, it is only for the Python bindings, the C++ API this PR uses doesn't depend on nvcomp. The only hard dependency of the KvikIO's C++ API is CUDAToolkit.

@madsbk madsbk changed the base branch from branch-22.04 to branch-22.06 March 24, 2022 15:22
@devavret
Copy link
Contributor

gbench on GDS

@devavret Is there a guide on how to run this?

You can build and run the PARQUET_READER_BENCH and PARQUET_WRITER_BENCH targets while the appropriate environment variables are set to use GDS. They end up in <builddir>/benchmarks/

@devavret
Copy link
Contributor

Are file_io_utilities.hpp/cpp obsolete now?

For the most part, yes (if not completely). AFAIK, KvikIO includes all the best parts of this implementation :)

@madsbk, could you remove those as part of this PR?

@madsbk
Copy link
Member Author

madsbk commented Mar 25, 2022

Are file_io_utilities.hpp/cpp obsolete now?

For the most part, yes (if not completely). AFAIK, KvikIO includes all the best parts of this implementation :)

@madsbk, could you remove those as part of this PR?

Will do, I just need to know how much to remove. It depends on the policy: do we want to change the cufile.json configuration?

TODO: before merging, this should be changed to KvikIO main repos
@madsbk
Copy link
Member Author

madsbk commented Apr 6, 2022

I am closing this PR in favor of #10593.

@madsbk madsbk closed this Apr 6, 2022
rapids-bot bot pushed a commit that referenced this pull request Apr 20, 2022
This PR is a new take on #10468 that is less intrusive. It keeps the existing GDS backend and adds a new option `LIBCUDF_CUFILE_POLICY=KVIKIO` that make cudf use KvikIO.  

 The default policy is still `LIBCUDF_CUFILE_POLICY=GDS` 

cc. @vuule, @devavret, @GregoryKimball

Authors:
  - Mads R. B. Kristensen (https://github.com/madsbk)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - Robert Maynard (https://github.com/robertmaynard)
  - Devavret Makkar (https://github.com/devavret)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #10593
@madsbk madsbk deleted the kvikio branch May 2, 2022 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants