Reconsider MLOperand methods

The MLOperand interface looks like:

interface MLOperand {
  MLOperandDataType dataType();
  sequence<unsigned long> shape();
};

Each of these methods meets the criteria for being an attribute (https://w3ctag.github.io/design-principles/#attributes-vs-methods), and are const once the MLOperand is constructed. Is we are to have an accessor on MLOperand for each key in the MLOperandDescriptor which created it, then these should be readonly attributes.

That being said, once we open this can of worms, there are several options we could choose from:

1. Getter which returns an `MLOperandDescriptor`

interface MLOperand {
-  MLOperandDataType dataType();
-  sequence<unsigned long> shape();
+  MLOperandDescriptor descriptor();
};

This is my preferred option. Unfortunately, JavaScript dictionaries are not read-only so (unless we want to push to introduce FrozenDictionary) we should use a getter.

2. Naive conversion to `readonly attribute`

interface MLOperand {
-  MLOperandDataType dataType();
+  readonly attribute MLOperandDataType dataType;
-  sequence<unsigned long> shape();
+  readonly attribute sequence<unsigned long> shape;
};

Seems reasonable. But why have an attribute called shape which refers to the same thing as MLOperandDescriptor.dimensions?

3. Rename `shape` to `dimensions`, or vice versa

dictionary MLOperandDescriptor {
  required MLOperandDataType dataType;
-  sequence<[EnforceRange] unsigned long> dimensions = [];
+  sequence<[EnforceRange] unsigned long> shape = [];
};

interface MLOperand {
-  MLOperandDataType dataType();
+  readonly attribute MLOperandDataType dataType;
-  sequence<unsigned long> shape();
+  readonly attribute sequence<unsigned long> shape;
};

MLOperandDescriptor takes a dimensions field, while querying this field from an MLOperand is done with the shape() method. As per https://w3ctag.github.io/design-principles/#attribute-reuse, we should re-use terms rather than unnecessarily introducing new vocabulary.

A quick CTRL-F of the spec text shows:

139 instances of "dimensions", mostly due to MLOperandDescriptor.dimensions
483 instances of "shape", which seems to be used more commonly in prose (e.g. "the shape of the input tensor")

We should decide to use one or the other (and possibly continue this discussion on another issue regardless)

4. Make `MLOperandDescriptor` an `interface`

-dictionary MLOperandDescriptor {
+interface MLOperandDescriptor {
  required MLOperandDataType dataType;
  sequence<[EnforceRange] unsigned long> dimensions = [];
};

interface MLOperand {
-  MLOperandDataType dataType();
-  sequence<unsigned long> shape();
+  readonly attribute MLOperandDescriptor descriptor;
};

This is my least favorite, but I figured I'd list this here for completeness.

m2c: 3(2|1)
(3 applied to 2 or 1) :)

I agree that we should do (3) regardless. Filed #669 to track that

Also filed #670 to track making MLOperandDescriptor.dimensions frozen. We should do this anyways, but especially if we're providing a getter that returns an MLOperandDescriptor then it would be nice if the array was frozen!

Given those expected improvements, my preference is (1)

Re: (2) - sequences are disallowed as the type of an attribute: https://webidl.spec.whatwg.org/#idl-sequence

This restriction exists so that it is clear to specification writers and API users that sequences are copied rather than having references to them passed around.

... which is a shame as a method does seem like overkill.

Rename shape to dimensions

Doh, as a fan of internal consistency in API's, I should have originally proposed dimensions() here for consistency 🤦‍♂️.

interface MLOperand {
  MLOperandDataType dataType();
- sequence<unsigned long> shape();
+ sequence<unsigned long> dimensions();
}

Options:

(a) Change helper MLOperand::shape() -> MLOperand::dimensions()
(b) Change field MLOperandDesc::dimensions -> MLOperandDesc::shape

Pros/Cons

(a) Change helper MLOperand::shape() -> MLOperand::dimensions()

➕ Resolves inconsistency with MLOperandDesc::dimensions.
➕ More consistent with nearly all other low-level backends: XNNPACK dims, ANN dimensions, DML dimensionCount and sizes, MPS MPSNDArraySizes, CudNN CUDNN_ATTR_TENSOR_DIMENSIONS, OneDNN dnnl_dims_t.
➕ Semantically makes more sense with common terminology where dimensions means a list of explicit sizes - we speak of the paper dimensions 8x11 and dresser dimensions 45x20x14 ("a measurable extent of some kind, such as length, breadth, depth, or height ... the final dimensions of the pond were 14 ft. x 8 ft").
➕ Much less API breakage impact, since shape() has limited scope.
➖ Dimensions might refer to the actual dimension list, or it might mean the number of dimensions (rank), just like how if you have a field named cats, it could either be a list cats (std::vector<Cats> cats) or a count of cats (uint32_t cats), and you can't tell from the field name alone.
➖ 5 characters longer to type (dimen~~sions~~).
➖ Referring to a set of dimensions (like the inputs to concatenation) is awkward because you can't just pluralize it - it's already plural! So you must say "dimensionsList".

(b) Change field MLOperandDesc::dimensions -> MLOperandDesc::shape

➕ Resolve inconsistency with MLOperand::shape.
➕ More consistent with high-level PyTorch-centric libraries: TensorFlow, PyTorch, NumPy, mlx
➕ 5 characters shorter to type (dimen~~sions~~).
➕ Achieves consistency with MLGraphBuilder::reshape() (though arguably that should have been named resize).
➖ A "shape" does not imply size, but the dimensions in WebNN are literally explicit sizes. "In geometry, shape excludes information about the object's location, scale, orientation, and reflection."[1]. A shape of size 2x4 and 4x8 are identical shapes, but in WebNN, they would not compare equal. "In geometry, two subsets of a Euclidean space have the same shape if one can be transformed to the other by a combination of translations, rotations (together also called rigid transformations), and uniform scalings." [1].
➖ Shapes can be anything (triangle, circle, rhombus), but in reality WebNN does not accept shapes, only N-dimensional hypercubes.
➖ Far greater API breakage potential since MLTensorDesc's are ubiquitous.

So it really doesn't look like shape is a clear win, but rather a toss-up, and semantically it remains dubious, even if a few other ML frameworks made such questionable naming decisions in the past. Also consider how we are going to define the word "shape" in the spec - it will literally be "a shape is a list of dimensions" 😉.

p.s. Let's also be consistent in...

interface MLTensor {
  readonly attribute MLOperandDataType dataType;
  readonly attribute FrozenArray<unsigned long> dimensions;
}

Hmm I'm still inclined to prefer shape over dimensions for the following reasons:

"dimensions" is ambiguous, since it can mean "some number of dimensions" (e.g. "the last two dimensions") or "shape" (e.g. [3, 4]). Consider this paragraph from the description of Broadcasting (emphasis mine):

The simplest example is the application of a scalar constant to an N-dimension tensor with element-wise binary operations such as add() or mul(). Rather than needing to allocate and populate a matching N-dimensional tensor containing multiple copies of the scalar constant, these element-wise operations allow the scalar constant to be used directly, and broadcast the scalar value across the N-dimensional tensor. With the following considerations, the same logic applies to tensors of other dimensions.

Which definition of "dimensions" is being used here? Should the more precise term "ranks" be used here (if it's the former definition)? Or is this referring to "tensors of other shapes" (the latter)?
shape is widely used by other ML frameworks
- numpy's ndarray
- CoreML's MLMultiArray
- TF uses "shape" and TFLite includes a shape() operator
From a compatibility impact perspective... it would be much less impactful to rename MLOperand::shape() to MLOperand::dimensions() than to rename MLOperandDescriptor::dimensions to MLOperandDescriptor::shape

I agree, which is why I think we should make this change while WebNN is still experimental :)

Hmm I'm still inclined to prefer shape over dimensions

I forgot I'd filed a separate issue about this! Please feel free to respond on #669 :)

Regardless of what we name that field, at this point I'm quite convinced we should convert the getter methods on MLOperand to readonly attributes, in line with the MLTensor interface currently implemented in Chromium and proposed in #754

interface MLOperand {
-  MLOperandDataType dataType();
+  readonly attribute MLOperandDataType dataType;
-  sequence<unsigned long> shape();
+  readonly attribute FrozenArray<unsigned long> shape;
};

@a-sully

Regardless of what we name that field, at this point I'm quite convinced we should convert the getter methods on MLOperand to readonly attributes

@a-sully I'm not opposed to that (fewer parentheses to type), but what of Anssi's comment here? #670 (comment)

The WebIDL spec is considering removing support for FrozenArray as a dictionary member type: whatwg/webidl#1399

@a-sully

@a-sully I'm not opposed to that (fewer parentheses to type), but what of Anssi's comment here? #670 (comment)

The WebIDL spec is considering removing support for FrozenArray as a dictionary member type: whatwg/webidl#1399

In this case we're using FrozenArray as a readonly attribute, so that's not relevant here. It would have potentially been relevant to this option:

1. Getter which returns an MLOperandDescriptor

@a-sully

@a-sully: Do you consider MLOperandDescriptor sufficiently complete, now that the struct looks like:

dictionary MLOperandDescriptor {
  required MLOperandDataType dataType;
  required sequence<[EnforceRange] unsigned long> shape;
};

https://www.w3.org/TR/webnn/#api-mloperanddescriptor

I'm now happy with MLOperandDescriptor 👍

As I mentioned in #666 (comment), I still think the MLOperand interface needs to change:

... we should convert the getter methods on MLOperand to readonly attributes, in line with the MLTensor interface currently implemented in Chromium and proposed in #754
interface MLOperand {
-  MLOperandDataType dataType();
+  readonly attribute MLOperandDataType dataType;
-  sequence<unsigned long> shape();
+  readonly attribute FrozenArray<unsigned long> shape;
};

webnn: Deprecate MLOperand members in favor of readonly attributes

Bug 1928001 [wpt PR 48877] - webnn: Deprecate MLOperand members in fa…

Navigation Menu

Repository navigation

Reconsider `MLOperand` methods #666

1. Getter which returns an `MLOperandDescriptor`

2. Naive conversion to `readonly attribute`

3. Rename `shape` to `dimensions`, or vice versa

4. Make `MLOperandDescriptor` an `interface`

Options:

Pros/Cons

1. Getter which returns an `MLOperandDescriptor`

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Reconsider MLOperand methods #666

Description

1. Getter which returns an MLOperandDescriptor

2. Naive conversion to readonly attribute

3. Rename shape to dimensions, or vice versa

4. Make MLOperandDescriptor an interface

Activity

zolkis commented on Apr 30, 2024

a-sully commented on Apr 30, 2024

inexorabletash commented on Jul 30, 2024

fdwr commented on Aug 28, 2024

Options:

Pros/Cons

fdwr commented on Aug 28, 2024

a-sully commented on Sep 9, 2024

a-sully commented on Sep 10, 2024

fdwr commented on Sep 17, 2024

a-sully commented on Sep 17, 2024

1. Getter which returns an MLOperandDescriptor

fdwr commented on Oct 29, 2024

a-sully commented on Oct 29, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Reconsider `MLOperand` methods #666

1. Getter which returns an `MLOperandDescriptor`

2. Naive conversion to `readonly attribute`

3. Rename `shape` to `dimensions`, or vice versa

4. Make `MLOperandDescriptor` an `interface`

1. Getter which returns an `MLOperandDescriptor`