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

Error handling for unsupported adapter features #1161

Open
martygrant opened this issue Dec 5, 2023 · 0 comments
Open

Error handling for unsupported adapter features #1161

martygrant opened this issue Dec 5, 2023 · 0 comments
Milestone

Comments

@martygrant
Copy link
Contributor

Opening a discussion into a separate issue regarding how to handle adapter errors such as when particular features are not supported which was been raised from #1127.

@JackAKirk highlighted that returning UR_RESULT_ERROR_UNSUPPORTED_FEATURE would show as PI_ERROR_INVALID_VALUE in DPC++ which would misinform users, and has suggested that we may need a change to the UR spec for error handling:

I guess you could do that as things stand. Although from what I understand it would require that all backends that can call this API won't be able to return any other errors that map to PI_ERROR_INVALID_OPERATION, and this will have to be guaranteed to remain true into the future. But these are technical implementation questions, which I don't think are the key issue to consider.

Since also presumably all PI_ERROR_* will be removed at some point when PI is removed, maybe the above solution would only be very temporary, and I can see one argument that in the future it might be decided that UR_RESULT_ERROR_UNSUPPORTED_FEATURE will be returned for all such cases where a feature isn't supported. Then the runtime can pass a more specific message if it receives this error.

The above paragraph is more of a specification question which in my point of view is the right type of question to be asking at this stage, since there doesn't appear (tell me if I am wrong) a clear specification in UR for dealing with errors. For such a specification to be stable into the future I can think of some requirements it would need to satisfy (in a rough order of importance):

Support both adapter agnostic and adapter specific errors.
Be compatible with the error requirements of existing supported language runtime specifications (assuming that the language runtime specifications have error handling fit for purpose, and aren't themselves looking into changes in future versions (SYCL is one), in such a case the error requirements to take account of will be the future versions not the existing ones).
Be future proof for error requirements of adapters that might be supported in the future.

For sure the above requirements would take some amount of thought to satisfy, but I don't see (particularly the first two) as being impossible. And before solving this problem is properly undertaken I see it as likely that we will just largely go in circles, or at best zig-zag to a stable solution.

At the moment the process of e.g. this PR is:

step 1. make a change to move in a particular direction due to a single requirement: e.g. get rid of dies within UR
step 2: run into some constraints like e.g. the one in this particular thread.
step 3: solve the constraints and merge pr
step 4: realize that we didn't consider some other constraints and have to start the process over.

Of course it is just my opinion, and it doesn't block this PR, but I'd recommend going straight to the finish line and first plan an error handling spec that is fit for purpose.
The other issue is that without a clear spec people deal with errors in an inconsistent way. If once someone has ideally fully considered the requirements they add docs to https://oneapi-src.github.io/unified-runtime/core/INTRO.html#error-handling for contributors that would be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants