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

[Relay to onnx conversion fixes][Pool, Pad] #8435

Merged
merged 2 commits into from
Jul 12, 2021
Merged

[Relay to onnx conversion fixes][Pool, Pad] #8435

merged 2 commits into from
Jul 12, 2021

Conversation

schilkunda-amba
Copy link
Contributor

Fixed issues in relay to onnx conversion of pool and pad ops.
Pool: Added missing ceil_mode in average pool and max pool conversion
Pad: Changed pad_value to input instead of attrs (#7860)

Updated unit tests and fixed formatting errors.

* added missing ceil_mode in average pool and max pool conversion
* Fixed issue in Pad conversion: changed pad_value to input instead of attrs
* Refer to PR: #7860
* Updated unit test for Pad
* Fixed some formatting errors
Copy link
Contributor

@trevor-m trevor-m left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks @schilkunda-amba

Copy link
Contributor

@u99127 u99127 left a comment

Choose a reason for hiding this comment

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

Thank you for separating out bug fixes from the new features from

I asked in #8362 to please fix the commit message to reflect that these are changes to the ONNX frontend. Relay isn't being converted to ONNX. Its a model in the ONNX format which is being lowered to Relay.

Can the commit message please be updated along with the subject line and summary line of your pull request(s) ? TIA.

regards
Ramana

@schilkunda-amba
Copy link
Contributor Author

@u99127 I should have probably clarified in the previous PR. The PR is indeed for converting relay to onnx (not the other way round). It's not part of the frontend lowering of onnx to relay.

Our compiler expects an onnx model as input (not relay). Hence, with the help of community, we implemented a relay to onnx convertor. Model in any framework -> Relay -> Onnx -> Compiler

Hope this clarifies!

@u99127
Copy link
Contributor

u99127 commented Jul 9, 2021

@u99127 I should have probably clarified in the previous PR. The PR is indeed for converting relay to onnx (not the other way round). It's not part of the frontend lowering of onnx to relay.

Our compiler expects an onnx model as input (not relay). Hence, with the help of community, we implemented a relay to onnx convertor. Model in any framework -> Relay -> Onnx -> Compiler

Hope this clarifies!

Oh, I'm sorry I missed this . Is there an RFC or something I can read about this new feature ?

Ramana

Thanks,
Ramana

@schilkunda-amba
Copy link
Contributor Author

@masahi masahi merged commit c3558a1 into apache:main Jul 12, 2021
@schilkunda-amba schilkunda-amba deleted the relay_to_onnx_fixes branch July 12, 2021 05:44
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
* [Relay to Onnx conversion][Pool]

* added missing ceil_mode in average pool and max pool conversion

* [Relay to Onnx conversion][Pad]

* Fixed issue in Pad conversion: changed pad_value to input instead of attrs
* Refer to PR: apache#7860
* Updated unit test for Pad
* Fixed some formatting errors
zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Mar 4, 2022
* [Relay to Onnx conversion][Pool]

* added missing ceil_mode in average pool and max pool conversion

* [Relay to Onnx conversion][Pad]

* Fixed issue in Pad conversion: changed pad_value to input instead of attrs
* Refer to PR: apache#7860
* Updated unit test for Pad
* Fixed some formatting errors
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.

4 participants