PR process improvements - Githubissues

openxla / xla

A machine learning compiler for GPUs, CPUs, and ML accelerators

Apache License 2.0

2.6k stars 407 forks source link

PR process improvements #2038

Open ddunl opened 1 year ago

ddunl commented 1 year ago

[ ] Run PRs against JAX tests on GitHub
[ ] Run PRs against Tensorflow tests on GitHub
[x] Automated notification of rollbacks on PRs
[x] Documentation on Copybara quirks
[ ] Unify triggering of CI under Github Actions. Right now Kokoro triggers automatically in some cases, but a single label should be all that's needed to trigger CI, and this should happen automatically for all members of the organization.

i-chaochen commented 1 year ago

I wonder how about the existing pending xla PRs on tensorflow repo? will be ignored and authors need to move pending ones by themselves to openXLA repo?

Thanks in advance!

ddunl commented 1 year ago

Great question! All the pending PRs on tensorflow will still be accepted as normal. No rush to move a PR over to this repo at all, the only constraint is that if it's not merged in a ~month then they will have to update the PR to accommodate the move of XLA from tensorflow/compiler to tensorflow/third_party. But if you're starting a new PR we would love to have it on this repository. I'll add something on my TODO list to make that clear to tensorflow contributors.

bhack commented 1 year ago

But if you're starting a new PR we would love to have it on this repository. I'll add something on my TODO list to make that clear to tensorflow contributors.

We could add a notice in the template and/or a notification with the bot.

bhack commented 1 year ago

Here another example with multiple issues: not informative title, not informative commit message, anon author, no public GitHub account subscribed to the ticket for comments: https://github.com/openxla/xla/pull/2649

ddunl commented 1 year ago

Unfortunately I think that one is one we are going to have a hard time fixing, that change was made automatically by a bot that applies clang tidy changes. I'll see if I can get them to change the description to do better than "Internal code change" though

bhack commented 1 year ago

If it is limited at this case just a description is enough.

bhack commented 1 year ago

@ddunl In that PR we have an authored commit with a valid GitHub user.

ddunl commented 1 year ago

I see there's a few like that now... This is added to my list of things to talk to the Copybara team about. Not clear to me why this is happening from a quick investigation. Thanks for pointing this out!

bhack commented 1 year ago

I see there's a few like that now... This is added to my list of things to talk to the Copybara team about. Not clear to me why this is happening from a quick investigation. Thanks for pointing this out!

This specific example is minor if we compare it with other priorities. It seems that the GitHub user is participating (at least from the GitHub UX). So we don't have the "ownership" but probably it will be notified for comments.