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][Op] Remove reverse attribute from reshape and reverse_reshape operators. #7086

Merged
merged 4 commits into from
Dec 31, 2020

Conversation

jwfromm
Copy link
Contributor

@jwfromm jwfromm commented Dec 10, 2020

Currently relay has separate operators for reshape and reverse_reshape however in c++ they were both implemented using shared functions. The weird part is that ReshapeAttrs has a reverse attribute that was used for reverse_reshape but not exposed in the python API of reshape. This often causes headaches when trying to create a new reshape operator in a pass from existing attributes, since python doesn't expect a reverse argument. As far as I can tell, there's no reason to have a separate reverse_reshape op. Consolidating the two and exposing reverse in python leads to a much cleaner and more obvious API without breaking any tests or use cases.

@jwfromm
Copy link
Contributor Author

jwfromm commented Dec 10, 2020

@vinx13 @icemelon9 @electriclilies can you guys take a look at this PR and let me know what you think?

@electriclilies
Copy link
Contributor

Looks good to me! Thanks for getting this up quickly

@u99127
Copy link
Contributor

u99127 commented Dec 11, 2020

Out of curiosity, what is the thinking around versioning of the relay ir ? Would the removal of nodes from the IR require version bumps since whenever I look at relay output I see a version info being printed ?

To be clear it appears that this change simplifies a few things and that looks good to me.

@jwfromm
Copy link
Contributor Author

jwfromm commented Dec 12, 2020

@u99127 thats a good question that I don't have an answer to. @tqchen do you have any thoughts on IR versioning in Relay? Also I think it's pretty important that either @icemelon9 or @vinx13 take a look at this PR to make sure I'm not missing something.

@icemelon
Copy link
Member

Originally I separate the reshape and reverse_reshape on purpose and also as requested by @tqchen, because the reshape API is closer to numpy's definition and the reverse one is more of a hack to support MXNet. The reason that ReshapeAttr has reverse option is that I want to reuse the reshape type relation function for reverse_reshape.

@tqchen maybe you can comment on this.

@electriclilies
Copy link
Contributor

@u99127 I think this change won't remove any nodes from the relay IR, since reverse_reshape just constructs a reshape node with the reverse attribute set to true. There is no standalone reverse_reshape op, only a constructor.
This makes transforming a graph with a reshape in it is difficult because when you encounter a reshape op, you must check the value of reverse in its attributes, then copy all of the attributes except for reverse to a new set of attributes, then pass those attributes into the reshape constructor or the reverse_reshape constructor depending on what the value of the reverse attribute is.

@jwfromm
Copy link
Contributor Author

jwfromm commented Dec 15, 2020

@tqchen i think we're gonna need your input to make a decision here. The issue Lily mentioned should be resolved. Our two options for fixing it are to consolidate the reshapes as in this PR, or create separate attributes and functions for reverse_reshape. Which do you prefer?

@tqchen
Copy link
Member

tqchen commented Dec 17, 2020

As a rule of thumb(standardizationover flexibility), we should always keep API def consistent with numpy API when there is a related case in numpy, even though it could mean additional duplications. per https://tvm.apache.org/docs/contribute/code_review.html#deliberate-on-api-and-data-structures

We can however, introduce additional attrs to reverse reshape given it is non-standard. I understand that there could be inconvenience here, but the benefits offered by standardization outweights the slight additional eng cost.

@jwfromm
Copy link
Contributor Author

jwfromm commented Dec 18, 2020

Based on TQs feedback we should keep reshape and reverse_reshape separate. We can do this while resolving the hidden reverse attribute by splitting the type relationship for the two ops. I've updated this PR to do that instead. @icemelon9 can you take another look?

@jwfromm jwfromm changed the title [Relay][Op] Consolidate reshape and reverse_reshape operators. [Relay][Op] Remove reverse attribute from reshape and reverse_reshape operators. Dec 18, 2020
@tqchen tqchen added the status: need update need update based on feedbacks label Dec 21, 2020
@jwfromm
Copy link
Contributor Author

jwfromm commented Dec 22, 2020

I think @tqchen and @icemelon9 need to take another look to confirm that these changes look good before we can merge. I believe this solution addresses their concerns while removing the hidden reverse attribute as elegantly as possible.

Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

lgtm from my side, @icemelon9 please double check

Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

minor comment

src/relay/op/tensor/transform.cc Outdated Show resolved Hide resolved
Copy link
Member

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

LGTM. Please update on Tianqi's comment.

@jwfromm
Copy link
Contributor Author

jwfromm commented Dec 30, 2020

@icemelon9, I made the change recommended by TQ. This should now be ready to merge.

@tqchen tqchen merged commit c6b766a into apache:main Dec 31, 2020
@tqchen tqchen added status: accepted and removed status: need update need update based on feedbacks labels Dec 31, 2020
tkonolige pushed a commit to tkonolige/incubator-tvm that referenced this pull request Jan 11, 2021
TusharKanekiDey pushed a commit to TusharKanekiDey/tvm that referenced this pull request Jan 20, 2021
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jan 21, 2021
electriclilies pushed a commit to electriclilies/tvm that referenced this pull request Feb 18, 2021
@jwfromm jwfromm deleted the kill_reverse_reshape branch April 12, 2023 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants