webmachinelearning / webnn

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

Make `MLOperandDescriptor.shape` a `required` dictionary member #758

Closed a-sully closed 2 weeks ago

a-sully commented 4 weeks ago

Follow-up to #669. Proposed diff:

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

MLOperandDescriptor.shape is currently optional and defaults to a scalar. This may lead to hard-to-track-down bugs, which I've been personally bitten by!

From @reillyeon on this comment:

[Requiring this member will] make declaring scalars more verbose but I think on the whole it seems like a good idea to help developers avoid mistakes.

a-sully commented 4 weeks ago

This is especially relevant in light of MLOperandDescriptor.dimensions being renamed to MLOperandDescriptor.shape (in #669). Callers who pass a dimensions field will see their code break, as the dimensions member will silently be ignored.

This alone is not sufficient reason for making shape required, though if we agree this is a good change then it would be nice to make it in tandem with #669!

fdwr commented 2 weeks ago

I wonder why it had an optional default to begin with? 🤔 Maybe it was for simple scalar constants, but with the constant overload that takes an MLNumber, this default may not be useful anymore anyway.

huningxin commented 2 weeks ago

I wonder why it had an optional default to begin with?

The optional default was for creating a scalar descriptor. I agreed making it required would be less error-prone.

a-sully commented 2 weeks ago

Put up #764. PTAL!

I'd like to merge the corresponding Chromium change in tandem with removing our grace-period-support for dimensions (so, about a milestone from now) so that anyone who hasn't migrated will break loudly rather than quietly. That shouldn't stop us from making the spec change now, though :)