Content: Define build() steps more rigorously#603
Content: Define build() steps more rigorously#603fdwr merged 5 commits intowebmachinelearning:mainfrom inexorabletash:content-build-steps
Conversation
- Adds validation that passed outputs are neither inputs nor constants, matching the Chromium implementation. - Traverses the graph, starting with outputs, to visit all connected operands. - Previously the outputs were iterated over to process inputs and constants, which didn't make any sense. Inputs are hooked up to [[inputsDescriptors]]. Nothing is specifically mentioned about constants, but an issue about transferring is referenced. (#566) - The impact of MLGraphBuilder re-use (#567) is called out, since it could allow for removing the graph traversal. - Populates graph's [[outputDescriptors]], which was previously just missing. (#448) - Makes most of the validation behavior of build() happen synchronously rather than "in parallel". A promise is still returned, of course. - Converting the graph into an implementation-defined format is called out, which remains in "in parallel" steps. Fixes #448, fixes #457, fixes #552.
Co-authored-by: Reilly Grant <[email protected]>
|
I'll look at this tomorrow, but I wonder if we expect an empty graph to behave like an empty |
Yeah, we were discussing that too. The Chromium impl seems to reject this (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/ml/webnn/ml_graph.cc;l=159) but maybe it's fine? |
| The <dfn method for=MLGraphBuilder>build(|outputs|)</dfn> method steps are: | ||
| </summary> | ||
| 1. Let |promise| be [=a new promise=]. | ||
| 1. If |outputs| is empty, then return [=a new promise=] [=rejected=] with a {{TypeError}}. |
There was a problem hiding this comment.
It's an interesting idea to consider what it would mean for a graph to have inputs but no outputs. I have seen some custom operators in models which emitted no outputs because they existed purely for other side effects, or they might exist for debugging (to print out or dump results). Within WebNN though there is no such thing anyway, and so this check makes sense 👍. The reverse is certainly true though that a graph can have outputs with no inputs. e.g. A graph with the fillSequence operator (renamed to a constant overload later) which fills a tensor with a sequence and has no input. The same would apply to random number generator operators too. (resolve me)
| 1. [=map/For each=] |name| → |operand| of |outputs|: | ||
| 1. If |name| is empty, then return [=a new promise=] [=rejected=] with a {{TypeError}}. | ||
| 1. If [=MLOperand/validating MLOperand=] given |operand| and [=this=] returns false, then return [=a new promise=] [=rejected=] with a {{TypeError}}. | ||
| 1. If |operand| is in [=this=]'s [=MLGraphBuilder/graph=]'s [=computational graph/inputs=] or [=computational graph/constants=], then return [=a new promise=] [=rejected=] with a {{TypeError}}. |
There was a problem hiding this comment.
Ah, so here is the check that would nullify the possibility of a nop graph (say one with just a constant and output), which conceptually seems valid to me, as one scenario is where you have a chained sequence of models with one step that can be an action or a nop, and it may be easier to express by always passing a valid (but empty) graph to that component which executes them all. Though, I'm not too worried about it either, since a caller can always insert a dummy identity node to satisfy this constraint. (resolve me)
SHA: 1062297 Reason: push, by fdwr Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* Synchronously validate input operands/validations Previously the spec had a "validate `MLOperand`" helper that (1) ensured the operand was from the passed `MLGraphBuilder` and (2) that the operand was internally consistent, and this was called during (3) `build()` and (4) only `concat()` among the vending methods. - (1) is needed but can be done when the `MLOperand` is created, giving better feedback, so (3) isn't needed. - (2) is not needed - `MLOperands` are immutable so they can't be created in a bad state. - (4) should be expanded to all `MLOperand` creations that take input `MLOperand`s. This renames the helper, ensures it is called by every `MLOperand` vending method that takes `MLOperand` inputs, and drops it from `build()` (although that will probably collide with PR #603 which should land first). Similar validation is added for `MLActivation`s. For #572 * Apply suggestions from code review Missed a few "if it exists" clauses Co-authored-by: Ningxin Hu <[email protected]> --------- Co-authored-by: Ningxin Hu <[email protected]>
This fell out of making build() more specific - these terms are no longer needed, and the makeup of the MLGraph can be described more abstractly. Discussed here: #603 (review)
4 participants
Adds validation that passed outputs are neither inputs nor constants, matching the Chromium implementation.
Traverses the graph, starting with outputs, to visit all connected operands.
Previously the outputs were iterated over to process inputs and constants, which didn't make any sense. Inputs are hooked up to [[inputsDescriptors]]. Nothing is specifically mentioned about constants, but an issue about transferring is referenced. (Don't transfer input ArrayBuffers #566)
The impact of MLGraphBuilder re-use (Can an MLGraphBuilder be reused? #567) is called out, since it could allow for removing the graph traversal.
Populates graph's [[outputDescriptors]], which was previously just missing. (Add traversal algorithm to buildSync() #448)
Makes most of the validation behavior of build() happen synchronously rather than "in parallel". A promise is still returned, of course.
Converting the graph into an implementation-defined format is called out, which remains in "in parallel" steps.
Fixes #448, fixes #457, fixes #552.
Preview | Diff