Skip to content
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

[REVIEW] Add a public copy API to DeviceBuffer #1128

Merged
merged 10 commits into from
Oct 18, 2022

Conversation

galipremsagar
Copy link
Contributor

Description

This PR adds a public copy API to DeviceBuffer, this is required for the ongoing copy-on-write work in rapidsai/cudf#11718

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@galipremsagar galipremsagar added 3 - Ready for review Ready for review by team non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Oct 10, 2022
@galipremsagar galipremsagar requested a review from a team as a code owner October 10, 2022 16:08
@galipremsagar galipremsagar self-assigned this Oct 10, 2022
@github-actions github-actions bot added the Python Related to RMM Python API label Oct 10, 2022
@galipremsagar galipremsagar added this to PR-WIP in v22.12 Release via automation Oct 10, 2022
@galipremsagar galipremsagar moved this from PR-WIP to PR-Needs review in v22.12 Release Oct 10, 2022
python/rmm/_lib/device_buffer.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/device_buffer.pyx Show resolved Hide resolved
python/rmm/tests/test_rmm.py Outdated Show resolved Hide resolved
@bdice
Copy link
Contributor

bdice commented Oct 11, 2022

It might be worth thinking about implementing or using the __copy__ or __deepcopy__ methods used by Python's copy module. See:

@shwina
Copy link
Contributor

shwina commented Oct 11, 2022

It might be worth thinking about implementing or using the copy or deepcopy methods used by Python's copy module.

In general, the SciPy/PyData ecosystem seems to have shied away from this model, instead providing explicit copy() methods for its types. In practice, I really only ever reach for copy() and deepcopy() for builtin types. I'd be curious if this has been your experience as well, @bdice?

@bdice
Copy link
Contributor

bdice commented Oct 11, 2022

It might be worth thinking about implementing or using the copy or deepcopy methods used by Python's copy module.

In general, the SciPy/PyData ecosystem seems to have shied away from this model, instead providing explicit copy() methods for its types. In practice, I really only ever reach for copy() and deepcopy() for builtin types. I'd be curious if this has been your experience as well, @bdice?

Interestingly, NumPy defines both: https://numpy.org/doc/stable/reference/generated/numpy.ndarray.__deepcopy__.html

I don’t have strong feelings here. Just wanted to mention that that object model exists, in case someone wants to use Python’s copy module to (deep) copy an arbitrary object containing a DeviceBuffer.

@shwina
Copy link
Contributor

shwina commented Oct 11, 2022

I suppose we could follow suit and implement __depcopy__ for people doing something like deepcopy([buf1, buf2]) (in addition to an explicit copy() method).

@galipremsagar
Copy link
Contributor Author

I suppose we could follow suit and implement __depcopy__ for people doing something like deepcopy([buf1, buf2]) (in addition to an explicit copy() method).

Added a __deepcopy__ in addition to copy

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor clarifications about deep/shallow copying before I approve.

python/rmm/tests/test_rmm.py Outdated Show resolved Hide resolved
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

LGTM, one minor question.

v22.12 Release automation moved this from PR-Needs review to PR-Reviewer approved Oct 17, 2022
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for review Ready for review by team labels Oct 17, 2022
@galipremsagar
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit c690822 into rapidsai:branch-22.12 Oct 18, 2022
v22.12 Release automation moved this from PR-Reviewer approved to Done Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to RMM Python API
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants