Drop "one of" default attribute check in LabelEncoder #5673
Conversation
Signed-off-by: Aditya Goel <agoel4512@gmail.com>
adityagoel4512
force-pushed
the
fix-shape-inference
branch
from
cff199d
to
9b90f6c
Compare
Codecov ReportAll modified lines are covered by tests ✅ 📢 Thoughts on this report? Let us know!. |
Signed-off-by: Aditya Goel <agoel4512@gmail.com>
adityagoel4512
force-pushed
the
fix-shape-inference
branch
from
fb86689
to
27eecd9
Compare
adityagoel4512
changed the title
| @@ -413,6 +391,21 @@ ONNX_ML_OPERATOR_SET_SCHEMA( | |||
| " must be the same in the LabelEncoder."); | |||
| } | |||
|
|
|||
| auto default_attr = ctx.getAttribute("default_tensor"); | |||
There was a problem hiding this comment.
The new test just ensures the consistency of the operator when default_tensor is set. According to the description of the PR, it was failing in any other case. No unit test is failing, so I assume it was not tested. Is it possible to fix the behaviour and add unit test to check it is doing as expected?
There was a problem hiding this comment.
It's not quite that. When implementing the operator in ORT, I was finding the type inference fails even if I have only specified one of the default_* attributes. There seems to be to do with the fact that the default_* attributes tend to have values specified since they are optional.
There was a problem hiding this comment.
Some failure cases are checked here. Is there something you think is missing?
There was a problem hiding this comment.
All tests involve default_tensor, there is no test to check that a default value is not defined or defined with an unexpected attribute such as (value_int64s, default_floats). A test like that would have failed with your change.
There was a problem hiding this comment.
there is no test to check that a default value is not defined
I don't think that is something we want, since the default value has a fallback (e.g. -1 for int64_t) specified in the specification. This is why it is not tested in the previous implementation I believe.
defined with an unexpected attribute such as (value_int64s, default_floats)
I think this will run into the same issues with ORT unfortunately, but I can check.
There was a problem hiding this comment.
Just to clarify Aditya's point (in answer to Xavier's question): since the op-spec specifies a default-value for the various attributes, we need to be careful when flagging an error here: we cannot do that unless we can distinguish whether an attribute-value was explicitly specified in the node, or whether it is getting a default-value from the op-spec. (We should not be flagging an error when we get a default-value from the op-spec.) At the root of this, we have the issue that shape-inference works with the InferenceContext abstract interface, which exposes the getAttribute method. If we want to do such error-checking, we need to extend the interface to distinguish between the two cases mentioned above.)
So, undoing this check makes sense to me. My two cents.
There was a problem hiding this comment.
Explained much better than me! Thanks
Signed-off-by: Aditya Goel <agoel4512@gmail.com>
adityagoel4512
force-pushed
the
fix-shape-inference
branch
from
e134640
to
b64678c
Compare
|
Just to make sure I understand: I guess the root-cause is some ambiguity in whether |
When implementing the operator in ORT, I was finding the type inference fails even if I have only specified one of the default_* attributes. I assume this has something to do with the fact that these attributes have default values if not set. |
|
@liqunfu : we may need this patch for the ONNX release. |
@adityagoel4512 , I will cherry-pick this pr and a few others to make a rc2. It will take some time to have ort use the rc2. In the meantime you may apply your change local to see if the fix solves your issue in ORT. I know that you are working on quit a few ops in ORT. Please let me know if you see any other issues that need to be fixed before onnx 1.15.0 release. Thank you. |
### Description Drop "one of" checking on the default value in the `LabelEncoder`. ### Motivation and Context When implementing `LabelEncoder` in onnxruntime (for the upcoming ai.onnx.ml opset 4), it became clear that the implemented type inference for LabelEncoder would not work well in practice - specifically, the type inference pass ensures that only one of the default_* attributes is set. Since some of these are optional with default values, this is not very workable in practice. This PR addresses this by dropping this check. Notice that this was [not checked](https://github.com/onnx/onnx/blob/f556cffc40dc769a2c8e3bb5fd9a90f1e7fbb7eb/onnx/defs/traditionalml/old.cc#L277-L338) in previous opsets anyway. --------- Signed-off-by: Aditya Goel <agoel4512@gmail.com>
Description
Drop "one of" checking on the default value in the
LabelEncoder.Motivation and Context
When implementing
LabelEncoderin onnxruntime (for the upcoming ai.onnx.ml opset 4), it became clear that the implemented type inference for LabelEncoder would not work well in practice - specifically, the type inference pass ensures that only one of the default_* attributes is set. Since some of these are optional with default values, this is not very workable in practice. This PR addresses this by dropping this check. Notice that this was not checked in previous opsets anyway.