[REVIEW] Change ways to access `ptr` in `Buffer` #12587

galipremsagar · 2023-01-20T14:08:19Z

Description

This PR:
With the introduction of copy-on-write(#11718), and spilling, there are different ways each Buffer implementation expects the code to behave when a .ptr is accessed, i.e., in case of COW, a ptr can be accessed for read or write purposes, in case of spilling the ptr can be accessed with a spill lock and the buffer will be spillable after the spill lock goes out of scope during execution. For these reasons we introduced ptr, mutable_ptr and data_array_view, _data_array_view, mask_array_view, and _mask_array_view. With so many ways to access buffer ptr and array views, this has become quite difficult for devs to know when to use what unless you are fully familiar with the implementation details. It will also lead us to a lot of special case handling for Buffer, SpillableBuffer, and CopyonWriteBuffer. For this reason, we have decided to simplify fetching the pointer with a get_ptr(mode="read"/"write") API, fetching data & mask array views will also become methods that accept mode like data_array_view(mode="read"/"write") & mask_array_view(mode="read"/"write"). It is the expectation that when the caller passed "read", they don't tamper with the buffer or the memory it is pointing to. In the case of "write", they are good to mutate the memory it is pointing to.

Note that even with mode="read"/"write" the caller should still, if appropriate, acquire a spill lock for the duration of the access. If this is not done, and the buffer is a SpillableBuffer, it will permanently be marked as unspillable.

Introduces get_ptr() to replace ptr property.
Replaces data_array_view & mask_array_view methods with data_array_view(mode=r/w) & mask_array_view(mode=r/w)

Checklist

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

wence-

Some small queries.

python/cudf/cudf/_lib/column.pyx

python/cudf/cudf/_lib/copying.pyx

python/cudf/cudf/core/buffer/buffer.py

python/cudf/cudf/core/column/categorical.py

python/cudf/cudf/core/column/numerical.py

python/cudf/cudf/core/df_protocol.py

python/cudf/cudf/core/frame.py

python/cudf/cudf/utils/queryutils.py

Co-authored-by: Lawrence Mitchell <[email protected]>

codecov · 2023-01-20T17:08:24Z

Codecov Report

Base: 86.58% // Head: 85.72% // Decreases project coverage by -0.86% ⚠️

Coverage data is based on head (810d921) compared to base (b6dccb3).
Patch has no changes to coverable lines.

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-23.02   #12587      +/-   ##
================================================
- Coverage         86.58%   85.72%   -0.86%     
================================================
  Files               155      155              
  Lines             24368    24911     +543     
================================================
+ Hits              21098    21356     +258     
- Misses             3270     3555     +285

Impacted Files	Coverage Δ
python/cudf/cudf/_version.py	`1.41% <0.00%> (-98.59%)`	⬇️
python/cudf/cudf/core/buffer/spill_manager.py	`72.50% <0.00%> (-7.50%)`	⬇️
python/cudf/cudf/utils/dtypes.py	`77.85% <0.00%> (-1.61%)`	⬇️
python/cudf/cudf/options.py	`86.11% <0.00%> (-1.59%)`	⬇️
python/cudf/cudf/core/single_column_frame.py	`94.30% <0.00%> (-1.27%)`	⬇️
python/cudf/cudf/io/json.py	`91.04% <0.00%> (-1.02%)`	⬇️
...ython/custreamz/custreamz/tests/test_dataframes.py	`98.38% <0.00%> (-1.01%)`	⬇️
python/dask_cudf/dask_cudf/io/csv.py	`96.34% <0.00%> (-1.00%)`	⬇️
python/dask_cudf/dask_cudf/io/parquet.py	`91.81% <0.00%> (-0.59%)`	⬇️
python/cudf/cudf/core/column/string.py	`88.00% <0.00%> (-0.53%)`	⬇️
... and 51 more

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.

galipremsagar · 2023-01-20T18:16:18Z

Some small queries.

Addressed 👍 This PR should be ready for another round of reviews.

madsbk

Overall looks good but I don't think we need the "internal_write" mode.

python/cudf/cudf/core/buffer/buffer.py

python/cudf/cudf/core/buffer/spillable_buffer.py

docs/cudf/source/developer_guide/library_design.md

python/cudf/cudf/_lib/column.pyx

python/cudf/cudf/core/buffer/spillable_buffer.py

Co-authored-by: Mads R. B. Kristensen <[email protected]>

wence-

Getting there, thanks! Some more minor things I think.

python/cudf/cudf/core/buffer/buffer.py

python/cudf/cudf/core/column/categorical.py

python/cudf/cudf/core/column/column.py

python/cudf/cudf/core/series.py

python/cudf/cudf/core/window/rolling.py

python/cudf/cudf/utils/applyutils.py

python/strings_udf/strings_udf/_typing.py

shwina · 2023-01-25T22:39:26Z

python/cudf/cudf/core/buffer/buffer.py

            "shape": (self.size,),
            "strides": None,
            "typestr": "|u1",
            "version": 0,
        }

+    @property
+    def _readonly_proxy_cai_obj(self):


Honestly, I don't think we should add extra property here given that it's used only in one other function (do we expect more uses for it?)

If not, I think we can just use cuda_array_interface_wrapper in those places instead.

It is only being used in data_array_view & mask_array_view, and one more place for copy-on-write scenarios: https://github.com/rapidsai/cudf/pull/12556/files#diff-e23cdfbd2c7e4be6f1d08ea3cec7e075e42851641068fe318985775f6b9efd99R214.

OK -- let's leave as is now, and we can take stock in the merge of copy-on-write.

python/cudf/cudf/core/buffer/buffer.py

Co-authored-by: Ashwin Srinath <[email protected]>

shwina · 2023-01-25T23:04:53Z

python/strings_udf/strings_udf/tests/test_string_udfs.py

+    with acquire_spill_lock():
+        kernel.forall(len(data))(str_views, output)


I think we should avoid setting this precedent; tests really shouldn't have to worry about spilling.

Discussed offline and removed this lock to keep things consistent where we are explicitly not testing for spilling ability.

python/strings_udf/strings_udf/tests/test_string_udfs.py

Pending reviews have been addressed, dismissing to proceed with merging.

galipremsagar · 2023-01-26T03:51:08Z

/merge

This PR adapts to the breaking changes being introduced in: rapidsai/cudf#12587 Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - AJ Schmidt (https://github.com/ajschmidt8) - Mark Harris (https://github.com/harrism) - Lawrence Mitchell (https://github.com/wence-) - Michael Wang (https://github.com/isVoid) URL: #871

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)

Fix test that reads directly from `cudf.Buffer` pointer to new `get_ptr(mode="read")`, in accordance with changes from rapidsai/cudf#12587 .

Fix test that reads directly from `cudf.Buffer` pointer to new `get_ptr(mode="read")`, in accordance with changes from rapidsai/cudf#12587 . Authors: - Peter Andreas Entschev (https://github.com/pentschev) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) - Lawrence Mitchell (https://github.com/wence-) URL: #1094

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)

get_ptr & _array_view refactor

15cbedf

galipremsagar added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. 4 - Needs cuDF (Python) Reviewer labels Jan 20, 2023

galipremsagar requested a review from a team as a code owner January 20, 2023 14:08

galipremsagar self-assigned this Jan 20, 2023

galipremsagar requested review from wence- and vyasr January 20, 2023 14:08

galipremsagar added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change breaking Breaking change and removed non-breaking Non-breaking change labels Jan 20, 2023

wence- requested changes Jan 20, 2023

View reviewed changes

Apply suggestions from code review

3697367

Co-authored-by: Lawrence Mitchell <[email protected]>

address reviews

2b1b48d

galipremsagar requested a review from wence- January 20, 2023 18:07

Merge branch 'branch-23.02' into ptr_refactor_1

0bba2a5

madsbk previously requested changes Jan 23, 2023

View reviewed changes

wence- reviewed Jan 23, 2023

View reviewed changes

python/cudf/cudf/core/buffer/spillable_buffer.py Outdated Show resolved Hide resolved

galipremsagar and others added 4 commits January 23, 2023 09:03

Apply suggestions from code review

9824471

Co-authored-by: Mads R. B. Kristensen <[email protected]>

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

7868437

drop internal_write

07a34b4

add docstring

afd3588

galipremsagar requested review from madsbk and wence- January 23, 2023 15:16

wence- reviewed Jan 23, 2023

View reviewed changes

galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer labels Jan 25, 2023

shwina reviewed Jan 25, 2023

View reviewed changes

python/cudf/cudf/core/buffer/buffer.py Outdated Show resolved Hide resolved

galipremsagar removed the 5 - Ready to Merge Testing and reviews complete, ready to merge label Jan 25, 2023

Update python/cudf/cudf/core/buffer/buffer.py

9619201

Co-authored-by: Ashwin Srinath <[email protected]>

shwina reviewed Jan 25, 2023

View reviewed changes

galipremsagar commented Jan 25, 2023

View reviewed changes

python/strings_udf/strings_udf/tests/test_string_udfs.py Outdated Show resolved Hide resolved

Update python/strings_udf/strings_udf/tests/test_string_udfs.py

98c3892

galipremsagar commented Jan 25, 2023

View reviewed changes

python/strings_udf/strings_udf/tests/test_string_udfs.py Outdated Show resolved Hide resolved

python/strings_udf/strings_udf/tests/test_string_udfs.py Outdated Show resolved Hide resolved

galipremsagar added 2 commits January 25, 2023 17:15

Update python/strings_udf/strings_udf/tests/test_string_udfs.py

1cf727b

Update python/strings_udf/strings_udf/tests/test_string_udfs.py

535128c

shwina approved these changes Jan 25, 2023

View reviewed changes

galipremsagar added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Jan 25, 2023

galipremsagar added 3 commits January 25, 2023 16:28

Fix

f1bb022

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

22418fb

Fix

810d921

rapids-bot bot merged commit f7d434d into rapidsai:branch-23.02 Jan 26, 2023

galipremsagar mentioned this pull request Jan 26, 2023

[REVIEW] Copy on write implementation #11718

Merged

5 tasks

pentschev added a commit to pentschev/dask-cuda that referenced this pull request Jan 27, 2023

Update cudf.Buffer pointer access method

07bab4e

Fix test that reads directly from `cudf.Buffer` pointer to new `get_ptr(mode="read")`, in accordance with changes from rapidsai/cudf#12587 .

pentschev mentioned this pull request Jan 27, 2023

Update cudf.Buffer pointer access method rapidsai/dask-cuda#1094

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Change ways to access `ptr` in `Buffer` #12587

[REVIEW] Change ways to access `ptr` in `Buffer` #12587

galipremsagar commented Jan 20, 2023 •

edited by wence-

Loading

wence- left a comment

codecov bot commented Jan 20, 2023 •

edited

Loading

galipremsagar commented Jan 20, 2023

madsbk left a comment

wence- left a comment

shwina Jan 25, 2023

galipremsagar Jan 25, 2023 •

edited

Loading

shwina Jan 25, 2023

shwina Jan 25, 2023

galipremsagar Jan 25, 2023

galipremsagar commented Jan 26, 2023

		with acquire_spill_lock():
		kernel.forall(len(data))(str_views, output)

[REVIEW] Change ways to access ptr in Buffer #12587

[REVIEW] Change ways to access ptr in Buffer #12587

Conversation

galipremsagar commented Jan 20, 2023 • edited by wence- Loading

Description

Checklist

wence- left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 20, 2023 • edited Loading

Codecov Report

galipremsagar commented Jan 20, 2023

madsbk left a comment

Choose a reason for hiding this comment

wence- left a comment

Choose a reason for hiding this comment

shwina Jan 25, 2023

Choose a reason for hiding this comment

galipremsagar Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

shwina Jan 25, 2023

Choose a reason for hiding this comment

shwina Jan 25, 2023

Choose a reason for hiding this comment

galipremsagar Jan 25, 2023

Choose a reason for hiding this comment

galipremsagar commented Jan 26, 2023

[REVIEW] Change ways to access `ptr` in `Buffer` #12587

[REVIEW] Change ways to access `ptr` in `Buffer` #12587

galipremsagar commented Jan 20, 2023 •

edited by wence-

Loading

codecov bot commented Jan 20, 2023 •

edited

Loading

galipremsagar Jan 25, 2023 •

edited

Loading