Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

GeorgeWeb
Copy link
Contributor

@GeorgeWeb GeorgeWeb commented Feb 29, 2024

This PR addresses #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: intel/llvm#12958

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GeorgeWeb GeorgeWeb marked this pull request as ready for review March 8, 2024 13:50
@GeorgeWeb GeorgeWeb requested review from a team as code owners March 8, 2024 13:50
@GeorgeWeb GeorgeWeb requested a review from jchlanda March 8, 2024 13:50
@GeorgeWeb GeorgeWeb force-pushed the georgi/get-event-info-from-native-fix branch 2 times, most recently from 56ac261 to 8f28dd2 Compare March 8, 2024 14:07
@GeorgeWeb GeorgeWeb force-pushed the georgi/get-event-info-from-native-fix branch from 8f28dd2 to 2f8dc5b Compare March 28, 2024 14:18
@kbenzie kbenzie added cuda CUDA adapter specific issues hip HIP adapter specific issues labels Apr 10, 2024
@JackAKirk
Copy link
Contributor

@GeorgeWeb I am just planning implementing the suggestion here: #1491 (comment)
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

@GeorgeWeb
Copy link
Contributor Author

@GeorgeWeb I am just planning implementing the suggestion here: #1491 (comment)

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 GeorgeWeb force-pushed the georgi/get-event-info-from-native-fix branch 3 times, most recently from c61956e to 33e1852 Compare June 13, 2024 10:25
@GeorgeWeb
Copy link
Contributor Author

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

@npmiller npmiller added the v0.10.x Include in the v0.10.x release label Jul 24, 2024
@alycm alycm added the ready to merge Added to PR's which are ready to merge label Jul 29, 2024
@omarahmed1111 omarahmed1111 force-pushed the georgi/get-event-info-from-native-fix branch from 33e1852 to df6e9d2 Compare July 29, 2024 15:29
@omarahmed1111 omarahmed1111 merged commit 1b4a8b8 into oneapi-src:main Jul 30, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda CUDA adapter specific issues hip HIP adapter specific issues ready to merge Added to PR's which are ready to merge v0.10.x Include in the v0.10.x release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants