[SPEC] Clarify the behaviour of UR warning messages #1055
Conversation
fabiomestre
changed the title
fabiomestre
force-pushed
the
fabio/clarify_ur_warnings
branch
from
b3680a0
to
c26d51c
Compare
* This commit updates the specification to clarify the behaviour of UR_RESULT_ERROR_ADAPTER_SPECIFIC when returning warning messages. * This is a temporary change until specific error codes for warnings are added.
fabiomestre
force-pushed
the
fabio/clarify_ur_warnings
branch
from
c26d51c
to
8c798b2
Compare
| /// | ||
| /// @details | ||
| /// To be used after another entry-point has returned | ||
| /// ::UR_RESULT_ERROR_ADAPTER_SPECIFIC in order to retrieve a message describing | ||
| /// the circumstances of the underlying driver error and the error code | ||
| /// returned by the failed driver entry-point. | ||
| /// | ||
| /// * If `pError` is ::UR_RESULT_SUCCESS, ::UR_RESULT_ERROR_ADAPTER_SPECIFIC |
There was a problem hiding this comment.
Although I understand it, I wonder if any adapter would add support to return a warning with SUCESS through urAdapterGetLastError. Returning a message through urAdapterGetLastError has an overhead (strncpy a mesage, keep track of the last message, etc).
this of course could be optional, but I'm concerned this would open the door for UR customers to start requesting warnings, given that the spec allows it, and then having pushback from underlying drivers or adapters that dont want to implement such warning because of performance considerations.
There was a problem hiding this comment.
For context, this spec change is a follow up to the discussion that happened in #989
At the moment, the UR spec says that UR_RESULT_ERROR_ADAPTER_SPECIFIC can be used to return a warning or error but doesn't specify how. The aim to this PR was to clarify what is the correct way to do it.
There are already adapters using UR_RESULT_ERROR_ADAPTER_SPECIFIC in this way. For example CUDA is doing it for a few entrypoints. In addition, this is the behaviour that SYCL-RT expects.
I understand that there are performance / design implications to returning a warning. The intent was for it to be an optional feature.
I guess that we have to decide whether we want to support warnings on UR or not. If the answer is yes, then that should be documented in the spec (which this PR aims to do). If the answer is no, then we need to update the spec, SYCL-RT and all the adapters that are using UR_RESULT_ERROR_ADAPTER_SPECIFIC in this way.
There was a problem hiding this comment.
As an interface layer between higher level languages and lower level drivers I don't think UR should stand in the way of either users requesting more information about why a feature they want to use isn't working or adapters informing users why performance is suboptimal.
As @fabiomestre says, the spec already supports this but does so in an unclear way. If you are concearned about the performance implecations in an adatper @jandres742 then perhaps we should think about ways of opting in to their use rather than being on by default, perahps with an environment variable.
There was a problem hiding this comment.
thanks @kbenzie , @fabiomestre . I wouldn't even want this to be with env var, as that implies having branches on several places, even on critical paths.
Dont know if we need to clarify on this PR or in the spec that the warnings are optional, but if that's the intention, then I'm ok with this.
There was a problem hiding this comment.
I think its reasonable to have spec language stating that emitting warnings is optional at the discretion of the adapter maintainers. @fabiomestre if you could update the spec language to clarify that?
There was a problem hiding this comment.
Added a bullet point to clarify that the warnings are optional in the latest commit.
could you elaborate on the temporary nature of this change? |
The solution would temporary because there are plans to split UR_RESULT_ERROR_ADAPTER_SPECIFIC into 2 different errors: #762 This would still require the adapters to set a message if they want to return a warning though. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1055 +/- ##
=======================================
Coverage ? 16.14%
=======================================
Files ? 220
Lines ? 30793
Branches ? 3481
=======================================
Hits ? 4973
Misses ? 25770
Partials ? 50 ☔ View full report in Codecov by Sentry. |
scripts/core/adapter.yml
Outdated
|
|
||
| * If `pError` is $X_RESULT_SUCCESS, $X_RESULT_ERROR_ADAPTER_SPECIFIC | ||
| represents a warning instead. This means that the entry-point call did not | ||
| fail. However it might not have behaved as expected. | ||
|
|
||
| * Using $X_RESULT_ERROR_ADAPTER_SPECIFIC to emit warnings is an optional | ||
| feature. Its usage is left at the discretion of adapter maintainers. |
There was a problem hiding this comment.
Indenting the bullet points makes them stand out more to the eye, also matches the surrounding style.
| * If `pError` is $X_RESULT_SUCCESS, $X_RESULT_ERROR_ADAPTER_SPECIFIC | |
| represents a warning instead. This means that the entry-point call did not | |
| fail. However it might not have behaved as expected. | |
| * Using $X_RESULT_ERROR_ADAPTER_SPECIFIC to emit warnings is an optional | |
| feature. Its usage is left at the discretion of adapter maintainers. | |
| * If `pError` is $X_RESULT_SUCCESS, $X_RESULT_ERROR_ADAPTER_SPECIFIC | |
| represents a warning instead. This means that the entry-point call did not | |
| fail. However it might not have behaved as expected. | |
| * Using $X_RESULT_ERROR_ADAPTER_SPECIFIC to emit warnings is an optional | |
| feature. Its usage is left at the discretion of adapter maintainers. |
of UR_RESULT_ERROR_ADAPTER_SPECIFIC when returning warning messages.
are added.