Fix urDeviceGetGlobalTimestampTest and urContextCreateWithNativeHandle #837
Conversation
fabiomestre
force-pushed
the
fabio/fix_timestamps
branch
from
f0fb7d7
to
185ff36
Compare
fabiomestre
force-pushed
the
fabio/fix_timestamps
branch
from
185ff36
to
fa2b2aa
Compare
| ur_result_t result = urContextCreateWithNativeHandle(native_context, 1, | ||
| &device, &props, &ctx); | ||
|
|
||
| if (result == UR_RESULT_ERROR_INVALID_OPERATION) { |
There was a problem hiding this comment.
That's not a valid error code for urContextCreateWithNativeHandle, if its not supported it should return UR_RESULT_ERROR_UNSUPPORTED_FEATURE.
| @@ -102,7 +103,12 @@ TEST_F(urDeviceGetGlobalTimestampTest, SuccessSynchronizedTime) { | |||
| const uint64_t allowedDiff = static_cast<uint64_t>( | |||
| std::min(deviceTimeDiff, hostTimeDiff) * allowedTimerError); | |||
|
|
|||
| ASSERT_LE(observedDiff, allowedDiff); | |||
There was a problem hiding this comment.
Why not change the allowdDiff/allowedTimerError?
There was a problem hiding this comment.
The problem of changing allowDiff is that there is no amount of time that is guaranteed to work.
I did run the test locally many times and there is some pretty big outliers. I suspect that it might be due to context switches in the OS which can take an unpredictable amount of time.
So I could change allowDiff to some huge number and the test would still not be guaranteed to pass 100% of the time.
| @@ -102,7 +103,12 @@ TEST_F(urDeviceGetGlobalTimestampTest, SuccessSynchronizedTime) { | |||
| const uint64_t allowedDiff = static_cast<uint64_t>( | |||
| std::min(deviceTimeDiff, hostTimeDiff) * allowedTimerError); | |||
|
|
|||
| ASSERT_LE(observedDiff, allowedDiff); | |||
There was a problem hiding this comment.
We are seeing that this test is flaky also on L0 in the CI (hence #836)...
| @@ -787,14 +787,15 @@ returns: | |||
| - "If the adapter has no underlying equivalent handle." | |||
| --- #-------------------------------------------------------------------------- | |||
| type: function | |||
| desc: "Returns synchronized Host and Device global timestamps." | |||
| desc: "Returns a reasonably synchronized Host and Device global timestamps." | |||
There was a problem hiding this comment.
what "reasonably" means here? are we adding that because L0 or other adapter is returning synchronized timestamps with some jitter?
There was a problem hiding this comment.
This is a small wording change aimed at managing expectations of the accuracy of this entrypoint in UR. I took the wording from the OpenCL spec. OpenCL is the only adapter which has native support for an equivalent entrypoint. And, even OpenCL does not provide guarantees on the accuracy of the synchronization (hence the "reasonably" wording). I think that the UR spec should be relaxed to at least be similar in scope to OpenCL. I don't think we will ever be able to enforce hard constraints on this entrypoint for CUDA / HIP (and probably L0 as well?).
This PR is marked as draft because I'm not sure what to do with the existing CTS testing. As it stands, OpenCL is probably the only adapter that can pass this reliably.
No description provided.