Drop dynamic tensor support#281
Conversation
index.bs
Outdated
| 1. The value of |dimension| must be greater than 0. | ||
| 1. Set |inputSize| to the product of |inputSize| and |dimension|. | ||
| 1. The kind of |value| must be compatible with |inputDesc|.{{MLOperandDescriptor/type}} according to [this table](#appendices-mloperandtype-arraybufferview-compatibility). | ||
| 1. The length of |value| must be the same as |inputSize|. |
There was a problem hiding this comment.
Clarify this is "byte length" of |value| ?
Same applies to ArrayBufferView used in the rest of the change.
There was a problem hiding this comment.
This is element length. The algorithm steps above compute the element length (|inputSize|) of |input|. So the element length of |value| is used accordingly.
Probably using "element length" instead of "length" of |value| would be clearer?
There was a problem hiding this comment.
We could add our own definition of "length" (or give it a more specific name such as "element length") into this spec and then refer to that definition from within this spec.
Examples:
https://infra.spec.whatwg.org/#string-length
https://infra.spec.whatwg.org/#byte-sequence-length
https://infra.spec.whatwg.org/#string-code-point-length
https://dom.spec.whatwg.org/#concept-node-length
https://streams.spec.whatwg.org/#pull-into-descriptor-byte-length
There was a problem hiding this comment.
Adding our own definition of "element length" (or simply "length") works.
We can say something like:
In the rest of document, <dfn>length</dfn> of an ArrayBufferView is:
- array.[[ArrayLength]] for TypedArray (link to TypedArray spec in JavaScript)
- ... for XXX
<dfn>byteLength</def> of an ArrayBufferView is:
- ... for TypedArray
- ... for XXX
Then use bikeshed's auto-link [=length=] to refer to these definitions, so there's no ambiguity.
There was a problem hiding this comment.
Thanks for the pointers.
After revisiting ArrayBufferView definition and usage in WebIDL spec, it looks like we'd better to use [[ByteLength]] for its viewing ArrayBuffer. That's actually raised by @wacky6 in his first comment.
So in the latest commit, I implemented the algorithm steps to calculate byte length of an MLOperandDescriptor. It then can be reused to validate the input and output ArrayBufferView's [[ByteLength].
Please take another look.
anssiko
left a comment
anssiko
left a comment
There was a problem hiding this comment.
Thanks @huningxin!
I provided some comments. I'm unblocking my review, and will defer to @wchao1115 @wacky6 on this design.
The byte length of an MLOperandDescriptor is reused to validate the input and output buffers.
index.bs
Outdated
| 1. Set |outputSize| to the product of |outputSize| and |dimension|. | ||
| 1. If |outputSize| is greater than the length of |value|, then: | ||
| 1. Let |outputDesc| be |graph|.{{MLGraph/[[outputDescriptors]]}}[|key|]. | ||
| 1. If the data type of |outputTensor| is not compatible with |outputDesc|.{{MLOperandDescriptor/type}}, then throw a {{DataError}} {{DOMException}} and stop. |
There was a problem hiding this comment.
nit: ideally "compatible" should link to an algorithm explaining what's compatible.
There was a problem hiding this comment.
Agreed. Thanks for pointing it out. Actually this PR inherits this issue. Could we file a corresponding issue in github and fix it in a separate PR?
There was a problem hiding this comment.
@wacky6 , I proposed a fix in the latest commit that validates the data type against the element type of TypedArray (by referring to the corresponding spec definition). Please take another look whether it looks good. Thanks.
|
Thanks for the review and approval. I am going to merge it. |
SHA: ebf5da2 Reason: push, by @huningxin Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This CL implements the WebNN spec update [1] that changes the value type of MLOperandDescriptor.dimensions from long to unsigned long. It also changes the implementation of MLOperand and MLGraphBuilder to use uint32_t accordingly. [1]: webmachinelearning/webnn#281 Bug: 1273291 Change-Id: I5e17004f6b37118223c5dd1d38a0c051b180a2b8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3863299 Reviewed-by: Jiewei Qian <[email protected]> Commit-Queue: ningxin hu <[email protected]> Cr-Commit-Position: refs/heads/main@{#1041375}
4 participants
fix #279
This is feature is not widely supported by target native ML APIs. Otherwise the implementation likely leads to higher latency for the 1st inference and shape change. A framework can still support dynamic-tensor in its own way, and it can build and compute WebNN graphs when the dynamic-tensor's sizes are known during model-inference time.
@wchao1115 @anssiko @yuhonglin @wacky6 @gramalingam @RafaelCintron , PTAL. Thanks!
Preview | Diff