Skip to content
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

Add backward conversions from 18->17 for reduce ops #5606

Merged
merged 20 commits into from Sep 26, 2023

Conversation

liqunfu
Copy link
Contributor

@liqunfu liqunfu commented Sep 20, 2023

Description

backward conversion is needed for reduce ops that have axes attribute changed to input

Motivation and Context

version converter

Signed-off-by: Liqun Fu <liqfu@microsoft.com>
@liqunfu liqunfu requested a review from a team as a code owner September 20, 2023 16:43
@xadupre
Copy link
Contributor

xadupre commented Sep 20, 2023

Should we add a unit test?

Signed-off-by: Liqun Fu <liqfu@microsoft.com>
@liqunfu
Copy link
Contributor Author

liqunfu commented Sep 20, 2023

Should we add a unit test?

Good point! there is an onnx\test\automatic_upgrade_test.py. we shall add a automatic_downgrade_test to catch such mistakes. #5607

justinchuby
justinchuby previously approved these changes Sep 20, 2023
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
@liqunfu liqunfu requested a review from a team as a code owner September 20, 2023 22:32
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lintrunner found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

Signed-off-by: Liqun Fu <liqfu@microsoft.com>
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
@liqunfu liqunfu added this to the 1.15 milestone Sep 21, 2023
@liqunfu
Copy link
Contributor Author

liqunfu commented Sep 21, 2023

Should we add a unit test?

added downgrade test

@justinchuby justinchuby changed the title reduceOps has missing backward conversions from 18->17 Add backward conversions from 18->17 for reduce ops Sep 21, 2023
onnx/test/automatic_downgrade_test.py Outdated Show resolved Hide resolved
onnx/test/automatic_downgrade_test.py Outdated Show resolved Hide resolved
liqunfu and others added 4 commits September 21, 2023 18:17
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: liqun Fu <liqun.fu@microsoft.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: liqun Fu <liqun.fu@microsoft.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: liqun Fu <liqun.fu@microsoft.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: liqun Fu <liqun.fu@microsoft.com>
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
onnx/test/automatic_downgrade_test.py Fixed Show fixed Hide fixed
@
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
@justinchuby
Copy link
Contributor

justinchuby commented Sep 22, 2023

I was going to merge now to unblock #5613 . Looks like we still need @onnx/sig-operators-approvers

@justinchuby justinchuby added this pull request to the merge queue Sep 26, 2023
Merged via the queue into main with commit b5111b8 Sep 26, 2023
35 checks passed
@justinchuby justinchuby deleted the liqun/reduce-convert-18-17 branch September 26, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants