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.

Sign up for GitHub

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

Jump to bottom

Add tests ensuring that cudf's default stream is always used #11875

Merged
merged 55 commits into from
Oct 21, 2022
Merged

Add tests ensuring that cudf's default stream is always used #11875

merged 55 commits into from
Oct 21, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Oct 7, 2022 •
edited
Loading

Description

This PR ensures that cudf's default stream is properly passed to all kernel launches so that nothing implicitly runs on the CUDA default stream. It adds a small library that is built during the tests and overloads CUDA functions to throw an exception when usage of the default stream is detected. It also fixes all remaining usage of anything other than cudf's default stream (I fixed most of the issues in previous PRs, but I found a few others when finalizing this one).

Resolves #11929
Resolves #11942

Important notes for reviewers:

  • The changeset is deceptively large. The vast majority of the changes are just a global find-and-replace of cudf::get_default_stream() for cudf::default_stream_value, as well as a few smaller fixes such as missing CUDF_TEST_PROGRAM_MAIN in a couple of tests and usage of rmm::cuda_stream_default. The meaningful changes are:
    • The new default stream getter/setter in default_stream.[hpp|cpp]
    • The addition of cpp/tests/utilities/identify_stream_usage
    • The changes to the base testing fixture in cpp/include/cudf_test/base_fixture.hpp to inject the custom stream.
    • The changes to CI in ci/gpu/build.sh to build and use the new library.
  • This PR is a breaking change because it moves the default stream into the detail namespace. Going forward the default stream may only be accessed using the public accessor cudf::get_default_stream(). I have added a corresponding setter, but it is also in the detail namespace since I do not want to publicly support changing the default stream yet, only for the purpose of testing. Reviewers, please leave comments if you disagree with those choices.
  • I have made getting and setting the default stream thread-safe, but there is still only a single stream. In multi-threaded applications we may want to support a stream per thread so that users could manually achieve PTDS with more fine-tuned control. Is this worthwhile? Even if it is, I'm inclined to wait for a subsequent PR to implement this unless someone feels strongly otherwise.
  • I'm currently only overloading cudaLaunchKernel. I can add overloads for other functions as well, but I didn't want to go through the effort of overloading every possible API. If reviewers have a minimal set that they'd like to see overloaded, let me know. I've included links to all the relevant pages of the CUDA runtime API in the identify_stream_usage.cu file if someone wants to look through them.

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
Add ability to use a custom stream as the cudf default stream.
53d34e9
@vyasr
First version of code for identifying default stream usage.
5a16ce2
@vyasr
Document all the functionality that we need to overload.
3b4912c
@vyasr
Remove unnecessary includes.
1719796
@vyasr
Split stack tracing logic out into a separate function.
07e8179
@vyasr
Minor cleanup.
0038673
@vyasr
Also account for cudaStreamLegacy and cudaStreamPerThread.
6136bf5
@vyasr
Change default stream to a settable variable and set it during tests.
6026e69
@vyasr
Fix remaining issues.
62f30a7
@vyasr
Move everything but the getter to detail namespace.
efbafdf
@vyasr
Move identify_streams into test utils.
f675c76
@vyasr
Some minor touchups to identify_streams.
664582b
@vyasr
Rename identify_streams to identify_stream_usage.
ad156bc
@vyasr
Add default stream identification to tests.
f789592
@vyasr
Move definition of default_stream_value into detail namespace.
9c67720
@vyasr
Add to CI.
0fdb2b5
@vyasr
Throw exceptions when default stream is detected instead of just prin…
6de4f7c 
…ting.
@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 breaking Breaking change labels Oct 7, 2022
@vyasr vyasr self-assigned this Oct 7, 2022
@vyasr vyasr requested review from a team as code owners October 7, 2022 18:04
@vyasr vyasr requested a review from bdice October 7, 2022 18:04
@upsj upsj mentioned this pull request Oct 19, 2022
Merged
3 tasks
jrhemstad
jrhemstad requested changes Oct 19, 2022
View reviewed changes
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

I have significant concerns about adding acquiring a mutex at so many places throughout the code just to enable this test case. Vyas and I will be meeting to discuss alternative solutions, but requesting changes for now to prevent merging as I do not think the pros outweigh the cons in the current solution.

@vyasr
Copy link
Contributor Author

vyasr commented Oct 19, 2022 •
edited
Loading

I have significant concerns about adding acquiring a mutex at so many places throughout the code just to enable this test case. Vyas and I will be meeting to discuss alternative solutions, but requesting changes for now to prevent merging as I do not think the pros outweigh the cons in the current solution.

I've resolved this issue by instead overloading get_default_stream in the identify_stream_usage library with a version that creates a function local static. The resulting stream is initialized after main so that it does not trigger the CUDA UB that I was running into in earlier iterations, and it obviates the need for the set_default_stream function, which has now been removed. Since the default stream is no longer settable, the mutex is no longer necessary. Also, the default stream is once again const.

@ttnghia ttnghia mentioned this pull request Oct 21, 2022
Merged
@vyasr
Copy link
Contributor Author

vyasr commented Oct 21, 2022 •
edited
Loading

I just merged branch-22.12 locally, rebuilt, and reran the tests to make sure that nothing has been merged since the last test run that would use a default stream and break this, so I think this is ready to merge and shouldn't cause any test failures afterwards.

@vyasr vyasr dismissed robertmaynard’s stale review October 21, 2022 14:44

Review has been addressed, and there are enough other approvals now

@vyasr
Copy link
Contributor Author

vyasr commented Oct 21, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit dec8bde into rapidsai:branch-22.12 Oct 21, 2022
@vyasr vyasr deleted the feature/default_stream_usage_identification branch October 21, 2022 14:45
@upsj upsj mentioned this pull request Oct 27, 2022
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Oct 27, 2022
@upsj
Refactor multibyte_split output_builder (#11945)
43eb7a0 
This PR moves the `output_builder` and `split_device_span` classes out of `multibyte_split` and adds an iterator for the `split_device_span`, enabling it to be used directly in Thrust algorithms.

I also included a fix from #11875 to make the integration easier once that is merged.

Authors:
  - Tobias Ribizel (https://github.com/upsj)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Mike Wilson (https://github.com/hyperbolic2346)

URL: #11945
@vyasr vyasr mentioned this pull request Nov 8, 2022
Merged
3 tasks
@vyasr vyasr mentioned this pull request Dec 16, 2022
Closed
3 tasks
@vyasr vyasr mentioned this pull request Feb 7, 2023
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Feb 10, 2023
@vyasr
Reenable stream identification library in CI (#12714)
1c0224f 
This PR reenables the preload library introduced for verifying stream usage in libcudf in #11875. This library was disabled during the GitHub Actions migration.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - AJ Schmidt (https://github.com/ajschmidt8)

Approvers:
  - Yunsong Wang (https://github.com/PointKernel)
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Bradley Dice (https://github.com/bdice)

URL: #12714
rapids-bot bot pushed a commit that referenced this pull request Mar 14, 2023
@vyasr
Enable detection of undesired stream usage (#12089)
55ed347 
This PR builds on #11875 and partially addresses #11943. This PR allows us to run all tests on a precise stream (the newly introduced `cudf::test::get_default_stream()`) and then verify that all CUDA APIs end up invoked on that stream. This implements the feature required in #11943, but to apply it universally across libcudf will require the API changes that will expose streams so I plan to make those changes incrementally after this PR is merged.

The preload library is now compiled twice, once to overload `cudf::get_default_stream` and once to overload `cudf::test::get_default_stream`. For now there is still some manual coordination associated with determining which one should be used with a given test, but once #12451 is merged and we start running all tests via ctest instead of direct invocation of the test executables we can start encoding this information in the CMake configuration of the tests by associating the require environment variables directly with the test executable using `set_tests_properties`.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - Ray Douglass (https://github.com/raydouglass)
  - Nghia Truong (https://github.com/ttnghia)

URL: #12089
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@upsj upsj upsj left review comments

@jlowe jlowe jlowe approved these changes

@bdice bdice bdice approved these changes

@sevagh sevagh sevagh approved these changes

@jrhemstad jrhemstad jrhemstad approved these changes

@davidwendt davidwendt davidwendt approved these changes

@brandon-b-miller brandon-b-miller brandon-b-miller approved these changes

@harrism harrism Awaiting requested review from harrism harrism is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@robertmaynard robertmaynard Awaiting requested review from robertmaynard

Assignees

@vyasr vyasr

Labels
3 - Ready for Review Ready for review by team breaking Breaking change CMake CMake build issue improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. tests Unit testing for project
Projects
None yet
Milestone
Enable streams
9 participants
@vyasr @davidwendt @jrhemstad @robertmaynard @jlowe @upsj @bdice @sevagh @brandon-b-miller