Skip to content

Output Tensor Shape Validation b/w ONNX inference and ORT - #7252

Merged
sumitsays merged 23 commits into
microsoft:masterfrom
sumitsays:output-shape-validation
May 3, 2021
Merged

Output Tensor Shape Validation b/w ONNX inference and ORT #7252
sumitsays merged 23 commits into
microsoft:masterfrom
sumitsays:output-shape-validation

Conversation

@sumitsays

@sumitsays sumitsays commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

Background:
During the execution of a model on CPU, output tensor shape generated by ONNX inference needs to be validated against the output tensor shape generated by ORT.
Today, this validation happens only if the space for output tensor is pre-allocated. But, to make the behavior consistent with GPU execution provider (DML), this validation needs to happen irrespective of the space pre-allocation.

Motivation:
Bug raised by a user, when a in-consistency has been seen b/w the CPU ep and GPU ep. Github Bug: 6075 ,

Implementation:
Before the actual execution of a operator on cpu, this validation will happen. As of now, if this validation fails, it will display/log a warning message stating the validation failure and continue with the execution. But ideally to make it consistent with GPU behavior, ORT should also stop the execution then and there if the validation fails. It is done like this for now because there are multiples existing tests fail because of this validation failure. First those tests need to be fixed, then ORT should throw an exception and stop the execution. Known tests which were failing because of validation failure:
• PadOpTest.Pad_Edge_DimWithZeroInput
• ReductionOpTest.ReduceDimWithZero
• Loop.SubgraphInputShadowsOuterScopeValue

There were more tests which were failing, but actual reason needs to be investigated. Few of them were:
• CudaKernelTest.LayerNorm_SmallSizeTensor_IntermediateAxis
• CudaKernelTest.SimplifiedLayerNorm_SmallSizeTensor_IntermediateAxis
This was the Azure build but it is not present anymore in the pipeline.

@sumitsays
sumitsays requested a review from a team as a code owner April 6, 2021 01:02
@jstoecker
jstoecker requested a review from askhade April 6, 2021 18:49
@sumitsays

Copy link
Copy Markdown
Contributor Author

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

@azure-pipelines

Copy link
Copy Markdown
Commenter does not have sufficient privileges for PR 7252 in repo microsoft/onnxruntime

@sumitsays

Copy link
Copy Markdown
Contributor Author

/azp run Windows GPU CI Pipeline, WIndows GPU TensorRT CI Pipeline, centos7_cpu, centos7_cpu (linux_centos_ci Debug), centos7_cpu (linux_centos_ci Release), orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-distributed, orttraining-amd-gpu-ci-pipeline, Linux Nuphar CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Commenter does not have sufficient privileges for PR 7252 in repo microsoft/onnxruntime

@fdwr

fdwr commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

/azp run Windows GPU CI Pipeline, WIndows GPU TensorRT CI Pipeline, centos7_cpu, centos7_cpu (linux_centos_ci Debug), centos7_cpu (linux_centos_ci Release), orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-distributed, orttraining-amd-gpu-ci-pipeline, Linux Nuphar CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 6 pipeline(s).

…ORT tests which are failing because of this validation
Comment thread onnxruntime/core/optimizer/optimizer_execution_frame.cc Outdated
Comment thread onnxruntime/core/framework/execution_frame.cc Outdated
Comment thread onnxruntime/core/framework/execution_frame.cc Outdated
Comment thread onnxruntime/core/framework/execution_frame.cc Outdated
Comment thread onnxruntime/core/framework/execution_frame.cc Outdated
Comment thread onnxruntime/core/framework/execution_frame.cc Outdated
Comment thread onnxruntime/core/framework/execution_frame.cc Outdated
Comment thread onnxruntime/core/framework/execution_frame.cc Outdated
Comment thread onnxruntime/test/providers/cpu/reduction/reduction_ops_test.cc Outdated
pranavsharma
pranavsharma previously approved these changes May 1, 2021
fdwr
fdwr previously approved these changes May 1, 2021

@fdwr fdwr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reapproving. ORT already signed off, but Sumit encountered a bad merge and rewound it to an earlier commit.

Remove duplicate function "GetLogger()"
Fixed typo in method name "TestConvTransposeOpInitializer"

@fdwr fdwr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reapproving after bad merge fixed (duplicate OptimizerExecutionFrame::GetLogger and test case name) and CI's pass. No other changes since ORT (Pranav and Hari) last signed off.

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.

4 participants