detect invalid nodes and invalid graph in onnxruntime before handing them to execution providers

[jeyblu]

Tests like TEST(BatchNormTest, InvalidScaleDim) have invalid values in the node attribute to test if invalid nodes are detected. It seems that it would make more sense for onnxruntime to check nodes in the graph and detect invalid ones before handing the graph to execution providers. If it's checked by onnxruntime, the code is written once in onnxruntime. If it's checked by execution providers, the code has to be written by every execution provider. It's one implementation vs n number of implementations for n number of execution providers.

[yuslepukhin]

There is onnx checker that is exposed in python.

[jeyblu]

But tests like TEST(BatchNormTest, InvalidScaleDim) are unit c++ tests that test onnxruntime and its execution providers. Because onnxruntime doesn't catch the invalid node, it got handed to the execution provider which also doesn't check for it. So this test didn't pass.

[jywu-msft]

ORT already uses onnx checker. see

onnxruntime/onnxruntime/core/graph/graph.cc

Line 2342 in d14b08d

checker::check_node(node_proto, ctx, lsc);

if the validation is op specific, then it's up to each execution provider to implement that specific validation. (because the EP may choose to implement all/part/none of the onnx spec for that operator)

[jeyblu]

I agree that where the spec allows room for variation in implementing an op, the EP should check. But this test has a scale shape that the spec explicitly deems to be invalid. Such cases should be deemed invalid no matter which EP handles it. Thus it seems that this check should be done and the invalid node be caught before it's handed to the EPs.

[jywu-msft]

by design, the runtime doesn't have any logic/validation pertaining to operator specifications.
we do call onnx checker though, so would it make more sense that onnx can do the validation (so all runtimes can benefit?)

[jeyblu]

Yes, it makes sense. If onnx checker is a utility meant to be used by runtimes to validate models, then it makes sense for it to catch invalid models and invalid nodes.

[jywu-msft]

it's part of onnx. https://github.com/onnx/onnx/blob/master/onnx/checker.cc

[jeyblu]

Request submitted to onnx. Thanks

[gramalingam]

A couple of different comments: (a) Agree that a check can and should be added to the shape-inference method for this op. (b) However, there is a difference between a static-checker and a runtime-checker. A static-checker like the ONNX checker can never catch all possible issues. For example, in this case, the tensor may be of an unknown rank during static-checking. So, kernels can't avoid runtime-checks. A shared checker may make sense, but then we would need a shared "runtime checker", not a "static checker".

[gramalingam]

E.g., for shared runtime checks, we should probably use something like this:

onnxruntime/onnxruntime/core/providers/cpu/nn/batch_norm_helper.h

Line 15 in 8f7422b

static common::Status ValidateInputs(const Tensor* X,

[jeyblu]

A common "runtime checker" makes sense. This will avoid every runtime and execution provider having to implement the same code.