Skip to content

Conversation

@anjakefala
Copy link
Contributor

@anjakefala anjakefala commented Aug 6, 2024

WIP. Feel free to make any comments while this is under active development!

  • Within Copy, in addition to accepting a MemoryManager as argument to the copy function, we could also accept a Device (see review comments here: GH-42222: [Python] Add bindings for CopyTo on RecordBatch and Array classes #42223)
  • add a test in test_io.py where Buffer is being tested, so there is also coverage of this new function outside of the CUDA tests
  • What is the difference between Copy and CopyNonOwned? Do we need both or should they be in the same method? (We decided to not add bindings for it.)

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions
Copy link

github-actions bot commented Aug 6, 2024

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

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.

Some small comments on the tests. I think it would also be fine for this PR to just focus on adding a copy method, and then this should be more or less ready?

@github-actions github-actions bot added awaiting changes Awaiting changes awaiting review Awaiting review awaiting committer review Awaiting committer review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Sep 5, 2024
@anjakefala anjakefala marked this pull request as ready for review September 6, 2024 18:02
@anjakefala
Copy link
Contributor Author

@github-actions crossbow submit test-cuda-python

@github-actions
Copy link

github-actions bot commented Sep 6, 2024

Revision: a64d54d

Submitted crossbow builds: ursacomputing/crossbow @ actions-b9914c094c

Task Status
test-cuda-python GitHub Actions

Copy link
Member

@danepitkin danepitkin left a comment

Choose a reason for hiding this comment

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

Nice work!

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 12, 2024
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 13, 2024
Copy link
Member

@danepitkin danepitkin left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Do you plan to add the other checkbox items to this PR? If not, then I give it approval 👍

@anjakefala
Copy link
Contributor Author

@danepitkin We decided to move those to a separate PR!

@anjakefala
Copy link
Contributor Author

@github-actions crossbow submit test-cuda-python

@anjakefala anjakefala force-pushed the kef/cuda-buffer-bindings branch from 2e10905 to 9319543 Compare September 19, 2024 19:03
@anjakefala
Copy link
Contributor Author

@github-actions crossbow submit test-cuda-python

@github-actions
Copy link

Unable to match any tasks for `test-cuda-python`
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/10947293941

@danepitkin
Copy link
Member

@github-actions crossbow submit test-cuda-python

@github-actions
Copy link

Unable to match any tasks for `test-cuda-python`
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/10948481926

@pitrou
Copy link
Member

pitrou commented Sep 19, 2024

@github-actions crossbow submit testcudapy*

@github-actions
Copy link

Revision: 9319543

Submitted crossbow builds: ursacomputing/crossbow @ actions-9f9dcb6f39

Task Status
test-cuda-python-ubuntu-22.04-cuda-11.7.1 GitHub Actions

@anjakefala anjakefala requested a review from pitrou September 19, 2024 21:11
@jorisvandenbossche jorisvandenbossche changed the title GH-43589: [Python] Add bindings for additional Buffer class non-CPU methods GH-43589: [Python] Add bindings for Buffer copy() method to other device Sep 20, 2024
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 20, 2024
@jorisvandenbossche
Copy link
Member

@anjakefala added a few more comments on the tests, but for the rest looks good!

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 20, 2024
@jorisvandenbossche
Copy link
Member

@github-actions crossbow submit testcudapy*

@github-actions
Copy link

Revision: 230abf2

Submitted crossbow builds: ursacomputing/crossbow @ actions-b9e1188e21

Task Status
test-cuda-python-ubuntu-22.04-cuda-11.7.1 GitHub Actions

@anjakefala
Copy link
Contributor Author

Is anything else needed before merging? The failed CI seems unrelated.

Parameters
----------
destination : pyarrow.MemoryManager or pyarrow.Device
Copy link
Member

Choose a reason for hiding this comment

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

Not required, but it may be nice to also allow a MemoryPool here.

Copy link
Member

Choose a reason for hiding this comment

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

We don't have a way to map a given MemoryPool to a device manager (AFAIK), so in that case you mean to use a different API under the hood to copy to the memory pool? (Buffer::CopySlice seems to allow that, or first explicitly allocate a new Buffer of the correct size using the memory pool and then copying the data to it?)

Copy link
Member

Choose a reason for hiding this comment

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

You can use

/// \brief Create a MemoryManager
///
/// The returned MemoryManager will use the given MemoryPool for allocations.
static std::shared_ptr<MemoryManager> memory_manager(MemoryPool* pool);

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

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

@github-actions github-actions bot added the Status: stale-warning Issues and PRs flagged as stale which are due to be closed if no indication otherwise label Nov 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changes Awaiting changes Component: Python Status: stale-warning Issues and PRs flagged as stale which are due to be closed if no indication otherwise

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants