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

Remove sequential filling overload of constant() #656

Merged
merged 1 commit into from
Apr 27, 2024

Conversation

a-sully
Copy link
Contributor

@a-sully a-sully commented Apr 25, 2024

Discussion on #492 (specifically, this comment #492 (comment)) concluded that this overload was not needed. Its behavior can be emulated by filling a buffer in JavaScript to be passed in with the buffer-based constant() overload


Preview | Diff

@fdwr
Copy link
Collaborator

fdwr commented Apr 25, 2024

Thanks Mr. Sully.

I originally proposed adding it, but model analysis (~700 models, 19 of which have the op) shows the:

  • (a) tiny tensors being generated, not worth the beefy GPU effort.
  • (b) increasing sequences are usually used for shape computation which can actually make it worse when the sequential fill is done on the GPU only to be copied back to the CPU for shape computation (meaning it would be better to keep that computation on the CPU to begin with). These cases should be ultimately optimized away by constant folding after shape inference too.
  • (c) very few occurrences, typically once or twice (and in the cases with many more occurrences, 30+, those are all shape related).

Additionally this sidesteps any of the implementation difference issues we encountered between TF and PyTorch and validation ambiguities about infinities in the data - if the caller wants infinity in the buffer, they upload their buffer with infinity (and WebNN doesn't stop them).

This operator was not integrated yet into Chromium and is not used currently by known callers (e.g. here).

Copy link
Collaborator

@fdwr fdwr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@fdwr
Copy link
Collaborator

fdwr commented Apr 25, 2024

@inexorabletash I can't find your comment about the spec missing the IDL definition, but removing this overload makes that earlier comment moot:

[SecureContext, Exposed=(Window, DedicatedWorker)]
interface MLGraphBuilder {
  ...
  // Create an operand for a graph constant.
  MLOperand constant(MLOperandDescriptor descriptor, ArrayBufferView bufferView);

  // Create a single-value operand from the specified number of the specified type.
  MLOperand constant(double value, optional MLOperandDataType type = "float32");
  ...
};

https://www.w3.org/TR/webnn/#idl-index

@inexorabletash
Copy link
Member

@inexorabletash I can't find your comment, but removal of this overload entirely make moot its current absence in the IDL definition:

Indeed. For posterity:

Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@a-sully
Copy link
Contributor Author

a-sully commented Apr 27, 2024

Thanks for the reviews! Please merge at your convenience :)

@fdwr fdwr merged commit 5b396da into webmachinelearning:main Apr 27, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Apr 27, 2024
SHA: 5b396da
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@a-sully a-sully deleted the remove-fillsequence branch April 27, 2024 03:17
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 25, 2024
This CL is to refactor WebNN API conformance tests infrastructure by
optimizing utils.js helper and moving tests from JSON files into each
test file.
It also removes tests of dropped `constant(fillSequence)` op of WebNN API
changes [1].

[1] [Remove sequential filling overload of
constant()](webmachinelearning/webnn#656)

Bug: 331692961
Change-Id: Ie57095d76ed1a87bcbd93dbade8962a1d4461627
Cq-Include-Trybots: luci.chromium.try:win11-blink-rel,mac14-blink-rel,mac14.arm64-blink-rel
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 25, 2024
This CL is to refactor WebNN API conformance tests infrastructure by
optimizing utils.js helper and moving tests from JSON files into each
test file.
It also removes tests of dropped `constant(fillSequence)` op of WebNN API
changes [1].

[1] [Remove sequential filling overload of
constant()](webmachinelearning/webnn#656)

Bug: 331692961
Change-Id: Ie57095d76ed1a87bcbd93dbade8962a1d4461627
Cq-Include-Trybots: luci.chromium.try:win11-blink-rel,mac14-blink-rel,mac14.arm64-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5668527
Auto-Submit: Feng Dai <feng.dai@intel.com>
Commit-Queue: Feng Dai <feng.dai@intel.com>
Reviewed-by: ningxin hu <ningxin.hu@intel.com>
Reviewed-by: David Baron <dbaron@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1332944}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 25, 2024
This CL is to refactor WebNN API conformance tests infrastructure by
optimizing utils.js helper and moving tests from JSON files into each
test file.
It also removes tests of dropped `constant(fillSequence)` op of WebNN API
changes [1].

[1] [Remove sequential filling overload of
constant()](webmachinelearning/webnn#656)

Bug: 331692961
Change-Id: Ie57095d76ed1a87bcbd93dbade8962a1d4461627
Cq-Include-Trybots: luci.chromium.try:win11-blink-rel,mac14-blink-rel,mac14.arm64-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5668527
Auto-Submit: Feng Dai <feng.dai@intel.com>
Commit-Queue: Feng Dai <feng.dai@intel.com>
Reviewed-by: ningxin hu <ningxin.hu@intel.com>
Reviewed-by: David Baron <dbaron@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1332944}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 30, 2024
…sts infrastructure, a=testonly

Automatic update from web-platform-tests
webnn: refactor WebNN API conformance tests infrastructure

This CL is to refactor WebNN API conformance tests infrastructure by
optimizing utils.js helper and moving tests from JSON files into each
test file.
It also removes tests of dropped `constant(fillSequence)` op of WebNN API
changes [1].

[1] [Remove sequential filling overload of
constant()](webmachinelearning/webnn#656)

Bug: 331692961
Change-Id: Ie57095d76ed1a87bcbd93dbade8962a1d4461627
Cq-Include-Trybots: luci.chromium.try​:win11-blink-rel,mac14-blink-rel,mac14.arm64-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5668527
Auto-Submit: Feng Dai <feng.dai@intel.com>
Commit-Queue: Feng Dai <feng.dai@intel.com>
Reviewed-by: ningxin hu <ningxin.hu@intel.com>
Reviewed-by: David Baron <dbaron@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1332944}

--

wpt-commits: 6a7911e50b773379ea947b2a6d18f36760910346
wpt-pr: 47291
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Aug 5, 2024
…sts infrastructure, a=testonly

Automatic update from web-platform-tests
webnn: refactor WebNN API conformance tests infrastructure

This CL is to refactor WebNN API conformance tests infrastructure by
optimizing utils.js helper and moving tests from JSON files into each
test file.
It also removes tests of dropped `constant(fillSequence)` op of WebNN API
changes [1].

[1] [Remove sequential filling overload of
constant()](webmachinelearning/webnn#656)

Bug: 331692961
Change-Id: Ie57095d76ed1a87bcbd93dbade8962a1d4461627
Cq-Include-Trybots: luci.chromium.try​:win11-blink-rel,mac14-blink-rel,mac14.arm64-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5668527
Auto-Submit: Feng Dai <feng.dai@intel.com>
Commit-Queue: Feng Dai <feng.dai@intel.com>
Reviewed-by: ningxin hu <ningxin.hu@intel.com>
Reviewed-by: David Baron <dbaron@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1332944}

--

wpt-commits: 6a7911e50b773379ea947b2a6d18f36760910346
wpt-pr: 47291
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Aug 7, 2024
…sts infrastructure, a=testonly

Automatic update from web-platform-tests
webnn: refactor WebNN API conformance tests infrastructure

This CL is to refactor WebNN API conformance tests infrastructure by
optimizing utils.js helper and moving tests from JSON files into each
test file.
It also removes tests of dropped `constant(fillSequence)` op of WebNN API
changes [1].

[1] [Remove sequential filling overload of
constant()](webmachinelearning/webnn#656)

Bug: 331692961
Change-Id: Ie57095d76ed1a87bcbd93dbade8962a1d4461627
Cq-Include-Trybots: luci.chromium.try​:win11-blink-rel,mac14-blink-rel,mac14.arm64-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5668527
Auto-Submit: Feng Dai <feng.dai@intel.com>
Commit-Queue: Feng Dai <feng.dai@intel.com>
Reviewed-by: ningxin hu <ningxin.hu@intel.com>
Reviewed-by: David Baron <dbaron@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1332944}

--

wpt-commits: 6a7911e50b773379ea947b2a6d18f36760910346
wpt-pr: 47291
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants