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.

Sign up for GitHub

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

Jump to bottom

Remove CumlArray.copy() #4958

Merged
merged 2 commits into from
Nov 1, 2022
Merged

Remove CumlArray.copy() #4958

merged 2 commits into from
Nov 1, 2022

Conversation

madsbk
Copy link
Member

@madsbk madsbk commented Oct 27, 2022 •
edited

cuDF's Buffer doesn't have Buffer.empty() since rapidsai/cudf#11447. Can we remove CumlArray.copy()?

@madsbk madsbk requested a review from a team as a code owner October 27, 2022 17:44
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Oct 27, 2022
@madsbk madsbk changed the title Fix CumlArray.copy() Removing CumlArray.copy() Oct 27, 2022
@madsbk madsbk changed the title Removing CumlArray.copy() Remove CumlArray.copy() Oct 27, 2022
@codecov-commenter
Copy link

Codecov Report

Base: 79.46% // Head: 79.51% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (29943e0) compared to base (921b5ee).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@               Coverage Diff                @@
##           branch-22.12    #4958      +/-   ##
================================================
+ Coverage         79.46%   79.51%   +0.04%     
================================================
  Files               183      183              
  Lines             11618    11612       -6     
================================================
+ Hits               9232     9233       +1     
+ Misses             2386     2379       -7
Flag Coverage Δ
dask 46.10% <ø> (+<0.01%) ⬆️
non-dask 68.97% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/cuml/common/array.py 97.19% <ø> (+2.08%) ⬆️
python/cuml/tsa/batched_lbfgs.py 73.33% <0.00%> (+3.33%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@wphicks wphicks added non-breaking Non-breaking change feature request New feature or request improvement Improvement / enhancement to an existing function and removed feature request New feature or request labels Oct 28, 2022
wphicks
wphicks approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

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

I'm definitely happy to see this go. I'm going to leave it for just a bit longer before merging to make sure there are no other objections, considering this touches such a core feature.

@beckernick
Copy link
Member

cc @lixfz for awareness, as I know you do some instance checking with cumlarray in https://github.com/DataCanvasIO/Hypernets (but don't see any copy() calls)

@wphicks wphicks added breaking Breaking change and removed non-breaking Non-breaking change labels Oct 28, 2022
dantegd
dantegd requested changes Oct 28, 2022
View reviewed changes
python/cuml/common/array.py
@@ -349,19 +349,6 @@ def serialize(self) -> Tuple[dict, list]:
frames = [CumlArray(f) for f in frames]
return header, frames

@nvtx.annotate(message="common.CumlArray.copy", category="utils",
domain="cuml_python")
def copy(self) -> Buffer:
Copy link
Member

Choose a reason for hiding this comment

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

cuml Array will no longer inherit from cuDF Buffer very soon (see @wphicks PR for CPU/GPU) and it will very likely diverge more from it, so cuDF's Buffer changes don't need to be mirrored. What are the advantages of removing it?

I would at least consider doing the change after PR 4908 goes in with all the changes that it is doing to cumlarray, which is still in fluctuation.

Copy link
Member Author

Choose a reason for hiding this comment

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

What are the advantages of removing it?

Removal of unused code. In its current state, calling .copy() raises an exception.
It should probably also be removed in #4908:

cuml/python/cuml/internals/array.py

Lines 639 to 649 in e6c56a8

def copy(self) -> CudfBuffer:
"""
Create a new Buffer containing a copy of the data contained
in this Buffer.
"""
from rmm._lib.device_buffer import copy_device_to_ptr
# TODO(wphicks): Generalize this for host/device
out = CudfBuffer.empty(size=self.size)
copy_device_to_ptr(self.ptr, out.ptr, self.size)
return out

Copy link
Member

Choose a reason for hiding this comment

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

Looking back at why I added it, it was a provisional fix when cudf Buffer did some changes but ended up going a different route to fix things, and it is now wrong and not needed indeed, so fully agree, will merge!

@dantegd dantegd added this to PR-WIP in v22.12 Release via automation Nov 1, 2022
@dantegd
Copy link
Member

dantegd commented Nov 1, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit c0995bd into rapidsai:branch-22.12 Nov 1, 2022
v22.12 Release automation moved this from PR-WIP to Done Nov 1, 2022
jakirkham pushed a commit to jakirkham/cuml that referenced this pull request Feb 27, 2023
@madsbk
Remove CumlArray.copy() (rapidsai#4958)
d999080 
cuDF's `Buffer` doesn't have `Buffer.empty()` since rapidsai/cudf#11447. Can we remove `CumlArray.copy()`?

Authors:
  - Mads R. B. Kristensen (https://github.com/madsbk)

Approvers:
  - William Hicks (https://github.com/wphicks)
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#4958
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dantegd dantegd dantegd approved these changes

@wphicks wphicks wphicks approved these changes

Assignees
No one assigned
Labels
breaking Breaking change Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function
Projects
No open projects
v22.12 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@madsbk @codecov-commenter @beckernick @dantegd @wphicks