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 stream parameter to hashing APIs #12090

Merged
merged 6 commits into from
Jun 13, 2023
Merged

Add stream parameter to hashing APIs #12090

merged 6 commits into from
Jun 13, 2023

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Nov 8, 2022 •
edited
Loading

Description

This PR adds a stream parameter to cudf::hash, allowing callers to request execution on a stream other than cudf's default stream.

Checklist

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

@github-actions github-actions bot added CMake CMake build issue conda libcudf Affects libcudf (C++/CUDA) code. labels Nov 8, 2022
@vyasr vyasr mentioned this pull request Nov 8, 2022
Merged
3 tasks
@vyasr
Copy link
Contributor Author

vyasr commented Nov 8, 2022 •
edited
Loading

To test this, run the following from the root of the repo:

GTEST_CUDF_STREAM_ERROR_MODE=print GTEST_CUDF_STREAM_MODE=new_testing_default LD_PRELOAD=./cpp/build/release/libcudf_identify_stream_usage_mode_testing.so ./cpp/build/release/gtests/HASHING_TEST  --gtest_filter="HashTest.MultiValue"

from the root of the repo. The output will be noisy because of all of the APIs that have not yet been updated to accept streams. Those will improve as I work on this more.

@vyasr vyasr force-pushed the feature/test_new_streams_api branch from 8a8f0d6 to cbbcc1f Compare November 8, 2022 01:25
@vyasr vyasr force-pushed the feature/test_new_streams_api branch from cbbcc1f to 35b29e4 Compare February 10, 2023 23:25
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@vyasr vyasr changed the base branch from branch-22.12 to branch-23.04 February 10, 2023 23:26
@vyasr vyasr force-pushed the feature/test_new_streams_api branch from 35b29e4 to 14dce92 Compare February 11, 2023 00:04
@github-actions github-actions bot added the ci label Feb 11, 2023
@vyasr vyasr force-pushed the feature/test_new_streams_api branch 2 times, most recently from c0da14e to 45d8223 Compare February 25, 2023 01:33
@vyasr vyasr added feature request New feature or request non-breaking Non-breaking change labels Feb 25, 2023
@vyasr vyasr force-pushed the feature/test_new_streams_api branch from 82fba41 to 8db038d Compare March 1, 2023 03:06
@github-actions github-actions bot added the Java Affects Java cuDF API. label Mar 1, 2023
@vyasr vyasr force-pushed the feature/test_new_streams_api branch from 175312d to dbc3853 Compare March 1, 2023 23:24
@github-actions github-actions bot removed the Java Affects Java cuDF API. label Mar 1, 2023
@vyasr
Copy link
Contributor Author

vyasr commented Mar 1, 2023

The last commit on this branch (dbc3853) includes all changes needed to get the test to pass. It turned out to almost entirely be changes to cudf's testing utilities, hashing looks like it didn't require much else to change. Reviewers wishing to test the changes (with the command that I posted in my previous comment) can revert that change to see the expected failures locally.

@vyasr vyasr mentioned this pull request Jun 2, 2023
Merged
3 tasks
@vyasr vyasr force-pushed the feature/test_new_streams_api branch from dbc3853 to 6d0dafb Compare June 2, 2023 19:52
@vyasr vyasr changed the base branch from branch-23.04 to branch-23.08 June 2, 2023 19:52
@vyasr
Copy link
Contributor Author

vyasr commented Jun 2, 2023

This PR now depends on #13506. It will not be merged into 23.08, but rather into an ongoing feature branch for streams development.

@vyasr vyasr force-pushed the feature/test_new_streams_api branch from 6d0dafb to 07e16d0 Compare June 2, 2023 21:23
@github-actions github-actions bot removed conda CMake CMake build issue labels Jun 2, 2023
@vyasr vyasr force-pushed the feature/test_new_streams_api branch from 07e16d0 to 9c2606a Compare June 3, 2023 00:11
rapids-bot bot pushed a commit that referenced this pull request Jun 5, 2023
@vyasr
Use test default stream (#13506)
c4818f9 
For the purpose of verifying that streams are properly forwarded through all libcudf APIs, libcudf tests will be rewritten to use `cudf::test::get_default_stream()` (introduced in #12089) instead of `cudf::get_default_stream()`. By default, these are identical, so this change is typically a no-op, but when using the preload library features added in #12089 we will be able to use a custom (non CUDA-default) stream in tests and verify that it is the only stream used. This PR contains a subset of changes needed to existing test functionality without making any changes to libcudf public APIs. These changes are extracted from #12090.

Contributes to #11943.

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

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - Mark Harris (https://github.com/harrism)

URL: #13506
@vyasr vyasr changed the base branch from feat/stream_ordered_apis to branch-23.08 June 9, 2023 17:17
@vyasr vyasr changed the title Add stream parameter to hashing APIs Add stream parameter to hashing APIs Jun 9, 2023
@vyasr vyasr changed the title Add stream parameter to hashing APIs Add stream parameter to hashing APIs Jun 9, 2023
@vyasr vyasr removed request for a team and robertmaynard June 9, 2023 17:21
@vyasr vyasr marked this pull request as ready for review June 9, 2023 17:21
@vyasr vyasr added this to the Enable streams milestone Jun 9, 2023
@vyasr vyasr added 3 - Ready for Review Ready for review by team and removed gpuCI Python Affects Python cuDF API. CMake CMake build issue labels Jun 9, 2023
@vyasr vyasr changed the title Add stream parameter to hashing APIs Add stream parameter to hashing APIs Jun 9, 2023
@github-actions github-actions bot added the CMake CMake build issue label Jun 9, 2023
@vyasr
Copy link
Contributor Author

vyasr commented Jun 9, 2023

@PointKernel @davidwendt apologies for the churn on this PR, it's gone through a ton of iterations as I've slowly extracted more and more of the logic into upstream PRs in order to incorporate the changes as cleanly as possible and with the most logical PR structure for reviews (and to avoid merging any stream-related API changes to the main branches until everything upstream is really prepared). This PR is finally ready now, and it's fairly minimal.

PointKernel
PointKernel approved these changes Jun 9, 2023
View reviewed changes
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.

Nice work!

I got my questions answered during today's lunch but it would be great to elaborate a bit on how the stream test works in general in the PR description. This is the beginning of a long list of similar PRs adding stream tests thus the explanation will facilitate the following review work a lot.

cpp/tests/streams/hash_test.cpp Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented Jun 13, 2023

I've added an extended description of the test framework in #13556. Hopefully that helps provide good context to point to for future reviewers, @PointKernel let me know what you think too.

@vyasr @davidwendt
Update cpp/include/cudf/hashing.hpp
5e255e9 
Co-authored-by: David Wendt <45795991+davidwendt@users.noreply.github.com>
@vyasr vyasr requested a review from davidwendt June 13, 2023 16:20
@vyasr
Copy link
Contributor Author

vyasr commented Jun 13, 2023

/merge

@rapids-bot rapids-bot bot merged commit f83c1fd into rapidsai:branch-23.08 Jun 13, 2023
@vyasr vyasr deleted the feature/test_new_streams_api branch June 13, 2023 22:29
This was referenced Jul 18, 2023
Merged
Merged
@vyasr vyasr mentioned this pull request Jul 24, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt approved these changes

@PointKernel PointKernel PointKernel approved these changes

@ttnghia ttnghia ttnghia approved these changes

Assignees

@vyasr vyasr

Labels
3 - Ready for Review Ready for review by team CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
Enable streams
Development

Successfully merging this pull request may close these issues.
4 participants
@vyasr @ttnghia @PointKernel @davidwendt