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

Add missing Thrust includes. #161

Merged
merged 4 commits into from
May 26, 2022
Merged

Add missing Thrust includes. #161

merged 4 commits into from
May 26, 2022

Conversation

bdice
Copy link
Contributor

@bdice bdice commented May 25, 2022

Closes #146

Description

This PR cleans up some #includes for Thrust. This is meant to help ease the transition to Thrust 1.16 / 1.17 when that is updated in rapids-cmake.

(I'm going to test this locally with Thrust 1.16 and will open the draft PR once I confirm it works.)

Context

Version 1.16 of Thrust reduced the number of internal header inclusions:

#1572 Removed several unnecessary header includes. Downstream projects may need to update their includes if they were relying on this behavior.

I am making similar changes across all RAPIDS libraries to clean up includes ("include what we use," in essence) to make sure we have compatibility with future versions of Thrust.

@PointKernel PointKernel added type: feature request New feature request helps: rapids Helps or needed by RAPIDS labels May 25, 2022
@bdice
Copy link
Contributor Author

bdice commented May 26, 2022

I checked the logs and the new thrust was used: CPM: adding package [email protected] (1.17.0-rc2).

All CI passed for commit 97044cc.

Next: wait for final Thrust 1.17.0 release, retest, then revert commits altering rapids-cmake behavior. Then this can be merged. We can update rapids-cmake once all repos are ready.

@bdice bdice marked this pull request as ready for review May 26, 2022 03:31
@PointKernel
Copy link
Member

PointKernel commented May 26, 2022

@bdice Thanks!

Any ETA for the thrust upgrade in rapids-cmake? I need to make a cudf PR to resolve the conflicts with newer version of cuco since all sentinel values are required to be passed via sentinel namespace (added in #142 ) for static_map. This would break all use cases of static_map in libcudf.

@bdice
Copy link
Contributor Author

bdice commented May 26, 2022

The upstream change in rapids-cmake is probably a week away from ready. The Thrust 1.17.0 release should come out next week, and we'll need to finalize some testing across all RAPIDS repos. This PR's changes to #includes can be merged, if we believe the Thrust 1.17.0-rc2 ➡️ 1.17.0 final update won't break includes. I think that's a safe bet. I will revert the changes to pull and test Thrust 1.17.0-rc2, then this can be reviewed and merged.

edit: It's ready for review 🚀

Tracking PR for rapids-cmake: rapidsai/rapids-cmake#199

Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

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

Looks GREAT to me!

include/cuco/static_multimap.cuh Outdated Show resolved Hide resolved
@PointKernel PointKernel merged commit 917f1e5 into NVIDIA:dev May 26, 2022
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Jun 7, 2022
Depends on NVIDIA/cuCollections#165 and NVIDIA/cuCollections#171

This is a preparation step to finally address #10841. It fetches the latest version of `cuco` that [adds missing thrust headers ](NVIDIA/cuCollections#161) and [improves `static_map::retrieve_all`](NVIDIA/cuCollections#169).

Authors:
  - Yunsong Wang (https://github.com/PointKernel)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #10983
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helps: rapids Helps or needed by RAPIDS type: feature request New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up thrust include groups
3 participants