Simple proposal to extend error handling. #64
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abrown
approved these changes
Dec 8, 2023
There was a problem hiding this comment.
I think this makes sense. I added a couple of name change suggestions but realized that then you would have to regenerate ml.md if you accept them. @shschaefer, let me know if you think code makes more sense than error (more specific) and I can follow up with a PR to this one.
wit/wasi-nn.wit
Outdated
| constructor(error: error-code, data: string); | ||
|
|
||
| /// Return the error code. | ||
| error: func() -> error-code; |
There was a problem hiding this comment.
Suggested change
| error: func() -> error-code; | |
| code: func() -> error-code; |
wit/wasi-nn.wit
Outdated
| } | ||
|
|
||
| resource error { | ||
| constructor(error: error-code, data: string); |
There was a problem hiding this comment.
Suggested change
| constructor(error: error-code, data: string); | |
| constructor(code: error-code, data: string); |
There was a problem hiding this comment.
I can fix this inline with the PR. OK?
There was a problem hiding this comment.
I pushed an update.
abrown
added a commit
to abrown/wasi-nn-spec
that referenced
this pull request
Jul 31, 2024
This change migrates the `error` type to return as a record instead of a resource. The previous logic behind WebAssembly#64, IIRC, is that users that may not wish to copy the potentially-large `data` string across the canonical ABI would not have to--they could call the `data` method on the resource if they really needed it. Technical complexity during implementation is making me reconsider this decision. While implementing this in Wasmtime, I realized that after an error happens, the current "error as resource" scheme results in a multi-step failure algorithm: - construct the internal host error - register the host error in the resource table--this may fail! - return the host error as a resource This complexity does not mean necessarily that the spec _must_ be changed; I believe there is a way to "make it work." But if users end up troubleshooting a resource failure that happens during a wasi-nn failure, we risk making things a bit too complex for them. And, if indeed the original argument for using resources was avoiding performance overhead, there is a case to be made that in failure modes performance is not the most critical for users. I'll open this seeking feedback, not to merge immediately.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per discussion in the WebAssembly Machine Learning Group, this proposal is a simple extension to the form of errors propagated by ML backends behind the WASI-NN interface.
The core of the proposal adds an error resource return type which is currently comprised of an explicit error code and a backend specific status message string. This allows for consistent marshalling of the base return type and framework specific handling of the extended return.
An alternate proposal could be to use the variant type from the Component Model and provide a more hierarchical or structured return. As the variation is expected to be in vendor specific framework code, the additional expressiveness seems unnecessary.
I also changed "timeout" to "busy" as well as added the "security" and "unknown" error types.