Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider MLOperand methods #666

Open
a-sully opened this issue Apr 30, 2024 · 2 comments
Open

Reconsider MLOperand methods #666

a-sully opened this issue Apr 30, 2024 · 2 comments
Labels

Comments

@a-sully
Copy link
Contributor

a-sully commented Apr 30, 2024

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.

@zolkis
Copy link
Collaborator

zolkis commented Apr 30, 2024

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

@a-sully
Copy link
Contributor Author

a-sully commented Apr 30, 2024

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants