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

Add support for operations needed for well-known transformers e.g. Segment Anything, Stable Diffusion, etc. #478

Merged
merged 19 commits into from
Dec 11, 2023

Conversation

wchao1115
Copy link
Collaborator

@wchao1115 wchao1115 commented Nov 11, 2023

Issue #375.

  1. Add the following ops: equal, lesser, greater, greaterOrEqual, lesserOrEqual, not, where, copy, sqrt, erf, expand, gather, layerNormalization, reduceArgMin, reduceArgMax, cast, triangular
  2. Remove squeeze
  3. Add a constant overload that accepts start, end, step.
  4. Add shape method in MLOperand for runtime tensor shape query.
  5. Add new data type int64 and uint64 for reduceArgMin and reduceArgMax.

Notes:

  • The proposed identity op is named copy to denote explicit action.
  • The proposed elementwiseIf is named where to be consistent with PyTorch and ONNX.
  • The proposed triangularMatrix is named triangular for brevity.
  • squeeze is removed. Framework seeking squeeze, unsqueeze, flatten should just use reshape. See canonical implementations of these ops as reshape in the reshape section.
  • Instead of the proposed unification of all normalizations under one new normalization operation, a new layerNormalization op is added. There are enough subtleties in the way affine parameters (e.g. scale, bias) are expected across batchNorm, instanceNorm, and layerNorm that could become ambiguous under the unified op. Additionally, downstream implementations of these normalizations have already been done in various platforms. The unification at the WebNN level could potentially confuse framework developers and affects currently defined platform-specific optimizations.
  • The proposed fillSequence op is added as a constant overload due to its nature of resource allocation activity.
  • The new sidebar loosens up the crowded MLGraphBuilder interface section for better op discoverability.

Big thanks to @fdwr for the thoughtful and thorough proposal.


Preview | Diff

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.

Thanks for the great work!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Contributor

@BruceDai BruceDai left a comment

Choose a reason for hiding this comment

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

Thanks @wchao1115 !
Here's a nit, PTAL.

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@inexorabletash
Copy link
Member

Looks like the build isn't clean any more, these issues should be fixed:

https://github.com/webmachinelearning/webnn/actions/runs/6836134208/job/18590708364?pr=478#step:3:885

index.bs Show resolved Hide resolved
@BruceDai
Copy link
Contributor

BruceDai commented Nov 17, 2023

Here's current status for baseline implementation and WPT tests of new adding ops, PTAL, thanks.

Ops Baseline WPT tests
equal, lesser, greater, greaterOrEqual, lesserOrEqual, not pr
where pr WIP
copy pr
reciprocal pr
sqrt pr
erf
expand pr WIP
gather
layerNormalization
reduceArgMin, reduceArgMax
cast
triangular

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 23, 2023
This CL implements MLOperand.dataType() method according to proposal
[1]. This method supports querying the operand data type at runtime.
This implementation names the method dataType() that aligns with the
MLOperandDescriptor.dataType [2].

[1]: webmachinelearning/webnn#478
[2]: https://www.w3.org/TR/webnn/#dom-mloperanddescriptor-datatype

Bug: 1273291
Change-Id: I18c19d089e310b0c24e73dbb8c028fa351782082
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5056627
Reviewed-by: Jiewei Qian <qjw@chromium.org>
Commit-Queue: ningxin hu <ningxin.hu@intel.com>
Cr-Commit-Position: refs/heads/main@{#1228454}
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
@wchao1115
Copy link
Collaborator Author

The new MLOperand methods type() and shape() don't have corresponding definitions.

@inexorabletash Fixed.

@wchao1115
Copy link
Collaborator Author

Looks like the build isn't clean any more, these issues should be fixed:

@inexorabletash Fixed.

index.bs Outdated Show resolved Hide resolved
@huningxin
Copy link
Contributor

Big thanks to @fdwr for the thoughtful and thorough proposal.

+1. I think we should acknowledge Dwayne for his outstanding contribution. @wchao1115 @anssiko

@wchao1115
Copy link
Collaborator Author

+1. I think we should acknowledge Dwayne for his outstanding contribution. @wchao1115 @anssiko

Fixed. 😊

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.

One functional comment added, plus some typos and wording proposals. Otherwise, given all other matters are deferrable as separate issues, tis looking good to me. Thanks Chai.

Thanks to Dwayne Robinson for his work...

Arigatou 😊.

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
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.

Minor things, else LGTM. 😎

@anssiko
Copy link
Member

anssiko commented Dec 7, 2023

All the discussions in this PR have been resolved, some comments spun into separate non-blocking issues as agreed with the reviewers, and the build is clean. Great work everyone involved!

@wchao1115 @huningxin please take a final look. Given this is a big PR with many micro-commits, I'd like to hear if you'd prefer to squash this. Below is my reasoning and recommendation.

For the earlier big PRs #446 we decided to keep the commit history in the main branch. For this PR here it looks like a squash merge might be preferred instead since there are many micro-commit that fix small issues and typos. GitHub will retain the commit history of this PR if we keep this PR around after the squash merge so references to these commits will continue to work. The only tradeoff I'm aware of is that the local git clones of this repo won't have access to the history of this PR by default. If you agree we want to squash this PR @wchao1115 please update your extensive PR summary in #478 (comment) to reflect the latest so we can reuse it as the squash commit message. This will help future contributors who will look at the history.

Looking forward to merging this soon 🚀

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 much!

@wchao1115 wchao1115 merged commit 6ce91ac into main Dec 11, 2023
2 checks passed
@wchao1115 wchao1115 deleted the transformer_ops branch December 11, 2023 06:00
github-actions bot added a commit that referenced this pull request Dec 11, 2023
…gment Anything, Stable Diffusion, etc. (#478)

SHA: 6ce91ac
Reason: push, by wchao1115

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

None yet

10 participants