-
Notifications
You must be signed in to change notification settings - Fork 48
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
Bugfix: Add missing 64-bit integers support for some reduction operators #695
base: main
Are you sure you want to change the base?
Conversation
`reduceL1`, `reduceProduct`, `reduceSum` and `reduceSumSquare` already support 32-bit integers. 64-bit integers should also be supported. Fix webmachinelearning#283, webmachinelearning#694
@fdwr, @huningxin, starting from ONNX Opset 18, all Reduce* ops support It mentions the data type expanding for
But it doesn't mention floating point values for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
#654 proposes removing 64-bit int support for some operators. What's the rationale for adding 64-bit int support for these operators? This may be a discussion for another issue, but does WebNN need to support 64-bit integer types at all given trends towards smaller data types (e.g. int4 and int8) for on-device ML and that some backends have little/no support for 64-bit values in the first place? (e.g. CoreML has ~no support, and most GPUs emulate support for int64) |
IIUC, #654 mentioned lower feature level DirectML (before FL 4.1) doesn't support 64-bit integers for some operators. Higher feature DirectML doesn't have that issue. As @inexorabletash mentioned we may assume the browser always carries a copy of library that ensures the highest feature level. If that's the case, we may close that issue.
As #694 mentioned, the safety checker model for stable diffusion turbo demo uses
We may want to follow the backend difference through #463 . |
This CL adds 64-bit integer support for reduceL1, reduceProduct, reduceSum and reduceSumSquare. It's based on the spec change being proposed by webmachinelearning/webnn#695. Bug: 328567884 Change-Id: Ia858b47082f81a9eb6ab3b9403e3773a752eb608 Cq-Include-Trybots: luci.chromium.try:win11-blink-rel,mac14-blink-rel,mac14.arm64-blink-rel
This CL adds 64-bit integer support for reduceL1, reduceProduct, reduceSum and reduceSumSquare. It's based on the spec change being proposed by webmachinelearning/webnn#695. Bug: 328567884 Change-Id: Ia858b47082f81a9eb6ab3b9403e3773a752eb608 Cq-Include-Trybots: luci.chromium.try:win11-blink-rel,mac14-blink-rel,mac14.arm64-blink-rel
This CL adds 64-bit integer support for reduceL1, reduceProduct, reduceSum and reduceSumSquare. It's based on the spec change being proposed by webmachinelearning/webnn#695. Bug: 328567884 Change-Id: Ia858b47082f81a9eb6ab3b9403e3773a752eb608 Cq-Include-Trybots: luci.chromium.try:win11-blink-rel,mac14-blink-rel,mac14.arm64-blink-rel
This CL adds 64-bit integer support for reduceL1, reduceProduct, reduceSum and reduceSumSquare. It's based on the spec change being proposed by webmachinelearning/webnn#695. Bug: 328567884 Change-Id: Ia858b47082f81a9eb6ab3b9403e3773a752eb608 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/+/5569544 Reviewed-by: ningxin hu <[email protected]> Reviewed-by: Austin Sullivan <[email protected]> Commit-Queue: Lisha Guo <[email protected]> Cr-Commit-Position: refs/heads/main@{#1309157}
This CL adds 64-bit integer support for reduceL1, reduceProduct, reduceSum and reduceSumSquare. It's based on the spec change being proposed by webmachinelearning/webnn#695. Bug: 328567884 Change-Id: Ia858b47082f81a9eb6ab3b9403e3773a752eb608 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/+/5569544 Reviewed-by: ningxin hu <[email protected]> Reviewed-by: Austin Sullivan <[email protected]> Commit-Queue: Lisha Guo <[email protected]> Cr-Commit-Position: refs/heads/main@{#1309157}
This CL adds 64-bit integer support for reduceL1, reduceProduct, reduceSum and reduceSumSquare. It's based on the spec change being proposed by webmachinelearning/webnn#695. Bug: 328567884 Change-Id: Ia858b47082f81a9eb6ab3b9403e3773a752eb608 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/+/5569544 Reviewed-by: ningxin hu <[email protected]> Reviewed-by: Austin Sullivan <[email protected]> Commit-Queue: Lisha Guo <[email protected]> Cr-Commit-Position: refs/heads/main@{#1309157}
This CL adds 64-bit integer support for reduceL1, reduceProduct, reduceSum and reduceSumSquare. It's based on the spec change being proposed by webmachinelearning/webnn#695. Bug: 328567884 Change-Id: Ia858b47082f81a9eb6ab3b9403e3773a752eb608 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/+/5569544 Reviewed-by: ningxin hu <[email protected]> Reviewed-by: Austin Sullivan <[email protected]> Commit-Queue: Lisha Guo <[email protected]> Cr-Commit-Position: refs/heads/main@{#1309157}
I know this is already implemented in Chromium so I don't mean to quibble too much over this, but I think it's worth questioning whether there should be a process for changing supported data types similar to the existing process for adding new operators |
…rt for some reduce operators, a=testonly Automatic update from web-platform-tests WebNN: Add missing 64-bit integers support for some reduce operators This CL adds 64-bit integer support for reduceL1, reduceProduct, reduceSum and reduceSumSquare. It's based on the spec change being proposed by webmachinelearning/webnn#695. Bug: 328567884 Change-Id: Ia858b47082f81a9eb6ab3b9403e3773a752eb608 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/+/5569544 Reviewed-by: ningxin hu <[email protected]> Reviewed-by: Austin Sullivan <[email protected]> Commit-Queue: Lisha Guo <[email protected]> Cr-Commit-Position: refs/heads/main@{#1309157} -- wpt-commits: 50c7448895efb9f6cfe0b37d77d018ea549d4b99 wpt-pr: 46568
…rt for some reduce operators, a=testonly Automatic update from web-platform-tests WebNN: Add missing 64-bit integers support for some reduce operators This CL adds 64-bit integer support for reduceL1, reduceProduct, reduceSum and reduceSumSquare. It's based on the spec change being proposed by webmachinelearning/webnn#695. Bug: 328567884 Change-Id: Ia858b47082f81a9eb6ab3b9403e3773a752eb608 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/+/5569544 Reviewed-by: ningxin hu <[email protected]> Reviewed-by: Austin Sullivan <[email protected]> Commit-Queue: Lisha Guo <[email protected]> Cr-Commit-Position: refs/heads/main@{#1309157} -- wpt-commits: 50c7448895efb9f6cfe0b37d77d018ea549d4b99 wpt-pr: 46568
I'd support adding a process for updating the operators. A PR is proposed: #705. PTAL. This PR is a follow-up bug fix for #283. In that issue, @fdwr confirmed L1, SUM_SQUARE, MULTIPLY, SUM reduce functions support 32 and 64 bit integers. However, I forgot to add 64-bit integers into the table of #283 which causes @inexorabletash 's PR #646 missing the 64-bit integers support for those reduce operators. As mentioned, this issue causes the safety checker model for stable diffusion turbo demo failing because it uses |
@Honry : The doc is correct.
Notice that similarly ReduceL1 was supported, but ReduceL2 (which has a square root that yields fractional values) does not have an int64 version (and also LogSum, LogSumExp...).
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks Ningxin.
@huningxin given that int64 is not supported on CoreML, how hard is it for the stable diffusion turbo demo to convert the model from int64 to int32? |
The original model casts to int64 before reduceSum. We'll try to change it to int32 and see whether it still works. Will keep this thread posted. |
After the investigation, the two More details:
|
@mwyrzykowski Do you know any plans to update CoreML to support int64? It's oddly inconsistent that all the other Apple ML API's (BNNS, MPS, MLX) support int64, but CoreML does not 🤔. Is CoreML still the right API these days to implement a WebNN backend, or is it left behind by newer ones? Thanks for any information or redirections.
|
@fdwr BNNS only runs on the CPU and MLX being open source can not use the ANE / NPU. MPS being backed by Metal only runs on the GPU. If running on the ANE is a goal, CoreML is necessary. |
Can confirm this is an important goal :) |
Concur, this is important (at least for those models that can fully run on ANN or those parts of a model that are viable to run on it). |
…rt for some reduce operators, a=testonly Automatic update from web-platform-tests WebNN: Add missing 64-bit integers support for some reduce operators This CL adds 64-bit integer support for reduceL1, reduceProduct, reduceSum and reduceSumSquare. It's based on the spec change being proposed by webmachinelearning/webnn#695. Bug: 328567884 Change-Id: Ia858b47082f81a9eb6ab3b9403e3773a752eb608 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/+/5569544 Reviewed-by: ningxin hu <[email protected]> Reviewed-by: Austin Sullivan <[email protected]> Commit-Queue: Lisha Guo <[email protected]> Cr-Commit-Position: refs/heads/main@{#1309157} -- wpt-commits: 50c7448895efb9f6cfe0b37d77d018ea549d4b99 wpt-pr: 46568
WebNN Spec adds missing 64-bit integers support for `reduceL1`, `reduceSum`, `reduceSumSquare` and `reduceProduct` ops at this [PR](webmachinelearning/webnn#695), which has already been implemented in Chromium. Update corresponding data type constraints in WebNN EP. Besides, WebNN CPU backend currently doesn't support `uint64` and `uint32` for these ops.
…20912) WebNN Spec adds missing 64-bit integers support for `reduceL1`, `reduceSum`, `reduceSumSquare` and `reduceProduct` ops at this [PR](webmachinelearning/webnn#695), which has already been implemented in Chromium. Update corresponding data type constraints in WebNN EP. Besides, WebNN CPU backend currently doesn't support `uint64` and `uint32` for these ops.
With @philloooo 's initial op set limits CR complete 🙂, we can see a path toward conditionally supporting data types (similar to how WebGPU does not support all texture data types on all backends). |
I am still curious to know:
|
@philloooo : Update - the int32 model is uploaded thanks to Belem and Adele (https://huggingface.co/microsoft/sd-turbo-webnn/tree/main/safety_checker), and @ibelem updated the sample (microsoft/webnn-developer-preview#11). |
@philloooo, thanks to @fdwr's tool: https://github.com/fdwr/Onnx2Text, I convert the model into a text file then manually edit the required parts, e.g. change the cast from 'int64' to 'int32', change the input data type of For this safety checker model, it's easy to convert it to use int32, for others, I can't tell how hard it is. It depends on how complicated the model is, case by case. |
Is there a way to automate that process? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @a-sully and others who have raised concerns regarding adding additional 64-bit type support where it does not exist with CoreML.
We could potentially expose some general uint64/int64 extension or int64 on certain operations extension, but I don't think making it required and expecting backends to emulate is realistic.
@philloooo: Well currently it's manual, and we may be able to write a tool that applies known patterns (like the
@mwyrzykowski: It's not required, as different backends will have different capabilities (tensor rank limits, data type support...), and the
So at a minimum, the one operation in ORT or CoreML backend that would be most useful to emulate is casting graph inputs/outputs. For example, to cast uint64 to uint32, either skip every other element in the ArrayBuffer before uploading to the GPU, or treat the uint64/int64 tensor input virtually (from CoreML's POV) as a uint32/int32 tensor with the lowest dimension doubled (because every 64-bit element is just 2 x 32-bit elements), and then in pseudocode¹...
¹ This is basically what the ORT DML EP used to do before native int64 support in DirectML.dll. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fdwr I think we should start with the set of operations which is the intersection of which all native frameworks support today, instead of expecting frameworks to catch up. In general, this seems like the wrong direction and will lead to fragmentation where certain models work on certain devices and not others.
I am trying to understand whether this is a device specific limits. Let's say if an implementation could map "cpu" MLContext to BNNS, "gpu" MLContext to MPS and "npu" MLContext to CoreML, would that mean only the "npu" MLContext not supporting 64bit integers? That device type specific difference can be detected through As you know, the Chromium prototype on Windows also relies on different native frameworks, TFLite for "cpu" MLContext and DirectML for "gpu" and "npu" MLContext. IIUC, the DirectML supported data types are also device dependent and can be detected through |
+1 to this sentiment. It would be nice if there was a common set of operators and data types which was guaranteed to be supported across platforms, which is what #573 is getting at. Then ...The question is whether a common set of data types exists. Unfortunately |
reduceL1
,reduceProduct
,reduceSum
andreduceSumSquare
already support 32-bit integers. 64-bit integers should also be supported.Fix #283, #694
Preview | Diff