Add missing validation steps for several ops#820
Add missing validation steps for several ops#820fdwr merged 2 commits intowebmachinelearning:mainfrom inexorabletash:missing-validation
Conversation
A follow-on to #706 and the [audit](https://docs.google.com/spreadsheets/d/1S5-bMWN1hDrkPGiHFCyX-OjcbEBj1a-aWNdtTQ2dcGg) by @huningxin that adds validation steps to several ops identified for edge cases in the Chromium prototype implementation. This touches the following ops: - convTranspose2d - outputSizes items must be valid dimensions - lstm - steps must be greater than 0 - pool2d - windowDimensions must be greater than 0 - pool2d - outputSizes items must be valid dimensions - pool2d - specified output sizes must be floor() or ceil() of calculated output sizes - split - splits (if a number) must be greater than 0 Fixes #818 as well.
|
@huningxin can you take an initial look? I went through the Chromium code and I believe this captures all of the validation missing from the spec for these ops, but I may have missed some. I left "Issue" blocks in this PR for what I believe end up being redundant steps. In a few places we can validate e.g. outputHeight and outputWidth specifically, or we can validate all dimensions of outputShape, or both. I would personally prefer to just validate the individual dimensions and trust chat batches/channels are pre-validated, but maybe there are subtle reasons to keep both. Thoughts? |
index.bs
Outdated
| 1. If its [=list/size=] is not 2, then [=exception/throw=] a {{TypeError}}. | ||
| 1. If any of its [=list/items=] is not a [=valid dimension=], then [=exception/throw=] a {{TypeError}}. | ||
|
|
||
| Issue: The preceding step appears redundant with the validation of |outputHeight| and |outputWidth| below. Remove it? |
There was a problem hiding this comment.
sgtm
But I suppose we still need to validate the user-supplied output size according to stride and calculated output size which is computed by ignoring output padding, and ensure the user-supplied output size satisfying
calculated output size <= user-supplied output size < calculated output size + stride
like Chromium impl does: https://source.chromium.org/chromium/chromium/src/+/main:services/webnn/public/cpp/graph_validation_utils.cc;l=782;drc=8636bb104c0e28b695adfe050dd1b70bfe05cdc8
There was a problem hiding this comment.
Removed the step (and issue), added that validation in a0ab8c7 - please verify?
index.bs
Outdated
| </dl> | ||
| 1. If any [=list/item=] in |outputShape| is not a [=valid dimension=], then [=exception/throw=] a {{TypeError}}. | ||
|
|
||
| Issue: The preceding step appears redundant with the validation of |outputHeight| and |outputWidth| above. Remove it? |
There was a problem hiding this comment.
outputChannels is calculated by step
Let |outputChannels| be |filterOutputChannels| * |options|.{{MLConvTranspose2dOptions/groups}}.
We may need to validate it right after that step? Like Chromium impl does: https://source.chromium.org/chromium/chromium/src/+/main:services/webnn/public/cpp/graph_validation_utils.cc;l=747;drc=8636bb104c0e28b695adfe050dd1b70bfe05cdc8
There was a problem hiding this comment.
Good point. Added validation in a0ab8c7. Please verify?
| 1. Otherwise: | ||
| 1. Let |outputHeight| be the result of [=MLGraphBuilder/calculating convtranspose output size=] given |inputHeight|, |filterHeight|, |padding|[0], |padding|[1], |strides|[0], |dilations|[0], and |outputPadding|[0]. | ||
| 1. Let |outputWidth| be the result of [=MLGraphBuilder/calculating convtranspose output size=] given |inputWidth|, |filterWidth|, |padding|[2], |padding|[3], |strides|[1], |dilations|[1] and |outputPadding|[1]. | ||
| 1. Let |outputHeight| be |calculatedOutputHeight| + |options|.{{MLConvTranspose2dOptions/outputPadding}}[0]. |
There was a problem hiding this comment.
This algorithm is simpler and clearer, the Chromium impl should be updated to this version. Thanks @inexorabletash !
|
@fdwr can you take a look too please? |
SHA: 9f58451 Reason: push, by fdwr Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
3 participants
A follow-on to #706 and the
audit by @huningxin that adds validation steps to several ops identified for edge cases in the Chromium prototype implementation.
This touches the following ops:
Fixes #818 as well.
Preview | Diff