Port thrust's pinned_allocator to cudf, since Thrust 1.17 removes the type #12004

robertmaynard · 2022-10-26T19:17:13Z

Description

Thrust 1.17 removes the experimental/pinned_allocator. While Thrust offers a replacement in thrust::system::cuda::universal_host_pinned_memory_resource. In doing so we also need to move the consumers to being CUDA sources which would negatively impact our compile time.

Instead we move Thrust's removed pinned_allocator into cudf as it allows usage from C++ sources and doesn't
require larger changes to handle the fact the value_type from the container becomes thrust::pointer<T>.

Note: We haven't seen a compile failure up to this point due to the fact that all CUDA 11.X toolkits provide a version
of thrust that has the experimental header. So when it wasn't found in our 1.17.2 location the compiler would fallback
to the one in the CTK. We can't rely on this behavior moving forward.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

bdice

I saw the Slack conversation about this -- all looks good to me.

robertmaynard · 2022-10-26T20:07:49Z

Moving this to do not merge as I missed another places that libcudf uses the experimental pinned allocator.

davidwendt · 2022-10-26T21:03:06Z

I did not see the Slack conversation. Why are the the .cpp files renamed? Are there any device code launches in the new header?

robertmaynard · 2022-10-26T21:53:11Z

I did not see the Slack conversation. Why are the the .cpp files renamed? Are there any device code launches in the new header?

Due to the usage of thrust::system::cuda::universal_host_pinned_memory_resource> it requires the input files to be CUDA for it to select the correct device when it tries to deallocate or resize the thrust::host_vector. I consider this a bug / oversight in the thrust implementation.

I am not sure this is the approach we should take compared to implementing our own pinned allocator that doesn't require compilation via CUDA.

davidwendt · 2022-10-26T22:05:09Z

I consider this a bug / oversight in the thrust implementation.

Is it an option to fix this in thrust and then include the fix into our code? I fear how this will spread.

Thrust 1.17 removes the experimental/pinned_allocator. While Thrust offers a replacement in `thrust::system::cuda::universal_host_pinned_memory_resource`. In doing so we also need to move the consumers to being CUDA sources which would negatively impact our compile time. Instead we move Thrust's removed pinned_allocator into cudf and continue to use it

robertmaynard · 2022-10-27T13:07:16Z

I consider this a bug / oversight in the thrust implementation.

Is it an option to fix this in thrust and then include the fix into our code? I fear how this will spread.

The entire thrust::mr design is being re-implemented currently so fixing this defect doesn't seem to be worthwhile.

robertmaynard · 2022-10-27T13:09:57Z

@bdice @davidwendt

I have completly redesigned the way we fix this issue. Instead of using universal_host_pinned_memory_resource I have instead ported the removed pinned_allocator to cudf. This simplifies the using code as it doesn't need to be aware that the value type of the containers become thrust::pointer<T> instead of T*. Plus it allows us to continue keeping these source files as C++.

Once NVIDIA/libcudacxx#309 has been merged we can explore using that instead of our custom pinned_allocator.

upsj

LGTM in general, the code style in the allocator seems a bit off to me, but that's just a small nit.

cpp/include/cudf/utilities/pinned_allocator.h

bdice

Approving - just some minor style hiccups to fix.

cpp/include/cudf/utilities/pinned_allocator.h

robertmaynard · 2022-10-27T15:40:53Z

@gpucibot merge

cpp/include/cudf/utilities/pinned_allocator.hpp

codecov · 2022-10-27T19:45:28Z

Codecov Report

Base: 87.40% // Head: 88.12% // Increases project coverage by +0.71% 🎉

Coverage data is based on head (35da554) compared to base (f72c4ce).
Patch has no changes to coverable lines.

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-22.12   #12004      +/-   ##
================================================
+ Coverage         87.40%   88.12%   +0.71%     
================================================
  Files               133      133              
  Lines             21833    22003     +170     
================================================
+ Hits              19084    19390     +306     
+ Misses             2749     2613     -136

Impacted Files	Coverage Δ
python/strings_udf/strings_udf/__init__.py	`86.27% <0.00%> (-10.61%)`	⬇️
python/cudf/cudf/io/text.py	`91.66% <0.00%> (-8.34%)`	⬇️
python/cudf/cudf/core/_base_index.py	`81.28% <0.00%> (-4.27%)`	⬇️
python/cudf/cudf/io/json.py	`92.06% <0.00%> (-2.68%)`	⬇️
python/strings_udf/strings_udf/_typing.py	`94.73% <0.00%> (-1.06%)`	⬇️
python/cudf/cudf/utils/utils.py	`89.91% <0.00%> (-0.69%)`	⬇️
python/cudf/cudf/testing/dataset_generator.py	`72.83% <0.00%> (-0.42%)`	⬇️
python/dask_cudf/dask_cudf/core.py	`73.72% <0.00%> (-0.41%)`	⬇️
python/cudf/cudf/io/parquet.py	`90.45% <0.00%> (-0.39%)`	⬇️
python/dask_cudf/dask_cudf/backends.py	`84.90% <0.00%> (-0.37%)`	⬇️
... and 28 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

robertmaynard · 2022-11-01T15:57:07Z

@gpucibot merge

robertmaynard · 2022-11-01T21:35:35Z

@gpucibot merge

davidwendt · 2022-11-01T21:59:37Z

Looks like you need ops approval before merging this.

msadang

LGTM

robertmaynard added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Oct 26, 2022

robertmaynard requested review from a team as code owners October 26, 2022 19:17

robertmaynard requested review from mythrocks and vuule October 26, 2022 19:17

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

bdice approved these changes Oct 26, 2022

View reviewed changes

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

robertmaynard added 2 commits October 27, 2022 08:49

cudf::host_span now handles thrust::host_vector<thrust::pointer<T>>

1e5058c

robertmaynard force-pushed the fix/issues_with_newer_cuda_versions branch from 276a9dd to dc3f7de Compare October 27, 2022 13:04

github-actions bot removed the CMake CMake build issue label Oct 27, 2022

robertmaynard changed the title ~~Move to thrust 1.17 universal_host_pinned_memory_resource~~ Port thrust's pinned_allocator to cudf, since Thrust 1.17 removes the type Oct 27, 2022

robertmaynard requested a review from bdice October 27, 2022 13:06

upsj approved these changes Oct 27, 2022

View reviewed changes

robertmaynard added 2 commits October 27, 2022 09:42

update pinned_allocator docs to match cudf style

5d0df4d

Switch typedefs over to using as requested in code review

9bfa70f

robertmaynard removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Oct 27, 2022

github-actions bot added the conda label Oct 27, 2022

Move pinned_allocator over to cudf::detail

4831883

robertmaynard requested a review from a team as a code owner October 27, 2022 14:09

bdice reviewed Oct 27, 2022

View reviewed changes

cpp/include/cudf/utilities/pinned_allocator.h Outdated Show resolved Hide resolved

bdice approved these changes Oct 27, 2022

View reviewed changes

cpp/include/cudf/utilities/pinned_allocator.h Outdated Show resolved Hide resolved

cpp/include/cudf/utilities/pinned_allocator.h Outdated Show resolved Hide resolved

robertmaynard added 4 commits October 27, 2022 10:56

Move pinned_allocator to .hpp

61f6bcb

Move pinned_allocator to .hpp

aa36330

Correct style issues found by CI

4443a4a

Correct doxygen issues found by CI

97c8ea7

jrhemstad reviewed Oct 27, 2022

View reviewed changes

cpp/include/cudf/utilities/pinned_allocator.hpp Outdated Show resolved Hide resolved

jrhemstad reviewed Oct 27, 2022

View reviewed changes

cpp/include/cudf/utilities/pinned_allocator.hpp Outdated Show resolved Hide resolved

robertmaynard added 3 commits October 27, 2022 11:53

Move pinned_allocator to include/cudf/detail

a6dff3f

Move pinned_allocator over to cudf exceptions

f2f2932

Correct style issues found by CI

a1a4668

robertmaynard requested a review from jrhemstad October 31, 2022 12:48

jrhemstad approved these changes Nov 1, 2022

View reviewed changes

robertmaynard added 3 commits November 1, 2022 10:59

Merge branch 'branch-22.12' into fix/issues_with_newer_cuda_versions

309514a

Correct style issues found by CI

4016169

data_chunk_source_factories.cpp use cudf's pinned_allocator

35da554

msadang approved these changes Nov 1, 2022

View reviewed changes

rapids-bot bot merged commit ac3f205 into rapidsai:branch-22.12 Nov 1, 2022

robertmaynard deleted the fix/issues_with_newer_cuda_versions branch November 1, 2022 22:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port thrust's pinned_allocator to cudf, since Thrust 1.17 removes the type #12004

Port thrust's pinned_allocator to cudf, since Thrust 1.17 removes the type #12004

robertmaynard commented Oct 26, 2022 •

edited

Loading

bdice left a comment

robertmaynard commented Oct 26, 2022

davidwendt commented Oct 26, 2022

robertmaynard commented Oct 26, 2022

davidwendt commented Oct 26, 2022

robertmaynard commented Oct 27, 2022

robertmaynard commented Oct 27, 2022

upsj left a comment

bdice left a comment

robertmaynard commented Oct 27, 2022

codecov bot commented Oct 27, 2022 •

edited

Loading

robertmaynard commented Nov 1, 2022

robertmaynard commented Nov 1, 2022

davidwendt commented Nov 1, 2022

msadang left a comment

Port thrust's pinned_allocator to cudf, since Thrust 1.17 removes the type #12004

Port thrust's pinned_allocator to cudf, since Thrust 1.17 removes the type #12004

Conversation

robertmaynard commented Oct 26, 2022 • edited Loading

Description

Checklist

bdice left a comment

Choose a reason for hiding this comment

robertmaynard commented Oct 26, 2022

davidwendt commented Oct 26, 2022

robertmaynard commented Oct 26, 2022

davidwendt commented Oct 26, 2022

robertmaynard commented Oct 27, 2022

robertmaynard commented Oct 27, 2022

upsj left a comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

robertmaynard commented Oct 27, 2022

codecov bot commented Oct 27, 2022 • edited Loading

Codecov Report

robertmaynard commented Nov 1, 2022

robertmaynard commented Nov 1, 2022

davidwendt commented Nov 1, 2022

msadang left a comment

Choose a reason for hiding this comment

robertmaynard commented Oct 26, 2022 •

edited

Loading

codecov bot commented Oct 27, 2022 •

edited

Loading