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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cpp/include/cudf/interop.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,14 @@ namespace CUDF_EXPORT cudf {
* @throw cudf::logic_error if the any of the DLTensor fields are unsupported
*
* @param managed_tensor a 1D or 2D column-major (Fortran order) tensor
* @param stream CUDA stream used for device memory operations and kernel launches
* @param mr Device memory resource used to allocate the returned table's device memory
*
* @return Table with a copy of the tensor data
*/
std::unique_ptr<table> from_dlpack(
DLManagedTensor const* managed_tensor,
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::device_async_resource_ref mr = cudf::get_current_device_resource_ref());

/**
Expand All @@ -79,12 +81,14 @@ std::unique_ptr<table> from_dlpack(
* or if any of columns have non-zero null count
*
* @param input Table to convert to DLPack
* @param stream CUDA stream used for device memory operations and kernel launches
* @param mr Device memory resource used to allocate the returned DLPack tensor's device memory
*
* @return 1D or 2D DLPack tensor with a copy of the table data, or nullptr
*/
DLManagedTensor* to_dlpack(
table_view const& input,
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::device_async_resource_ref mr = cudf::get_current_device_resource_ref());

/** @} */ // end of group
Expand Down
9 changes: 6 additions & 3 deletions cpp/src/interop/dlpack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -297,16 +297,19 @@ DLManagedTensor* to_dlpack(table_view const& input,
} // namespace detail

std::unique_ptr<table> from_dlpack(DLManagedTensor const* managed_tensor,
rmm::cuda_stream_view stream,
rmm::device_async_resource_ref mr)
{
CUDF_FUNC_RANGE();
return detail::from_dlpack(managed_tensor, cudf::get_default_stream(), mr);
return detail::from_dlpack(managed_tensor, stream, mr);
}

DLManagedTensor* to_dlpack(table_view const& input, rmm::device_async_resource_ref mr)
DLManagedTensor* to_dlpack(table_view const& input,
rmm::cuda_stream_view stream,
rmm::device_async_resource_ref mr)
{
CUDF_FUNC_RANGE();
return detail::to_dlpack(input, cudf::get_default_stream(), mr);
return detail::to_dlpack(input, stream, mr);
}

} // namespace cudf
1 change: 1 addition & 0 deletions cpp/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,7 @@ ConfigureTest(STREAM_DICTIONARY_TEST streams/dictionary_test.cpp STREAM_MODE tes
ConfigureTest(STREAM_FILLING_TEST streams/filling_test.cpp STREAM_MODE testing)
ConfigureTest(STREAM_GROUPBY_TEST streams/groupby_test.cpp STREAM_MODE testing)
ConfigureTest(STREAM_HASHING_TEST streams/hash_test.cpp STREAM_MODE testing)
ConfigureTest(STREAM_INTEROP streams/interop_test.cpp STREAM_MODE testing)
ConfigureTest(STREAM_JOIN_TEST streams/join_test.cpp STREAM_MODE testing)
ConfigureTest(STREAM_JSONIO_TEST streams/io/json_test.cpp STREAM_MODE testing)
ConfigureTest(STREAM_LABELING_BINS_TEST streams/labeling_bins_test.cpp STREAM_MODE testing)
Expand Down
46 changes: 46 additions & 0 deletions cpp/tests/streams/interop_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* Copyright (c) 2024, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include <cudf_test/base_fixture.hpp>
#include <cudf_test/column_wrapper.hpp>
#include <cudf_test/default_stream.hpp>

#include <cudf/interop.hpp>
#include <cudf/table/table_view.hpp>

#include <dlpack/dlpack.h>

struct dlpack_deleter {
void operator()(DLManagedTensor* tensor) { tensor->deleter(tensor); }
};

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.

{
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.

}

TEST_F(DLPackTest, FromDLPack)
{
using unique_managed_tensor = std::unique_ptr<DLManagedTensor, dlpack_deleter>;
cudf::test::fixed_width_column_wrapper<int32_t> col1({});
cudf::test::fixed_width_column_wrapper<int32_t> col2({});
cudf::table_view input({col1, col2});
unique_managed_tensor tensor(cudf::to_dlpack(input, cudf::test::get_default_stream()));
auto result = cudf::from_dlpack(tensor.get(), cudf::test::get_default_stream());
}
Loading