Differentiate shape and type inference errors #5519

liqunfu · 2023-08-23T04:24:02Z

Description

Shape inference raises 2 types of inference errors. Currently there is a single inference error for them. It is necessary to differentiate them because shape inference errors are minor where type inference errors are fatal. This PR is to make the 2 types of errors and use it in backend and python.

Motivation and Context

this pr is part of out effort to handle #4986

validate with:
microsoft/onnxruntime#18948

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

github-advanced-security

lintrunner found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

onnx/test/automatic_upgrade_test.py

onnx/test/test_backend_test.py

justinchuby · 2023-08-23T04:58:43Z

onnx/cpp2py_export.cc

@@ -577,31 +577,39 @@ PYBIND11_MODULE(onnx_cpp2py_export, onnx_cpp2py_export) {
  auto shape_inference = onnx_cpp2py_export.def_submodule("shape_inference");
  shape_inference.doc() = "Shape Inference submodule";
  py::register_exception<InferenceError>(shape_inference, "InferenceError");
+  py::register_exception<TypeInferenceError>(shape_inference, "TypeInferenceError");


Just making sure: Are we subclassing InferenceError? It would be nice if that is the case because users can then opt to catch InferenceError for all cases, or catch TypeInferenceError etc. for more specific errors

yes we are subclassing TypeInferenceError and ShapeInferenceError from InferenceError.

justinchuby · 2023-08-23T16:24:53Z

onnx/checker.cc

@@ -1036,7 +1036,7 @@ void check_model(const std::string& model_path, bool full_check, bool skip_opset
  check_model(model, ctx);

  if (full_check) {
-    ShapeInferenceOptions options{true, 1, false};
+    ShapeInferenceOptions options{true, FailAnyInferenceError, false};


Would it be possible to scope this enum for clarity? e.g. errors::FailAnyInferenceError or shape_inference::FailAnyInferenceError etc.

@jcwchen any suggestions?

that is correct, this PR is far from complete. There are many changes on API surface so we want to make it right before merge. I am more concerned on the Python side. Naming is one thing that we need to be precise. In terms of scope, it is already under ShapeInference module but adding errors.
It is less a concern on the c++ side however.

Curious why it's less of a concern on the c++ side?

I should have said it was a concern for both. It is just that python api is more visible.

@justinchuby it is better to scope the enum. do you think InferenceErrorMode instead of error is better?

enum class InferenceErrorMode : uint32_t {
IgnoreInferenceError, // Ignore any inference errors
FailAnyInferenceError, // Fail on any inference error
FailShapeInferenceError, // Fail on any shape inference error, like merging existing shape with inferred etc.
FailTypeInferenceError // Fail on any type inference error
};

gramalingam · 2023-08-24T15:57:35Z

onnx/defs/shape_inference.h

@@ -75,11 +78,23 @@ class InferenceError final : public std::runtime_error {
  std::string expanded_message_;
 };

+class ShapeInferenceError final : public InferenceError {


Re. the naming: I wonder if it is better to call these ShapeError and TypeError. (I realize that this naming comes from the original macro names.)

Also, I think these might not be enough: some errors (like missing attributes or wrong attribute-value) are not really "shape error". So, I wonder if we should either rename ShapeError to ValueError or at least add another exception called ValueError or something more generic.

Good point @gramalingam
I like to keep ShapeError for its original use, adding AttributeError (more specific than ValueError) for errors raised from attribute checking. In addition to ShapeError, TypeError, and AttributeError, InferShape can also raise ValidationError via schema->CheckInputOutputType. It is currently swallowed as a shapeError which is in correct.

gramalingam · 2023-08-24T16:01:47Z

onnx/defs/shape_inference.h

@@ -18,18 +18,22 @@ namespace ONNX_NAMESPACE {

 using Dim = TensorShapeProto_Dimension;

+enum InferenceErrorMode : uint8_t {


nit: I think we could easily use uint32_t. Cost is not significant, and may help us in long run.

I also wonder whether we need to expose anything like this externally? Who are the intended users and what will they use it for? May be it is better to focus on the intended use, which is internal ... specifically, we want to be able to propagate context information so that we know whether we are processing a top-level node, or a node contained inside a conditional (or loop? not sure about that). If it is a node inside a conditional, we want to tolerate some errors, since the node may never execute. So, I wonder if we really need this enumeration or expose it externally.

Another concern here: the checker throws ValidationError. For the example discussed above, it may be necessary to catch/handle checker-errors as well, since a missing attribute might show up as a checker error.

I think it might be helpful to validate ONNX with this PR: #5488 (with converters etc.) to see what failures we run into, if any. And create some test-cases with if-then-else that we would like to successfully pass the checker, and approach this top-down, to make sure we are able to do what we want.

the enum is exposed via: infer_shapes, infer_shapes_path. It originally used a binary strict_mode, which can be replaced with IgnoreInferenceError or other enum values. In that sense, we can keep the 2 interfaces as it was so that the enum does not have to be visible from python. For checker, there is an option to run shape inference. In this case, the shape inference error mode is set to be FailAnyInferenceError. This means shape inference specific exception is thrown. As an example:

onnx/onnx/test/checker_test.py

Line 499 in abf0fa7

self.assertRaises(shape_inference.TypeError, checker.check_model, model, True)

…ypes Signed-off-by: Liqun Fu <liqfu@microsoft.com>

onnx/test/test_backend_test.py

onnx/shape_inference/implementation.cc

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

onnx/shape_inference.py

onnx/test/shape_inference_test.py

onnx/shape_inference.py

@@ -20,7 +20,7 @@
 def infer_shapes(
    model: ModelProto | bytes,
    check_type: bool = False,
-    strict_mode: bool = False,
+    error_mode: C.InferenceErrorMode = C.InferenceErrorMode.IgnoreInferenceError,


onnx/shape_inference.py

@@ -20,7 +20,7 @@
 def infer_shapes(
    model: ModelProto | bytes,
    check_type: bool = False,
-    strict_mode: bool = False,
+    error_mode: C.InferenceErrorMode = C.InferenceErrorMode.IgnoreInferenceError,


onnx/shape_inference.py

@@ -169,3 +174,6 @@


 InferenceError = C.InferenceError
+TypeError = C.TypeError
+ShapeError = C.ShapeError


onnx/test/test_backend_test.py

onnx/shape_inference.py

@@ -169,3 +174,6 @@


 InferenceError = C.InferenceError
+TypeError = C.TypeError


onnx/shape_inference/implementation.cc

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

onnx/test/model_inference_test.py

onnx/test/inference_function_test.py

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

codecov · 2023-12-25T09:56:04Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f1c59a5) 56.45% compared to head (5bcf659) 56.46%.

Files	Patch %	Lines
onnx/shape_inference.py	75.00%	1 Missing ⚠️
onnx/test/model_inference_test.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5519      +/-   ##
==========================================
+ Coverage   56.45%   56.46%   +0.01%     
==========================================
  Files         504      504              
  Lines       29865    29875      +10     
  Branches     4484     4484              
==========================================
+ Hits        16860    16870      +10     
  Misses      12188    12188              
  Partials      817      817

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

onnx/test/inference_function_test.py

@@ -19,7 +19,12 @@
    make_tensor_value_info,
 )
 from onnx.numpy_helper import from_array
-from onnx.shape_inference import InferenceError, infer_node_outputs
+from onnx.shape_inference import (
+    InferenceError,


onnx/test/test_backend_test.py

@@ -17,6 +17,12 @@
 from onnx import ModelProto, NodeProto, TensorProto
 from onnx.backend.base import Device, DeviceType
 from onnx.backend.test.runner import BackendIsNotSupposedToImplementIt
+from onnx.shape_inference import (
+    InferenceError,


onnx/test/test_backend_test.py

+from onnx.shape_inference import (
+    InferenceError,
+    InferenceErrorMode,
+    ShapeError,


onnx/test/test_backend_test.py

+    InferenceError,
+    InferenceErrorMode,
+    ShapeError,
+    TypeError,


onnx/test/inference_function_test.py

+from onnx.shape_inference import (
+    InferenceError,
+    InferenceErrorMode,
+    ShapeError,
+    infer_node_outputs,
+)


onnx/test/test_backend_test.py

+from onnx.shape_inference import (
+    InferenceError,
+    InferenceErrorMode,
+    ShapeError,
+    TypeError,
+)


onnx/shape_inference/implementation.cc

+  // const std::vector<std::string>& getErrors() const {
+  //   return inference_errors;
+  // }


justinchuby · 2023-12-28T15:22:28Z

onnx/shape_inference.py

@@ -20,7 +20,7 @@
 def infer_shapes(
    model: ModelProto | bytes,
    check_type: bool = False,
-    strict_mode: bool = False,


We need to keep the strict_mode option for backwards compatibility

justinchuby · 2023-12-28T15:25:58Z

Would it make sense for it to throw errors anyways, and then let the user decide what errors to catch and ignore? This way we can simplify the interface and retain the original options without having to introduce an enum to the api.

differentiate shape and type inference errors

1846156

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

liqunfu requested review from a team as code owners August 23, 2023 04:24

Merge branch 'main' into liqun/shape-inference-with-error-mode

00cffa9

github-advanced-security bot found potential problems Aug 23, 2023

View reviewed changes

onnx/test/automatic_upgrade_test.py Fixed Show fixed Hide fixed

onnx/test/test_backend_test.py Fixed Show fixed Hide fixed

justinchuby reviewed Aug 23, 2023

View reviewed changes

justinchuby self-assigned this Aug 23, 2023

justinchuby reviewed Aug 23, 2023

View reviewed changes

gramalingam reviewed Aug 24, 2023

View reviewed changes

gramalingam added this to the 1.15 milestone Aug 28, 2023

liqunfu added 2 commits September 5, 2023 14:52

rename shape/typeInferenceError ro shape/typeError, propagate error t…

7a25ab4

…ypes Signed-off-by: Liqun Fu <liqfu@microsoft.com>

Merge branch 'main' into liqun/shape-inference-with-error-mode

4dbf23c

github-advanced-security bot found potential problems Sep 5, 2023

View reviewed changes

onnx/test/test_backend_test.py Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Sep 5, 2023

View reviewed changes

onnx/shape_inference/implementation.cc Fixed Show fixed Hide fixed

justinchuby removed their assignment Sep 6, 2023

liqunfu modified the milestones: 1.15, 1.16 Sep 14, 2023

gramalingam mentioned this pull request Oct 5, 2023

Cleanup shape inference implementation #5596

Merged

merge main

a34720d

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

github-advanced-security bot found potential problems Dec 25, 2023

View reviewed changes

onnx/shape_inference/implementation.cc Fixed Show fixed Hide fixed

liqunfu added 2 commits December 25, 2023 00:27

error type correction

73eeb91

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

more error type corrections

099bdcf

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

github-advanced-security bot found potential problems Dec 25, 2023

View reviewed changes

onnx/test/model_inference_test.py Fixed Show fixed Hide fixed

onnx/test/model_inference_test.py Fixed Show fixed Hide fixed

onnx/test/inference_function_test.py Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Dec 25, 2023

View reviewed changes

onnx/test/inference_function_test.py Fixed Show fixed Hide fixed

liqunfu added 3 commits December 25, 2023 01:08

no-exception

8d0644a

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

fix more error types

1c574ec

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

fix more error types

5ee4e02

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

liqunfu added 2 commits December 25, 2023 02:08

no-excep

cfa8267

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

lint

abf0fa7

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

github-advanced-security bot found potential problems Dec 25, 2023

View reviewed changes

onnx/shape_inference/implementation.cc

Comment on lines +755 to +757

// const std::vector<std::string>& getErrors() const {

// return inference_errors;

// }

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.

justinchuby reviewed Dec 28, 2023

View reviewed changes

justinchuby changed the title ~~differentiate shape and type inference errors~~ Differentiate shape and type inference errors Dec 28, 2023

liqunfu added 2 commits January 11, 2024 07:57

Merge branch 'main' into liqun/shape-inference-with-error-mode

d6e8efb

Merge branch 'main' into liqun/shape-inference-with-error-mode

5bcf659

justinchuby modified the milestones: 1.16, 1.17 Feb 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Differentiate shape and type inference errors #5519

Differentiate shape and type inference errors #5519

liqunfu commented Aug 23, 2023 •

edited

github-advanced-security bot left a comment

justinchuby Aug 23, 2023 •

edited

liqunfu Aug 23, 2023

justinchuby Aug 23, 2023 •

edited

liqunfu Aug 23, 2023

justinchuby Aug 23, 2023

liqunfu Aug 25, 2023

liqunfu Dec 25, 2023

gramalingam Aug 24, 2023

liqunfu Sep 5, 2023

gramalingam Aug 24, 2023

gramalingam Aug 31, 2023

gramalingam Aug 31, 2023

gramalingam Aug 31, 2023

liqunfu Dec 28, 2023

codecov bot commented Dec 25, 2023 •

edited

justinchuby Dec 28, 2023

justinchuby commented Dec 28, 2023

		@@ -18,18 +18,22 @@ namespace ONNX_NAMESPACE {

		using Dim = TensorShapeProto_Dimension;

		enum InferenceErrorMode : uint8_t {

		@@ -169,3 +174,6 @@


		InferenceError = C.InferenceError
		TypeError = C.TypeError

Differentiate shape and type inference errors #5519

Are you sure you want to change the base?

Differentiate shape and type inference errors #5519

Conversation

liqunfu commented Aug 23, 2023 • edited

Description

Motivation and Context

github-advanced-security bot left a comment

Choose a reason for hiding this comment

justinchuby Aug 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinchuby Aug 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 25, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

justinchuby commented Dec 28, 2023

liqunfu commented Aug 23, 2023 •

edited

justinchuby Aug 23, 2023 •

edited

justinchuby Aug 23, 2023 •

edited

codecov bot commented Dec 25, 2023 •

edited