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.

Sign up for GitHub

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

Jump to bottom

[Performance] Inconsistent Tensor Output with Optimizations Enabled/Disabled (Pad Fusion Issue) #22177

Closed
vinmazz opened this issue Sep 23, 2024 · 2 comments · Fixed by #23190
Closed

[Performance] Inconsistent Tensor Output with Optimizations Enabled/Disabled (Pad Fusion Issue) #22177

vinmazz opened this issue Sep 23, 2024 · 2 comments · Fixed by #23190
Labels
performance issues related to performance regressions stale issues that have not been addressed in a while; categorized by a bot

Comments

@vinmazz
Copy link

vinmazz commented Sep 23, 2024

Describe the issue

The tensor output results of the same ONNX model with the same inputs vary depending on whether optimizations are enabled or disabled. The issue specifically involves the Pad Fusion optimizer, particularly in cases where there is a pad layer followed by an average pool layer.

To reproduce

  1. Load the ONNX model with optimizations enabled.
  2. Perform inference with a specific set of inputs.
  3. Load the same ONNX model with optimizations disabled.
  4. Perform inference with the same set of inputs.
  5. Compare the tensor output results.

Urgency

No response

Platform

Windows

OS Version

10.0.19045

ONNX Runtime Installation

Released Package

ONNX Runtime Version or Commit ID

1.19.2

ONNX Runtime API

Python

Architecture

X64

Execution Provider

Default CPU

Execution Provider Library Version

No response

Model File

Pad_AveragePool_ONNX_model.zip

Is this a quantized model?

No
@vinmazz vinmazz added the performance issues related to performance regressions label Sep 23, 2024
@github-actions GitHub Actions
Copy link
Contributor

This issue has been automatically marked as stale due to inactivity and will be closed in 30 days if no further activity occurs. If further support is needed, please provide an update and/or more details.

@github-actions github-actions bot added the stale issues that have not been addressed in a while; categorized by a bot label Oct 23, 2024
@mayeut
Copy link
Contributor

mayeut commented Dec 18, 2024

Seems to have been introduced by #21556

mayeut added a commit to mayeut/onnxruntime that referenced this issue Dec 24, 2024
@mayeut
fix: Pad/AveragePool fusion
f2d0897 
Fusing Pad & AveragePool requires AveragePool to use `count_include_pad=1`.
If the AveragePool already set some padding and `count_include_pad=0`, fusion can't happen.

This PR adds a condition to perform fusion depending on those attributes. If fusion occurs,  `count_include_pad` is always set to `1`.

Fix microsoft#22177
@mayeut mayeut mentioned this issue Dec 24, 2024
Merged
@snnn snnn closed this as completed in 4b0cee3 Jan 7, 2025
snnn pushed a commit that referenced this issue Jan 8, 2025
@mayeut @snnn
fix: Pad/AveragePool fusion (#23190)
1a1f669 
### Description
Fusing Pad & AveragePool requires AveragePool to use
`count_include_pad=1`. If the AveragePool already set some padding and
`count_include_pad=0`, fusion can't happen.

This PR adds a condition to perform fusion depending on those
attributes. If fusion occurs, `count_include_pad` is always set to `1`.

### Motivation and Context
Fix #22177 (mislabelled as a performance issue but there's an actual bug
in the implementation)
Bug introduced in #21556
tarekziade pushed a commit to tarekziade/onnxruntime that referenced this issue Jan 10, 2025
@mayeut @tarekziade
fix: Pad/AveragePool fusion (microsoft#23190)
b4e2789 
### Description
Fusing Pad & AveragePool requires AveragePool to use
`count_include_pad=1`. If the AveragePool already set some padding and
`count_include_pad=0`, fusion can't happen.

This PR adds a condition to perform fusion depending on those
attributes. If fusion occurs, `count_include_pad` is always set to `1`.

### Motivation and Context
Fix microsoft#22177 (mislabelled as a performance issue but there's an actual bug
in the implementation)
Bug introduced in microsoft#21556
guschmue pushed a commit that referenced this issue Jan 12, 2025
@mayeut @guschmue
fix: Pad/AveragePool fusion (#23190)
3bfcdfa 
### Description
Fusing Pad & AveragePool requires AveragePool to use
`count_include_pad=1`. If the AveragePool already set some padding and
`count_include_pad=0`, fusion can't happen.

This PR adds a condition to perform fusion depending on those
attributes. If fusion occurs, `count_include_pad` is always set to `1`.

### Motivation and Context
Fix #22177 (mislabelled as a performance issue but there's an actual bug
in the implementation)
Bug introduced in #21556
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
performance issues related to performance regressions stale issues that have not been addressed in a while; categorized by a bot
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Pad/AveragePool fusion mayeut/onnxruntime
2 participants
@mayeut @vinmazz