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

KvikIO as an alternative GDS backend #10593

Merged
merged 16 commits into from
Apr 20, 2022
Merged

KvikIO as an alternative GDS backend #10593

merged 16 commits into from
Apr 20, 2022

Conversation

madsbk
Copy link
Member

@madsbk madsbk commented Apr 5, 2022 •
edited
Loading

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

@madsbk madsbk added 2 - In Progress Currently a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 5, 2022
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Apr 5, 2022
@codecov
Copy link

codecov bot commented Apr 5, 2022 •
edited
Loading

Codecov Report

Merging #10593 (1a6341e) into branch-22.06 (65b1cbd) will increase coverage by 0.03%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.06   #10593      +/-   ##
================================================
+ Coverage         86.35%   86.38%   +0.03%     
================================================
  Files               142      142              
  Lines             22335    22335              
================================================
+ Hits              19287    19294       +7     
+ Misses             3048     3041       -7
Impacted Files Coverage Δ
python/cudf/cudf/core/column/string.py 89.22% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.64% <0.00%> (+0.23%) ⬆️
python/cudf/cudf/core/column/numerical.py 96.17% <0.00%> (+0.29%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 92.79% <0.00%> (+1.27%) ⬆️

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 65b1cbd...1a6341e. Read the comment docs.

@madsbk madsbk force-pushed the add_kvikio_as_gds_backend branch 2 times, most recently from b3d22c5 to 14eefe8 Compare April 5, 2022 17:00
@madsbk madsbk force-pushed the add_kvikio_as_gds_backend branch from 14eefe8 to ab8aeb0 Compare April 6, 2022 07:21
@madsbk madsbk marked this pull request as ready for review April 6, 2022 13:19
@madsbk madsbk requested review from a team as code owners April 6, 2022 13:19
@madsbk madsbk requested review from cwharris and ttnghia April 6, 2022 13:19
@madsbk madsbk mentioned this pull request Apr 6, 2022
Closed
robertmaynard
robertmaynard requested changes Apr 6, 2022
View reviewed changes
cpp/CMakeLists.txt Outdated
@@ -530,6 +532,7 @@ target_include_directories(
cudf
PUBLIC "$<BUILD_INTERFACE:${DLPACK_INCLUDE_DIR}>"
"$<BUILD_INTERFACE:${JITIFY_INCLUDE_DIR}>"
"$<BUILD_INTERFACE:${KvikIO_INCLUDE_DIR}>"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this we should link to the kvikio::kvikio target, and remove KvikIO_INCLUDE_DIR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in bcb0a8a, is this sufficient?
Can I ask you to take a look at KvikIO's CMakefile also and see if I am doing the right thing? I am still a novice when it comes to cmake :)

@vuule vuule self-requested a review April 6, 2022 23:41
robertmaynard
robertmaynard approved these changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMake changes LGTM ( presuming the referenced used_by_cudf_for_testing branch gets merged upstream ).

devavret
devavret approved these changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I would have made a new kvikio class inheriting from datasource but this is fine too.

vyasr
vyasr requested changes Apr 8, 2022
View reviewed changes
cpp/cmake/thirdparty/get_kvikio.cmake Outdated Show resolved Hide resolved
vuule
vuule reviewed Apr 19, 2022
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few nitpicks.

cpp/src/io/utilities/datasource.cpp Outdated Show resolved Hide resolved
cpp/src/io/utilities/data_sink.cpp Outdated Show resolved Hide resolved
@madsbk madsbk added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Apr 19, 2022
@madsbk madsbk requested a review from vyasr April 19, 2022 11:57
@robertmaynard
Copy link
Contributor

Once kvikio becomes a dependency of cudf, you will also need to move your burn-down and code-freeze points to be inline with cudf and rmm. Since we can't have cudf go into code-freeze and lock to a tag of kvikio that still has active burn-down activity.

@madsbk
Copy link
Member Author

madsbk commented Apr 20, 2022

Thanks everyone for the reviews, I have added some doc (fad1055) and I think this is ready to be merged.

@vyasr
Copy link
Contributor

vyasr commented Apr 20, 2022

@vuule since this is a significant cuIO change involving GDS and you haven't approved yet I'll let you pull the trigger on merging when you're ready.

@vuule
Copy link
Contributor

vuule commented Apr 20, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 01d08af into rapidsai:branch-22.06 Apr 20, 2022
This was referenced Apr 21, 2022
Merged
Closed
@madsbk madsbk deleted the add_kvikio_as_gds_backend branch April 21, 2022 06:35
@davidwendt davidwendt mentioned this pull request Apr 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vuule vuule vuule left review comments

@ttnghia ttnghia ttnghia approved these changes

@devavret devavret devavret approved these changes

@vyasr vyasr vyasr approved these changes

@robertmaynard robertmaynard robertmaynard approved these changes

@cwharris cwharris Awaiting requested review from cwharris cwharris is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@madsbk @robertmaynard @vyasr @vuule @devavret @ttnghia