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

Graph optimization removes significant casts even if all optimizations are disabled #17565

Closed
cbourjau opened this issue Sep 15, 2023 · 4 comments · Fixed by #17953
Closed

Graph optimization removes significant casts even if all optimizations are disabled #17565

cbourjau opened this issue Sep 15, 2023 · 4 comments · Fixed by #17953
Labels
stale issues that have not been addressed in a while; categorized by a bot

Comments

@cbourjau
Copy link
Contributor

Describe the issue

Consider the following model

image

where a is a float64 input, the first cast is to float32 and the second one is back to float64. The model is stored in original_model.onnx below.

Running

    import onnxruntime as ort

    opts = ort.SessionOptions()
    opts.graph_optimization_level = ort.GraphOptimizationLevel.ORT_DISABLE_ALL
    opts.optimized_model_filepath = "model_disable_all_optimizations.onnx"

    ort.InferenceSession("original_model.onnx", opts)

yields the following model:

image

The cast are significant. Removing them changes the output of the model. Furthermore, I would expect no optimizations at all to take place if opts.graph_optimization_level = ort.GraphOptimizationLevel.ORT_DISABLE_ALL.

To reproduce

The respective models are attached.

models.zip

Urgency

The (disabled) optimization produces wrong results in surprising ways. Users may rely on the truncations from the cast. We found this bug because the lack of these casts produced wrong results in a subsequent TreeEnsembleRegressor.

Platform

Mac

OS Version

12.3.1

ONNX Runtime Installation

Released Package

ONNX Runtime Version or Commit ID

1.15.1

ONNX Runtime API

Python

Architecture

ARM64

Execution Provider

Default CPU

Execution Provider Library Version

No response

@wangyems
Copy link
Contributor

there is a way to disable certain optimizers: e.g

kwargs["disabled_optimizers"] = disabled_optimizers

@tianleiwu
Copy link
Contributor

The InsertCastTransformer cannot be disabled right now:

InsertCastTransformer insert_cast_transformer{"CastFloat16Transformer", cpu_regs};

There are some similar issues reported related to this: #8787.

Solution is to update the logic for precision loss detection here:

if (src_type_group > dst_type_group) {

and/or disable RemoveDuplicateCastTransforme when all optimization are disabled.

Copy link
Contributor

This issue has been automatically marked as stale due to inactivity and will be closed in 7 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 31, 2023
@cbourjau
Copy link
Contributor Author

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

The issue is not resolved, albeit it might be a duplicate.

tianleiwu pushed a commit that referenced this issue Oct 31, 2023
The `RemoveDuplicateCastTransformer` fairly naively removed Cast nodes
from the graph without considering precision loss when using the same
`TypeGroup`. For instance, F64 -> F32 -> F64 would be optimised out of
the graph.

I also noticed that signedness was not accounted for, which is not
covered by any existing issue but is a problem. For example doing int ->
unsigned int -> int produces very different values for negative inputs
and so should not be optimised out

One could argue that we shouldn't be performing such cast elimination at
all (at least not in this transformer). The original scope might be well
restricted to only eliminating unnecessary casts from the
`InsertCastTransformer` and no others.

### Motivation and Context
This should fix #17565,
ttps://github.com//issues/9915 and
#8787.
kleiti pushed a commit to kleiti/onnxruntime that referenced this issue Mar 22, 2024
The `RemoveDuplicateCastTransformer` fairly naively removed Cast nodes
from the graph without considering precision loss when using the same
`TypeGroup`. For instance, F64 -> F32 -> F64 would be optimised out of
the graph.

I also noticed that signedness was not accounted for, which is not
covered by any existing issue but is a problem. For example doing int ->
unsigned int -> int produces very different values for negative inputs
and so should not be optimised out

One could argue that we shouldn't be performing such cast elimination at
all (at least not in this transformer). The original scope might be well
restricted to only eliminating unnecessary casts from the
`InsertCastTransformer` and no others.

### Motivation and Context
This should fix microsoft#17565,
ttps://github.com/microsoft/issues/9915 and
microsoft#8787.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale issues that have not been addressed in a while; categorized by a bot
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants