webmachinelearning / webnn

🧠 Web Neural Network API
https://www.w3.org/TR/webnn/
Other
341 stars 42 forks source link

Reconsider `MLOperand` methods #666

Open a-sully opened 1 month ago

a-sully commented 1 month ago

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:

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.

zolkis commented 1 month ago

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

a-sully commented 1 month ago

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)