-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Update PR labeling requirements #4618
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Hey @NicolasHug! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
Thanks for the review! I think we'll still need to be a bit mindful because if we completely forget to tag something and then tag it with e.g. just "modules: dataset", the bot won't be activated again to tell us that we're still missing a primary label. But hopefully these cases should be rare. @fmassa @datumbox @prabhat00155 @pmeier let's just try to remember to always include one of these labels when merging a PR:
Thanks! |
This script is mainly about release generation, right? We can potentially have a bot to put status for a PR before it's merged. So if there is no required label, we will put a red mark. Something like CLA bot does. This way, a person merging would see it as a reminder to put a label. What do you think? |
I think this bot has been working fairly well for us so far, but I'm happy to consider alternatives. I prefer it this way because I don't think we want the CI to go red, or prevent someone from merging just because there's no label. Sometimes, people from outside the team (pytorch core devs, infra devs, people from the OSS-team) will need to merge PRs in torchvision and they're not familiar with our process, so we don't want to bother them with that. |
thanks for the answer! I completely understand where you are coming from. I agree, that it's empowering to enable to move fast and not to make strict rules :) |
Reviewed By: NicolasHug Differential Revision: D31758307 fbshipit-source-id: 81bd81ab26b5813aa4cc9b08903d6538feb33777
I just took care of categorizing all the PRs of the upcoming release. Overall the current labeling in place is tremendously useful (thanks @pmeier for the bot!!).
Our release notes consists of mainly these sections:
Since each of these has a 1-1 mapping to one of our tags, I'd like to make such tags mandatory for labeling a PR. Often, these tags are missing, and this forces the one who's categorizing the release notes to manually check every single one of these. If you take a look at the last cell (11) of this notebook that I used, there were about 150 such PRs for this release. That's a lot!! It would save a lot of time if we would have these tags properly set right from the beginning.
Since it's not always incredibly obvious what a PR should be labeled with, we can have a new "other" tag to use as an escape and which would be manually treated, but hopefully we wouldn't use this one too much.
cc @seemethere