typo: delibrately -> deliberately

These two lines don't look like they're used

I think it might be better to just take a at::Device or at::DeviceIndex since you don't need a stream, just a device.

I think you should remove happened() since it is the same as query(), but is incorrect if the Event was opened from an IPC handle.

I think this needs to check that this and other were created. Otherwise, the event_ fields may have undefined values.

e.g. throw python_error() instead of return false

Should also check the return value and throw python_error() if necessary here. PyModule_AddObject returns -1 on failure.

PyLong_FromLong can also potentially fail. Instead of checking PyLong_FromLong and PyTuple_New call, you can combine everything with one Py_BuildValue which will simplify the code:

return Py_BuildValue("(ii)", least_priority, greatest_priority);

() means tuple
i means int

https://docs.python.org/3/c-api/arg.html?highlight=buildvalue#c.Py_BuildValue

I'm ambivalent about this constructor. I think just keeping the one below which is more explicit about the device is better.

If you do keep this one, it should be marked explicit to avoid allowing implicit conversions in C++. The above CUDAEvent(unsigned int flags) should also be marked explicit.

I'm not sure if this cast violates the strict aliasing rule in C++. It might not because handle_bytes is a char*, but I'm not sure. It may still violate the rule because of the underlying type of the object in the Python API.

To be safe, I think it's slightly preferable to copy into a cudaIpcEventHandle_t here. Something like:

cudaIpcEventHandle_t handle;
std::mempcy(&handle, handle_bytes, sizeof(handle));
new (&self->cuda_event) at::cuda::CUDAEvent(&handle);

I think this also needs to happen on the event's device. From what I've seen it may segfault if called from the wrong device.

(Also, add a test for this please)

Will do, thanks!

I think the signature should be:

CUDAEvent(Device device, const cudaIpcEventHandle_t* handle);

It's better to be explicit about the device (instead of assuming the current device) because it's important that the event be reconstructed on the same device that it was created on.

(I'm ambivalent about the choice of Device vs. DeviceIndex, but I think Device will work better with our other APIs).

I hesitated a bit on this API change. It feels a little inconsistent with Event.record() where it implicitly getting the device from the current stream. If we make the new API explicit, do we also want to enforce existing API to be explicit?

The difference is that the device here is required information. The exported event already has a device, it's just not encoded in a visible way in cudaIpcEventHandle_t. The full information to import an event is both the device and the cudaIpcEventHandle_t.

hmm, I am not sure if this is just luck or guaranteed behavior. The test_event_multi_gpu test here seems to suggest that an event can be reconstructed from handle on any device (does not have to be the same as the exported event). Let me run some more test.

I'm confused by the description here. I don't see what's special about synchronize(). In general, it's undefined behavior to call any function on an imported event after the exported event has been freed.

Perhaps it's better to paraphrase the CUDA documentation. Something like:

Opens an interprocess event handle exported from another process.

The imported event behaves like a locally created event with the cudaEventDisableTiming flag
specified. Performing operations on the imported event after the exported event has been freed
will result in undefined behavior.

Sorry, I forgot to clean up this doc before that push. I updated the doc in subsequent push. I think it is a little bit more complicated than CUDA's official doc. For example, the official doc did not state whether we can all methods on the exported event when the imported event is destructed. From what I see, I think the dependency is symmetric. Whenever a method (except record?) is called, that last recording event (can be either exported or imported event) must not be destructed.

For example, the official doc did not state whether we can all methods on the exported event when the imported event is destructed.

This can be assumed and need not be stated.

From what I see, I think the dependency is symmetric. Whenever a method (except record?) is called, that last recording event (can be either exported or imported event) must not be destructed.

I checked and I don't think this is the case. You can call synchronize(), wait(), and query() on the original event even if it's only recorded (and deleted) in another process.

Yes, I just verified, you are right. I might have misread some test results yesterday.

I think this should be an AT_CHECK with a useful error message instead of AT_ASSERT

You can make device_index_ a DeviceIndex instead of int64_t and remove all the casts to int16_t in things like CUDAGuard guard(static_cast<int16_t>(device_index_));

I don't think we should document _handle. It's intended as a private API. The preferred way to pass an even to another process is to pass the event itself (not to call ipc_handle() and Event(_handle=...))

I thought about this a bit, and am a little inclined to think that using handle might actually be better in some cases. Suppose we have a parent process spawning a bunch of children process, with each child working on a different device. We have at least two options to synchronize parent and children:

Option 1. Create all events in the parent process, and pass an event (associated with the correct device) to each child. In this case, the parent process need to do many ctx switching, and will need to use a loop synchronize with all children. Moreover, if children also need to synchronize between each other, every child need to keep all other children's event object, and will also need to use a loop to synchronize.

Option 2. Create an event on master, and then pass the same handle to each child. In this case, every process has only a single event, and looks more concise. (actually, let me add a test for creating two events from the same handle).

I don't have a strong preference, and open to any discussion.

You can implement both cases in user code without ever calling the Event(_handle=...) constructor or explicitly calling ipc_handle().

Something like:

event = torch.cuda.Event()
p1 = mp.Process(target=target1, args=(event,))
p2 = mp.Process(target=target1, args=(event,))
p3 = mp.Process(target=target1, args=(event,))
p1.start(); p2.start(); p3.start()

It's better to pass the event instead of the handle because then we can encode the device and ensure that it's opened on the correct device.

I might miss something. I thought if p1, p2, and p3 operate on different devices, they cannot use that event to record, no?

I think we have an unverified assumption regarding whether an event can be reconstructed from handle on any device. Let me run some more tests.

This predates your change, but the description is wrong. cudaEventBlockingSync affects the type of synchronization used in synchronize() (busy-wait vs. blocking). It doesn't affect wait().

Remove the added bit about "query". It's not a stream operation.

record to -> record

Suggestion:

r"""Wrapper around a CUDA event.

CUDA events are synchronization markers that can be used to monitor the device's progress,
to accurately measure timing, and to synchronize CUDA streams.

The underlying CUDA events are lazily initialized when the event is first recorded or exported to another process. After creation, only streams on the same device may record the event. However, streams on any device can wait on the event.

   .. _CUDA documentation:
   https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__EVENT.html

"""

Remove "This can be called on any device.". That's notable for CUDA's cudaEventQuery(), but we make sure that Python methods can be called from any device by setting the current device if necessary.

The documentation in https://pytorch.org/docs/stable/cuda.html#torch.cuda.Stream.wait_event is a bit better.

The events can be recorded on different devices. Suggestion:

r"""Returns the elapsed time in milliseconds between two events.

Both events must be created with `enable_timing=True` and have already be recorded and completed (that is `event.query()` must be True).

NOTE: If either event was last recorded in a non-NULL stream, the resulting time may be greater than expected (even if both used the same stream).

        .. note:: This is a wrapper around ``cudaEventElapsedTime()``: see `CUDA
           documentation`_ for more info.

        .. _CUDA documentation:
           https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__EVENT.html
"""

I thought I tests two events on different devices, and it hit CUDA error. Let me double check.

The following code run into RuntimeError: CUDA error: invalid resource handle error when calling e0.elapsed_time(e1). But I can still remove that from doc as CUDA does not explicitly say that.

        d0 = torch.device('cuda:0')
        d1 = torch.device('cuda:1')
    
        with torch.cuda.device(d0):
            s0 = torch.cuda.current_stream()
            e0 = torch.cuda.Event(enable_timing=True)
            torch.cuda._sleep(10)  # spin for about 50 ms on device1
            s0.record_event(e0)
    
        with torch.cuda.device(d1):
            s1 = torch.cuda.current_stream()
            e1 = torch.cuda.Event(enable_timing=True)
            torch.cuda._sleep(50000000)  # spin for about 50 ms on device1
            s1.record_event(e1)
    
        e0.synchronize()
        e1.synchronize()
        e0.elapsed_time(e1)

Hmmm --- yeah I'm seeing the same thing now. My original test might have been wrong.

The "on the event's device" doesn't make sense in this context. Suggestion:

r"""Waits for the event to complete. 

Waits until the completion of all work currently captured in this event. This prevents the CPU thread from proceeding until the event completes.

         .. note:: This is a wrapper around ``cudaEventSynchronize()``: see `CUDA
           documentation`_ for more info.

        .. _CUDA documentation:
           https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__EVENT.html

This isn't accurate. (The original statement below is accurate)

Don't take in DeviceIndex here. It unnecessarily complicates the signature with redundant information (i.e. it now allows invalid calls).

void ipc_handle(cudaIpcEventHandle_t * handle);