Shall we update the spec to add constraints on the axes and the input rank for the reduction operator? #681

mei1127 · 2024-05-08T03:33:48Z

(This was raised by @a-sully in Chromium CL-5496412 review.)

For WebNN's reduction operator:
Austion proposed to update the spec to add constraints on the axes and the input rank.

The Chromium code which validates reductions also fails if
the input rank is zero
values in axes are duplicated
Shall we update the spec and add WPTs accordingly?

/cc @a-sully

Suggested behavior matches the Chromium prototype implementation. Fixes webmachinelearning#681

inexorabletash · 2024-05-09T22:46:33Z

Makes sense to me - I put up a PR

fdwr · 2024-05-10T03:40:40Z

also fails if the input rank is zero

(repeating here too)
Nop reductions are valid (where axes are empty), and scalars are just another instance of a nop:

x = tensorflow.constant([3,3,3], dtype=tensorflow.float32)
y = tensorflow.reduce_sum(x, axis=[])

# value: tf.Tensor([3. 3. 3.], shape=(3,), dtype=float32)
# shape: (3,)

x = tensorflow.constant([], dtype=tensorflow.float32)
y = tensorflow.reduce_sum(x, axis=[])

# value: tf.Tensor([], shape=(0,), dtype=float32)
# shape: (0,)

What's not valid is a scalar reduction with non-empty axes, because that would index axes beyond the rank, but the existing check that each axis < inputRank already validates that condition, and so checking input rank is redundant and unnecessarily exclusionary. So, we should remove that line in Chromium.

@fdwr

crrev.com/c/5496321 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f

inexorabletash · 2024-05-10T23:01:43Z

Groovy. I have a Chromium CL (with WPT changes) up to remove the rank check crrev.com/c/5529350 but I'm a bit rusty so it may not land for a bit.

@fdwr

crrev.com/c/5496321 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel

@fdwr

crrev.com/c/5496321 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel

fdwr · 2024-05-11T02:27:52Z

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt

Oh interesting. So making the change in Chromium land actually pushes it back into web-platform-tests/wpt.

inexorabletash · 2024-05-11T03:42:34Z

Yeah, we have two-way sync set up with WPT. Mozilla and Apple have something similar. It's not perfect and requires some babysitting, but it's super great when it works.

@fdwr

crrev.com/c/5496321 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel

@fdwr

crrev.com/c/5496321 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel

@fdwr

crrev.com/c/5496321 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel

@fdwr

crrev.com/c/5496321 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel

@fdwr

crrev.com/c/5448942 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel

@fdwr

crrev.com/c/5448942 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel

@fdwr

crrev.com/c/5448942 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel

@fdwr

crrev.com/c/5448942 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel

@fdwr

crrev.com/c/5448942 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5529350 Reviewed-by: Phillis Tang <[email protected]> Reviewed-by: ningxin hu <[email protected]> Commit-Queue: Joshua Bell <[email protected]> Cr-Commit-Position: refs/heads/main@{#1301369}

@fdwr

crrev.com/c/5448942 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5529350 Reviewed-by: Phillis Tang <[email protected]> Reviewed-by: ningxin hu <[email protected]> Commit-Queue: Joshua Bell <[email protected]> Cr-Commit-Position: refs/heads/main@{#1301369}

@fdwr

crrev.com/c/5448942 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5529350 Reviewed-by: Phillis Tang <[email protected]> Reviewed-by: ningxin hu <[email protected]> Commit-Queue: Joshua Bell <[email protected]> Cr-Commit-Position: refs/heads/main@{#1301369}

@fdwr

…testonly Automatic update from web-platform-tests WebNN: Allow 0-D (scalar) reductions crrev.com/c/5448942 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5529350 Reviewed-by: Phillis Tang <[email protected]> Reviewed-by: ningxin hu <[email protected]> Commit-Queue: Joshua Bell <[email protected]> Cr-Commit-Position: refs/heads/main@{#1301369} -- wpt-commits: 22a06eb294cd23846add1c8ca8e234bc9d7260c6 wpt-pr: 46209

@fdwr

…testonly Automatic update from web-platform-tests WebNN: Allow 0-D (scalar) reductions crrev.com/c/5448942 added a restriction on the reduction ops (including argMin/argMax) that the rank of the input tensor must be greater than zero. In webmachinelearning/webnn#681, @fdwr points out that 0-D (scalar) reductions are fine, it's just a no-op. Per that discussion, this removes a restriction. There are already WPT cases for argMin/argMax (although they fail for other reasons); cases are added for the plethora of reduction ops (which also fail for other reasons). Change-Id: Ib10a9b0bb3062f9d2b6001d9a4963b14e751d14f Cq-Include-Trybots: luci.chromium.try:mac14-blink-rel,mac14.arm64-blink-rel,win11-blink-rel Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5529350 Reviewed-by: Phillis Tang <[email protected]> Reviewed-by: ningxin hu <[email protected]> Commit-Queue: Joshua Bell <[email protected]> Cr-Commit-Position: refs/heads/main@{#1301369} -- wpt-commits: 22a06eb294cd23846add1c8ca8e234bc9d7260c6 wpt-pr: 46209

mei1127 changed the title ~~Shall we update the spec to specify the input rank can't be zero for reduce operator?~~ Shall we update the spec to specify the input rank and the axes for reduce operator? May 8, 2024

mei1127 changed the title ~~Shall we update the spec to specify the input rank and the axes for reduce operator?~~ Shall we update the spec to add constraints on the axes of the input rank and reduction operators? May 8, 2024

mei1127 changed the title ~~Shall we update the spec to add constraints on the axes of the input rank and reduction operators?~~ Shall we update the spec to add constraints on the axes and the input rank for the reduction operator? May 8, 2024

inexorabletash added operator specific question labels May 8, 2024

inexorabletash added a commit to inexorabletash/webnn that referenced this issue May 9, 2024

Validate axes and input rank for reduction ops

4055abb

Suggested behavior matches the Chromium prototype implementation. Fixes webmachinelearning#681

inexorabletash mentioned this issue May 9, 2024

Validate no duplicate axes for reduction ops #683

Merged

inexorabletash self-assigned this May 9, 2024

chromium-wpt-export-bot mentioned this issue May 10, 2024

WebNN: Allow 0-D (scalar) reductions web-platform-tests/wpt#46209

Merged

huningxin closed this as completed in #683 May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shall we update the spec to add constraints on the axes and the input rank for the reduction operator? #681

Shall we update the spec to add constraints on the axes and the input rank for the reduction operator? #681

mei1127 commented May 8, 2024 •

edited

Loading

inexorabletash commented May 9, 2024

fdwr commented May 10, 2024 •

edited

Loading

inexorabletash commented May 10, 2024

fdwr commented May 11, 2024

inexorabletash commented May 11, 2024

Shall we update the spec to add constraints on the axes and the input rank for the reduction operator? #681

Shall we update the spec to add constraints on the axes and the input rank for the reduction operator? #681

Comments

mei1127 commented May 8, 2024 • edited Loading

inexorabletash commented May 9, 2024

fdwr commented May 10, 2024 • edited Loading

inexorabletash commented May 10, 2024

fdwr commented May 11, 2024

inexorabletash commented May 11, 2024

mei1127 commented May 8, 2024 •

edited

Loading

fdwr commented May 10, 2024 •

edited

Loading