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

MAINT: Label new PRs with relevant languages and modules #16870

Merged
merged 4 commits into from
Sep 5, 2022

Conversation

jjerphan
Copy link
Contributor

@jjerphan jjerphan commented Aug 20, 2022

Reference issue

Rework of #14677

Relates to #14709 and #14967

What does this implement/fix?

Maintainers and some contributors might be interested in browsing PRs based on the languages they modify or modules they modify.

The "Cython", "Fortran" and "C/C++", as well as labels for each modules exist to do so, but have to be manually added during triage work.

This commit adds a GitHub workflow to automatically label Pull Requests based on file they modify. The workflow is only run once when the PRs, allowing adapting the labels manually then if needed.

The setup is inspired from scikit-learn's, which relies on @thomasjpfan's project: thomasjpfan/labeler.

Additional information

I have defined globs based on the pattern I saw in the code base.
Yet, there might be better patterns.

Also this setup can be extended to label PRs based on the sub-modules they modify.

Moreover it is possible to run a script to label existing Pull Requests.

This PR is inspired from scikit-learn/scikit-learn#19850.

cc @tupui, @rgommers

Maintainers and some contributors might be interested in
browsing PRs based on the languages they modify.
The "Cython", "Fortran" and "C/C++" exist to do so, but
have to be manually added during triage work.

This commit add a GitHub workflow to automatically label
Pull Requests with the "Cython", "Fortran" and "C/C++"
labels if they modify files whose names match one of the
associated defined globs.

The setup is inspired from scikit-learn's, which relies on
@thomasjpfan's work.

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@tupui
Copy link
Member

tupui commented Aug 20, 2022

Thanks @jjerphan. I had done that and we actually removed it after testing it out.

The reason was that it was running after every commit and sometimes going agains manual updates. Like you would add a label and it would remove it (or the other way around). It was based on an official action (they have a few bug reports and even some PRs about that, but the action is abandoned).

Does this action do something else? I am personally happy to reconsider something like that.

@tupui tupui added the github Items related to the code repository label Aug 20, 2022
@tupui
Copy link
Member

tupui commented Aug 24, 2022

We just had our community meeting and the reception was positive. Provided the action does not remove any manual update of the labels from a maintainer.

To the proposed configuration, I would add filters to add the label for the modules too. These are the tedious ones that should not require much thinking.

jjerphan and others added 2 commits August 24, 2022 14:57
Based on scikit-learn's setup and
scipy#14677.

The the 'any' for `scipy.sparse` has been changed to 'all'
for a correct (I think) labeling.

Co-authored-by: Pamphile Roy <roy.pamphile@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@jjerphan jjerphan marked this pull request as ready for review August 24, 2022 19:03
@jjerphan jjerphan changed the title MAINT: Label new PRs with some modified files' languages MAINT: Label new PRs with relevant languages and modules Aug 24, 2022
Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thanks for the update @jjerphan. @thomasjpfan could you review this since it's your extension?

On my side I only have one comment about merging the two config files to only have a single job.

.github/labeler-modules.yml Outdated Show resolved Hide resolved
.github/workflows/pull-request-labeler.yml Outdated Show resolved Hide resolved
.github/labeler-modules.yml Outdated Show resolved Hide resolved
Co-authored-by: Pamphile Roy <roy.pamphile@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My take on this is 👍 Since I was involved in the previous work, I will let another maintainer have a look and merge if they also agree.

Again, thank you both for your help!

@jjerphan
Copy link
Contributor Author

Following the @thomasjpfan's remark in #16870 (comment), actions/labeler can be used as the max-labels feature added in thomasjpfan/labeler is not used.

@tupui
Copy link
Member

tupui commented Aug 30, 2022

Following the @thomasjpfan's remark in #16870 (comment), actions/labeler can be used as the max-labels feature added in thomasjpfan/labeler is not used.

Ideally yes an official action sounds nice. But as I said before, this action's maintenance is not ideal (maybe this is changing as a release was recently made). This is what we used and there was some sync problem-still not resolved, see actions/labeler#113. So if your PR works, I prefer to go with it instead. And I know Thomas would support this better 😉

I would plan to merge this on Friday unless there are more comments/concerns.

@tupui
Copy link
Member

tupui commented Sep 5, 2022

Alright, let's try this out. Thanks everyone 🙇

@tupui tupui merged commit 4508bef into scipy:main Sep 5, 2022
@jjerphan jjerphan deleted the maint/file-ext-labeler branch September 5, 2022 08:36
@jjerphan
Copy link
Contributor Author

jjerphan commented Sep 5, 2022

Thank you!

IIRC, maintainer can run a script to update Pull Requests that were created before the merge of this PR.

@thomasjpfan: SciPy's maintainers are interested running it and if this does not necessitate too much search, do you still have the script you have used for scikit-learn to update Pull Requests?

@tupui
Copy link
Member

tupui commented Sep 5, 2022

Don't worry, I am not sure that would fly as we are pretty conservative and cautious.

@tupui
Copy link
Member

tupui commented Sep 19, 2022

@jjerphan there might be an issue with the labeller. I can see that it was never run even on PR made by maintainers where we don't need to accept workflows to run first.

@jjerphan
Copy link
Contributor Author

Thanks for following-up.

In this case, I think it is be sensible trying to change the pull_request_target from created to something else and see if it resolves it.

It is a bit documented on this piece of documentation about pull_request_target and on this blog post section.

I think we can try replacing created with ready_for_review?

@tupui
Copy link
Member

tupui commented Sep 20, 2022

I would suggest to test this out in a personal fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github Items related to the code repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants