openxla / stablehlo

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

Regenerated pass documentation to fix bazel build failure at head #2352

Closed abhigunj closed 4 months ago

abhigunj commented 4 months ago

documentation is outdated after https://github.com/openxla/stablehlo/pull/2253

Validated that the change actually fixes the failure

Follow up: why https://github.com/openxla/stablehlo/pull/2253 CI check did not complain?

mlevesquedion commented 4 months ago

documentation is outdated after #2253

Validated that the change actually fixes the failure

Follow up: why #2253 CI check did not complain?

Thanks for fixing the docs!

I think I know what happened. Unfortunately, we've seen this before: https://github.com/openxla/stablehlo/pull/1996

The PR that introduced the breakage is https://github.com/openxla/stablehlo/pull/2253. The last time CI ran on that PR is May 7: https://github.com/openxla/stablehlo/actions/runs/8988090362. The doc build change was introduced May 14: https://github.com/openxla/stablehlo/pull/2310. GH doesn't re-run CI for pending PRs when main is updated (which is reasonable, that would lead to quadratic CI runs). However, I think it would be reasonable to force CI to re-run before merging. Given that this has happened twice, I'll take a quick look to see if we can prevent this from happening again.

mlevesquedion commented 4 months ago

I don't have access to repo settings so I can't fiddle with this, but apparently we can force a PR to be up to date with main before it can be merged: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/managing-a-branch-protection-rule#:~:text=Require%20branches%20to%20be%20up%20to%20date%20before%20merging.

Concretely, turning on that setting means that we will need to rebase branches onto main every time we want to merge a branch, if a commit has been merged into main since the last commit in the branch. For example, if I have 5 PRs ready to merge, I won't be able to merge them in parallel; I will have to: 1. merge one of the PRs into main, 2. rebase the next PR onto main, 3. goto 1 until no PRs are left. If that becomes too onerous, merge queues may help: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue

While the two incidents we've had so far weren't too bad (especially this second one: we want our docs to be up to date, but it's not like any code was broken), I personally think having broken code at HEAD is a fairly big deal. I don't think having to rebase all changes would be too bad for frequent contributors, but it may be annoying for newer contributors, especially because our workflows only run when they get approval from a maintainer. Maybe we can revisit that, though. I personally don't think there's much of a reason to avoid running CI, except if the PR is changing GH action configuration (there may be other cases).