Skip to content

Conversation

@matthiasdiener
Copy link
Collaborator

@matthiasdiener matthiasdiener commented Sep 24, 2024

Implements part of #547. Continued in inducer/arraycontext#282.

TODOs:

  • add to docs
  • add tests
  • check TaggableCLArray support

@matthiasdiener matthiasdiener self-assigned this Sep 24, 2024
@matthiasdiener
Copy link
Collaborator Author

Is this going in the direction you had in mind @inducer ?

Copy link
Owner

@inducer inducer left a 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
Copy link
Owner

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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


# {{{ TransferMapper

class TransferMapper(CopyMapper):
Copy link
Owner

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.

Copy link
Collaborator

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?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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


from pyopencl.array import Array as CLArray, to_device

if isinstance(expr.data, CLArray) and not self.to_device:
Copy link
Owner

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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))?

Copy link
Owner

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?

Copy link
Collaborator Author

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):
Copy link
Collaborator

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:
Copy link
Collaborator

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?

Comment on lines +1973 to +1978
return DataWrapper(
data=data,
shape=expr.shape,
axes=expr.axes,
tags=expr.tags,
non_equality_tags=expr.non_equality_tags)
Copy link
Collaborator

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.

@matthiasdiener
Copy link
Collaborator Author

Closing here, will be continued in inducer/arraycontext#282

@matthiasdiener matthiasdiener deleted the cl-transfer-mapper branch October 11, 2024 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants