-
Notifications
You must be signed in to change notification settings - Fork 915
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
Changes from all commits
c55fb0a
028b06e
3ba2750
e63b9ab
75df6a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) | ||||
{ | ||||
cudf::table_view empty(std::vector<cudf::column_view>{}); | ||||
cudf::to_dlpack(empty, cudf::test::get_default_stream()); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question, stream testing here depends on the compile-time option
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I learned something new today. |
||||
} | ||||
|
||||
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()); | ||||
} |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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:
cudf/cpp/tests/utilities/identify_stream_usage.cpp
Line 135 in f54c1a5
TLDR: If we invoke
cudf::to_dlpack(empty, cudf::get_default_stream())
in this test, a runtime error is thrown.