Skip to content

Proposal for UR new warnings mechanism #1330

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
Closed
fabiomestre opened this issue Feb 9, 2024 · 2 comments
Closed

Proposal for UR new warnings mechanism #1330

fabiomestre opened this issue Feb 9, 2024 · 2 comments
Assignees
@martygrant
Labels
specification Changes or additions to the specification

Comments

@fabiomestre
Copy link
Contributor

fabiomestre commented Feb 9, 2024 •
edited
Loading

The warning mechanism that currently exists in Unified Runtime is ambiguous and not very intuitive.

This is the current state (for adapters that use this functionality):

  • The entrypoint that wants to send a warning sets an error message for urAdapterGetLastError and returns UR_RESULT_ERROR_ADAPTER_SPECIFIC
  • The call to urAdapterGetLastError returns UR_RESULT_SUCCESS to signal that it is a warning.
  • If it is an actual error, The call to urAdapterGetLastErrorreturns UR_RESULT_ERROR_ADAPTER_SPECIFIC instead.

This is not intuitive because urAdapterGetLastError is the only entrypoint that can return an error code in a success state. In addition, setting UR_RESULT_SUCCESS to signal a warning doesn't make much sense.

After some internal discussion, we think that the following changes could improve the current workflow:

  • Create a new loader entrypoint that accepts a callback function. This callback function will be called everytime a UR adapter emits a warning.
  • The application would be able to opt out of warnings by simply not setting the callback function.
  • At runtime, we can provide an environment variable to allow the user to enable/disable warnings.
  • urAdapterGetLastError should be simplified to always return UR_RESULT_SUCCESS unless something is wrong with its parameters. This would make it behave in a way that is more in line with other UR entrypoints.

Risks:

  • This would require some changes on the way that SYCL Runtime handles warnings and adapter specific errors from UR.

Related issue: #762
@fabiomestre fabiomestre added the specification Changes or additions to the specification label Feb 9, 2024
@fabiomestre fabiomestre changed the title Proposal for UR warnings mechanism Proposal for UR new warnings mechanism Feb 9, 2024
@fabiomestre fabiomestre added the needs-discussion This needs further discussion label Feb 12, 2024
@fabiomestre
Copy link
Contributor Author

This was discussed on the WG meeting of 14/02/2024. There was no objections to this approach. So we will go ahead with adding the new entrypoint to the specification.

@fabiomestre fabiomestre removed the needs-discussion This needs further discussion label Feb 14, 2024
@martygrant martygrant self-assigned this May 23, 2024
@kbenzie kbenzie mentioned this issue Jun 5, 2024
Merged
@martygrant martygrant mentioned this issue Jun 12, 2024
Closed
@martygrant martygrant mentioned this issue Feb 20, 2025
Merged
@kbenzie kbenzie mentioned this issue Apr 8, 2025
Closed
martygrant added a commit to intel/llvm that referenced this issue Apr 10, 2025
@martygrant @kbenzie
[UR] Logger callback function sink (#17095)
87acf73 
Migrated from oneapi-src/unified-runtime#1748

This PR implements
oneapi-src/unified-runtime#1330 through a new
logger sink: a user configurable callback. It introduces some spec
additions:

- `typedef void (*ur_logger_output_callback_t)(ur_logger_level_t level,
const char *pLoggerMsg, void *pUserData)`
- `urSetLoggerCallback(ur_adapter_handle_t hAdapter,
ur_logger_output_callback_t pfnLoggerCallback, void *pUserData,
ur_logger_level_t level`
)`
- `urSetLoggerCallbackLevel(ur_adapter_handle_t hAdapter,
ur_logger_level_t level)`
- `typedef enum ur_logger_level_t` (moved the logger::level enum into
the spec)

This new logger sink will only be constructed once a user makes a call
to `urSetLoggerCallback` (which is called from the UR
`urAdapterSetLoggerCallback` entry point), supplying their own callback
function. They can set the minimum logging level through
`urSetLoggerCallbackLevel`. Any subsequent logging calls will
additionally make a call to the supplied callback where the log level,
message and user data will be sent.

A callback has been setup in the SYCL RT in `sycl/source/detail/ur.cpp`
to print logs to `std::err`:
```
void urLoggerCallback([[maybe_unused]] ur_logger_level_t level, const char *msg,
                      [[maybe_unused]] void *userData) {
  std::cerr << msg << std::endl;
}
```

A new test suite `LoggerWithCallbackSink` has been added to test this
new functionality.

---------

Co-authored-by: Kenneth Benzie (Benie) <k.benzie83@gmail.com>
@martygrant
Copy link
Contributor

This has been implemented in intel/llvm#17095

kbenzie added a commit that referenced this issue Apr 14, 2025
@martygrant @kbenzie
Logger callback function sink (#17095)
0cf9d6e 
Migrated from #1748

This PR implements
#1330 through a new
logger sink: a user configurable callback. It introduces some spec
additions:

- `typedef void (*ur_logger_output_callback_t)(ur_logger_level_t level,
const char *pLoggerMsg, void *pUserData)`
- `urSetLoggerCallback(ur_adapter_handle_t hAdapter,
ur_logger_output_callback_t pfnLoggerCallback, void *pUserData,
ur_logger_level_t level`
)`
- `urSetLoggerCallbackLevel(ur_adapter_handle_t hAdapter,
ur_logger_level_t level)`
- `typedef enum ur_logger_level_t` (moved the logger::level enum into
the spec)

This new logger sink will only be constructed once a user makes a call
to `urSetLoggerCallback` (which is called from the UR
`urAdapterSetLoggerCallback` entry point), supplying their own callback
function. They can set the minimum logging level through
`urSetLoggerCallbackLevel`. Any subsequent logging calls will
additionally make a call to the supplied callback where the log level,
message and user data will be sent.

A callback has been setup in the SYCL RT in `sycl/source/detail/ur.cpp`
to print logs to `std::err`:
```
void urLoggerCallback([[maybe_unused]] ur_logger_level_t level, const char *msg,
                      [[maybe_unused]] void *userData) {
  std::cerr << msg << std::endl;
}
```

A new test suite `LoggerWithCallbackSink` has been added to test this
new functionality.

---------

Co-authored-by: Kenneth Benzie (Benie) <k.benzie83@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@martygrant martygrant

Labels
specification Changes or additions to the specification
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@fabiomestre @martygrant