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

[RFC] Simplify release note creation #3351

Open
vfdev-5 opened this issue Feb 4, 2021 · 12 comments
Open

[RFC] Simplify release note creation #3351

vfdev-5 opened this issue Feb 4, 2021 · 12 comments

Comments

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 4, 2021

Few ideas on how to simplify the release note creation by structuring PRs.

For example, those projects are using tools to automate some part of the process:

There are pros/cons for automation approaches:

  • automation
  • should explain how to use the bots for the PR note writing

cc @datumbox

@datumbox
Copy link
Contributor

datumbox commented Feb 4, 2021

Thanks for the write up @vfdev-5, this is very useful.

Here are some thoughts on this:

  1. We don't need a "perfect" solution. Anything that helps us organize the majority of PRs and reduce the amount of time we spend on release notes by say 70% is good enough IMO.
  2. It does not have to be fully automated or make modifications on the repo. We can adopt a solution that requires some manual editing and improve it as we go.
  3. We will need a way to classify/tag PRs before we merge them. It might be good to put a CI rule that won't allow merging unless the PR is properly marked to ensure we don't miss any update.

Any thoughts @fmassa, @NicolasHug?

@NicolasHug
Copy link
Member

I agree with @datumbox comment.

@fmassa and I wrote the release notes last week and we spent the vast majority of the time labeling the PRs. As it's very time-consuming, so it'd be good to have a process that would make this faster for the next releases. There are 2 label levels:

  • Which area is affected: OPS (operators), TRANS (tranforms), MOD (models), REF (references), IO, DAT (datasets), UTILS, MOB (mobile), BUILD
  • The nature of the changes: BCB (BC breaking), FIX, IMP (improvement), DOC, DEP (deprecation), INT (internal, for things like refactoring or CI stuff), TEST (improved tests or new test for an existing thing), MISC.

Note: the labels aren't necessarily mutually exclusive so it's fine to have e.g. OPS FIX DEP or MOD BC-Breaking on the same PR. Also the names aren't set in stone, we don't have to be crazy strict about these as the process is still manual anyway.

I think that if reviewers prefixed the commit message with these labels before squash-and-merging, it would greatly help the release note writing. Also while we're adding these prefixes, we might as well edit the commit message to something more descriptive than what the current PR title says (as sometimes the original titles aren't changelog-friendly).

@fmassa
Copy link
Member

fmassa commented Mar 5, 2021

I like the simplicity of the approach proposed by @NicolasHug of tagging the commit message, but I think we might more often than not forget to manually change the commit title before committing to the repo. And once the PR has been merged, it is not possible to change it.

I would propose an intermediate approach which uses github labels to categorize the PRs.
We already add labels for issues, but we haven't used labels for PRs yet, and I think this would be a good moment to start.

Getting github labels in an automated fashion can be obtained either via gh pr view <pr_id> (but it shows too much information and it doesn't look like we can customize it to only show the labels), or follow something a bit more custom which is what PyTorch is doing for their release notes.

By slightly tweaking the already existing scripts from PyTorch we can customize it for this use-case.

So the idea would be the following:

  • before merging a PR, try to add the relevant labels to it (and if possible improve the commit message)
  • each week, the oncall will go over unlabelled PRs that have been merged and add labels to them.

This will allow us to remove the (current) biggest burden from the release notes process (PR categorization).

I think adding per-commit release notes messages might be too much, and the highlights section should normally be done just before a release (as we get the full context of the release already there).

Thoughts?

@datumbox
Copy link
Contributor

datumbox commented Mar 6, 2021

+1 on the overall approach and I agree on implementing this with label as GitHub titles can't be changed post merge.

@NicolasHug
Copy link
Member

OK, I agree that using github labels instead of commit messages might be more flexible.

I just went and tagged all the merged PRs since the 0.9 release. Let's try to set them before merging ;) !

Getting github labels in an automated fashion can be obtained either via gh pr view <pr_id>

That's great! If we can't tell gh to only return the labels we can always do gh pr view 3434 | grep labels | cut -f2- -d":" then (until the output format changes)

@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Mar 9, 2021

There is also a way to auto label PRs (via GitHub Actions) depending on which folder is modified. If any interest I can provide more details.

@NicolasHug
Copy link
Member

I think we might want to keep things manual for now, as the labelling is often more subtle than just checking which folder gets modified. For example for a new feature, tests will be added, but it doesn't mean that it should be labelled with Test.

In terms of automation though, something that might be useful is to prevent merging (or at least hint at not mergning) unless the PR is properly tagged. Do you know if that's easily doable, maybe with GitHub Actions? I guess we could add a new CI check for that but that may be overkill

@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Mar 9, 2021

Well, normally, we label usually submodules of the lib and other important parts. But I see what you mean and agree that some labels can be misleading.

Do you know if that's easily doable, maybe with GitHub Actions?

It depends on what exactly means "PR is properly tagged"... Just fail a run on a PR that does not have any labels shouldn't be hard...

@NicolasHug
Copy link
Member

It depends on what exactly means "PR is properly tagged"

I'm thinking of preventing a merge unless at least one of a set of labels is present (the set being the equivalent of the one in #3351 (comment)).

But ideally, this would be done when we try to merge the PR at the very end, not as a CI job. It's better to tag (or re-tag) right before merging because the scope of a PR may change throughout its existence, potentially making some labels obsolete. Also more CI jobs = higher chances of a flaky CI = unhappy maintainers and contributors.

@datumbox
Copy link
Contributor

Is it worth creating a short list here of the tags we are supposed to use for tagging the PRs? It might be worth defining what they mean. For example, what's the difference between enhancement and improvement? Are they the duplicates?

Finally I noticed that we have a tag called fix. I understand that this is intended to be used for issues marked as bug. How everyone feels about using the same tags on PRs and issues, rather than having pairs like fix and bug?

@NicolasHug
Copy link
Member

Is it worth creating a short list here of the tags we are supposed to use for tagging the PRs?

Yes although I'd suggest to do that later, once we've used the new system for a bit longer. It's likely that some tags will be added / removed until then

Regarding your following points, I created improvement and fix as they better match the names used in the changelog. Using the same tags for issues and PR sounds fine to me

@fmassa
Copy link
Member

fmassa commented Mar 12, 2021

I would encourage us using the same tags as what we use for issues already, It's easier to rename an already existing tag than to merge two tags later on I believe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants