Skip to content

MNT Adds autolabler for modules #16520

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Feb 27, 2020
Merged

MNT Adds autolabler for modules #16520

merged 6 commits into from
Feb 27, 2020

Conversation

thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Feb 21, 2020 •
edited
Loading

Automatically adds labels based on the folder that was changed.

@thomasjpfan thomasjpfan changed the title Testing autolabeler MNT Adds autolabler for modules Feb 21, 2020
@thomasjpfan
Copy link
Member Author

The failure on the lalaber is because this PR can not run against itself. This is most likely due to permissions.

An example of this PR working is here: thomasjpfan#18 (This is a PR on my own fork)

jnothman
jnothman reviewed Feb 22, 2020
View reviewed changes
Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

I think if a PR touches a lot of modules, then these labels probably don't apply. Can we limit the auto labelling to the case where few modules are touched??

@thomasjpfan
Copy link
Member Author

I forked the labeler repo and added a max-labels feature: If the number of matched labels is greater than max-labels then no labels are added (default=100)

In this PR I set the number to 3.

rth
rth approved these changes Feb 25, 2020
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Hard to check that this will behave fully as expected once merged, but I guess it's only labels in the end.

Thanks Thomas!

@rth rth requested a review from adrinjalali February 27, 2020 11:34
adrinjalali
adrinjalali reviewed Feb 27, 2020
View reviewed changes
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Nice, I think @cmarmo had the same idea at some point

.github/workflows/labeler.yml
triage:
runs-on: ubuntu-latest
steps:
- uses: thomasjpfan/labeler@master
Copy link
Member

Choose a reason for hiding this comment

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

question, does this have to be on your user?

Copy link
Member

Choose a reason for hiding this comment

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

From a comment above

I forked the labeler repo and added a max-labels feature

It's probably safer than running third party code from master (or a tag which can be force pushed) anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I just don't know what this thing does really, trying to understand :D

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to update this PR with my v2.2.0 tag.

@adrinjalali adrinjalali merged commit 9b1928d into scikit-learn:master Feb 27, 2020
@adrinjalali
Copy link
Member

Let see how it works. My worry is about its maintainability. We could maybe move the labler repo to the scikit-learn org.

@rth
Copy link
Member

rth commented Feb 27, 2020

Looks like it does fail in PRs e.g #16348 (comment)

Run thomasjpfan/labeler@master
  with:
    repo-token: ***
    max-labels: 3
    configuration-path: .github/labeler.yml
##[error]File not found: '/home/runner/work/_actions/thomasjpfan/labeler/master/lib/main.js'

@rth rth mentioned this pull request Feb 27, 2020
Merged
@rth
Copy link
Member

rth commented Feb 27, 2020

Also email notifications for failed jobs seem to be on,

Subject: [scikit-learn/scikit-learn] PR run failed: Pull Request Labeler - DOC Fix versionchanged in OneHotEncoder (96504b7)

Run failed for fix-ohe-versionchanged (96504b7)

Repository: scikit-learn/scikit-learn
Workflow: Pull Request Labeler
Duration: 16.0 seconds
Finished: 2020-02-27 13:03:43 UTC

View results
Jobs:

    97177f48-1003-5b21-734c-976eeef6ed97 failed (1 annotation)

@cmarmo
Copy link
Contributor

cmarmo commented Feb 27, 2020

Nice, I think @cmarmo had the same idea at some point

Yes, but this time I didn't even have the time to start working on that... and already saw the PR... ;)

@rth rth mentioned this pull request Feb 27, 2020
Merged
@thomasjpfan
Copy link
Member Author

For details, the main.js needs to be generated from the typescript, which is traditionally built into the release.

This is built into the release v2.2.0.

@rth
Copy link
Member

rth commented Feb 27, 2020

I see. Should someone push a fix to master?

@thomasjpfan thomasjpfan mentioned this pull request Feb 27, 2020
Merged
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
@thomasjpfan
MNT Adds autolabler for modules (scikit-learn#16520)
5827c99 
* MNT Adds labeler

* BUG Fix

* Double quotes are better

* BUG Fix

* MNT Adds build ci tag

* MNT Use fork for new feature
gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
@thomasjpfan @gio8tisu
MNT Adds autolabler for modules (scikit-learn#16520)
927af4d 
* MNT Adds labeler

* BUG Fix

* Double quotes are better

* BUG Fix

* MNT Adds build ci tag

* MNT Use fork for new feature
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman left review comments

@rth rth rth approved these changes

@adrinjalali adrinjalali adrinjalali approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thomasjpfan @adrinjalali @rth @cmarmo @jnothman