| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Sorry, something went wrong.
|
Thanks Leo! Should I take over the first two commits in gh-602? |
Sorry, something went wrong.
|
Let's just review/merge #602 first, this branch was based on the one there. |
Sorry, something went wrong.
There was a problem hiding this comment.
Is there a timeline issue here as well? from_dlpack should do nothing with the new copy and device keywords beyond forwarding them to the producer. So if the producer doesn't support that functionality yet, what happens?
Sorry, something went wrong.
There was a problem hiding this comment.
Just leaving two notes below.
First, in the call today we discussed one issue in this PR. When two libraries exchange data via copying, potentially across devices, there might not be a common stream object that can be recognized and used by both libraries. The discussed solutions involve different ways of requesting synchronous copies (see HackMD).
See the 2nd note below.
Sorry, something went wrong.
In that discussion I was perhaps missing a more precise problem statement. Also note that that HackMD link isn't public, so here is what it says for potential solutions: @seberg suggested: The important thing is that stream=None (or not passing) synchronizes if you specify device="cpu". What happens if a stream is passed, seems not to important to me the consumer passed it, so they must expect the right thing. @leofang suggested: piggyback on stream=None to request sync copy, when device is not the same. Solution 2: add stream=-2 to signal the two devices do not have a common language (stream/queue). I had a look at what PyTorch does, and it's basically the same as @seberg's suggestion I believe. From https://pytorch.org/docs/master/notes/cuda.html#asynchronous-execution: In general, the effect of asynchronous computation is invisible to the caller, because (1) each device executes operations in the order they are queued, and (2) PyTorch automatically performs necessary synchronization when copying data between CPU and GPU or between two GPUs. Hence, computation will proceed as if every operation was executed synchronously. It goes on to explain a non_blocking keyword, but that's not relevant to the kDLCPU device (only pinned memory).
Since we only want to enable copying to host (where there is no stream concept) at this moment, do we really need to go into this for the purposes of getting this PR in? It looks to me like this can be deferred. |
Sorry, something went wrong.
Well, there are two things we need, I guess:
Streams really should always have been fully type safe, but OK (which would resolve this, we could just say: one of them, figure it out based on type). Maybe I should change my proposal to, this: Streams belong to the target device unless the producer knows it cannot possibly belong to it (the stream is typed to be a CudaStream, say an integer subclass). The important part being here that the default must be in terms of the target! |
Sorry, something went wrong.
I'd say that:
Since only None is accepted, changing that to now require something else (like -2) looks like an unnecessary backwards compat break.
That sounds good to add. Needs to avoid the string "cpu" and use text like "when the device= keyword receives an argument representing the CPU/host device". |
Sorry, something went wrong.
|
Well, the more I think about it the more I think it makes sense to say: If dl_device= is passed the stream must refer to the specified target devices definition for streams. For example if the dl_device=CPU only stream=None is valid and full synchronization must occur even if the data is currently on a CUDA device. That covers what we need, and in principle allows a cpu -> gpu exporter to use a stream. A gpu -> cpu can also use stream if the CPU is specified as CUDAManaged or CUDAHost. Now there might be a point for more complex synchronization schemes in the future. But maybe that just needs a future extension. (The annoyance is that it would be nice to not need a try/except for such a future extension, but beggars can't be choosers) |
Sorry, something went wrong.
|
I see that you are continuing the discussion we had during the meeting on the subject
I think Sebastian is right to conclude that
The stream object and the dl_device object must go in as a valid pair. One subtlety is that while it is in theory possible for a D2H copy to be asynchronously performed on a stream, with the current "one-way" DLPack protocol there is no safe way to allow this. DLPack is one-way in the sense that during handshaking only the consumer would offer stream (and now dl_device too, with this PR) to the producer, not the other way around.1 Given no major concern with this PR based on the discussions in the original issue, in the call, and above, let me proceed to make necessary changes to refine this PR.
Interesting, I didn't know that. I thought we just need to log in HackMD to view it? Footnotes
|
Sorry, something went wrong.
The new introspection API should allow that, but I agree that it's probably not worth it to go there.
No, HackMD has permissions. It's also transient; for any issue/PR with a relevant discussion we should be posting a summary on GitHub.
Sounds right. One thing that stood out to me related to this in this PR is that the "only CPU/host is supported" is part of the from_dlpack changes, but the __dlpack__ changes are more general. Is that on purpose (if so, I don't mind)? |
Sorry, something went wrong.
There was a problem hiding this comment.
Overall this is starting to look close to ready - a few more comments.
Sorry, something went wrong.
|
(Requested Aaron's review from the array-api-compat perspective.) |
Sorry, something went wrong.
|
Thanks for looping me in. Personally, I find there are two goals in here which are slightly intermingled
My main thought is that we should keep API support for G0 to be simple and ensure from_dlpack to come with clear intent as in zero copy. Because G1 might indeed opens up significant implementation overhead, as well as different possible ways of synchronization that I think would merit a different API(to_device?). My main question is how to phase out the API, so most libraries can start with G0. But happy to hear about others thoughts as well |
Sorry, something went wrong.
Nobody is required to implement the cross device API, we could bump minor API (so consumers can know whether or not the flag should be set). Flagging IS_COPY (and a code change) is thus only needed if the library is atypical and doesn't use zero-copy exchange. For the Array-API, this is a bit different: because I think there the intention is to additionally require dl_device="cpu" to work. A maybe slightly overpowered way to allow comparing two libraries in tests (e.g. by converting both to the same CPU library, like NumPy). |
Sorry, something went wrong.
That is the right framing I think indeed.
I think a separate API would be more problematic to do, because:
I think one should always start there if that's an option. from_dlpack(y, device=x.device) will be zero-copy if the data is already on the same device. If we really wanted to ensure always-zero-copy by default, then the copy keyword could default to False rather than None; that way from_dlpack(y, device=x.device) would never be useful and authors would have to write from_dlpack(y, copy=None, device=x.device) (or copy=True). Does it really matter though if this is using from_dlpack with the device keyword or some other function like xp.from_dlpack_with_devicetransfer? The former seems preferable, and I don't think it detracts from the "prefer zero-copy" nature of DLPack. |
Sorry, something went wrong.
|
Thanks @rgommers for the comments. I think as long as packages can phase their implementation status this would be fine. |
Sorry, something went wrong.
|
Thanks @tqchen @rgommers @seberg for the comments. Indeed, G0 (zero copy) has been the goal and we've achieved that since the first version of the standard. What's new with this PR is
|
Sorry, something went wrong.
|
Q: Does anyone know why the doc build has a warning? /home/circleci/repo/src/array_api_stubs/_draft/array_object.py:docstring of array_api_stubs._draft.array_object._array.__dlpack__:1: WARNING: py:class reference target not found: Enum I thought Enum has been used in the return value type before and it worked just fine? |
Sorry, something went wrong.
Those warnings are a real pain. I pushed a fix. For why it was needed here and not in __dlpack_device__ right below it, I have no explanation beyond "autodoc is buggy" |
Sorry, something went wrong.
There was a problem hiding this comment.
All looks good to me now. It looks like all comments were addressed; I plan to merge this within the next day or two unless there are more comments.
Sorry, something went wrong.
|
Reminder to all: Remember to review dmlc/dlpack#136 too, which goes hand-in-hand with this PR. |
Sorry, something went wrong.
|
The accompanying DLPack PR was merged, and DLPack 1.0rc was released. Let's merge this. Thanks everyone for the discussion! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Blocked by #602.
Close #626. As discussed there, we expand the DLPack data exchange protocol to formalize copies. In particular, we allow two common needs:
In principle, arbitrary cross-device copies could be allowed too, but the consensus in #626 was that limiting to device-to-host copies is enough for now. This also avoids the needs of actual exhaustive library support and clarifying unforeseen semantics issues.
This PR requires an accompanying PR to the DLPack repo (dmlc/dlpack#136), in order for the producer to signal if a copy has been made (through a new bit mask), so please review both together.