openxla / stablehlo

Backward compatible ML compute opset inspired by HLO/MHLO
Apache License 2.0
404 stars 112 forks source link

Define code style #15

Open burmako opened 2 years ago

burmako commented 2 years ago

Currently, StableHLO doesn't have a formally-defined code style, but it will be good to have one. We could start with inheriting MLIR-HLO's code style to maximize familiarity for existing CHLO/MHLO users.

bhack commented 2 years ago

We will need a code style also for https://github.com/openxla/stablehlo/issues/32

jpienaar commented 2 years ago

Note: CHLO/MHLO has mixed code style (which would be good to rectify). I'd say adopting either MLIR's style guide or Google's style guide (C++ side, Python side MLIR doesn't have en explicit one but close to Google's Python one in practice) would allow most familiarity. With the former being more consistent for end to end. (Builders and the like could also follow different style but that ends up being confusing).

burmako commented 2 years ago

Further questions to handle when defining code style, via jpienaar's review of a recent PR:

burmako commented 2 years ago

The last two weeks have been pretty hectic as we've been bootstrapping a GitHub-centric StableHLO development process from the Google-centric MLIR-HLO development process. During that time, we have set up the basic infrastructure (pull requests, issues, CI) and are almost done setting up regular integrates from StableHLO into MLIR-HLO.

I anticipate that we'll get to defining code style fairly soon (within the next 1-2 weeks).

In the meantime, @GleasonK has set up a CI check with runs git-clang-format (#49) and opened a ticket to also run clang-tidy likely bootstrapping it from MLIR-HLO (#61). In #76, @joker-eph has explored what it would take to switch the codebase from Google's clang-format style that our codebase currently follows to LLVM's clang-format style.

burmako commented 2 years ago

Another part of code style is Markdown formatting. Let's find a formatter / linter, so that we can automate changes like https://github.com/openxla/stablehlo/pull/128. (Also see Kevin's comment in that PR).

sdasgup3 commented 2 years ago

Regarding formatting markdowns: Adding links to the markdown might break the 80 lines constraint. The script can ignore those.

burmako commented 2 years ago

We're getting closer to the point when all the urgent work is done (e.g. our code now ships as part of MLIR-HLO which is great, but we still need an RFC process which is blocking quite a few things), at which point I'm planning to write something up about this.

One more item to comment on in the code style style is commit messages. Here's MLIR's guidance about this (thank you, Mehdi, for the link!): https://mlir.llvm.org/getting_started/Contributing/#commit-messages.

burmako commented 2 years ago

The linked pull request contributed to defining code style (or, more precisely, codifying a baseline that we'll build on), but this ticket is far from being resolved at this point :) Reopening.

GleasonK commented 2 years ago

Good catch. The phrasing "should resolve \<issue> soon" from that PR falls under the words to link/close an issue.

GleasonK commented 1 year ago

Another part of code style is Markdown formatting.

@scottamain has been looking into this recently: #795, #792, #793, #729