Skip to content

Conversation

@danepitkin
Copy link
Member

@danepitkin danepitkin commented Aug 22, 2024

Rationale for this change

ChunkedArrays that are backed by non-cpu memory should not segfault when the user invokes an incompatible API.

What changes are included in this PR?

  • Add IsCpu() to ChunkedArray
  • Throw a python exception for known incompatible APIs on non-cpu device

Are these changes tested?

Unit tests

Are there any user-facing changes?

The user should no longer see segfaults for certain APIs, just python exceptions.

@danepitkin danepitkin marked this pull request as draft August 22, 2024 21:27
@github-actions github-actions bot added the awaiting review Awaiting review label Aug 22, 2024
@github-actions
Copy link

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider not caching this piece of information as state in the ChunkedArray instance and instead derive it from the chunks when we need it.

Additionally, one advantage of chunking is the flexibility that it brings regarding allocation of buffers (they don't have to be contiguous), so now requiring that all chunks be allocated on the same device seems too rigid.

I proposed a solution to this: chunked arrays producing a DeviceAllocationTypeSet with all the allocation types of the chunks. This set can be represented by a single 64-bit word in memory (I used C++ <bitset>) so it can be copied and matches very efficiently.

Here is the draft PR: https://github.com/apache/arrow/pull/43542/files#diff-b4ffb36b29cfaa2cf9be4fab774921b8344efdc595a358b02c3187ba04141f7eR89

Copy link
Member Author

@danepitkin danepitkin Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your PR! I think this would work great for PyArrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could move this "caching" to the Python side for now (if we don't want to do this in C++, which I think is certainly fine), or otherwise wait on your PR #43542 to land.
(and we should maybe still consider caching the DeviceAllocationTypeSet result? It might be cheap to calculcate in C++, but we still call this before every call of many methods on the python object)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, didn't see that the PR was already updated in the meantime :)

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Aug 22, 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An is_cpu predicate on chunked arrays can be defined without us forcing a single device type for chunked arrays. This would unblock the Python checks without ruling out the possibility of arrays with mixed device allocations.

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 23, 2024
Copy link
Contributor

@felipecrv felipecrv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C++ part looks good to me.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Aug 23, 2024
@danepitkin danepitkin marked this pull request as ready for review August 23, 2024 21:06
@danepitkin
Copy link
Member Author

@github-actions crossbow submit test-cuda-python

@github-actions
Copy link

Revision: 06dfe493466961225babc34d90e48ce17eadf970

Submitted crossbow builds: ursacomputing/crossbow @ actions-291bb70866

Task Status
test-cuda-python GitHub Actions

@danepitkin
Copy link
Member Author

@github-actions crossbow submit test-cuda-python

@github-actions
Copy link

Revision: e5bf77396ee1d63a1c88ed143caed8550d75093f

Submitted crossbow builds: ursacomputing/crossbow @ actions-92f2f949c3

Task Status
test-cuda-python GitHub Actions

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Aug 26, 2024
@danepitkin
Copy link
Member Author

@github-actions crossbow submit test-cuda-python

@github-actions
Copy link

Revision: 739f2d70a40e1956ceac7fb496d6b313e612bab7

Submitted crossbow builds: ursacomputing/crossbow @ actions-b7a5f6c953

Task Status
test-cuda-python GitHub Actions

@danepitkin
Copy link
Member Author

@github-actions crossbow submit test-cuda-python

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 26, 2024
@github-actions
Copy link

Revision: 0ac2ca4548d3484e3fdee44a18475c938cc8aa50

Submitted crossbow builds: ursacomputing/crossbow @ actions-41d0c41240

Task Status
test-cuda-python GitHub Actions

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@github-actions github-actions bot removed the awaiting change review Awaiting change review label Aug 27, 2024
@jorisvandenbossche jorisvandenbossche changed the title GH-43728:[Python] ChunkedArray fails gracefully on non-cpu devices GH-43728: [Python] ChunkedArray fails gracefully on non-cpu devices Sep 4, 2024
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if self._init_is_cpu == False:
if not self._init_is_cpu:

@jorisvandenbossche
Copy link
Member

Need to fix some conflicts now I merged the other one

@danepitkin danepitkin force-pushed the danepitkin/chunked-array-device-type branch from 8a0f28c to a1d857a Compare September 4, 2024 14:25
@danepitkin
Copy link
Member Author

@github-actions crossbow submit test-cuda-python

@github-actions
Copy link

github-actions bot commented Sep 4, 2024

Revision: a1d857a

Submitted crossbow builds: ursacomputing/crossbow @ actions-c503d748a8

Task Status
test-cuda-python GitHub Actions

@conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

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

khwilson pushed a commit to khwilson/arrow that referenced this pull request Sep 14, 2024
…ices (apache#43795)

### Rationale for this change

ChunkedArrays that are backed by non-cpu memory should not segfault when the user invokes an incompatible API.

### What changes are included in this PR?

* Add IsCpu() to ChunkedArray
* Throw a python exception for known incompatible APIs on non-cpu device

### Are these changes tested?

Unit tests

### Are there any user-facing changes?

The user should no longer see segfaults for certain APIs, just python exceptions.
* GitHub Issue: apache#43728

Authored-by: Dane Pitkin <dpitkin@apache.org>
Signed-off-by: Dane Pitkin <dpitkin@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants