GH-40078: [C++] Import/Export ArrowDeviceArrayStream by zeroshade · Pull Request #40807 · apache/arrow

zeroshade · 2024-03-26T22:31:52Z

Rationale for this change

The original PRs for adding support for importing and exporting the new C Device interface (#36488 / #36489) only added support for the Arrays themselves, not for the stream structure. We should support both.

What changes are included in this PR?

Adding parallel functions for Import/Export of streams that accept ArrowDeviceArrayStream.

Are these changes tested?

Test writing in progress, wanted to get this up for review while I write tests.

Are there any user-facing changes?

No, only new functions have been added.

GitHub Issue: [C++] Import/Export ArrowDeviceArrayStream #40078

github-actions · 2024-03-26T22:32:21Z

⚠️ GitHub issue #40078 has been automatically assigned in GitHub to PR creator.

paleolimbot

I'm excited this will work for ChunkedArray as well!

jorisvandenbossche

Thanks for working on this!

jorisvandenbossche · 2024-03-28T21:06:08Z

I don't fully understand this part. AFAIK a RecordBatch currently is agnostic the device of its buffers. So for example you can have a RecordBatch backed by buffers that live in CUDA memory, but then this method will always hardcoded return NULL, which is not correct in that case?

Yea, at the moment I don't have an implementation there. I put a note on the ImportDeviceRecordBatchReader documentation if you look at bridge.h:

/// We are not yet bubbling the sync events from the buffers up to /// the `GetSyncEvent` method of an imported RecordBatch. This will be added in a future /// update.

Is the idea that longer term there would be a generic implementation here in RecordBatch that checks the sync events of its underlying buffers? Because in practice, we don't subclass RecordBatch in Arrow C++ to add CUDA support, so that method cannot be overriden

It was either that or introducing a subclass, yes.

Okay, I've propagated the event and device_type throughout the record batch and the Make functions (defaulting it to nullptr) which should allow us to ensure this is all correct now. I'll update the documentation comments accordingly

jorisvandenbossche · 2024-03-28T21:11:39Z

Potentially related to my other comment, but the existing ExportDeviceArray has a sync keyword that the user of this API needs to provide. Is there a reason those methods don't have that?
The returned ArrowDeviceArrayStream itself doesn't have a sync event member, but the ArrowDeviceArrays that it will return still have that. The user shouldn't pass the sync event to set in those arrays?

This is why I put the GetSyncEvent on the RecordBatch object since there could potentially be a different sync event for each record batch returned by the reader. Right now it currently is hardcoded to return null, but the GetSyncEvent function is virtual, so someone could potentially have a RecordBatchReader that returns a class which inherits from RecordBatch that implements GetSyncEvent to return the correct event object etc.

Would it make sense to add GetSyncEvent() when it actually does something? It seems like this is perhaps guessing at a future API that we don't know will exist yet or that we are not sure will be used?

I've made a bunch of changes propagating the sync event and the device type now, so GetSyncEvent now does something :)

Thoughts?

felipecrv

LGTM. Seems to match the structure of the single array x non-stream case.

jorisvandenbossche · 2024-04-04T06:10:04Z

This is going to be wrong if we import a RecordBatchReader from a non-CPU ArrowDeviceArrayStream? Or is that right now not yet possible?

As of yet, I haven't implemented figuring out what type to send here. Since we aren't storing the device type at the record batch level, this would need to spin through the buffers of its columns which could potentially be from multiple devices if a user did something weird. So I'm not sure what the best solution here is. I'd rather not add a RecordBatch level device_type member, but I don't yet know how to best handle the situation if a consumer does something bad

It seems like for now this will have to be supplied by the caller on construction? That seems like a better intermediate state than returning a value that could lead to segfaults?

Well, this is an abstract class, so there's no constructor here :-)

So the suggestion is we should add a device_type member to the RecordBatch class and have it be provided?

I've propagated the device_type throughout the readers now and to the static Make methods for constructing the readers, so now this should be correct.

jorisvandenbossche · 2024-04-04T06:10:16Z

Needs some tests?

pitrou · 2024-04-04T15:50:17Z

Can we instead add the necessary declarations to type_fwd.h?

We would have to move the nested Device::SyncEvent outside of the Device class to do that since you can't forward declare an inner class like that. It would also require pushing the entire DeviceAllocationType declaration to type_fwd.h which I'm not sure if we want to do. So I don't think we can do this.

I think at least moving DeviceAllocationType to type_fwd.h would make sense.

As for Device::SyncEvent, that's indeed a problem with nested classes (and a good reason to avoid them :-)). That could be fixed by moving SyncEvent out of Device and adding a compatibility alias.

I think at least moving DeviceAllocationType to type_fwd.h would make sense.

I've done that here #43853

Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

conbench-apache-arrow · 2024-05-22T03:06:05Z

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 8169d6e.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

pitrou · 2024-05-23T12:52:55Z

+  int type = 0;
+  for (const auto& buf : buffers) {
+    if (!buf) continue;
+    if (type == 0) {


This condition implies that, conversely, in non-debug mode we could immediately return when we encounter a buffer? Instead of continue looping on all buffers and children...

pitrou · 2024-05-23T12:53:56Z

  /// \see GetNullCount
  int64_t ComputeLogicalNullCount() const;

+  /// \brief Returns the device_type of the underlying buffers and children


Nit, but we tend to use infinitives in docstring ("return", not "returns")

zeroshade requested review from bkietz, jorisvandenbossche, paleolimbot and pitrou March 26, 2024 22:31

github-actions Bot added Component: C++ awaiting committer review Awaiting committer review labels Mar 26, 2024

paleolimbot reviewed Mar 27, 2024

View reviewed changes

Comment thread cpp/src/arrow/c/bridge.cc Outdated

Comment thread cpp/src/arrow/c/bridge.h Outdated

Comment thread cpp/src/arrow/c/helpers.h Outdated

Comment thread cpp/src/arrow/record_batch.h Outdated

github-actions Bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels Mar 27, 2024

zeroshade force-pushed the import-export-device-stream branch from deb9fa3 to 37e93b0 Compare March 27, 2024 16:43

github-actions Bot added awaiting changes Awaiting changes Component: Python awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Mar 27, 2024

jorisvandenbossche reviewed Mar 28, 2024

View reviewed changes

felipecrv reviewed Apr 4, 2024

View reviewed changes

Comment thread cpp/src/arrow/c/helpers.h Outdated

Comment thread cpp/src/arrow/c/bridge.cc Outdated

jorisvandenbossche reviewed Apr 4, 2024

View reviewed changes

paleolimbot reviewed Apr 4, 2024

View reviewed changes

Comment thread cpp/src/arrow/c/helpers.h Outdated

pitrou reviewed Apr 4, 2024

View reviewed changes

Comment thread cpp/src/arrow/record_batch.h Outdated

pitrou reviewed Apr 4, 2024

View reviewed changes

Comment thread cpp/src/arrow/record_batch.h Outdated

pitrou reviewed Apr 4, 2024

View reviewed changes

Comment thread cpp/src/arrow/record_batch.h Outdated

pitrou reviewed Apr 4, 2024

View reviewed changes

Comment thread cpp/src/arrow/record_batch.h Outdated

zeroshade and others added 24 commits May 9, 2024 14:41

added tests

fcd8dc9

fix lint

dca4503

fix docs

3211ef3

more doxygen fixes

ff521ab

updates from feeback

aeff09f

add comment about device types

35ef670

lint

8443150

Update cpp/src/arrow/c/bridge.h

406ecf1

Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

update from feedback

c22768a

linting

bca54e2

use static const null_sync_event

d5a6332

clean up templates

71e152c

use overload instead of if constexpr

c9e63fd

linting

df40625

Update cpp/src/arrow/c/bridge.cc

9284893

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

Update cpp/src/arrow/c/bridge.cc

915b723

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

Update cpp/src/arrow/c/bridge_test.cc

46f436e

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

Update cpp/src/arrow/c/bridge_test.cc

0a07bee

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

updates from feedback and lint

7b89fbf

fix the python failure

4915884

lint

3dc2d78

add debug check to confirm device_type in ArrayData constructor

331ce10

fix lint

9b14645

add a null check for children in device_type

381bc7d

jorisvandenbossche mentioned this pull request May 15, 2024

[Python] Ensure (Chunked)Array/RecordBatch/Table methods don't crash with non-CPU data #41665

Closed

4 tasks

zeroshade mentioned this pull request May 21, 2024

[C++] Import/Export ArrowDeviceArrayStream #40078

Closed

pitrou reviewed May 23, 2024

View reviewed changes

zeroshade mentioned this pull request May 24, 2024

MINOR: [C++] Slight improvement for ArrayData device_type #41814

Merged

Uh oh!

Conversation

zeroshade commented Mar 26, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Mar 26, 2024

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipecrv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche commented Apr 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

conbench-apache-arrow Bot commented May 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

zeroshade commented Mar 26, 2024 •

edited by github-actions Bot

Loading