[onnx-3] Make axis on concat required. #390
Conversation
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
onnx/defs/tensor/defs.cc
Outdated
| .Attr("axis", | ||
| "Which axis to concat on", | ||
| AttributeProto::INT, | ||
| OPTIONAL) | ||
| REQUIRED) |
There was a problem hiding this comment.
"required" is the default value, so you may just remove "OPTIONAL" here. Ideally, there're only two APIs, one for required attribute, one for optional attribute with default value (and remove this flag finally).
|
Very interesting. When I tried to change it to "required", it broke several backend test cases (test_backend_test.py, cases: test_densenet121|test_inception_v1|test_inception_v2|test_shufflenet|test_squeezenet). I thought that these test models need to be updated accordingly. It's only my local issue? :) |
onnx/defs/schema.h
Outdated
| @@ -25,6 +25,7 @@ using OperatorSetVersion = int; | |||
|
|
|||
| const char* const ONNX_DOMAIN = ""; | |||
| const bool OPTIONAL = false; | |||
| const bool REQUIRED = true; | |||
There was a problem hiding this comment.
This may not be needed as the default is "required".
|
Yeah, I think if we need to regenerate these models. The concats don't have axis in these models. |
|
@anderspapitto could you take a look? |
|
I've uploaded a new version of shufflenet, will do the others after I wait a bit to make sure I didn't break anything |
|
@anderspapitto there is no need to “wait”, you can manually test the new model files |
|
good to merge when CI passes |
|
❌ Build onnx 0.3.1173 failed (commit 4aad5fd910 by @bddppq) |
* [onnx-3] Make axis on concat required. Signed-off-by: Edward Z. Yang <ezyang@fb.com> * No need to explicitly specifying REQUIRED * Fix version bump * Re-generate docs
* [onnx-3] Make axis on concat required. * No need to explicitly specifying REQUIRED * Fix version bump * Re-generate docs
Fixes #374
Signed-off-by: Edward Z. Yang ezyang@fb.com