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

Test/remove thrust vector usage #11813

Merged

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Sep 29, 2022

Description

This PR removes usage of thrust::device_vector from almost all of our tests. Since the construction of a device vector is not stream-ordered, we should be using rmm::device_uvector instead wherever possible. There is one remaining use of thrust::device_vector, but that is in an test explicitly verifying that device_vector can convert implicitly to a device_span so it's worth keeping that there.

I am working on automated tooling to detect any usage of stream 0 in tests as part of a push to prioritize stream-safety in libcudf, and this PR is a prerequisite to adding such tooling to our CI pipeline since at that point any test using stream 0 would fail. Since there is at least one test where I anticipate stream 0 will always be used (the one described above), I should be able to add specific tests to an allowlist as needed. It's an open question whether the added complexity required by the changes in this PR is a worthwhile tradeoff to be able to programmatically detect stream 0 usage. If reviewers feel that the additional complexity is too high, we can revert some (or all) of these changes and I can just plan for allowing stream 0 usage in all of the necessary tests. This PR demonstrates how we would go about removing it if we choose to do so, though.

Checklist

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

@vyasr vyasr added 3 - Ready for Review Ready for review by team tests Unit testing for project libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 29, 2022
@vyasr vyasr self-assigned this Sep 29, 2022
@vyasr vyasr requested a review from a team as a code owner September 29, 2022 00:17
@vyasr vyasr requested review from upsj and vuule September 29, 2022 00:17
@vyasr
Copy link
Contributor Author

vyasr commented Oct 3, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit ba0febe into rapidsai:branch-22.12 Oct 4, 2022
@vyasr vyasr deleted the test/remove_thrust_vector_usage branch October 5, 2022 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change tests Unit testing for project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants