Improve description of device handling, add `array.to_device` #171

rgommers · 2021-04-26T17:06:44Z

Closes gh-157

Closes data-apisgh-157

kgryte

LGTM.

leofang · 2021-04-27T16:28:52Z

LGTM, but I guess we need someone from TensorFlow to give a green light? IIRC TF allows implicit transfer.

rgommers · 2021-04-27T16:40:25Z

LGTM, but I guess we need someone from TensorFlow to give a green light? IIRC TF allows implicit transfer.

Right, this PR is meant to be just a clarification based on what we already discussed earlier. Quoting from #39 (comment):

Also, I am not sure we should enforce constraints on where inputs and outputs can be placed for an operation. Such constraints can make it harder to write portable library code where you don't control the inputs and may have to start by copying all inputs to the same device. Tensorflow runtime is allowed to copy inputs to the correct device if needed.

That is a good question, should it be enforced or just recommended? Having device transfers be explicit is usually better (implicit transfers can make for hard to track down performance issues), but perhaps not always.

I think in the end this is an execution rather than a syntax/semantics question, so maybe the "with the convention ..." phrase in this PR should be replaced with a phrase that's more like "strong recommendation".

Cc @edloper, @agarwal-ashish

agarwal-ashish · 2021-04-29T07:56:06Z

LGTM.

One nit is that the device property on operations likely specifies where the kernel runs, which is typically but not necessarily, the same as the device where outputs are placed.

rgommers · 2021-04-29T08:03:01Z

One nit is that the device=None property on operations likely specifies where the kernel runs, which is typically but not necessarily, the same as the device where outputs are placed.

Thanks @token. device=None is a keyword for array creation functions, so if it's not specified I think then the device that the array is placed on is "whatever the default device or placement strategy of the library is".

agarwal-ashish · 2021-04-29T08:17:11Z

One nit is that the device=None property on operations likely specifies where the kernel runs, which is typically but not necessarily, the same as the device where outputs are placed.

Thanks @token. device=None is a keyword for array creation functions, so if it's not specified I think then the device that the array is placed on is "whatever the default device or placement strategy of the library is".

Sorry I meant even when the device is specified, a particular op may choose to output on a different device than where the kernel is executed (e.g. copy, shape, rpc related ops, etc). If this property is restricted to array creation functions only then the two devices will likely match.

rgommers · 2021-04-29T08:26:55Z

If this property is restricted to array creation functions only then the two devices will likely match.

It is. Thanks for clarifying. Are the cases where an array creation call would still end up on a different device restricted to avoiding an out-of-memory error? If so, we can explicitly add that as a note.

edloper · 2021-04-29T16:44:47Z

spec/design_topics/device_support.md


-This standard chooses to add support for method 3 (local control), because it's the most explicit and granular, with its only downside being verbosity. A context manager may be added in the future - see {ref}`device-out-of-scope` for details.
+This standard chooses to add support for method 3 (local control), with the convention that execution takes place on the same device where all argument arrays are allocated. The rationale for choosing method 3 is because it's the most explicit and granular, with its only downside being verbosity. A context manager may be added in the future - see {ref}`device-out-of-scope` for details.


Just to clarify: for libraries that follow this convention, it would be an error to perform an operation with tensors that are not all allocated on the same device?

leofang · 2021-05-19T19:28:38Z

spec/API_specification/array_object.md

+
+-   **out**: _&lt;array&gt;_
+
+    -   an array with the same data and dtype, located on the specified device.


If self is already on device, do we expect a no-op (returning self) or a copy?

Good point, should specify that. I'd say no-op.

leofang · 2021-09-14T05:21:08Z

Let's get this PR in, as no body objects and we don't want to drag indefinitely. We can continue the discussion and pick up any loose ends in the follow-up PR #259.

Improve description of device handling, add array.to_device

b8b2c91

Closes data-apisgh-157

rgommers added the Narrative Content Narrative documentation content. label Apr 26, 2021

rgommers requested a review from oleksandr-pavlyk April 26, 2021 17:06

kgryte approved these changes Apr 26, 2021

View reviewed changes

oleksandr-pavlyk approved these changes Apr 27, 2021

View reviewed changes

edloper reviewed Apr 29, 2021

View reviewed changes

leofang reviewed May 19, 2021

View reviewed changes

This was referenced Sep 13, 2021

The spec of to_device() method is missing #256

Closed

Address a few device related issues #259

Merged

kgryte mentioned this pull request Sep 13, 2021

Add specifications for returning upper (triu) and lower (tril) triangular matrices #243

Merged

leofang merged commit e810a81 into data-apis:main Sep 14, 2021

rgommers deleted the device-support-tweak branch September 14, 2021 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve description of device handling, add `array.to_device` #171

Improve description of device handling, add `array.to_device` #171

rgommers commented Apr 26, 2021

kgryte left a comment

leofang commented Apr 27, 2021

rgommers commented Apr 27, 2021

agarwal-ashish commented Apr 29, 2021 •

edited

Loading

rgommers commented Apr 29, 2021

agarwal-ashish commented Apr 29, 2021

rgommers commented Apr 29, 2021

edloper Apr 29, 2021

leofang May 19, 2021 •

edited

Loading

rgommers May 19, 2021

leofang commented Sep 14, 2021


		This standard chooses to add support for method 3 (local control), because it's the most explicit and granular, with its only downside being verbosity. A context manager may be added in the future - see {ref}`device-out-of-scope` for details.
		This standard chooses to add support for method 3 (local control), with the convention that execution takes place on the same device where all argument arrays are allocated. The rationale for choosing method 3 is because it's the most explicit and granular, with its only downside being verbosity. A context manager may be added in the future - see {ref}`device-out-of-scope` for details.


		- out: _<array>_

		- an array with the same data and dtype, located on the specified device.

Improve description of device handling, add array.to_device #171

Improve description of device handling, add array.to_device #171

Conversation

rgommers commented Apr 26, 2021

kgryte left a comment

Choose a reason for hiding this comment

leofang commented Apr 27, 2021

rgommers commented Apr 27, 2021

agarwal-ashish commented Apr 29, 2021 • edited Loading

rgommers commented Apr 29, 2021

agarwal-ashish commented Apr 29, 2021

rgommers commented Apr 29, 2021

edloper Apr 29, 2021

Choose a reason for hiding this comment

leofang May 19, 2021 • edited Loading

Choose a reason for hiding this comment

rgommers May 19, 2021

Choose a reason for hiding this comment

leofang commented Sep 14, 2021

Improve description of device handling, add `array.to_device` #171

Improve description of device handling, add `array.to_device` #171

agarwal-ashish commented Apr 29, 2021 •

edited

Loading

leofang May 19, 2021 •

edited

Loading