Use owning Arrow types in C++ to expose data to Python - #18402
Merged
rapids-bot[bot] merged 46 commits intoApr 16, 2025
Merged
Conversation
…and use that to fix invalid test
This reverts commit 2d7ea5de01d141a6650068c4c3a513e2bc67b3c8.
… use those to construct gpumemoryviews safely
vyasr
added
libcudf
bdice
approved these changes
Apr 10, 2025
bdice
left a comment
bdice
left a comment
Contributor
There was a problem hiding this comment.
Packaging and C++ look good. I skimmed Python/Cython changes too.
shrshi
approved these changes
Apr 10, 2025
shrshi
left a comment
shrshi
left a comment
Contributor
There was a problem hiding this comment.
A couple of nits in the C++ code, but LGTM otherwise!
3 tasks
Matt711
approved these changes
Apr 10, 2025
Matt711
left a comment
Matt711
left a comment
Member
There was a problem hiding this comment.
Just non-blocking suggestions, thanks!
| @@ -0,0 +1,9 @@ | |||
| # Copyright (c) 2025, NVIDIA CORPORATION. | |||
Member
There was a problem hiding this comment.
I think were missing type stubs _interop_helpers.pyi. Is that intentional?
Contributor
Author
There was a problem hiding this comment.
Yes, because this file pretty much exclusively exposes pure Cython functions. There is nothing visible to Python except the ColumnMetadata type, but that is (for the moment) primarily publicly exposed via the interop module that already has the type stubs for that.
| NANOARROW_RETURN_NOT_OK(set_contents(child_contents, tmp->children[i])); | ||
| } else { | ||
| NANOARROW_RETURN_NOT_OK(cudf::type_dispatcher( | ||
| child->type(), dispatch_to_arrow_device{}, std::move(*child), stream, mr, child_ptr)); |
Contributor
There was a problem hiding this comment.
I'm a bit paranoid on this: Do we have a guarantee that the initialization param_2 = std::move(*child) happens before param_0 = child->type()? If not, we may rely on the compiler's unspecified behavior for correctness.
Contributor
There was a problem hiding this comment.
cudf::type_dispatcher passes std::move(*child) with a forwarding reference, so the move has not happened yet when the function is entered, whereas param_0 = child->type() is sequenced before the function is entered. So perhaps it is fine.
Contributor
Author
There was a problem hiding this comment.
I believe that we are safe because while we may not be guaranteed anything about the order in which the two parameters are evaluated, we are guaranteed that all of the parameters are evaluated before the function call begins. std::move doesn't actually do anything, it's just a cast to an rvalue that then allows the object to be modified. Therefore, even if we pass an rvalue ref to *child into the type_dispatcher (which forwards it along to the dispatch_to_arrow_device functor), we are guaranteed that param1 = child->type() is evaluated before *child is actually modified in any way that could evaluate it.
kingcrimsontianyu
approved these changes
Apr 14, 2025
kingcrimsontianyu
left a comment
kingcrimsontianyu
left a comment
Contributor
There was a problem hiding this comment.
The C++ part looks good to me. Left one small suggestion, and one small question to confirm.
…data_structures_python
…data_structures_python
Contributor
Author
|
/merge |
Contributor
Author
|
I forgot to post these benchmarks earlier, so posting for posterity (tl;dr this PR did not affect performance): BeforeAfter |
Open
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR leverages #18084 to rework the Python layer of Arrow interchange. With this change, we can now expose the Arrow capsule interfaces for pylibcudf Columns and Tables. This PR also paves the way for exposing the device capsules, which will allow us to provide zero-copy Arrow views into pylibcudf objects.
To get everything working, this PR also makes some ancillary changes:
from_arrowbenchmark.Checklist