Should MLContext.compute() throw an exception if the inputs validation steps fail?

[huningxin]

Regarding to the existing MLContext.compute() algorithm, if input validation steps fail, it returns a rejected promise:

3. If graph.[[context]] is not this, then return a new promise rejected with a TypeError.

4. If graph.[[context]].[[contextType]] is not "default", then return a new promise rejected with an "OperationError" DOMException.

5. If validating graph resources given inputs and graph.[[inputDescriptors]] returns false, then return a new promise rejected with a TypeError.

6. If validating graph resources given outputs and graph.[[outputDescriptors]] returns false, then return a new promise rejected with a TypeError.

In Chromium CL-5409549 review, @a-sully suggested these synchronous input validation steps should throw a TypeError:

From the spec's perspective, the steps (at a very high level) should be:

1. Validate inputs. If anything fails, throw a TypeError
2. Create a promise
3. Post async work to some task source. If anything fails when running async on that task source, reject the promise
4. Return the promise

Additionally, the following two steps may need to re-throw the exception of transferring an MLNamedArrayBufferViews steps because its transfer an ArrayBuffer steps will throw a TypeError if DetachArrayBuffer fails.

7. Let transferredInputs be the result of transferring MLNamedArrayBufferViews inputs with realm.

8. Let transferredOutputs be the result of transferring MLNamedArrayBufferViews outputs with realm.

[a-sully]

In Chromium CL-5409549 review, @a-sully suggested these synchronous input validation steps should throw a TypeError:

From the spec's perspective, the steps (at a very high level) should be:

1. Validate inputs. If anything fails, throw a TypeError

I've been searching for precedent here, and I think I might be coming around to rejecting these as promises. A key difference between compute() and the graph-building methods we updated to throw TypeError in #589 is that those methods are all sync, while compute() is async. The Streams spec includes this text in promise-bearing methods (e.g.) which call algorithms that may throw exceptions:

Throwing an exception is treated the same as returning a rejected promise

Note that input validation errors will still be thrown by the WebIDL bindings if incorrect types are passed and for constraints like [EnforceRange]. So perhaps a reasonable bar would be if this validation could theoretically be expressed in WebIDL. Many of the validation steps mentioned above don't seem expressible in WebIDL, so perhaps it's okay if these are rejected?

Additionally, the following two steps may need to re-throw the exception of transferring an MLNamedArrayBufferViews steps because its transfer an ArrayBuffer steps will throw a TypeError if DetachArrayBuffer fails.

7. Let transferredInputs be the result of transferring MLNamedArrayBufferViews inputs with realm.

8. Let transferredOutputs be the result of transferring MLNamedArrayBufferViews outputs with realm.

Using the argument above, we should probably convert these thrown exceptions into promise rejections

[inexorabletash]

Note that input validation errors will still be thrown by the WebIDL bindings if incorrect types are passed and for constraints like [EnforceRange].

That's not true. Web methods that return a promise never throw synchronously. This is possible in JavaScript but considered incredibly bad API design. See: https://www.w3.org/2001/tag/doc/promises-guide#errors - as that says,

For Web IDL-based specs, this is taken care of automatically if you declare your operations to return a promise type. Any exceptions thrown by such operations, or by the Web IDL-level type conversions and overload resolution, are automatically converted into rejections.

[inexorabletash]

The Chromium/Blink implementation also takes care of this automagically - if control returns to script from a promise-vending IDL function, any thrown exceptions are turned into promise rejections. I assume other implementations have similar plumbing.

[a-sully]

Note that input validation errors will still be thrown by the WebIDL bindings if incorrect types are passed and for constraints like [EnforceRange].

That's not true. Web methods that return a promise never throw synchronously

TIL! In that case, I withdraw my suggestion that we should throw synchronously. Let's update this issue to instead ensure that all promise-bearing methods reject promises where they may currently throw? (such as steps 7 and 8 above)

[huningxin]

Thanks @inexorabletash and @a-sully for the discussion! According to Austin's last comment, I am going to close this issue and open a new one to track

ensure that all promise-bearing methods reject promises where they may currently throw? (such as steps 7 and 8 above)

[huningxin]

ensure that all promise-bearing methods reject promises where they may currently throw? (such as steps 7 and 8 above)

Before I open a new issue, I am not sure the exceptions thrown in step 7 and 8 are converted to promise rejection automatically? As https://www.w3.org/2001/tag/doc/promises-guide#errors says

For Web IDL-based specs, this is taken care of automatically if you declare your operations to return a promise type. Any exceptions thrown by such operations, or by the Web IDL-level type conversions and overload resolution, are automatically converted into rejections.

Should we make the conversation for step 7 and 8 explicitly?

[inexorabletash]

It should be automatic, but being explicit for subtle things like this is a good idea. Something like "If that throws an exception, return a promise rejected with the exception".

[huningxin]

Opened #634