-
Notifications
You must be signed in to change notification settings - Fork 16
add a CL device<->host transfer mapper #548
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.
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
Conversation
|
Is this going in the direction you had in mind @inducer ? |
a906d51 to
09553cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, two three minor nits.
| class TransferMapper(CopyMapper): | ||
| def __init__(self, to_device: bool, queue: Any, allocator: Any = None) -> None: | ||
| super().__init__() | ||
| self.to_device = to_device |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefer two different mappers, one per direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in inducer/arraycontext#282
|
|
||
| # {{{ TransferMapper | ||
|
|
||
| class TransferMapper(CopyMapper): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this guy/these guys would be better off in arraycontext.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the idea be to use to_numpy/from_numpy instead of .data.get then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in inducer/arraycontext#282
|
|
||
| from pyopencl.array import Array as CLArray, to_device | ||
|
|
||
| if isinstance(expr.data, CLArray) and not self.to_device: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the to-host version of this should error if a non-numpy array is encountered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-numpy or non-CL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be worthwhile to support a scenario such as transfer_to_host(transfer_to_host(foo_dag))?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Not sure.
Off the bat: maybe not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed support for this scenario in inducer/arraycontext#282
|
|
||
| # {{{ TransferMapper | ||
|
|
||
| class TransferMapper(CopyMapper): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the idea be to use to_numpy/from_numpy instead of .data.get then?
|
|
||
| from pyopencl.array import Array as CLArray, to_device | ||
|
|
||
| if isinstance(expr.data, CLArray) and not self.to_device: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-numpy or non-CL?
| return DataWrapper( | ||
| data=data, | ||
| shape=expr.shape, | ||
| axes=expr.axes, | ||
| tags=expr.tags, | ||
| non_equality_tags=expr.non_equality_tags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this return an instance of a new DataWrapper type that can __hash__ by data instead of id? That would remove the need for data wrapper deduplication, if I understand the intended use of these mappers correctly.
|
Closing here, will be continued in inducer/arraycontext#282 |
Implements part of #547. Continued in inducer/arraycontext#282.
TODOs: