Skip to content

MNT Adds autolabler for modules#16520

Merged
adrinjalali merged 6 commits intoscikit-learn:masterfrom
thomasjpfan:only_change_setup
Feb 27, 2020
Merged

MNT Adds autolabler for modules#16520
adrinjalali merged 6 commits intoscikit-learn:masterfrom
thomasjpfan:only_change_setup

Conversation

@thomasjpfan
Copy link
Copy Markdown
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
Copy Markdown
Member Author

thomasjpfan commented Feb 21, 2020

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
Copy Markdown
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
Copy Markdown
Member Author

thomasjpfan commented Feb 22, 2020

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

@adrinjalali adrinjalali Feb 27, 2020

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
Copy Markdown
Member

@rth rth Feb 27, 2020

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
Copy Markdown
Member

@adrinjalali adrinjalali Feb 27, 2020

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
Copy Markdown
Member Author

@thomasjpfan thomasjpfan Feb 27, 2020

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
Copy Markdown
Member

adrinjalali commented Feb 27, 2020

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

thomasjpfan commented Feb 27, 2020

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
Copy Markdown
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

@adrinjalali adrinjalali adrinjalali approved these changes

+1 more reviewer

@rth rth rth approved these changes

Reviewers whose approvals may not affect merge requirements

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