Fix in label synchronization bot according to issue #40758 #40839
Conversation
TestcaseAs a first testcase I use #39881, since this has two merge commits and 
Since one of the merge commits ( 
Using the new The assertion is confirmed in the last line: |
|
Documentation preview for this PR (built with commit 0a3cfe2; changes) is ready! 🎉 |
.github/sync_labels.py
Outdated
| if Status.needs_work.value in self.get_labels(): | ||
| for com in date_commits: | ||
| message = com['messageHeadline'] | ||
| if message.startswith('Merge') and message.find('develop') > 0: |
There was a problem hiding this comment.
| if message.startswith('Merge') and message.find('develop') > 0: | |
| if message.startswith("Merge branch 'develop' into "): |
Would this work? Since it's more specific it's less likely to have a false positive.
There was a problem hiding this comment.
Never mind, I see you already mentioned this. You can ignore this suggestion if you think people changing the merge commit message could be an issue.
There was a problem hiding this comment.
There are also default messages that wouldn't match, for example in #40448: Merge remote-tracking branch 'upstream/develop' into remove-incremental
.github/sync_labels.py
Outdated
|
|
||
| # ignore merge commits with the develop branch for commit_date if needs_work is set | ||
| date_commits = list(self._commits) | ||
| if Status.needs_work.value in self.get_labels(): |
There was a problem hiding this comment.
Is there a reason not to include the needs info tag as well here?
There was a problem hiding this comment.
No! I will include it in the next commit.
This is a bit complicated. I've created a dummy PR (soehms#16) in my fork relative to the branch of this PR. I've declared the latter as the default branch so that GH-Action uses the changes in You should request changes on this PR. Afterwards I will merge the change concerning |
We cannot test the new changes with this PR because GH-Actions inherits the code from the base branch of the PR, which is
That's right! This is the case when all of the bot's triggers are activated, meaning synchronization is bidirectional, meaning a change in the GH states translates into a change in the labels and vice versa. Currently, the direction from states to labels isn't even fully activated (changes in the review state are missing). For the return path, the activation of label changes is missing. You can see the current activation status in the header of #35927: 
If all triggers are enabled, your scenario works like this:
|
Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>
|
Sorry, time is over for the moment. I'll do the |
|
I'm wondering (loud) if it makes sense to never change needs work to needs review. Take for example this PR: Vincent had a few remarks, some but not all of them were addressed in a new commit - and the bot put it now onto needs review, although it is still in needs work. This would also align it a bit better with the github interface: Vincent's review still shows as "requested changes" although there was a commit afterwards. But I'm probably missing something essential here, in that case, sorry for the noise. Btw, what's blocking the activation of
This would be quite convenient to have, and looks very similar to the already active "marking a draft PR as ready for review" |
I think this came up on sage-devel. The problem is that changing labels requires certain GitHub permissions that not all contributors (particularly new contributors) have. So if a reviewer requests changes, the changes are made, and the reviewer doesn't look at the changes then the PR will be stuck marked as "needs work" and will probably get ignored. If that happened to one of my PRs I could change the label. If it happens to a new contributor they wouldn't have any way of bringing attention to their PR (besides tagging people, and they probably wouldn't know who to tag and might not be comfortable tagging random people they've never interacted with). I think the main issue with the bot changing the labels to needs review when it doesn't need a review is that it makes it harder to find PRs that actually need a review. This PR should cut down on that noise a lot, even if there may still be some situations where the bot changes the label too early. |
I also had Vincent's point in mind. On the other hand, a developer who can't set labels has two more options for setting
In the sage-devel thread, you stated that our
I agree that we shouldn't use these labels directly if possible. However, they may still be necessary to reflect the status of the PR and facilitate searching within PRs. A re-review request attracts less attention than a Therefore, we should now focus on one-way synchronization from GH tools to labels. I agree that the more accurately the labels reflect the GH status, the better. From this perspective, I support your suggestion.
I really don't know the answer. I think it just disappeared from the screen.
I agree! If you'd like to activate it, please let me know the date. I'll make the announcement then. When this is done, the one-way sync mentioned above will be completed. I think this will be enough if the labels will only be set manually when there is no other option! |
It's possible to test it, which involves forking Sage, enabling GitHub actions on the fork, then merge the PR into the develop branch of the fork, and open a pull request on the fork. |
We should make things easy for first-time contributors, so we can't expect them to know about this. Especially as many Sage contributors are mathematicians who may have little (if any) familiarity with git/GitHub.
Same comment applies, new contributors may not know this. Ultimately I think the point of this PR is to decrease the noise of unnecessary "needs review" tags with our current workflow. |
I think that's what soehms#16 is for. |
I agree! But we probably need to change the developer guide regarding the use of status labels and focus more on the use of GH tools. I think that's implicit in Tobias's suggestion. |
I checked this at soehms#16. The GH review status remained. 
After Re-reqesting changes it looks like this: 
|
This assumption was wrong (see soehms#16 (comment)). |
Setting to needs work then (unless you meant to fix it in a follow-up. I think not changing the status to "needs review" after merging develop is enough for this PR if you'd prefer to keep this small so it can be merged sooner). |
That's ok for me! |
Is there anything else that needs to be done here then? It seemed to work on soehms#16. |
Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>
I don't think so!
Yes! |
|
Great! Approved. Thanks for the contribution! |
This is a good point, and we should indeed make it easy for new contributors to get their PR reviewed. Would it be possible though to restrict this mechanism of resetting the "needs work" label to PRs from people not in the sagemath-triage team? Everyone in that team can change the label themselves.
GH actually has built-in tools to search for PRs that need review / have changes requested. https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/filtering-and-searching-issues-and-pull-requests#filtering-pull-requests-by-review-status The problem is that those tools are not very useful at the moment in the sage repo since GH reviews are not used consistently but instead people just manipulate the "needs review" label. For newcomers that are familiar with GH, this is another sage-specific thing they would have to learn. The only real difference is that our labels are a bit more flashy than how GH represents the status - compare "s: needs work" to "Changes requested":
I hope at some point the "s: needs work" and "s: needs review" could become read only, and are exclusively set by the bot.
This has to be done by someone in @sagemath/core |
Thanks for the review! |
If the bot would login as an organization member (as I explained here) this should be possible.
Thanks! Good link. I never used that.
I hope so too. But a lot of convincing work certainly still needs to be done.
Ah! I remember that you did that in the past, didn't you. |
sagemathgh-40839: Fix in label synchronization bot according to issue sagemath#40758 <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> This PR fixes sagemath#40758. > The action https://github.com/sagemath/sage/blob/develop/.github/sync_labels.py should not change "needs work" (maybe "needs info" too, but we don't use that as much so I'm not sure if it has different behaviour currently) to "needs review" when the commit message matches the default message for a merge commit. Alternatives Considered This is achieved by ignoring merge commits when the latest commit date is determined in case a `s: needs_work` label exists. Since `s: needs_work` persists if there is a more recent review requesting changes this should solve the problem. > Merging develop should still trigger a new CI run. I don't imagine that this change would effect that though. This remains unchanged. > Check whether merging from the command line with git and merging from the GitHub web interface generate the same default merge commit messages. In both cases, the predefined message begins with `Merge` and contains the word `develop`. I just check that this is valid. However, note that the user (at least in the in the command line) has the option of changing the messages. Therefore, we generally cannot detect merge commits exactly. This PR also implements a new method `test_method` just for test purposes. This allows to run arbitrary methods of the class from the command line. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#40839 Reported by: Sebastian Oehms Reviewer(s): Sebastian Oehms, Vincent Macri
This PR fixes #40758.
This is achieved by ignoring merge commits when the latest commit date is determined in case a
s: needs_worklabel exists. Sinces: needs_workpersists if there is a more recent review requesting changes this should solve the problem.This remains unchanged.
In both cases, the predefined message begins with
Mergeand contains the worddevelop. I just check that this is valid. However, note that the user (at least in the in the command line) has the option of changing the messages. Therefore, we generally cannot detect merge commits exactly.This PR also implements a new method
test_methodjust for test purposes. This allows to run arbitrary methods of the class from the command line.📝 Checklist
⌛ Dependencies