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

[WebNN] Allow ops to handle ignoring an empty tensor as input #22972

Merged
merged 2 commits into from
Dec 7, 2024

Conversation

Honry
Copy link
Contributor

@Honry Honry commented Nov 29, 2024

Description

Some ops should allow empty tensor as input, e.g. roi, scales inputs in Resize

Motivation and Context

It avoid some unexpected fallback for optional input with empty tensor.
e.g. roi and scales are both optional inputs in Resize, in some models they have non-empty name but with empty initializer presented as [0], WebNN currently will fallback all nodes with 0 dimension, which is not expected.
image

@Honry
Copy link
Contributor Author

Honry commented Nov 29, 2024

@fdwr, @guschmue, PTAL, thanks!

cc/ @shiyi9801

@fdwr
Copy link
Contributor

fdwr commented Dec 3, 2024

in some models they have non-empty name but with empty initializer presented as [0]

That's illegal for ONNX models, and the model file should be fixed (along with whatever degenerate tool produced it), as scale's name string "const_empty_float__132" should be "".

"There are two ways to leave an optional input or output unspecified: the first, available only for trailing inputs and outputs, is to simply not provide that input or output; the second method is to use an empty string in place of an input or output name."
https://github.com/onnx/onnx/blob/main/docs/IR.md#optional-inputs-and-outputs

The documentation for Resize also says:

"If ‘sizes’ is needed, the user can use an empty string as the name of ‘scales’ in this operator’s input list."
https://onnx.ai/onnx/operators/onnx__Resize.html

I also worry about applying this because an absent/missing tensor is conceptually distinct from a present but empty tensor, and this check makes the two cases indistinguishable, which matters because a missing tensor might have a default value that shouldn't be confused for an explicitly empty tensor.

Does the CPU EP apply this hack? If so, that adds some weight to the decision, but if not, I'd really rather the model file be corrected.

@fdwr
Copy link
Contributor

fdwr commented Dec 3, 2024

/azp run ONNX Runtime Web CI Pipeline,Windows GPU CI Pipeline,Linux Android Emulator QNN CI Pipeline

@fdwr
Copy link
Contributor

fdwr commented Dec 3, 2024

/azp run Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,Windows ARM64 QNN CI Pipeline,Windows CPU CI Pipeline

@fdwr
Copy link
Contributor

fdwr commented Dec 3, 2024

/azp run Windows GPU CUDA CI Pipeline,Windows GPU DML CI Pipeline,Windows GPU Doc Gen CI Pipeline

@fdwr
Copy link
Contributor

fdwr commented Dec 3, 2024

/azp run Windows GPU TensorRT CI Pipeline,onnxruntime-binary-size-checks-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,Windows x64 QNN CI Pipeline,Big Models

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@Honry
Copy link
Contributor Author

Honry commented Dec 3, 2024

That's illegal for ONNX models, and the model file should be fixed (along with whatever degenerate tool produced it), as scale's name string "const_empty_float__132" should be "".

I think so, I've encountered several such illegal models :(, but they can pass model initialization in ORT.

I also worry about applying this because an absent/missing tensor is conceptually distinct from a present but empty tensor, and this check makes the two cases indistinguishable, which matters because a missing tensor might have a default value that shouldn't be confused for an explicitly empty tensor.

Assume that all initializers should either have shape information or have an empty tensor. That's the main reason why we skip checking the shape information for initializers here.

Besides, we have additional validation for initializers in each op impl, if some op doesn't allow empty initializer, we do just fallback.

Does the CPU EP apply this hack? If so, that adds some weight to the decision, but if not, I'd really rather the model file be corrected.

CPU EP firstly tries to get constant input, if it is empty tensor, it will use an internal default one.
e.g. for roi, it can not be found from initializers[1], it will use a default roi array with all zeros.

[1] https://github.com/microsoft/onnxruntime/blob/main/onnxruntime/core/providers/cpu/tensor/upsamplebase.h#L233
[2] https://github.com/microsoft/onnxruntime/blob/main/onnxruntime/core/providers/cpu/tensor/upsample.cc#L1378C10-L1378C26

@fdwr
Copy link
Contributor

fdwr commented Dec 3, 2024

I think so, I've encountered several such illegal models

😥 Do you know the producer_name (visible either via Netron or Onnx2Text) so we can hunt down the rogue exporter/tool? e.g.

ir_version: 3
producer_name: "onnx-caffe2"

or

ir_version: 7
producer_name: "pytorch"

@Honry
Copy link
Contributor Author

Honry commented Dec 4, 2024

e.g. model: https://github.com/onnx/models/blob/main/validated/vision/object_detection_segmentation/ssd-mobilenetv1/model/ssd_mobilenet_v1_13-qdq.onnx, Resize__142 is inside a subgraph of a Loop control flow.

ir_version: 7
producer_name: "onnx.quantize"
producer_version: "0.1.0"
        initializer {
          dims: 0
          data_type: 1
          name: "roi__129"
          raw_data: ""
        }

@fdwr
Copy link
Contributor

fdwr commented Dec 4, 2024

CPU EP firstly tries to get constant input, if it is empty tensor, it will use an internal default one.
e.g. for roi, it can not be found from initializers[1], it will use a default roi array with all zeros.

So then the CPU EP executes this model fine? Sigh.

model: https://github.com/onnx/models/blob/main/validated/vision/object_detection_segmentation/ssd-mobilenetv1/model/ssd_mobilenet_v1_13-qdq.onnx, Resize__142 is inside a subgraph of a Loop control flow.

:( Oh no, it's inside a model from the official ONNX repo. I commented here: onnx/models#587

if some op doesn't allow empty initializer

There's still a semantic distinction between an empty tensor and a missing tensor. Being present but size 0 is different than missing completely (in which case we should use the default value from the spec). Can we limit this hack to just Resize rather than globally in the base class?

@Honry
Copy link
Contributor Author

Honry commented Dec 4, 2024

Can we limit this hack to just Resize rather than globally in the base class?

Yes we can, but my question is, do we really need to check the shape for constant input? I believe ONNX doesn't allow an initializer with dynamic shape, nor 0 dimension.

@fdwr

This comment was marked as outdated.

@Honry
Copy link
Contributor Author

Honry commented Dec 5, 2024

ONNX allows initializers of 0 dimensions. Whether it's that useful or not, it's certainly legal, but treating distinct things equivalently seems like a recipe for future subtle bugs 🫤.

That surprises me, your example shows a non-initializer input with 0 dimension, but I just change it to an initializer input with 0 dimension, it just works.

image

Can we limit this hack to just Resize rather than globally in the base class?

All right, I will do this, thank you for your patient!

Honry added 2 commits December 5, 2024 10:12
ONNX initializers should have shape information,
skip the shape check if the input is an initializer.
@Honry Honry force-pushed the skip-input-initializer branch from 7667026 to 58b598f Compare December 5, 2024 02:21
@Honry Honry changed the title [WebNN] Skip input shape check for initializers [WebNN] Allow ops to handle ignoring an empty tensor as input Dec 5, 2024
@Honry
Copy link
Contributor Author

Honry commented Dec 5, 2024

@fdwr, I just update the PR to provide a flag for each op, allow it to handle ignoring an empty tensor as input. PTAL again, thanks!

Copy link
Contributor

@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
Contributor

fdwr commented Dec 6, 2024

/azp run ONNX Runtime Web CI Pipeline,Windows GPU CI Pipeline,Linux Android Emulator QNN CI Pipeline

@fdwr
Copy link
Contributor

fdwr commented Dec 6, 2024

/azp run Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,Windows ARM64 QNN CI Pipeline,Windows CPU CI Pipeline

@fdwr
Copy link
Contributor

fdwr commented Dec 6, 2024

/azp run Windows GPU CUDA CI Pipeline,Windows GPU DML CI Pipeline,Windows GPU Doc Gen CI Pipeline

@fdwr
Copy link
Contributor

fdwr commented Dec 6, 2024

/azp run Windows GPU TensorRT CI Pipeline,onnxruntime-binary-size-checks-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,Windows x64 QNN CI Pipeline,Big Models

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@fdwr
Copy link
Contributor

fdwr commented Dec 6, 2024

/azp run Python format, orttraining-linux-ci-pipeline

Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@fdwr fdwr merged commit 6d9636f into microsoft:main Dec 7, 2024
77 checks passed
ankitm3k pushed a commit to intel/onnxruntime that referenced this pull request Dec 11, 2024
…oft#22972)

### Description
Some ops should allow empty tensor as input, e.g. roi, scales inputs in
Resize
### Motivation and Context
It avoid some unexpected fallback for optional input with empty tensor.
e.g. roi and scales are both optional inputs in Resize, in some models
they have non-empty name but with empty initializer presented as `[0]`,
WebNN currently will fallback all nodes with 0 dimension, which is not
expected.

![image](https://github.com/user-attachments/assets/599ba351-b5f6-49ac-8a1f-69fb28dbaf9b)
ankitm3k pushed a commit to intel/onnxruntime that referenced this pull request Dec 11, 2024
…oft#22972)

### Description
Some ops should allow empty tensor as input, e.g. roi, scales inputs in
Resize
### Motivation and Context
It avoid some unexpected fallback for optional input with empty tensor.
e.g. roi and scales are both optional inputs in Resize, in some models
they have non-empty name but with empty initializer presented as `[0]`,
WebNN currently will fallback all nodes with 0 dimension, which is not
expected.

![image](https://github.com/user-attachments/assets/599ba351-b5f6-49ac-8a1f-69fb28dbaf9b)
ankitm3k pushed a commit to intel/onnxruntime that referenced this pull request Dec 11, 2024
…oft#22972)

### Description
Some ops should allow empty tensor as input, e.g. roi, scales inputs in
Resize
### Motivation and Context
It avoid some unexpected fallback for optional input with empty tensor.
e.g. roi and scales are both optional inputs in Resize, in some models
they have non-empty name but with empty initializer presented as `[0]`,
WebNN currently will fallback all nodes with 0 dimension, which is not
expected.

![image](https://github.com/user-attachments/assets/599ba351-b5f6-49ac-8a1f-69fb28dbaf9b)
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.

2 participants