Return errors as records, not resources #75

abrown · 2024-07-31T22:34:35Z

This change migrates the error type to return as a record instead of a resource. The previous logic behind #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.

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.

abrown · 2024-07-31T22:35:19Z

cc: @shschaefer

In bytecodealliance#8873, we stopped tracking the wasi-nn's upstream WIT files temporarily because it was not clear to me at the time how to implement errors as CM resources. This PR fixes that, resuming tracking in the `vendor-wit.sh` and implementing what is needed in the wasi-nn crate. This leaves several threads unresolved, though: - it looks like the `vendor-wit.sh` has a new mechanism for retrieving files into `wit/deps`--at some point wasi-nn should migrate to use that (?) - it's not clear to me that "errors as resources" is even the best approach here; I've opened [bytecodealliance#75] to consider the possibility of using "errors as records" instead. [bytecodealliance#75]: WebAssembly/wasi-nn#75

In bytecodealliance#8873, we stopped tracking the wasi-nn's upstream WIT files temporarily because it was not clear to me at the time how to implement errors as CM resources. This PR fixes that, resuming tracking in the `vendor-wit.sh` and implementing what is needed in the wasi-nn crate. This leaves several threads unresolved, though: - it looks like the `vendor-wit.sh` has a new mechanism for retrieving files into `wit/deps`--at some point wasi-nn should migrate to use that (?) - it's not clear to me that "errors as resources" is even the best approach here; I've opened [bytecodealliance#75] to consider the possibility of using "errors as records" instead. [bytecodealliance#75]: WebAssembly/wasi-nn#75 prtest:full

In bytecodealliance#8873, we stopped tracking the wasi-nn's upstream WIT files temporarily because it was not clear to me at the time how to implement errors as CM resources. This PR fixes that, resuming tracking in the `vendor-wit.sh` and implementing what is needed in the wasi-nn crate. This leaves several threads unresolved, though: - it looks like the `vendor-wit.sh` has a new mechanism for retrieving files into `wit/deps`--at some point wasi-nn should migrate to use that (?) - it's not clear to me that "errors as resources" is even the best approach here; I've opened [bytecodealliance#75] to consider the possibility of using "errors as records" instead. - this PR identifies that the constructor for errors is in fact unnecessary, prompting an upstream change ([bytecodealliance#76]) that should eventually be implemented here. [bytecodealliance#75]: WebAssembly/wasi-nn#75 [bytecodealliance#76]: WebAssembly/wasi-nn#76 prtest:full

In #8873, we stopped tracking the wasi-nn's upstream WIT files temporarily because it was not clear to me at the time how to implement errors as CM resources. This PR fixes that, resuming tracking in the `vendor-wit.sh` and implementing what is needed in the wasi-nn crate. This leaves several threads unresolved, though: - it looks like the `vendor-wit.sh` has a new mechanism for retrieving files into `wit/deps`--at some point wasi-nn should migrate to use that (?) - it's not clear to me that "errors as resources" is even the best approach here; I've opened [#75] to consider the possibility of using "errors as records" instead. - this PR identifies that the constructor for errors is in fact unnecessary, prompting an upstream change ([#76]) that should eventually be implemented here. [#75]: WebAssembly/wasi-nn#75 [#76]: WebAssembly/wasi-nn#76 prtest:full

This change alters the `wasi-nn` world to split out two different modes of operation: - `inference`: this continues the traditional mechanism for computing with wasi-nn, by passing named `tensor`s to a `context`. Now that `tensor`s are resources, we pass all inputs and return all outputs together, eliminating `get-input` and `set-output` - `prompt`: this new mode expects a `string` prompt which is passed along to a backend LLM. The returned string is not streamed, but could be in the future This change also adds metadata modification of the `graph` via `list-properties`, `get-property` and `set-property`. It is unclear whether these methods should hang off the `context` objects instead (TODO). It is also unclear whether the model of `load`-ing a `graph` and then initializing it into one of the two modes via `inference::init` or `prompt::init` is the best approach; most graphs are one or the other so it does not make sense to open the door to `init` failures. [bytecodealliance#74] (replace `load` with `load-by-name`) is replicated in this commit. [bytecodealliance#75] (return errors as records) and [bytecodealliance#76] (remove the error constructor) is superseded by this commit, since every error is simply returned as a `string` and the `error` resource is removed. [bytecodealliance#74]: WebAssembly/wasi-nn#74 [bytecodealliance#75]: WebAssembly/wasi-nn#75 [bytecodealliance#76]: WebAssembly/wasi-nn#76

This change proposes a different approach to simplifying errors than PR WebAssembly#75 (errors as records). Here we take that approach one step further and completely remove the error code portion entirely, returning only a `string` in the error case. Is this an anti-pattern, though? Some would argue that this is the case, since users might have to resort to string-matching for custom error handling. Others, however, might argue that the error codes were low value anyways since we expect to only see one or maybe two variants as failures to a single call. In any case, this PR offers an alternative to WebAssembly#75 for discussion sake.

abrown mentioned this pull request Aug 1, 2024

wasi-nn: track upstream specification bytecodealliance/wasmtime#9056

Merged

abrown mentioned this pull request Aug 13, 2024

Return errors as strings, not resources #78

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return errors as records, not resources #75

Return errors as records, not resources #75

abrown commented Jul 31, 2024

abrown commented Jul 31, 2024

Return errors as records, not resources #75

Are you sure you want to change the base?

Return errors as records, not resources #75

Conversation

abrown commented Jul 31, 2024

abrown commented Jul 31, 2024