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

Expose stream-ordering to interop APIs #17397

Merged
merged 5 commits into from
Nov 21, 2024

Conversation

shrshi
Copy link
Contributor

@shrshi shrshi commented Nov 21, 2024

Description

Adds stream parameter to

cudf::from_dlpack
cudf::to_dlpack

Added stream gtests to verify correct stream forwarding.

Reference: #13744

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 libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Nov 21, 2024
@shrshi shrshi added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 21, 2024
@shrshi shrshi marked this pull request as ready for review November 21, 2024 09:18
@shrshi shrshi requested a review from a team as a code owner November 21, 2024 09:18

struct DLPackTest : public cudf::test::BaseFixture {};

TEST_F(DLPackTest, ToDLPack)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these two tests contain assertions, such as EXPECT_NO_THROW ?
Irrelevant to this PR, I noticed that many other tests out there under cpp/tests/streams don't have assertions. I'm curious if there's any special consideration for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stream tests only verify if the stream is being forwarded correctly. We do not check functional correctness here; the test tracks the test stream passed and verifies that it is used for all device operations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I guess my question is what would be a signal of problem for these tests. Say I artificially introduced a bug where one device operation is on a stream separate from others. Do we expect cudf::to_dlpack() to error out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, I misunderstood what you were asking. Yes, if we use a stream other than cudf::test::get_default_stream() for a device operation anywhere in the test, a runtime error is thrown. CUDA APIs are overloaded to check the passed stream and either raise an exception for an unexpected stream, or forward arguments to the original function.
Reference:

throw std::runtime_error("cudf_identify_stream_usage found unexpected stream!");

TLDR: If we invoke cudf::to_dlpack(empty, cudf::get_default_stream()) in this test, a runtime error is thrown.

TEST_F(DLPackTest, ToDLPack)
{
cudf::table_view empty(std::vector<cudf::column_view>{});
cudf::to_dlpack(empty, cudf::test::get_default_stream());
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to (also) use a non-default stream for testing purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, stream testing here depends on the compile-time option STREAM_MODE_TESTING. When it is defined, cudf::test::get_default_stream() returns a non-default stream and the test throws for CUDA calls being invoked on any stream other than cudf::test::get_default_stream.
Reference:

rmm::cuda_stream_view const get_default_stream()

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I learned something new today.
In this case, I still think EXPECT/ASSERT_NO_THROW is a good practice as opposed to let the application itself throw, but that's a nit for this PR.

@shrshi
Copy link
Contributor Author

shrshi commented Nov 21, 2024

/merge

@rapids-bot rapids-bot bot merged commit 78db66b into rapidsai:branch-25.02 Nov 21, 2024
104 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake CMake build issue 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.

3 participants