Checker should validate the node's inputs/outputs have names when its formal parameter is Variadic

[jcwchen]

Description

• Checker should validate the node's inputs have names when its formal parameter is Variadic
• Checker should validate the node's outputs have names when its formal parameter is Variadic
• Add 2 tests for these 2 cases

Motivation and Context
Closes #3969. Current checker does not validate the Loop inputs/outputs have names when its formal parameter is Variadic.

[gramalingam]

A quick comment: I suspect that issue 3969 will need a specific check in Loop operator's shape-inference method. I doubt if a generic check will be sufficient. Eg., note that a loop might use 5 loop-carried dependences ... then, it should be checking it has 5 outputs to handle them all. In fact, we should already have logic that updates the output-types for all these outputs ... that logic should ensure that these outputs do exist.

[jcwchen]

note that a loop might use 5 loop-carried dependences ... then, it should be checking it has 5 outputs to handle them all. In fact, we should already have logic that updates the output-types for all these outputs ... that logic should ensure that these outputs do exist.

Are you saying that the input number of Loop's state variable (except M and condition) should equal to the output number? If so, yes I think current Loop's shape inference should cover that by type propagation.

A question: does OpSchema::Variadic allow any optional output (or input)? If not, current checker does not check that and that's why I added a generic check (checking OpSchema::Variadic cannot have any optional input/output). IIUC, this generic check should cover cases like issue 3969. Please correct me if I am wrong. Thank you!

[gramalingam]

Ok, I think there are two separate problems. For your question about Variadic: we allow the op-spec to specify a "min arity" which is the minimum number of expected parameters. This is used to calculate min_input_ and max_input_ (and similarly for outputs). This is checked here:

onnx/onnx/defs/schema.cc

Line 151 in e4e1797

if (node.input_size() < min_input_ || node.input_size() > max_input_) {

... but you are right, it is not being checked whether the inputs < min_input_ are an empty string. So, a check like you are adding is missing. But I think this check should be limited to inputs up to min_input_ and outputs up to min_output_.

[gramalingam]

But I think the loop problem is a separate one, because the loop does not specify a minimum number of loop-carried dependences. That check is specific to that op, and hence must be in that op's shape-inference method.

[jcwchen]

But I think this check should be limited to inputs up to min_input_ and outputs up to min_output_.

To confirm: So it is OK that for Variadic parameter, the input (or output) whose index > min_input_ (or min_output_) has any optional variable? I wasn't sure about this case and it seems a weird use case to me. If yes, I will update my current check for only considering index smaller than min_input_ (or min_output_).

But I think the loop problem is a separate one, because the loop does not specify a minimum number of loop-carried dependences. That check is specific to that op, and hence must be in that op's shape-inference method.

Thank you for the clarification. So another missing check only for Loop op is to make sure the exact output number needs to be exactly the same as the input number? If so, yes I can include that check into this PR as well.

[gramalingam]

To confirm: So it is OK that for Variadic parameter, the input (or output) whose index > min_input_ (or min_output_) has any optional variable? I wasn't sure about this case and it seems a weird use case to me. If yes, I will update my current check for only considering index smaller than min_input_ (or min_output_).

You are right, these are all weird cases, and are not expected to happen in practice. I think the check you have is fine too, and let us keep it the way you have. I think the main case we want to catch is, for example, if users call the Max (or Mean, or Min, etc.) operator https://github.com/onnx/onnx/blob/main/docs/Operators.md#Max with zero inputs, because we expect 1 or more inputs. This is already being caught if the user omits the parameters completely, but if they use empty string it won't be caught. But you are right that if they call Min(x, "", y), it is a bit weird too ... should we complain, or just treat it as Min (x, y)? It may be safer to complain, it helps simplify the backends/runtimes from handling these weird cases.

[gramalingam]

So another missing check only for Loop op is to make sure the exact output number needs to be exactly the same as the input number?

Yes. But I look at the existing code. This requires some non-trivial extension. The existing InferenceContext does not give us a direct API to check if an input/output exists. The right solution might be to extend it to add hasInput and hasOutput methods. (We could use a hack of returning nullptr for getInputType and getOutputType for this purpose, but even this would require some change, eg., in onnxruntime, to do this.)

[jcwchen]

It may be safer to complain, it helps simplify the backends/runtimes from handling these weird cases.

Agreed. I will keep it then.

Yes. But I look at the existing code. This requires some non-trivial extension. The existing InferenceContext does not give us a direct API to check if an input/output exists.

I might miss something here -- Can't we simply just use getNumInputs and getNumOutputs from current InferenceContext? It seems to me they respectively represent the number of n.input and n.output and we can simply compare the number of them? (If we have the OpSchema::Variadic check which this PR brings, we can make sure all inputs or outputs except M and cond do exist)

[gramalingam]

Can't we simply just use getNumInputs and getNumOutputs from current InferenceContext?

Yes, you are right. Combined with the separate variadic check, this should work. I just realized that there is one disdvantage with the variadic check itself. It works for current ops, but if we add an op in the future where the variadic parameters have some specific interpretation, users might want to pass in inputs like [X, "", Y] for a variadic parameter. But I guess we can handle it if and when we need such an op.

[garymm]

Attached a model that is now declared illegal. This model is produced by ORT training tests:
ORT-training-model.zip

It seems like the code here creates a node where some inputs may be empty, followed by non-empty inputs:
https://github.com/microsoft/onnxruntime/blob/084165c748bcd6fbb3fca4d60b1ece911c04c043/orttraining/orttraining/core/graph/gradient_builder.cc#L1766-L1774

It seems it was intentionally added in this PR:
microsoft/onnxruntime#8105

I'm not sure why they can't just include all inputs in that for loop. I think it might be a performance optimization.
@jcwchen @gramalingam do you think we should make the checker more lenient here, or ask the ORT team to try to change their code to be compliant? (Maybe they could use some constant value rather than empty string for example)

[gramalingam]

I think we should make the checker more lenient here. (As discussed above.) It does mean that checking that all inputs are specified for a loop op may be harder (as discussed above). But given the time-constraint, we could postpone the stronger-check for loops to after current release (as it might be non-trivial)?

[jcwchen]

The final decision is reverting this PR for now: #4283. Thanks for catching this.