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

Rename the region accessors for ReduceOp and ReduceWindowOp to match the intent and hlo spec #335

Closed
wants to merge 1 commit into from

Conversation

sdasgup3
Copy link
Member

@sdasgup3 sdasgup3 commented Oct 14, 2022

The existing name body suits more the region name of whileOp

@sdasgup3 sdasgup3 added Spec Migrate to MHLO PR that needs to be migrated to MLIR-HLO labels Oct 14, 2022
@sdasgup3 sdasgup3 requested a review from burmako October 14, 2022 22:17
@sdasgup3 sdasgup3 self-assigned this Oct 14, 2022
@sdasgup3
Copy link
Member Author

sdasgup3 commented Oct 14, 2022

@GleasonK
I think this change can have compatibility issues. Are we ready at this point in ascertaining the compatibility guarantees?

@sdasgup3 sdasgup3 requested a review from GleasonK October 14, 2022 22:24
@burmako
Copy link
Contributor

burmako commented Oct 14, 2022

Another question is how strongly we feel about renaming these specific fields right away.

There are a lot of opportunities for improving various names in StableHLO/MHLO, but we've been holding off so far to avoid complicating the ongoing migration from MHLO to StableHLO, with the only exception of renaming operands to inputs because operands will stop working in a few weeks.

Perhaps we could start collecting renaming ideas in a ticket similar to #289 (@atondwal has an entire document with ideas), so that at a later point we could evaluate them all in concern and then execute on all of them at once to avoid slow dripping breaking changes.

@GleasonK
Copy link
Member

@GleasonK
I think this change can have compatibility issues. Are we ready at this point in ascertaining the compatibility guarantees?

We don't offer API compatibility guarantees so I don't think this should be an issue. Can confirm by checking out main, serializing a file, and try to deserialize in this branch. Happy to help with any of that if needed.

Copy link
Contributor

@burmako burmako left a comment

Choose a reason for hiding this comment

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

As mentioned in the comment above, I'd like to propose to table the renaming discussions until later when we have a chance to review all the names. This way we'll ensure that: 1) the opset has uniform naming style, 2) all C++/Python breaking changes related to naming are done at once instead of spread over weeks or months of breakages.

@sdasgup3 sdasgup3 mentioned this pull request Oct 17, 2022
@sdasgup3
Copy link
Member Author

Superseded by #351

@sdasgup3 sdasgup3 closed this Oct 17, 2022
@sdasgup3
Copy link
Member Author

sdasgup3 commented Oct 17, 2022

@burmako Agreed and makes sense to me.
@GleasonK Thanks for the information! No action from you end at this point.

@sdasgup3 sdasgup3 mentioned this pull request Oct 18, 2022
@sdasgup3 sdasgup3 deleted the rename-reduce-regions branch January 31, 2023 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Migrate to MHLO PR that needs to be migrated to MLIR-HLO Spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants