[Relay][Op] Remove reverse attribute from reshape and reverse_reshape operators.

[jwfromm]

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]

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

[electriclilies]

Looks good to me! Thanks for getting this up quickly

[u99127]

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]

@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]

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]

@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]

@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]

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]

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]

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.

[tqchen]

lgtm from my side, @icemelon9 please double check

[tqchen]

minor comment

[icemelon]

LGTM. Please update on Tianqi's comment.

[jwfromm]

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