-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat(python): Clarify interaction between the CDeviceArray, the CArrayView, and the CArray #409
Changes from 10 commits
feada27
e940caa
0d8bbe9
569618f
10f167b
414bbc4
b6e76d3
0a96880
83349ac
6aa1cac
25a8dfc
0c88a30
9c4f22d
5c1b424
306f543
7b798a3
10fbcc5
ebcbd5e
4ce9e8b
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 |
---|---|---|
|
@@ -427,7 +427,7 @@ def c_array_view(obj, schema=None) -> CArrayView: | |
if isinstance(obj, CArrayView) and schema is None: | ||
return obj | ||
|
||
return CArrayView.from_array(c_array(obj, schema)) | ||
return c_array(obj, schema).view() | ||
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. Very cool! |
||
|
||
|
||
def c_buffer(obj, schema=None) -> CBuffer: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,10 +17,9 @@ | |
|
||
import pytest | ||
|
||
import nanoarrow as na | ||
from nanoarrow import device | ||
|
||
pa = pytest.importorskip("pyarrow") | ||
|
||
|
||
def test_cpu_device(): | ||
cpu = device.cpu() | ||
|
@@ -31,12 +30,51 @@ def test_cpu_device(): | |
cpu = device.resolve(1, 0) | ||
assert cpu.device_type == 1 | ||
|
||
pa_array = pa.array([1, 2, 3]) | ||
|
||
darray = device.c_device_array(pa_array) | ||
def test_c_device_array(): | ||
# Unrecognized arguments should be passed to c_array() to generate CPU array | ||
darray = device.c_device_array([1, 2, 3], na.int32()) | ||
|
||
assert darray.device_type == 1 | ||
assert darray.device_id == 0 | ||
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. Out of curiosity, are there enums that can be used here instead of values 1, 0? Or do we need to wait for DLPack support. |
||
assert darray.array.length == 3 | ||
assert "device_type: 1" in repr(darray) | ||
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. If there are enums, it would be nice to print the name (e.g. 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. This was a bit of a rabbit hole but a very good rabbit hole. There's no enum, but there is an ABI-stable set of defines that I turned into one and the result is much better! |
||
|
||
assert darray.schema.format == "i" | ||
|
||
assert darray.array.length == 3 | ||
assert darray.array.device_type == device.cpu().device_type | ||
assert darray.array.device_id == device.cpu().device_id | ||
|
||
darray_view = darray.view() | ||
assert darray_view.length == 3 | ||
assert list(darray_view.buffer(1)) == [1, 2, 3] | ||
|
||
# A CDeviceArray should be returned as is | ||
assert device.c_device_array(darray) is darray | ||
|
||
# A CPU device array should be able to export to a regular array | ||
array = na.c_array(darray) | ||
assert array.schema.format == "i" | ||
assert array.buffers == darray.array.buffers | ||
|
||
|
||
def test_c_device_array_protocol(): | ||
# Wrapper to prevent c_device_array() from returning early when it detects the | ||
# input is already a CDeviceArray | ||
class CDeviceArrayWrapper: | ||
def __init__(self, obj): | ||
self.obj = obj | ||
|
||
def __arrow_c_device_array__(self, requested_schema=None): | ||
return self.obj.__arrow_c_device_array__(requested_schema=requested_schema) | ||
|
||
darray = device.c_device_array([1, 2, 3], na.int32()) | ||
wrapper = CDeviceArrayWrapper(darray) | ||
|
||
darray2 = device.c_device_array(wrapper) | ||
assert darray2.schema.format == "i" | ||
assert darray2.array.length == 3 | ||
assert darray2.array.buffers == darray.array.buffers | ||
|
||
with pytest.raises(NotImplementedError): | ||
device.c_device_array(wrapper, na.int64()) |
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.
This function is called frequently after initialization. Is it worth allowing
__cinit__
to set device type/id?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.
I would like to move away from any arguments in
__cinit__
in most cases because a user could theoretically callnanoarrow.CArray(...)
and get very strange errors. They should really all beClassName._construct()
or something (but maybe in a future PR).