GH-33984: [C++][Python] DLPack implementation for Arrow Arrays (producer) #38472

AlenkaF · 2023-10-26T12:43:03Z

Rationale for this change

DLPack is selected for Array API protocol so it is important to have it implemented for Arrow/PyArrow Arrays also. This is possible for primitive type arrays (int, uint and float) with no validity buffer. Device support is not in scope of this PR (CPU only).

What changes are included in this PR?

ExportArray and ExportDevice methods on Arrow C++ Arrays
__dlpack__ method on the base PyArrow Array class exposing ExportArray method
__dlpack_device__ method on the base PyArrow Array class exposing ExportDevice method

Are these changes tested?

Yes, tests are added to dlpack_test.cc and test_array.py.

Are there any user-facing changes?

No.

Closes: [C++][Python] Implementation of the dlpack #33984

AlenkaF · 2023-10-26T13:44:27Z

cc @rok: this is an initial draft for the dlpack support. Will still need to create a separate method for fixed shape tensor array plus add more tests (inspecting the produced PyCapsule with cffi).

python/pyarrow/_dlpack.pxi

cpp/src/arrow/dlpack.cc

cpp/src/arrow/dlpack.h

cpp/src/arrow/dlpack.cc

python/pyarrow/_dlpack.pxi

python/pyarrow/array.pxi

python/pyarrow/includes/libarrow.pxd

python/pyarrow/tests/test_array.py

jorisvandenbossche · 2023-11-22T14:17:15Z

TODO: C++ tests, tests with arrays with offsets

kkraus14 · 2023-11-22T15:26:50Z

cpp/src/arrow/dlpack.cc

What happens if someone hands an array backed by CudaBuffer buffers?

I think we need to validate these are CPU buffers, or better yet support the cases where we have non-CPU buffers.

This makes sense yes. We planned to support only CPU device at first. But I should not hardcode it like I did but validate they they are in fact CPU buffers and raise if not.

Added a check for CPU device: 6c886fd
It would be easy to add support for other cases in a follow-up I think. Not sure about the tests though =)

cpp/src/arrow/dlpack.cc

cpp/src/arrow/dlpack.h

cpp/src/arrow/c/dlpack.cc

…change for loop

AlenkaF · 2023-12-12T13:39:48Z

@pitrou thank you so much for the reviews! I addressed all your comments so far.

kkraus14 · 2023-12-13T00:43:44Z

cpp/src/arrow/c/dlpack.cc

+  // Create ManagerCtx with the reference to
+  // the data of the array
+  std::shared_ptr<ArrayData> array_ref = arr->data();
+  std::unique_ptr<ManagerCtx> ctx(new ManagerCtx);


nitpick: I think it's best practice to use std::make_unique instead of wrapping new call. Any reason we can't use make_unique here?

Happy to change to make_unique. @pitrou any thoughts?

Given that we have the explicit delete as well, I personally like seeing the new+delete combo explicitly, even though make_unique is exactly equivalent AFAIU

Unless that delete call can then be replaced with a unique_ptr reset() ?

cpp/src/arrow/c/dlpack.cc

AlenkaF · 2023-12-13T09:00:54Z

@kkraus14 I am double-checking if I am correct in assuming DLpack only supports byte-packed booleans. Or are bit-packed booleans also supported but not really used?

I am not sure what came out of the discussion in dmlc/dlpack#75 and I am finding the docs unclear:
example in the docs suggest underlying storage size of bool is 8 bits, but there is no specific info about bool1.

Will be happy to hear your thoughts on this.

AlenkaF · 2023-12-18T07:25:48Z

@pitrou would you have some time to run through this PR again?

pitrou

+1 after a couple minor cleanups. Thank you @AlenkaF !

pitrou · 2023-12-19T17:38:01Z

@github-actions crossbow submit -g cpp

pitrou · 2023-12-19T17:38:10Z

@github-actions crossbow submit cuda

github-actions · 2023-12-19T17:40:26Z

Revision: 49a978f

Submitted crossbow builds: ursacomputing/crossbow @ actions-bbd691007d

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

github-actions · 2023-12-19T17:40:28Z

Revision: 49a978f

Submitted crossbow builds: ursacomputing/crossbow @ actions-b8525f4884

Task	Status
conda-linux-aarch64-cuda-py3
conda-linux-ppc64le-cuda-py3
conda-linux-x64-cuda-py3
conda-win-x64-cuda-py3
test-cuda-cpp
test-cuda-python

AlenkaF · 2023-12-19T19:03:31Z

Thank you @pitrou for all the suggestions and help!
Have to add this was a very nice way to learn some C++ ❤️

Listing possible follow-up issues:

conbench-apache-arrow · 2023-12-20T13:34:13Z

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

There was 1 benchmark result indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-20 02:11:37Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-05, scale_factor=1

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

…(producer) (apache#38472) ### Rationale for this change DLPack is selected for Array API protocol so it is important to have it implemented for Arrow/PyArrow Arrays also. This is possible for primitive type arrays (int, uint and float) with no validity buffer. Device support is not in scope of this PR (CPU only). ### What changes are included in this PR? - `ExportArray` and `ExportDevice` methods on Arrow C++ Arrays - `__dlpack__` method on the base PyArrow Array class exposing `ExportArray` method - `__dlpack_device__` method on the base PyArrow Array class exposing `ExportDevice` method ### Are these changes tested? Yes, tests are added to `dlpack_test.cc` and `test_array.py`. ### Are there any user-facing changes? No. * Closes: apache#33984 Lead-authored-by: AlenkaF <frim.alenka@gmail.com> Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

github-actions bot added Component: Python awaiting review Awaiting review labels Oct 26, 2023

AlenkaF commented Oct 26, 2023

View reviewed changes

python/pyarrow/_dlpack.pxi Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 26, 2023

pitrou reviewed Oct 26, 2023

View reviewed changes

python/pyarrow/_dlpack.pxi Outdated Show resolved Hide resolved

pitrou reviewed Oct 26, 2023

View reviewed changes

python/pyarrow/_dlpack.pxi Outdated Show resolved Hide resolved

pitrou reviewed Oct 26, 2023

View reviewed changes

python/pyarrow/_dlpack.pxi Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 27, 2023

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 8, 2023

github-actions bot added the Component: C++ label Nov 16, 2023

jorisvandenbossche reviewed Nov 22, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting review Awaiting review awaiting committer review Awaiting committer review and removed awaiting change review Awaiting change review awaiting review Awaiting review awaiting changes Awaiting changes labels Nov 22, 2023

kkraus14 reviewed Nov 22, 2023

View reviewed changes

pitrou reviewed Nov 22, 2023

View reviewed changes

cpp/src/arrow/dlpack.h Outdated Show resolved Hide resolved

pitrou reviewed Nov 22, 2023

View reviewed changes

cpp/src/arrow/dlpack.h Outdated Show resolved Hide resolved

AlenkaF mentioned this pull request Nov 23, 2023

[C++][Python] DLPack on FixedShapeTensorArray/FixedShapeTensorScalar #38868

Open

jorisvandenbossche reviewed Nov 28, 2023

View reviewed changes

cpp/src/arrow/c/dlpack.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels Nov 28, 2023

AlenkaF added 6 commits December 12, 2023 06:17

dlpack.cc - Add unnamed namespace and optimize code in ExportDevice

15ead8f

dlpack_test.cc - remove gen, add a check for memory deallocation and …

5e87138

…change for loop

_dlpack.pxi - remove unsued imports

2160ecd

array.pxi - simplify the code and change docstrings

8bb7173

test_dlpack.py - add a check for memory deallocation

9389941

Change docstrings in __dlpack_device__

94dec4b

kkraus14 approved these changes Dec 13, 2023

View reviewed changes

Add null type empty array to C++ tests

811d2b5

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Dec 13, 2023

AlenkaF changed the title ~~GH-33984: [Python] __dlpack__ implementation (producer)~~ GH-33984: [C++][Python] DLPack implementation for Arrow Arrays (producer) Dec 19, 2023

Nits and cleanups

49a978f

pitrou approved these changes Dec 19, 2023

View reviewed changes

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Dec 19, 2023

pitrou merged commit 6c326db into apache:main Dec 19, 2023

pitrou removed the awaiting change review Awaiting change review label Dec 19, 2023

AlenkaF deleted the gh-33984-dlpack-producer branch December 19, 2023 18:59

AlenkaF mentioned this pull request May 15, 2024

[C++][Python] Update DLPack version #41670

Open

GH-33984: [C++][Python] DLPack implementation for Arrow Arrays (producer) #38472

GH-33984: [C++][Python] DLPack implementation for Arrow Arrays (producer) #38472

Uh oh!

Conversation

AlenkaF commented Oct 26, 2023 • edited 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!

AlenkaF commented Oct 26, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jorisvandenbossche commented Nov 22, 2023

Uh oh!

kkraus14 Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

AlenkaF Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

AlenkaF Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlenkaF commented Dec 12, 2023

Uh oh!

kkraus14 Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

AlenkaF Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlenkaF commented Dec 13, 2023

Uh oh!

AlenkaF commented Dec 18, 2023

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Dec 19, 2023

Uh oh!

pitrou commented Dec 19, 2023

Uh oh!

github-actions bot commented Dec 19, 2023

Uh oh!

github-actions bot commented Dec 19, 2023

Uh oh!

AlenkaF commented Dec 19, 2023

Uh oh!

conbench-apache-arrow bot commented Dec 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

AlenkaF commented Oct 26, 2023 •

edited

Loading