-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor shape_inference_test for old Op versions PR1 #5263
Conversation
@jcwchen Do you have time to take a look at this change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! I like the idea, but IIRC (I tried to test it before), we will probably need to modify quite a few existing tests to accommodate old opset versions. Still, it's great to have a starting point now and we can decouple the modified work if needed.
84d442c
to
a6453ce
Compare
@jcwchen I refined my code and applied this pattern to several Ops. For most Ops, the one test case fits all versions of this Op. Until now, I found there are Ops that have some incompatible tests. Here is a simple summary:
By the way, there seems a lot of work to do. Is it better to split this change to several PRs? |
1e57456
to
194d2c8
Compare
194d2c8
to
cee20ac
Compare
5d82692
to
27357e2
Compare
99b75eb
to
15351cb
Compare
da5069d
to
55a27d3
Compare
Signed-off-by: Yuhong Guo <yuhong.gyh@antgroup.com>
Signed-off-by: Yuhong Guo <yuhong.gyh@antgroup.com>
Signed-off-by: Yuhong Guo <yuhong.gyh@antgroup.com>
Signed-off-by: Yuhong Guo <yuhong.gyh@antgroup.com>
Signed-off-by: Yuhong Guo <yuhong.gyh@antgroup.com>
Signed-off-by: Yuhong Guo <yuhong.gyh@antgroup.com>
Signed-off-by: Yuhong Guo <yuhong.gyh@antgroup.com>
Signed-off-by: Yuhong Guo <yuhong.gyh@antgroup.com>
Signed-off-by: Yuhong Guo <yuhong.gyh@antgroup.com>
28f3ac5
to
64811ec
Compare
Signed-off-by: Yuhong Guo <yuhong.gyh@antgroup.com>
64811ec
to
3cd4ef4
Compare
@jcwchen I will apply this test scheme to all other test functions in a later PR and try to fix the Reshape issue when op verison <= 5 after that. |
When making updates to shape inference logic, should we reflect these changes in older shape inference implementations (e.g. Line 5032 in 925840b
|
@adityagoel4512 I'm not sure. @jcwchen what do you think about this question? |
Good question... It's really case by case. I would say if there is a critical bug in old shape inference, we should. A common scenario is all opset versions do not behave correctly in some corner cases and ONNX should fix them consistently among all versions. Another common scenario is if some situations only happen in the latest opset version (e.g., new attribute or new documentation) rather than older ones, we don't need to update/add such a logic in older opest versions. Also we need to be very careful about some shape inference changes might break runtime backward compatibility. |
That makes sense to me, I suppose it'll require careful reviews then. |
Agree with Jacky. If the fix makes sense for older op versions, ideally we should make the change to older version too. |
### Description <!-- - Describe your changes. --> onnx#4160 ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> Shape inference tests for older opset versions are not supported before this PR. This refactor would help to test old-versioned ops. This PR introduces a function `all_versions_for` to help generate test functions for all versions of one Op. The users can use `@parameterized.expand(all_versions_for("${op_name}"))` to annotate the test function to apply the test function to all op versions. The test function should have the arguments like `def test_op(self, version_name, op_version)`. `version_name` is usually unused but it's better to keep that to make the parameterized test name more readable. For example, with this `version_name`, the test name will have a version suffix showed as follows. ![image](https://github.com/onnx/onnx/assets/19584326/70ab74ec-5108-4c24-80bf-c04802858582) --------- Signed-off-by: Yuhong Guo <yuhong.gyh@antgroup.com> Co-authored-by: Yuhong Guo <yuhong.gyh@antgroup.com> Signed-off-by: Aditya Goel <agoel4512@gmail.com>
Description
#4160
Motivation and Context
Shape inference tests for older opset versions are not supported before this PR. This refactor would help to test old-versioned ops.
This PR introduces a function
all_versions_for
to help generate test functions for all versions of one Op. The users can use@parameterized.expand(all_versions_for("${op_name}"))
to annotate the test function to apply the test function to all op versions. The test function should have the arguments likedef test_op(self, version_name, op_version)
.version_name
is usually unused but it's better to keep that to make the parameterized test name more readable. For example, with thisversion_name
, the test name will have a version suffix showed as follows.