Github action for auto-assigning reviewers #35846
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
Should be ready, pinging @ydshieh and @ArthurZucker for review! (In future, these pings will be automatic 😅 ) |
|
The That is why it is secure by default (and not blocked by the So here we need to find the actual PR commit, checkout to it, but keep the job/steps secure when doing this. It's kind involved. If you prefer, I can work on this workflow part next week. |
ArthurZucker
left a comment
ArthurZucker
left a comment
There was a problem hiding this comment.
Fully trust @ydshieh on this one for the safety part!
|
Guess I can take over this PR unless @Rocketknight1 has any other thought. Will do it this week. |
|
I'm sorry, I just got distracted by other stuff! The PR should be ready, though - if you want to merge it or take over, go ahead @ydshieh |
|
I took a look, and @Rocketknight1 is using Furthermore, the script Therefore, my previous concern regarding security is not presenting. Great job @Rocketknight1 ! |
|
I will review this PR in a more detailed manner later. |
| @@ -0,0 +1,71 @@ | |||
| import os | |||
There was a problem hiding this comment.
and potentially doc helper
|
cc @ydshieh I'm seeing typing failures that I think your PR already fixed - do you know why the CI is still running the old verison? |
|
maybe you haven't rebase / merge the latest main branch ..? |
|
It's rebased! |
Rocketknight1
force-pushed
the
auto-assign-reviewers
branch
from
41bb399 to
508b273
Compare
|
A second rebase fixed it - maybe I just needed to clear a cache @ydshieh! Sorry to bother you. Should be ready for final review now though, and licence is added @ArthurZucker ! |
ydshieh
left a comment
ydshieh
left a comment
There was a problem hiding this comment.
works for me (especially no security concerns I can think of), only a few nit questions.
Great 💯
| print(f"Reviewers already requested: {users_requested}") | ||
| return | ||
|
|
||
| locs_per_owner = Counter() |
There was a problem hiding this comment.
My bad, I somehow missed this until now! LOCs = lines of code
There was a problem hiding this comment.
maybe i am just not familiar with common abbreviation here. But if you think it would help in general / in the future, let's add a comment about the meaning.
.github/scripts/assign_reviewers.py
Outdated
| locs_per_owner[owner] += file.changes | ||
|
|
||
| # Assign the top 2 based on locs changed as reviewers | ||
| top_owners = locs_per_owner.most_common(3) |
There was a problem hiding this comment.
3 here seems a bit strange
There was a problem hiding this comment.
The reason for 3 is that the author might be included as well. Therefore, we take the top 3 in order, discard the author if present, and if we still have 3 then we discard the lowest value.
There was a problem hiding this comment.
sounds like a good comment to be included :-)
There was a problem hiding this comment.
Good point - I cleaned it up to be a bit more obvious!
|
|
||
| # More high-level globs catch cases when specific rules later don't apply | ||
| /src/transformers/models/*/*processing* @molbap @yonigozlan @qubvel | ||
| /src/transformers/models/*/image_processing* @qubvel |
There was a problem hiding this comment.
i guess @qubvel will be ping for all /src/transformers/models/*/image_processing_*_fast* too? Not sure if he will be super happy
There was a problem hiding this comment.
Ah, good point! This needs a rewrite
Rocketknight1
force-pushed
the
auto-assign-reviewers
branch
from
6a1bbb6 to
dc39a55
Compare
|
The auto-assign action picks 2-3 reviewers based on file changes. How exactly does it rank them? Should we refine the logic to avoid unnecessary pings (e.g., skipping PR authors automatically)? |
|
@machinelearningprodigy They're ranked by how many lines of code touch files that are "owned" by that person, where ownership is defined in the |
Rocketknight1
force-pushed
the
auto-assign-reviewers
branch
from
9a860bf to
ccb865b
Compare
This draft PR adds a Github action to assign reviewers. It takes a similar approach to the CODEOWNERS file, but it tries to choose one or two most relevant reviewers rather than just pinging absolutely everyone.