unified-runtime icon indicating copy to clipboard operation
unified-runtime copied to clipboard

[CUDA][HIP] Return error on silently failing urEventGetInfo queries for interop created events

Open GeorgeWeb opened this issue 1 year ago • 3 comments

This PR addresses https://github.com/oneapi-src/unified-runtime/issues/1235 by returning an error on event queries, specifically UR_EVENT_INFO_COMMAND_QUEUE, that cannot be supported for the CUDA and HIP adapters. This is an effect of creating the ur_event from a native event which leads to not being able to fully initialize the UR object.

intel/llvm CI: https://github.com/intel/llvm/pull/12958

GeorgeWeb avatar Feb 29 '24 15:02 GeorgeWeb

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 12.51%. Comparing base (1265304) to head (d6a4dcb). Report is 4 commits behind head on main.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1399   +/-   ##
=======================================
  Coverage   12.51%   12.51%           
=======================================
  Files         239      239           
  Lines       35942    35942           
  Branches     4076     4076           
=======================================
  Hits         4498     4498           
  Misses      31440    31440           
  Partials        4        4           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Feb 29 '24 15:02 codecov-commenter

@GeorgeWeb I am just planning implementing the suggestion here: https://github.com/oneapi-src/unified-runtime/issues/1491#issuecomment-2083074881 which will I think make this implementation inconsistent because interop events will then have the possibility of having an associated native queue when instantiated with make_event interface that takes a native queue (e.g. stream). Although an error message may still be required since the existing make_event interface will still exist that doesn't take a native queue. So the check such I guess then be that the queue associated with the event is not null, rather than the check for ownership?

@alexbatashev FYI

JackAKirk avatar Apr 29 '24 17:04 JackAKirk

@GeorgeWeb I am just planning implementing the suggestion here: https://github.com/oneapi-src/unified-runtime/issues/1491#issuecomment-2083074881

which will I think make this implementation inconsistent because interop events will then have the possibility of having an associated native queue when instantiated with make_event interface that takes a native queue (e.g. stream). Although an error message may still be required since the existing make_event interface will still exist that doesn't take a native queue. So the check such I guess then be that the queue associated with the event is not null, rather than the check for ownership?

@alexbatashev FYI

@JackAKirk That sounds like a plan. I'll update this error on nullptr queue instead.

GeorgeWeb avatar Apr 29 '24 20:04 GeorgeWeb

@oneapi-src/unified-runtime-maintain just a ping to have a look as that's been stale for some time. Thanks!

GeorgeWeb avatar Jun 13 '24 10:06 GeorgeWeb