Skip to content

Add missing Thrust #includes.#11457

Merged
rapids-bot[bot] merged 1 commit into
rapidsai:branch-22.10from
bdice:fix-thrust-includes
Aug 4, 2022
Merged

Add missing Thrust #includes.#11457
rapids-bot[bot] merged 1 commit into
rapidsai:branch-22.10from
bdice:fix-thrust-includes

Conversation

@bdice
Copy link
Copy Markdown
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 requested review from PointKernel and nvdbaranec 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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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