Rollback of `DeviceBufferLike` #12009

madsbk · 2022-10-27T09:45:00Z

This PR replaces DeviceBufferLike with Buffer and clear the way for a spillable sub-class of Buffer.

Context

The introduction of the DeviceBufferLike protocol was motivated by the spilling work, which we initially thought would have to be implemented in Cython. However, it can be done in pure Python, which makes DeviceBufferLike an unneeded complexity.

Review notes

In order to introduce a spillable-buffer in the future, we still use a factory function, as_buffer(), to create Buffers.
buffer.py is moved into the submodule core.buffer to ease organization when adding the spillable-buffer and spilling manager.

Breaking

This PR breaks external use of Buffer e.g. Buffer.__init__ raise an exception now and the "constructor-kwargs" header from #4164 has been removed.
Submitted a PR to fix this in cuml: rapidsai/cuml#4965

Checklist

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

cc. @shwina @vyasr

vyasr

Some small questions to address before we finalize this.

python/cudf/cudf/core/abc.py

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

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

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

…uffer_like_rollback

madsbk · 2022-10-28T10:55:55Z

Thanks for the review @vyasr. I think it is a good idea to use the as_buffer factory function exclusively.
Now, as_buffer creates a Buffer instance through Buffer.from_device_memory() and Buffer.from_host_memory().

This also enables SpillableBuffer to overwrite specific parts of the buffer constructor.

codecov · 2022-10-28T16:36:45Z

Codecov Report

Base: 88.09% // Head: 86.86% // Decreases project coverage by -1.23% ⚠️

Coverage data is based on head (25813ac) compared to base (f0b4c4f).
Patch coverage: 87.92% of modified lines in pull request are covered.

❗ Current head 25813ac differs from pull request most recent head ff17377. Consider uploading reports for the commit ff17377 to get more accurate results

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-22.12   #12009      +/-   ##
================================================
- Coverage         88.09%   86.86%   -1.24%     
================================================
  Files               133      135       +2     
  Lines             21982    21985       +3     
================================================
- Hits              19366    19097     -269     
- Misses             2616     2888     +272

Impacted Files	Coverage Δ
python/cudf/cudf/core/frame.py	`93.68% <ø> (ø)`
python/cudf/cudf/core/groupby/groupby.py	`91.30% <ø> (ø)`
python/cudf/cudf/core/index.py	`92.88% <ø> (ø)`
python/cudf/cudf/core/indexed_frame.py	`92.03% <ø> (ø)`
python/cudf/cudf/core/reshape.py	`83.36% <ø> (ø)`
python/cudf/cudf/core/series.py	`95.78% <ø> (ø)`
python/cudf/cudf/io/text.py	`91.66% <ø> (ø)`
python/cudf/cudf/testing/testing.py	`81.81% <ø> (ø)`
python/cudf/cudf/utils/docutils.py	`97.67% <ø> (ø)`
python/cudf/cudf/core/column/timedelta.py	`90.75% <50.00%> (ø)`
... and 28 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.

…uffer_like_rollback

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

Rollback rapidsai@61fe635

…uffer_like_rollback

The "constructor-kwargs" is removed in rapidsai/cudf#12009

vyasr

One small suggestion, otherwise LGTM!

vyasr · 2022-11-01T18:34:14Z

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

+T = TypeVar("T", bound="Buffer")
+
+
+def cuda_array_interface_wrapper(ptr: int, size: int, owner: object = None):


Maybe out of scope for this PR, but we use the SimpleNamespace.__cuda_array_interface__ trick in multiple other places as well (mostly in column code IIRC) and I wonder if we could use the same helper function in multiple places. It's not immediately obvious why one would create this object so having all the places that do so call to a single, well-documented helper function could be nice. Happy to push that to a downstream refactoring PR though.

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

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

shwina · 2022-11-02T16:01:17Z

@gpucibot merge

The "constructor-kwargs" is being removed in rapidsai/cudf#12009 Authors: - Mads R. B. Kristensen (https://github.com/madsbk) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) URL: #4965

Support the new `Buffer` in rapidsai/cudf#12009 Fixes #1032 Authors: - Mads R. B. Kristensen (https://github.com/madsbk) Approvers: - Peter Andreas Entschev (https://github.com/pentschev) URL: #1033

The "constructor-kwargs" is being removed in rapidsai/cudf#12009 Authors: - Mads R. B. Kristensen (https://github.com/madsbk) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) URL: rapidsai#4965

madsbk added 5 commits October 27, 2022 10:08

replaced DeviceBufferLike with Buffer.

7710fc7

fix test_buffer_slice_fail

28f86bd

generalize deserialize()

3b5def9

impl. _init_from_host_memory()

6ec27ae

doc

cb1b224

madsbk added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 27, 2022

madsbk requested a review from a team as a code owner October 27, 2022 09:45

madsbk requested review from vyasr and isVoid October 27, 2022 09:45

github-actions bot added the Python Affects Python cuDF API. label Oct 27, 2022

vyasr requested changes Oct 27, 2022

View reviewed changes

python/cudf/cudf/core/abc.py Show resolved Hide resolved

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

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

python/cudf/cudf/core/column/column.py Show resolved Hide resolved

Merge branch 'branch-22.12' of github.com:rapidsai/cudf into device_b…

ff87c53

…uffer_like_rollback

rapidsai deleted a comment from shwina Oct 28, 2022

madsbk added 5 commits October 28, 2022 10:12

deligate to as_buffer

f5e49a9

use as_buffer() everywhere

0286814

doc

8ebeb9a

clean up type hints and serialization

b69a9ef

fix doc

b0b185e

madsbk added breaking Breaking change and removed non-breaking Non-breaking change labels Oct 28, 2022

strings_udf: fix missing import of as_buffer

0267bca

Merge branch 'branch-22.12' of github.com:rapidsai/cudf into device_b…

810c817

…uffer_like_rollback

vyasr reviewed Oct 31, 2022

View reviewed changes

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

madsbk added 4 commits November 1, 2022 10:10

Buffer.__init__ now raise exception

eb721f5

clean up

6fd341e

remove "constructor-kwargs"

062626a

Rollback rapidsai@61fe635

Merge branch 'branch-22.12' of github.com:rapidsai/cudf into device_b…

9b96167

…uffer_like_rollback

madsbk added a commit to madsbk/cuml that referenced this pull request Nov 1, 2022

Impl. CumlArray.deserialize()

80148ec

The "constructor-kwargs" is removed in rapidsai/cudf#12009

madsbk added a commit to madsbk/cuml that referenced this pull request Nov 1, 2022

Impl. CumlArray.deserialize()

256a860

The "constructor-kwargs" is removed in rapidsai/cudf#12009

madsbk mentioned this pull request Nov 1, 2022

Impl. CumlArray.deserialize() rapidsai/cuml#4965

Merged

vyasr approved these changes Nov 1, 2022

View reviewed changes

shwina reviewed Nov 1, 2022

View reviewed changes

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

shwina reviewed Nov 1, 2022

View reviewed changes

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

shwina reviewed Nov 1, 2022

View reviewed changes

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

madsbk added 2 commits November 2, 2022 08:39

Extend cuda_array_interface_wrapper

93f4164

_from_host_memory(): use numpy.array()

ff17377

shwina approved these changes Nov 2, 2022

View reviewed changes

rapids-bot bot merged commit d6a9e4a into rapidsai:branch-22.12 Nov 2, 2022

jnke2016 mentioned this pull request Nov 2, 2022

[BUG]: Change in the external use of cuDF 'Buffer' rapidsai/cugraph#2881

Closed

2 tasks

pentschev mentioned this pull request Nov 2, 2022

Proxify tests failing rapidsai/dask-cuda#1032

Closed

madsbk mentioned this pull request Nov 3, 2022

Support the new Buffer in cudf rapidsai/dask-cuda#1033

Merged

madsbk deleted the device_buffer_like_rollback branch November 3, 2022 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rollback of `DeviceBufferLike` #12009

Rollback of `DeviceBufferLike` #12009

madsbk commented Oct 27, 2022 •

edited

Loading

vyasr left a comment

madsbk commented Oct 28, 2022

codecov bot commented Oct 28, 2022 •

edited

Loading

vyasr left a comment

vyasr Nov 1, 2022

madsbk Nov 2, 2022

shwina commented Nov 2, 2022

		T = TypeVar("T", bound="Buffer")


		def cuda_array_interface_wrapper(ptr: int, size: int, owner: object = None):

Rollback of DeviceBufferLike #12009

Rollback of DeviceBufferLike #12009

Conversation

madsbk commented Oct 27, 2022 • edited Loading

Context

Review notes

Breaking

Checklist

vyasr left a comment

Choose a reason for hiding this comment

madsbk commented Oct 28, 2022

codecov bot commented Oct 28, 2022 • edited Loading

Codecov Report

vyasr left a comment

Choose a reason for hiding this comment

vyasr Nov 1, 2022

Choose a reason for hiding this comment

madsbk Nov 2, 2022

Choose a reason for hiding this comment

shwina commented Nov 2, 2022

Rollback of `DeviceBufferLike` #12009

Rollback of `DeviceBufferLike` #12009

madsbk commented Oct 27, 2022 •

edited

Loading

codecov bot commented Oct 28, 2022 •

edited

Loading