Assign a reviewer even when a codeowner has left, and route models by modality (#48085)
* Assign a reviewer even when a codeowner has left
A review request naming a non-collaborator is rejected as a whole, so a
codeowner who has left the repo takes their valid co-owners down with them and
the PR ends up with no reviewer at all. The exception was caught and printed,
leaving a green job and a silently unassigned PR (#48070: `Top owners
[('gante', 4)]` -> `Reviews may only be requested from collaborators`).
Request each owner on its own call, skip the ones that cannot be asked and let
the next-ranked owner take the slot, and report every skip as a workflow
warning instead of a line in a step log.
`@gante`, sole owner of `generation/` and `tests/generation/`, is no longer a
collaborator (last commit on main: 87af2c1f42, 2026-06-12), so every PR
touching only generation has gone unassigned since. Generation as a whole goes
to `@Cyrilvallez`; continuous batching, which is its own subsystem, goes to
`@remi-or` (54 of its commits, against 8 for the next contributor) via two
entries in the specific-files section so they take priority.
* Fix codeowners entries that never matched a file
Three of these were silently inert:
- `kosmos_2` and `xclip` are spelled `kosmos2` and `x_clip` in `models/`, so
both patterns matched nothing and those files fell through to the `*`
catch-all instead of reaching `@zucchini-nlp`.
- `graphormer` is no longer in the library; drop the line.
`@arthurzucker` on the llava line is `@ArthurZucker` everywhere else. A login is
case-insensitive on GitHub, but the tally in `assign_reviewers.py` keyed on the
literal string, so llava changes counted twice under two spellings and could
both cost him a top-2 slot and request him twice. Tally per person instead, and
match the PR author against codeowners case-insensitively for the same reason.
* Route model reviewers by modality instead of a per-model list
The per-model list had stopped working: 219 of 495 models (44%) matched nothing
but the `*` catch-all because nobody added a line for them, 39 entries named
models that are no longer in the library, and two were misspelled. Every new
model needs a line, and that is the step that gets skipped.
Route by modality instead. The modality is the model's section in
docs/source/en/_toctree.yml, which every model must already appear in
(`utils/check_doc_toc.py::ensure_all_models_in_toctree`, part of
`make check-repository-consistency`), so the signal cannot rot the way a
hand-kept list does. Seven `@@modality/<slug>` rules replace ~300 per-model
lines, and cover the 219 models the list was missing.
A model that needs a specific reviewer says so in its own file:
# Reviewers: @someone
in the leading comment block of `modular_<model>.py` / `modeling_<model>.py`.
The modular converter copies that block verbatim into the generated modeling
file (`header=modular_mapper.python_module.header`), so the tag survives
regeneration.
Resolution order, most specific first: in-file tag, explicit codeowners rule,
the model's modality, the catch-all. Verified against every model directory:
230 resolve identically, 219 were unowned and now reach a modality owner, 57
change only because `@yonigozlan` is removed (below), and none resolve to
nobody. Non-model paths are untouched.
Owner changes requested by the model owners:
- `@yonigozlan` is removed (he is no longer a repo collaborator, so GitHub
rejected every request naming him); `@guarin` takes the vision-only side.
- `/src/transformers/models/*/video_processing_*` was missing entirely and
landed on the catch-all; it goes to `@zucchini-nlp`.
Kept as genuine exceptions: 19 per-model lines whose owner is not the modality
owner. `@NielsRogge` holds 6 of them and has no commit or review in the last 3
months, so those still need a decision.
* Drop the owners who cannot be requested at all
`@yonigozlan`, `@MekkCyber` and `@zach-huggingface` are not collaborators of the
repository, so GitHub rejects any review request naming them — they cannot serve
as reviewers, whatever the file says. Remove them rather than leave rules that
resolve to somebody unreachable.
Every affected rule keeps `@SunMarc`, so nothing is left without an owner:
`integrations/`, `quantizers/`, `trainer.py` and `trainer_utils.py` now name him
alone. All 13 owners remaining in the file are current collaborators.
* Route the paths the file was silently missing
Three more rules matched no file at all, so their owners were never asked:
- `/src/transformers/pipeline/` — the directory is `pipelines/`. Every pipeline
PR has been falling to the catch-all.
- `/src/transformers/models/*/image_processing_*_fast*` — no file is named that
any more; the `image_processing*` glob above it covers what exists.
- `/src/transformers/models/nougat/mod*_nougat*` — nougat ships a processor and a
tokenizer, no `mod*` file.
And three directories the modality fallback cannot place, because their doc page
is named differently (`openai` is documented as `openai-gpt`, `timm_backbone` as
`timm_wrapper`) or because they hold no model to review (`deprecated`, now
explicitly unowned). Everything else under `models/` is routed by modality.
* Check that every model can still reach a reviewer
Both ways this file breaks are silent: a rule whose pattern matches nothing looks
fine forever (39 such entries had accumulated, plus `/src/transformers/pipeline/`
which never matched the `pipelines/` directory), and a model nobody claims
quietly falls to the catch-all. Nothing checked either, and nothing documented
how a reviewer is picked in the first place.
`utils/check_reviewers.py` joins `make check-repository-consistency` and fails
when a model reaches nobody but the catch-all, when a rule matches no tracked
file, when a `@@modality/<slug>` names an unknown modality, when a modality the
toctree uses has no owner, when one owner is spelled two ways, or when a
`# Reviewers:` tag is malformed. Every failure prints the three places ownership
can be declared. Paths outside `models/` that have no owner (59 today, from
`modeling_utils.py` to `trainer_pt_utils.py`) are a note rather than a failure;
`--strict` lists them and fails.
The resolution logic moves to `.github/scripts/codeowners_resolver.py` so the
workflow and the check answer identically instead of drifting apart. Reading
files is injected: the workflow prefers the PR head for files it touches, the
check reads the working tree. A directory whose name extends a doc page's name is
treated as a variant of that model (`dinov3_vit` -> `dinov3`,
`wav2vec2_with_lm` -> `wav2vec2`) and shares its modality, which covers the last
model directories the toctree does not name directly.
`docs/source/en/pr_checks.md` now documents the system: when the workflow runs,
the four-step resolution order, where to declare ownership, and why a review can
only be requested from a collaborator.
* Say what the @@ rules are and why reading the head is safe
* Hand reviewer assignment over to transformers-ci
The resolution logic and the workflow now live in huggingface/transformers-ci;
this repo calls the reusable workflow and keeps the data it reads,
`.github/scripts/codeowners_for_review_action`, because who owns what is this
repo's decision. `utils/check_reviewers.py` stays too, and imports the same
resolver from the `transformers-ci` package, which the `quality` extra now
installs — one resolver, so the check cannot drift from the workflow.
A PR cannot re-route its own review: the codeowners file is read from the base
and never from the head, and the security gate already refuses a PR from a
non-collaborator that touches anything outside src/, tests/, docs/ and utils/.
That is placement doing the work, so the file now says to keep it there.
* Install the reviewer resolver in CI, not through setup.py
(we cant rely on a @git+https in setup.py that breaks twine)
* Let the checker runner install what the checkers need
Renames the file to `utils/checkers-requirements.txt` and makes it the home for
anything a checker needs that must not go in `setup.py` — not just the reviewer
resolver. `utils/checkers.py` installs it on the first run in an environment, so
running a checker, or `make check-repo`, is enough; nobody has to know the file
exists.
A stamp keyed on the file and the interpreter keeps it to one `pip` call per
environment. That is not a micro-optimisation: pip re-clones a URL requirement
every time it is asked, which is ~5s, and the checkers are meant to be cheap to
run in a loop. Cold run 5.0s, warm 0.5s.
Installing is best-effort. It never fails the run — working offline is normal,
and most checkers do not care — so a checker that needs one of these packages
is the one that decides what to do about it. `check_reviewers.py` skips with the
install line locally and errors in CI, unchanged. `TRANSFORMERS_SKIP_CHECKER_REQUIREMENTS=1`
turns the install off entirely.
* Only install checker requirements for checkers that need them
* Do not build the install message from a patched REPO_ROOT
* Stop running the repo utils tests in two jobs at once T
Tarek Ziade committed
e277d1edd6caec5f069555606a971bcbab21910c
Parent: bb593bf
Committed by GitHub <noreply@github.com>
on 8/21/2026, 2:01:52 PM