Skip to content

WIP: Implement __setitem__ for DeviceBuffer#351

Closed
jakirkham wants to merge 1 commit into
rapidsai:branch-0.16from
jakirkham:add_devbuf_setitem
Closed

WIP: Implement __setitem__ for DeviceBuffer#351
jakirkham wants to merge 1 commit into
rapidsai:branch-0.16from
jakirkham:add_devbuf_setitem

Conversation

@jakirkham
Copy link
Copy Markdown
Member

@jakirkham jakirkham commented Apr 21, 2020

This is a rough first pass at implementing __setitem__ for DeviceBuffer. It works based on a quick check, but the error handling could be improved.

@jakirkham jakirkham mentioned this pull request Apr 21, 2020
Closed
@GPUtester
Copy link
Copy Markdown
Contributor

GPUtester commented Apr 21, 2020

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@jrhemstad
Copy link
Copy Markdown
Contributor

jrhemstad commented Apr 21, 2020

What is this doing?

@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented Apr 22, 2020

Initially PR ( dask/distributed#3732 ) was using Numba to view and write into DeviceBuffers, but the overhead was a bit high as @kkraus14 suspected and later testing bore out. This is an attempt to add __setitem__ to bypass needing Numba. That said, it appears using CuPy works well. We may still want this, but don't seem to need it currently.

@harrism
Copy link
Copy Markdown
Member

harrism commented Apr 22, 2020

Can you explain what __setitem__ is for us Python noobs?

@kkraus14
Copy link
Copy Markdown
Contributor

kkraus14 commented Apr 22, 2020

__setitem__ is a Python special method that allows you to do object[key] = some_other_object. For us here, we want to handle where key is a slice which under the hood we use to translate to a pointer and a size to run a memcopy.

kkraus14
kkraus14 reviewed Apr 22, 2020
View reviewed changes
Comment thread python/rmm/_lib/device_buffer.pyx
Comment on lines +120 to +122
rng = range(self.size)[key]
if not isinstance(rng, range):
rng = range(rng, rng + 1)
Copy link
Copy Markdown
Contributor

@kkraus14 kkraus14 Apr 22, 2020

Choose a reason for hiding this comment

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

We should probably handle negative slices / scalars here

Copy link
Copy Markdown
Member Author

@jakirkham jakirkham Apr 24, 2020

Choose a reason for hiding this comment

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

Negative indices are normalized by slicing the range object.

In [1]: range(5)[-1]
Out[1]: 4

Currently we are raising an error for negative indices or any other step size that is not 1 since this could be a bit tricky to do in practice and it's unclear if we have use cases for those yet.

@harrism harrism added Python Related to RMM Python API 2 - In Progress Currently a work in progress labels May 28, 2020
@harrism harrism changed the base branch from branch-0.14 to branch-0.15 May 28, 2020 00:34
@harrism
Copy link
Copy Markdown
Member

harrism commented Aug 7, 2020

@jakirkham @kkraus14 what are the plans for this? Moving to 0.16.

@harrism harrism changed the base branch from branch-0.15 to branch-0.16 August 7, 2020 03:30
@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented Aug 7, 2020

I think we still want this. Has slipped lately though due to other priorities. Will take a look after the release.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 16, 2021

This PR has been marked rotten due to no recent activity in the past 90d. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 16, 2021

This PR has been marked stale due to no recent activity in the past 30d. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be marked rotten if there is no activity in the next 60d.

@harrism
Copy link
Copy Markdown
Member

harrism commented Feb 16, 2021

@jakirkham @kkraus14 can we close this?

@kkraus14
Copy link
Copy Markdown
Contributor

kkraus14 commented Feb 17, 2021

Yes, closing this for now. Feel free to reopen if desired.

@kkraus14 kkraus14 closed this Feb 17, 2021
@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented Feb 17, 2021

Yeah sounds good. I think people can probably just use CuPy or Numba for this today

@jakirkham jakirkham deleted the add_devbuf_setitem branch February 17, 2021 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@kkraus14 kkraus14 kkraus14 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

2 - In Progress Currently a work in progress Python Related to RMM Python API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jakirkham @GPUtester @jrhemstad @harrism @kkraus14