[REVIEW] Adapt to changes in `cudf.core.buffer.Buffer` #5154

galipremsagar · 2023-01-24T17:18:48Z

This PR adapts to the breaking changes being introduced in: rapidsai/cudf#12587

wence-

Looks right to me.

wence- · 2023-01-24T17:39:41Z

python/cuml/internals/array.py

@@ -255,7 +255,7 @@ def __init__(self,
                    self._mem_type = GlobalSettings().memory_type

                try:
-                    data = data.ptr
+                    data = data.get_ptr()


This is correct (cuml is taking ownership of the buffer, so we can't do anything), but I think this is an impossible branch? How could we get here given that if data is a Buffer that exposes get_ptr then it also has a __cuda_array_interface__ property so we'll never end up in this branch?

You are right, we will never branch for a Buffer to this part of the code. Reverted the change.

Can a cuml-ite cast some light on what is meant to be going on in this function? I think that this patch:

diff --git a/python/cuml/internals/array.py b/python/cuml/internals/array.py index 808b92fe3..f9386ee8a 100644 --- a/python/cuml/internals/array.py +++ b/python/cuml/internals/array.py @@ -242,9 +242,7 @@ class CumlArray(): 'Must specify dtype when data is passed as a' ' {}'.format(type(data)) ) - if isinstance(data, (CudfBuffer, DeviceBuffer)): - self._mem_type = MemoryType.device - elif mem_type is None: + if mem_type is None: if GlobalSettings().memory_type in ( None, MemoryType.mirror ): @@ -254,13 +252,6 @@ class CumlArray(): ) self._mem_type = GlobalSettings().memory_type - try: - data = data.ptr - if shape is None: - shape = (data.size,) - self._owner = data - except AttributeError: # Not a buffer object - pass if isinstance(data, int): self._owner = owner else:

Just deletes unreachable code:

Both CudfBuffer and DeviceBuffer advertise a __cuda_array_interface__ property

Only CudfBuffer and DeviceBuffer have a .ptr property

So if data is either a CudfBuffer or a DeviceBuffer then we will never fall through the initial

try: data.__cuda_array_interface__ except AttributeError: ...

check.

@wphicks can you comment?

@wence- Yep, that patch looks right to me. This code was based on an earlier version that did not take advantage of __cuda_array_interface__ at all. To answer the question about what this function is meant to, it helps construct a unified interface for all the data types cuML accepts. This function harvests all the data necessary for the CumlArray to populate a __cuda_array_interface__ or __array_interface__ of its own, even if the original object does not have an array interface.

thanks, I'll submit something as a followup, in the interests of keeping Prem's change as small as possible

dantegd

Code changes look good, just had the comment about the temporary changes

dantegd · 2023-01-25T21:44:52Z

ci/build_cpp.sh

+LIBCUDF_CHANNEL=$(rapids-get-artifact ci/cudf/pull-request/12587/046025a/cudf_conda_cpp_cuda11_$(arch).tar.gz)
+
 rapids-print-env

 rapids-logger "Begin cpp build"

-rapids-mamba-retry mambabuild conda/recipes/libcuml
+rapids-mamba-retry mambabuild \
+  --channel "${LIBCUDF_CHANNEL}" \
+  conda/recipes/libcuml


libcudf is not a depencency of libcuml just for context. These chanes are just temporary to test things, right?

Yup, all the .sh file changes will be reverted before merging this PR. Hence tagged it DO NOT MERGE for now.

dantegd · 2023-01-25T21:45:51Z

ci/build_python.sh

+PY_VER=${RAPIDS_PY_VERSION//./}
+LIBCUDF_CHANNEL=$(rapids-get-artifact ci/cudf/pull-request/12587/046025a/cudf_conda_cpp_cuda11_$(arch).tar.gz)
+CUDF_CHANNEL=$(rapids-get-artifact ci/cudf/pull-request/12587/046025a/cudf_conda_python_cuda11_${PY_VER}_$(arch).tar.gz)


Same question as above. Python cuml does use cudf so it makes more sense, but these seem like temporary changes for testing, right?

Yup, same as above

dantegd · 2023-01-25T21:46:04Z

ci/test_python_common.sh

+PY_VER=${RAPIDS_PY_VERSION//./}
+LIBCUDF_CHANNEL=$(rapids-get-artifact ci/cudf/pull-request/12587/046025a/cudf_conda_cpp_cuda11_$(arch).tar.gz)
+CUDF_CHANNEL=$(rapids-get-artifact ci/cudf/pull-request/12587/046025a/cudf_conda_python_cuda11_${PY_VER}_$(arch).tar.gz)


ditto as above

python/cuml/tests/test_input_utils.py

codecov-commenter · 2023-01-26T11:46:07Z

Codecov Report

Base: 69.26% // Head: 69.07% // Decreases project coverage by -0.20% ⚠️

Coverage data is based on head (ac49d6a) compared to base (de32125).
Patch coverage: 91.11% of modified lines in pull request are covered.

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-23.02    #5154      +/-   ##
================================================
- Coverage         69.26%   69.07%   -0.20%     
================================================
  Files               192      192              
  Lines             12333    12370      +37     
================================================
+ Hits               8543     8544       +1     
- Misses             3790     3826      +36

Impacted Files	Coverage Δ
python/cuml/testing/strategies.py	`92.20% <90.47%> (-1.87%)`	⬇️
...ing/text/stem/porter_stemmer_utils/suffix_utils.py	`91.11% <100.00%> (+0.20%)`	⬆️
python/cuml/internals/available_devices.py	`64.28% <0.00%> (-21.43%)`	⬇️
python/cuml/neighbors/kernel_density.py	`72.07% <0.00%> (-15.59%)`	⬇️
python/cuml/testing/utils.py	`88.13% <0.00%> (-0.90%)`	⬇️
python/cuml/internals/array.py	`87.94% <0.00%> (-0.20%)`	⬇️

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

We should probably investigate the test failure we're seeing here rather than just doing a rerun. It looks like we're getting an incorrect array shape for that test, which could certainly be caused by a change to the CumlArray logic. Could we check to see if this is a regression using the Hypothesis reproducer instructions before merge?

ajschmidt8 · 2023-01-26T15:49:50Z

(i deleted my /merge comment to un-queue this PR from being merged since it seems like it's still in progress)

ajschmidt8 · 2023-01-26T16:06:13Z

Admin merging with permission from @dantegd since these failures are unrelated.

This PR adapts to the breaking changes being introduced in: rapidsai/cudf#12587 Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Lawrence Mitchell (https://github.com/wence-) - AJ Schmidt (https://github.com/ajschmidt8) - Dante Gama Dessavre (https://github.com/dantegd)

@wphicks

Noticed while reviewing #5154. Plus an extra (probably benign) typo-bug while I'm here. cc @wphicks Authors: - Lawrence Mitchell (https://github.com/wence-) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - William Hicks (https://github.com/wphicks) URL: #5166

Adapt to changes in Buffer class

ce34c67

galipremsagar requested a review from a team as a code owner January 24, 2023 17:18

github-actions bot added the Cython / Python Cython or Python issue label Jan 24, 2023

wence- approved these changes Jan 24, 2023

View reviewed changes

galipremsagar added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 24, 2023

temporary changes

d040343

galipremsagar requested a review from a team as a code owner January 24, 2023 21:52

github-actions bot added the ci label Jan 24, 2023

galipremsagar added 3 commits January 24, 2023 15:57

revert

867ae08

revert

efd0bf8

Merge remote-tracking branch 'upstream/branch-23.02' into ptr_refactor

5fd7172

galipremsagar added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jan 25, 2023

ajschmidt8 approved these changes Jan 25, 2023

View reviewed changes

dantegd requested changes Jan 25, 2023

View reviewed changes

dantegd approved these changes Jan 25, 2023

View reviewed changes

galipremsagar added 2 commits January 25, 2023 19:55

revert temporary changes

be7446f

revert temporary changes

6840b5f

github-actions bot removed the ci label Jan 26, 2023

galipremsagar removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jan 26, 2023

Merge branch 'branch-23.02' into ptr_refactor

722c800

galipremsagar commented Jan 26, 2023

View reviewed changes

python/cuml/tests/test_input_utils.py Outdated Show resolved Hide resolved

galipremsagar added 3 commits January 26, 2023 04:00

Update python/cuml/tests/test_input_utils.py

a8043f7

style

d32b182

style

ac49d6a

wence- mentioned this pull request Jan 26, 2023

Simplify cuml array #5166

Merged

wphicks requested changes Jan 26, 2023

View reviewed changes

ajschmidt8 merged commit f6d72fc into rapidsai:branch-23.02 Jan 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Adapt to changes in `cudf.core.buffer.Buffer` #5154

[REVIEW] Adapt to changes in `cudf.core.buffer.Buffer` #5154

galipremsagar commented Jan 24, 2023

wence- left a comment

wence- Jan 24, 2023

galipremsagar Jan 25, 2023

wence- Jan 25, 2023

wphicks Jan 25, 2023

wence- Jan 25, 2023

wence- Jan 26, 2023

dantegd left a comment

dantegd Jan 25, 2023

galipremsagar Jan 25, 2023 •

edited

Loading

dantegd Jan 25, 2023

galipremsagar Jan 25, 2023

dantegd Jan 25, 2023

galipremsagar Jan 25, 2023

codecov-commenter commented Jan 26, 2023

wphicks left a comment

ajschmidt8 commented Jan 26, 2023

ajschmidt8 commented Jan 26, 2023

[REVIEW] Adapt to changes in cudf.core.buffer.Buffer #5154

[REVIEW] Adapt to changes in cudf.core.buffer.Buffer #5154

Conversation

galipremsagar commented Jan 24, 2023

wence- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantegd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galipremsagar Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 26, 2023

Codecov Report

wphicks left a comment

Choose a reason for hiding this comment

ajschmidt8 commented Jan 26, 2023

ajschmidt8 commented Jan 26, 2023

[REVIEW] Adapt to changes in `cudf.core.buffer.Buffer` #5154

[REVIEW] Adapt to changes in `cudf.core.buffer.Buffer` #5154

galipremsagar Jan 25, 2023 •

edited

Loading