Rename MLOperandDescriptor's "dimensions" key to "shape" by a-sully · Pull Request #676 · webmachinelearning/webnn

a-sully · 2024-05-03T22:50:21Z

Fixes #669

a-sully · 2024-09-16T17:43:16Z

This is not a mass find-and-replace because, as I mentioned in #666 (comment):

"dimensions" is ambiguous, since it can mean "some number of dimensions" (e.g. "the last two dimensions") or "shape" (e.g. [3, 4])

I've combed through all the uses of "dimensions" and updated uses of the latter definition to "shape" while keeping uses of the former definition as-is (though this process was manual and therefore fallible. Apologies if I missed anything!)

Thanks!

P.S. I'd like to follow up this change with #758 - I'll put up a PR shortly after this merges

a-sully · 2024-09-16T17:44:55Z

P.S. I'd like to follow up this change with #758 - I'll put up a PR shortly after this merges

P.P.S. I'd like to follow up that change with the changes proposed here #666 (comment)

fdwr · 2024-09-16T18:23:27Z

Sorry for the delay, and I realize this is tedious work. @huningxin and I talked about this some last week, and most importantly, we want to be sure it's done in a way that smoothly transitions existing samples, ORT Web, and people using the former to work on drivers, in a way that doesn't cause hiccups by allowing a grace period with the old name. It may still be an experimental API, but at least 24 people are affected these days by breaking changes.

My personal preference aside, we all agree with intra-API consistency is the most important, and so the current MLOperand::shape() and MLOperandDesc::dimensions disconnect should be resolved one way or another. Full considerations moved to #666 (comment).

reillyeon · 2024-09-16T19:09:24Z

I don't want to spend too much time bikeshedding on this but my reasons for preferring "shape" are:

"shape" is shorter and easier to spell.
"dimensions" could mean either "shape" or "rank" depending on whether it's interpreted as a vector or a scalar.

a-sully · 2024-09-16T19:43:44Z

we want to be sure it's done in a way that smoothly transitions existing samples, ORT Web, and people using the former to work on drivers, in a way that doesn't cause hiccups by allowing a grace period with the old name. It may still be an experimental API, but at least 24 people are affected these days by breaking changes.

Having a grace period where either dimensions or shape are accepted seems reasonable. We can discuss specifics on the Chromium CL, which I'll update shortly

I don't want to spend too much time bikeshedding on this but my reasons for preferring "shape" are:

Agreed, though I'll mention another developer-ergonomics reason I encountered while refactoring the WebNN WPTs, which is that since "dimensions" - while referring to one set of dimensions (i.e. one shape) - is plural, referring to a collection of dimensions is awkward and results in use of variable names like dimensionsArray, rather than just shapes

zolkis · 2024-09-16T19:59:03Z

(Thinking aloud). The way tensor definitions use these terms (for instance here) is that shape combines the use of rank (the number of indices required to get individual elements of a tensor), axis (the composing indices in the rank) and dimension(the number or elements on an axis).

Scalar: rank 0, no axes, no dimension, shape: ().
Euclidean vector (x, y, z): rank 1, one axis with 3 dimensions (on the 0th axis). Shape: (3).
2D matrix of 5 rows and 4 columns: rank 2, i.e. 2 axes (0 and 1), with given dimensions on rows (axis 0) and columns (axis 1). Shape: (5, 4).

In this sense, dimensions() would be ambiguous and shape() would be more exact (contrary to intuition in English).

fdwr · 2024-09-17T07:59:51Z

🤔 Have you considered sizes? It avoids the concern with dimensions (list of dimensions vs dimension count), it's equally short as shape and easy to spell, and it avoids the grammatical issues with shape ("... shape excludes information about the object's location, scale, orientation, and reflection... [unlike a] figure [which] is a representation including both shape and size..." [1]).

After collecting the pro's/con's above and elsewhere, and updating my comment here, I see why this was difficult, because there was no clear winner in this toss-up (if there was, there wouldn't be any discussion ⚖️).

Having a grace period where either dimensions or shape are accepted seems reasonable

Cool, that's really the most important aspect of this. I'm going to approve this because you're clearly intent your direction to avoid dimensions, and you're willing to do the breakage fixup work, but do also consider sizes.

fdwr

👀

index.bs

a-sully · 2024-09-17T15:57:13Z

Thank you for the review! Please merge this at your convenience (the button is not available to me)

fdwr · 2024-09-17T21:30:47Z

Please merge this at your convenience

For stylistic and minor wording choices, I'm content to merge changes, but for bug fixes and content changes, I need my co-editor Ningxin's approval too.

Renaming MLOperandDescriptor.dimensions to MLOperandDescriptor.shape is proposed in this spec PR: webmachinelearning/webnn#676 To avoid breaking all uses of WebNN, this CL adds support for specifying 'shape' without removing support for 'dimensions'. Callers which pass 'dimensions' will see a console warning suggesting they update their code to use 'shape'. This CL was created primarily using targeted find-and-replaces, followed by running git cl format. This CL has no behavioral changes, other than the aforementioned logging. Bug: 365813262 Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel Change-Id: I6b37ebc505a37686709f006b3ecafcefd3e2016d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5502631 Reviewed-by: Reilly Grant <[email protected]> Commit-Queue: Austin Sullivan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1356886}

huningxin

Thanks for putting this PR together. Just some minor clarification questions.

And could you also replace dimensions with shape in WebNN explainer: https://github.com/webmachinelearning/webnn/blob/main/explainer.md

index.bs

a-sully · 2024-09-18T04:30:31Z

And could you also replace dimensions with shape in WebNN explainer: https://github.com/webmachinelearning/webnn/blob/main/explainer.md

Sure, I'll put up a follow-up PR shortly 👍

huningxin

LGTM!

…sions SHA: 53a24db Reason: push, by huningxin Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Renaming MLOperandDescriptor.dimensions to MLOperandDescriptor.shape is proposed in this spec PR: webmachinelearning/webnn#676 To avoid breaking all uses of WebNN, this CL adds support for specifying 'shape' without removing support for 'dimensions'. Callers which pass 'dimensions' will see a console warning suggesting they update their code to use 'shape'. This CL was created primarily using targeted find-and-replaces, followed by running git cl format. This CL has no behavioral changes, other than the aforementioned logging. Bug: 365813262 Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel Change-Id: I6b37ebc505a37686709f006b3ecafcefd3e2016d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5502631 Reviewed-by: Reilly Grant <[email protected]> Commit-Queue: Austin Sullivan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1356886}

… discourage use of dimensions, a=testonly Automatic update from web-platform-tests webnn: Add MLOperandDescriptor.shape and discourage use of dimensions Renaming MLOperandDescriptor.dimensions to MLOperandDescriptor.shape is proposed in this spec PR: webmachinelearning/webnn#676 To avoid breaking all uses of WebNN, this CL adds support for specifying 'shape' without removing support for 'dimensions'. Callers which pass 'dimensions' will see a console warning suggesting they update their code to use 'shape'. This CL was created primarily using targeted find-and-replaces, followed by running git cl format. This CL has no behavioral changes, other than the aforementioned logging. Bug: 365813262 Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel Change-Id: I6b37ebc505a37686709f006b3ecafcefd3e2016d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5502631 Reviewed-by: Reilly Grant <[email protected]> Commit-Queue: Austin Sullivan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1356886} -- wpt-commits: 1aa1ed11f44b7d560d3debc4c16ba691da0aa594 wpt-pr: 48086

… discourage use of dimensions, a=testonly Automatic update from web-platform-tests webnn: Add MLOperandDescriptor.shape and discourage use of dimensions Renaming MLOperandDescriptor.dimensions to MLOperandDescriptor.shape is proposed in this spec PR: webmachinelearning/webnn#676 To avoid breaking all uses of WebNN, this CL adds support for specifying 'shape' without removing support for 'dimensions'. Callers which pass 'dimensions' will see a console warning suggesting they update their code to use 'shape'. This CL was created primarily using targeted find-and-replaces, followed by running git cl format. This CL has no behavioral changes, other than the aforementioned logging. Bug: 365813262 Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel Change-Id: I6b37ebc505a37686709f006b3ecafcefd3e2016d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5502631 Reviewed-by: Reilly Grant <reillygchromium.org> Commit-Queue: Austin Sullivan <asullychromium.org> Cr-Commit-Position: refs/heads/main{#1356886} -- wpt-commits: 1aa1ed11f44b7d560d3debc4c16ba691da0aa594 wpt-pr: 48086 UltraBlame original commit: 07f0bc50e723e5fde447a9fbcb888a800f1cf186

… discourage use of dimensions, a=testonly Automatic update from web-platform-tests webnn: Add MLOperandDescriptor.shape and discourage use of dimensions Renaming MLOperandDescriptor.dimensions to MLOperandDescriptor.shape is proposed in this spec PR: webmachinelearning/webnn#676 To avoid breaking all uses of WebNN, this CL adds support for specifying 'shape' without removing support for 'dimensions'. Callers which pass 'dimensions' will see a console warning suggesting they update their code to use 'shape'. This CL was created primarily using targeted find-and-replaces, followed by running git cl format. This CL has no behavioral changes, other than the aforementioned logging. Bug: 365813262 Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel Change-Id: I6b37ebc505a37686709f006b3ecafcefd3e2016d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5502631 Reviewed-by: Reilly Grant <[email protected]> Commit-Queue: Austin Sullivan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1356886} -- wpt-commits: 1aa1ed11f44b7d560d3debc4c16ba691da0aa594 wpt-pr: 48086

inexorabletash mentioned this pull request Jul 23, 2024

Add helper for creating MLOperandDescriptor to simplify steps #737

Merged

a-sully force-pushed the rename-MLOperandDescriptor-dimensions branch from e6703a8 to c27f8a7 Compare July 30, 2024 20:18

dimensions -> shape

25af965

a-sully marked this pull request as ready for review September 16, 2024 17:35

fdwr requested review from fdwr and huningxin September 16, 2024 18:28

fdwr mentioned this pull request Sep 17, 2024

Rename MLOperandDescriptor's dimensions to shape #669

Closed

fdwr approved these changes Sep 17, 2024

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

a-sully force-pushed the rename-MLOperandDescriptor-dimensions branch from c27f8a7 to bbfa491 Compare September 17, 2024 15:51

chromium-wpt-export-bot mentioned this pull request Sep 18, 2024

webnn: Add MLOperandDescriptor.shape and discourage use of dimensions web-platform-tests/wpt#48086

Merged

huningxin reviewed Sep 18, 2024

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

This was referenced Sep 18, 2024

Rename MLOperandDescriptor.dimensions to MLOperandDescriptor.shape webmachinelearning/webnn-samples#273

Closed

[Web] Rename WebNN EP MLOperandDescriptor's "dimensions" key to "shape" microsoft/onnxruntime#22120

Closed

address fdwr feedback

870885a

a-sully force-pushed the rename-MLOperandDescriptor-dimensions branch from bbfa491 to 870885a Compare September 18, 2024 04:29

huningxin approved these changes Sep 18, 2024

View reviewed changes

huningxin merged commit 53a24db into webmachinelearning:main Sep 18, 2024

ibelem mentioned this pull request Sep 18, 2024

Use both MLOperandDescriptor.dimensions and MLOperandDescriptor.shape webmachinelearning/webnn-samples#274

Merged

a-sully deleted the rename-MLOperandDescriptor-dimensions branch September 18, 2024 15:13

Comments

Conversation

a-sully commented May 3, 2024 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-sully commented Sep 16, 2024

Uh oh!

a-sully commented Sep 16, 2024

Uh oh!

fdwr commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reillyeon commented Sep 16, 2024

Uh oh!

a-sully commented Sep 16, 2024

Uh oh!

zolkis commented Sep 16, 2024

Uh oh!

fdwr commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fdwr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

a-sully commented Sep 17, 2024

Uh oh!

fdwr commented Sep 17, 2024

Uh oh!

huningxin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

a-sully commented Sep 18, 2024

Uh oh!

huningxin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

a-sully commented May 3, 2024 •

edited by pr-preview bot

Loading

fdwr commented Sep 16, 2024 •

edited

Loading

fdwr commented Sep 17, 2024 •

edited

Loading