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

Make MLOperandDescriptor.shape a required dictionary member #758

Closed
a-sully opened this issue Sep 10, 2024 · 4 comments · Fixed by #764
Closed

Make MLOperandDescriptor.shape a required dictionary member #758

a-sully opened this issue Sep 10, 2024 · 4 comments · Fixed by #764
Labels

Comments

@a-sully
Copy link
Contributor

a-sully commented Sep 10, 2024

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
Copy link
Contributor Author

a-sully commented Sep 10, 2024

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
Copy link
Collaborator

fdwr commented Sep 23, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

a-sully commented Sep 24, 2024

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 :)

@fdwr fdwr closed this as completed in #764 Sep 25, 2024
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 16, 2024
See webmachinelearning/webnn#758

Also removes the temporary support for passing "dimensions" which was
added in https://crrev.com/e7e99aa5

Bug: 365813262
Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,win11-blink-rel
Change-Id: Ib714ae540da7fbd7d55365dc739bfb8dbf266406
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850659
Reviewed-by: ningxin hu <ningxin.hu@intel.com>
Commit-Queue: ningxin hu <ningxin.hu@intel.com>
Auto-Submit: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1369192}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 16, 2024
See webmachinelearning/webnn#758

Also removes the temporary support for passing "dimensions" which was
added in https://crrev.com/e7e99aa5

Bug: 365813262
Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,win11-blink-rel
Change-Id: Ib714ae540da7fbd7d55365dc739bfb8dbf266406
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850659
Reviewed-by: ningxin hu <ningxin.hu@intel.com>
Commit-Queue: ningxin hu <ningxin.hu@intel.com>
Auto-Submit: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1369192}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 18, 2024
…required dictionary member, a=testonly

Automatic update from web-platform-tests
webnn: Make MLOperandDescriptor.shape a required dictionary member

See webmachinelearning/webnn#758

Also removes the temporary support for passing "dimensions" which was
added in https://crrev.com/e7e99aa5

Bug: 365813262
Cq-Include-Trybots: luci.chromium.try​:mac14-blink-rel,win11-blink-rel
Change-Id: Ib714ae540da7fbd7d55365dc739bfb8dbf266406
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850659
Reviewed-by: ningxin hu <ningxin.hu@intel.com>
Commit-Queue: ningxin hu <ningxin.hu@intel.com>
Auto-Submit: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1369192}

--

wpt-commits: c2d015181a4956b2f91061595ec7da93823ec3ec
wpt-pr: 48637
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Oct 21, 2024
…required dictionary member, a=testonly

Automatic update from web-platform-tests
webnn: Make MLOperandDescriptor.shape a required dictionary member

See webmachinelearning/webnn#758

Also removes the temporary support for passing "dimensions" which was
added in https://crrev.com/e7e99aa5

Bug: 365813262
Cq-Include-Trybots: luci.chromium.try​:mac14-blink-rel,win11-blink-rel
Change-Id: Ib714ae540da7fbd7d55365dc739bfb8dbf266406
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850659
Reviewed-by: ningxin hu <ningxin.hu@intel.com>
Commit-Queue: ningxin hu <ningxin.hu@intel.com>
Auto-Submit: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1369192}

--

wpt-commits: c2d015181a4956b2f91061595ec7da93823ec3ec
wpt-pr: 48637
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Oct 22, 2024
…required dictionary member, a=testonly

Automatic update from web-platform-tests
webnn: Make MLOperandDescriptor.shape a required dictionary member

See webmachinelearning/webnn#758

Also removes the temporary support for passing "dimensions" which was
added in https://crrev.com/e7e99aa5

Bug: 365813262
Cq-Include-Trybots: luci.chromium.try​:mac14-blink-rel,win11-blink-rel
Change-Id: Ib714ae540da7fbd7d55365dc739bfb8dbf266406
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850659
Reviewed-by: ningxin hu <ningxin.hu@intel.com>
Commit-Queue: ningxin hu <ningxin.hu@intel.com>
Auto-Submit: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1369192}

--

wpt-commits: c2d015181a4956b2f91061595ec7da93823ec3ec
wpt-pr: 48637
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 22, 2024
…required dictionary member, a=testonly

Automatic update from web-platform-tests
webnn: Make MLOperandDescriptor.shape a required dictionary member

See webmachinelearning/webnn#758

Also removes the temporary support for passing "dimensions" which was
added in https://crrev.com/e7e99aa5

Bug: 365813262
Cq-Include-Trybots: luci.chromium.try​:mac14-blink-rel,win11-blink-rel
Change-Id: Ib714ae540da7fbd7d55365dc739bfb8dbf266406
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850659
Reviewed-by: ningxin hu <ningxin.huintel.com>
Commit-Queue: ningxin hu <ningxin.huintel.com>
Auto-Submit: Austin Sullivan <asullychromium.org>
Cr-Commit-Position: refs/heads/main{#1369192}

--

wpt-commits: c2d015181a4956b2f91061595ec7da93823ec3ec
wpt-pr: 48637

UltraBlame original commit: 9f95e773e94fb574ef3183aeac32974ee03e0cd9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 22, 2024
…required dictionary member, a=testonly

Automatic update from web-platform-tests
webnn: Make MLOperandDescriptor.shape a required dictionary member

See webmachinelearning/webnn#758

Also removes the temporary support for passing "dimensions" which was
added in https://crrev.com/e7e99aa5

Bug: 365813262
Cq-Include-Trybots: luci.chromium.try​:mac14-blink-rel,win11-blink-rel
Change-Id: Ib714ae540da7fbd7d55365dc739bfb8dbf266406
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850659
Reviewed-by: ningxin hu <ningxin.huintel.com>
Commit-Queue: ningxin hu <ningxin.huintel.com>
Auto-Submit: Austin Sullivan <asullychromium.org>
Cr-Commit-Position: refs/heads/main{#1369192}

--

wpt-commits: c2d015181a4956b2f91061595ec7da93823ec3ec
wpt-pr: 48637

UltraBlame original commit: 9f95e773e94fb574ef3183aeac32974ee03e0cd9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 22, 2024
…required dictionary member, a=testonly

Automatic update from web-platform-tests
webnn: Make MLOperandDescriptor.shape a required dictionary member

See webmachinelearning/webnn#758

Also removes the temporary support for passing "dimensions" which was
added in https://crrev.com/e7e99aa5

Bug: 365813262
Cq-Include-Trybots: luci.chromium.try​:mac14-blink-rel,win11-blink-rel
Change-Id: Ib714ae540da7fbd7d55365dc739bfb8dbf266406
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850659
Reviewed-by: ningxin hu <ningxin.huintel.com>
Commit-Queue: ningxin hu <ningxin.huintel.com>
Auto-Submit: Austin Sullivan <asullychromium.org>
Cr-Commit-Position: refs/heads/main{#1369192}

--

wpt-commits: c2d015181a4956b2f91061595ec7da93823ec3ec
wpt-pr: 48637

UltraBlame original commit: 9f95e773e94fb574ef3183aeac32974ee03e0cd9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants