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. #11457

Merged
merged 1 commit into from
Aug 4, 2022

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Aug 3, 2022

Description

Thrust 1.16 removed internal header inclusions that libcudf relied on. This PR adds missing #includes that were found automatically by a script I wrote. See notes on #10489. This was previously applied in #10489 but the script became more sophisticated (and libcudf has changed) since I last applied it, so more missing #includes were found.

Required for #11437 to upgrade to Thrust 1.17. This change has been separated from #11437 to minimize that PR's diff. Some additional changes will be needed on that PR but we don't want to hold off on fixing these includes, as recommended by @davidwendt.

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 bdice changed the title Add missing thrust #includes. Add missing Thrust #includes. Aug 3, 2022
@bdice bdice self-assigned this Aug 3, 2022
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Aug 3, 2022
@bdice bdice added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 3, 2022
@bdice bdice marked this pull request as ready for review August 3, 2022 23:45
@bdice bdice requested a review from a team as a code owner August 3, 2022 23:45
@bdice bdice mentioned this pull request Aug 3, 2022
3 tasks
@bdice bdice added the tech debt label Aug 3, 2022
@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

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

❗ Current head ed78b3a differs from pull request most recent head 9e586dc. Consider uploading reports for the commit 9e586dc to get more accurate results

@@               Coverage Diff               @@
##             branch-22.10   #11457   +/-   ##
===============================================
  Coverage                ?   86.47%           
===============================================
  Files                   ?      144           
  Lines                   ?    22856           
  Branches                ?        0           
===============================================
  Hits                    ?    19764           
  Misses                  ?     3092           
  Partials                ?        0           

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

Copy link
Contributor

@ttnghia ttnghia left a comment

Choose a reason for hiding this comment

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

So this is adding missing headers. I wish we can have removal of redundant headers as well. I sometimes have to spend a lot of time manually doing that for the files I've been editing.

@bdice
Copy link
Contributor Author

bdice commented Aug 4, 2022

So this is adding missing headers. I wish we can have removal of redundant headers as well. I sometimes have to spend a lot of time manually doing that for the files I've been editing.

That would be nice. I don't have a way to do that right now, but perhaps I can alter my script to remove all Thrust headers and re-add only those that the file actually uses. We can't make use of tooling like iwyu (I think it requires clang compatibility and does not support CUDA) but we can try the approach with my header fixer script.

@bdice
Copy link
Contributor Author

bdice commented Aug 4, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 217243c into rapidsai:branch-22.10 Aug 4, 2022
rapids-bot bot pushed a commit that referenced this pull request Aug 11, 2022
Updates the bundled version of Thrust to 1.17.0. I will run benchmarks and include results in a comment below.

Depends on #11457.

Supersedes #10489, #10577, #10586. Closes #10841. **This should be merged concurrently with rapidsai/rapids-cmake#231

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - David Wendt (https://github.com/davidwendt)
  - Nghia Truong (https://github.com/ttnghia)
  - Robert Maynard (https://github.com/robertmaynard)

URL: #11437
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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