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.

Sign up for GitHub

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

Jump to bottom

[SPEC] Clarify the behaviour of UR warning messages #1055

Closed
wants to merge 3 commits into from
Closed

[SPEC] Clarify the behaviour of UR warning messages #1055

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c798b2
[SPEC] Clarify the behaviour of UR warning messages
fabiomestre Nov 9, 2023
64765ea
Clarify that warnings are optional
fabiomestre Nov 28, 2023
e5dd7bd
Fix indentation
fabiomestre Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion include/ur_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,14 +847,21 @@ urAdapterRetain(
);

///////////////////////////////////////////////////////////////////////////////
/// @brief Get the last adapter specific error.
/// @brief Get the last adapter specific warning or error.
///
/// @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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@fabiomestre fabiomestre Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@kbenzie kbenzie Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a bullet point to clarify that the warnings are optional in the latest commit.

/// represents a warning instead. This means that the entry-point call did not
/// fail. However it might not have behaved as expected.
///
/// * Using ::UR_RESULT_ERROR_ADAPTER_SPECIFIC to emit warnings is an optional
/// feature. Its usage is left at the discretion of adapter maintainers.
///
/// * Implementations *must* store the message and error code in thread-local
/// storage prior to returning ::UR_RESULT_ERROR_ADAPTER_SPECIFIC.
///
Expand Down
9 changes: 8 additions & 1 deletion scripts/core/adapter.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,19 @@ params:
[in] Adapter handle to retain
--- #--------------------------------------------------------------------------
type: function
desc: "Get the last adapter specific error."
desc: "Get the last adapter specific warning or error."
details: |
To be used after another entry-point has returned
$X_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 $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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indenting the bullet points makes them stand out more to the eye, also matches the surrounding style.

Suggested change
* 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍


* Implementations *must* store the message and error code in thread-local
storage prior to returning $X_RESULT_ERROR_ADAPTER_SPECIFIC.
Expand Down
9 changes: 8 additions & 1 deletion source/loader/ur_libapi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,14 +353,21 @@ ur_result_t UR_APICALL urAdapterRetain(
}

///////////////////////////////////////////////////////////////////////////////
/// @brief Get the last adapter specific error.
/// @brief Get the last adapter specific warning or error.
///
/// @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
/// represents a warning instead. This means that the entry-point call did not
/// fail. However it might not have behaved as expected.
///
/// * Using ::UR_RESULT_ERROR_ADAPTER_SPECIFIC to emit warnings is an optional
/// feature. Its usage is left at the discretion of adapter maintainers.
///
/// * Implementations *must* store the message and error code in thread-local
/// storage prior to returning ::UR_RESULT_ERROR_ADAPTER_SPECIFIC.
///
Expand Down
9 changes: 8 additions & 1 deletion source/ur_api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,14 +311,21 @@ ur_result_t UR_APICALL urAdapterRetain(
}

///////////////////////////////////////////////////////////////////////////////
/// @brief Get the last adapter specific error.
/// @brief Get the last adapter specific warning or error.
///
/// @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
/// represents a warning instead. This means that the entry-point call did not
/// fail. However it might not have behaved as expected.
///
/// * Using ::UR_RESULT_ERROR_ADAPTER_SPECIFIC to emit warnings is an optional
/// feature. Its usage is left at the discretion of adapter maintainers.
///
/// * Implementations *must* store the message and error code in thread-local
/// storage prior to returning ::UR_RESULT_ERROR_ADAPTER_SPECIFIC.
///
Expand Down