Repository navigation
[Docs] Add PR checklist skill for coding agents - #57084
Conversation
Add a maintainer-informed PR review workflow, require agents to use it before submission, and organize the PR template around claims and validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Benjamin Chislett <bchislett@nvidia.com>
|
Documentation preview: https://vllm--57084.org.readthedocs.build/en/57084/ |
|
|
||
| Recurring instances where replication may be acceptable by design: | ||
| - New model code: It may be necessary to replicate certain patterns or boilerplate when adding support for a new model. Note that this is less relevant for layer subcomponents, which can often be reused from existing models (such as Attention and MoE modules). | ||
| - Speculative decoding: Certain patterns or boilerplate may need to be replicated when implementing new speculative decoding mechanisms. Effort should still be made to reuse common functionality and subcomponents even when core architecture discourages inheritance. |
There was a problem hiding this comment.
These might be better fitted in each folder's agent.md, as different part of the codebase has different guidelines.
|
Love the statement that it is "mandatory for agents, optional for humans". Some reviewer pointed out that agent generated PR descriptions tend to be long. Do we want to "limit to 100-200 words"? LGTM overall |
|
@faradawn I indeed share the frustration of overly wordy PR descriptions. However, we don't want to suppress agents from sharing relevant drawbacks. My compromise here was to enforce header sections that are length-constrained:
Perhaps I should extend this to explicitly mention the "claims" section. For Validation and Notes sections, I thought it would be better to let these sections be wordy -- we often do want to see extensive validation results, and a detailed root-cause trace, for tricky issues. I thought about requiring that these be sorted into details: example
some very long validation content could go in here But I'm not sure if that adds much value if there's not anything else in the description we're trying to scroll towards. |
|
Good point that we want the agents to share potential drawbacks. I like the idea that of a "short 1-2 line summary". |
|
Tagging survey respondents who flagged this. Please react 👍 or comment. @AndreasKaratzas — you flagged that agent PR have long text. Does the "1–2 line limit" fix that? @tlrmchlsmth — "way too much text everywhere." Does this PR address your concern? Also @njhill, @bbrowning, @DarkLight1337, @tjtanaa, @mgoin, @esmeetu, @aoshen02, @penli9 - 👍 or comments welcome. Aiming to merge by Friday Sep 25. |
|
My prompt to the agent usually is "write 3-5 bullet point list with your modifications followed up by a 3-6 sentence mini-paragraph with the story and motivation behind these changes" (including buildkite link or regression PR if the PR is targeting a CI regression fix). For kernel PRs, I do like the occasional print screen of the traces as well, to ensure that this was not 100% vibe coded, contrary it was validated and the author rendered the traces before and after their modifications. Also table about gains and perf evals are good, but again a prompt could be limit this to one lm eval table and a bench table that contains average and stats across 10 bench runs. |
njhill
left a comment
There was a problem hiding this comment.
Thanks @benchislett for taking to the initiative with this. Looks great to me overall. I've suggested a few additions inline.
Something else we could consider adding but possibly as a later improvement, is pointers to examples of some of the points, in the form of prior PR or existing code sections / features that exemplify our expectations.
For Validation and Notes sections, I thought it would be better to let these sections be wordy -- we often do want to see extensive validation results, and a detailed root-cause trace, for tricky issues.
I think it would still be worthwhile to emphasize conciseness for explanations etc. I often find it quite time consuming to wade through verbose slop descriptions to understand the salient points.
|
@benchislett just in case it wasn't obvious, most of my comments are suggesting additions rather than replacements. Also I just had another thought that we could add a specific directive to ensure cpu<->gpu checks aren't introduced on the main model execution thread (though we now have the checker in CI, some still slip through since not all configurations are exercised in CI). |
|
Got very busy this weekend. Will update tomorrow. Thanks very much everyone for feedback! |
|
Going to trigger CI just to see what it wants to run. Ideally nothing, since there are no code changes |
|
/ci run |
|
✅ Triggered Buildkite CI #92177 for commit |
CI selector (shadow): 0 test steps (0 jobs) instead of 0 (0 jobs)Shadow mode: this changes nothing about what CI runs. It shows what the evidence-based selector would pick for this PR, next to today's rules. How it works. Feedback welcome: reply here if it would skip a step this change needs, or runs something unrelated.
Docs-only change: today's rules run no tests. Selector would run (0)none Would skip (today's rules run them) (0)none Would add (today's rules do not run them) (0)none AMD mirrors: would skip (0)none AMD mirrors: would add (0)none CI results (2026-09-30 18:52 UTC)0 passed, 0 failed, 0 pending. No failures to judge. 4 changed files · base |
Overview
(DISCLAIMER: I USED THE NEW SKILL TO HAVE GPT-6 AUTHOR THE PR DESCRIPTION BELOW, COMMIT THE CHANGES AND OPEN THIS PR. TREAT IT AS A REVIEWABLE EXAMPLE ARTIFACT. ALL CODE CHANGES WERE HAND WRITTEN WITH CARE BY ME.)
Add a
pr-checklistskill for coding agents preparing, reviewing, or revisingvLLM pull requests. Integrate it with the agent contribution instructions and
the PR template.
Claims
complexity, tradeoffs, validation, test quality, code clarity, and PR evidence.
the repository, emphasizing meaningful coverage rather than test volume.
AGENTS.md.with checklist and AI-assistance disclosure fields.
The intended benefit is better-prepared contributions and less avoidable
maintainer follow-up. This PR does not claim a measured reduction in review
rounds or review time.
Validation
This change is documentation and agent instructions only. No serving-engine,
model, kernel, dependency, or runtime behavior is modified; runtime tests,
model evaluations, and performance benchmarks are not applicable.
Diff and structure checks
scraping scripts, plots, and intermediate drafts are excluded.
AGENTS.mdhas 159 lines and the skill has 153 lines, within the applicableinstruction-document budgets.
inlining instruction. This remains in the proposed diff.
Current-skill behavioral smoke test
A fresh agent context read the current skill and applied it to four
hypothetical scenarios without executing tests or changing files:
applicable and assessed workflow consistency and documentation.
configuration, correctness, and serving-performance evidence rather than
skipping the PR-contents section.
requested appropriate evidence, and identified a stale description.
between the expected zero acceptance and the skill's nonzero-acceptance
instruction. This remains an outstanding caveat below.
These are qualitative instruction-following checks, not automated regression
tests or evidence of real-world review-time savings.
Details
Motivation and research
The drafting process examined the latest 50 vLLM PRs reviewed by each of
benchislettandmgoin, ordered by the reviewer's latest submitted review.The samples contain 97 distinct PRs and 342 nonempty authored review/comment
messages. Recurring themes included compatibility across execution paths,
appropriate ownership and reuse, concise implementations, meaningful rather
than excessive tests, and claims supported by evidence.
A separate exploratory analysis measured additions and deletions under
vllm/and compared them with description-based complexity grades. Itreinforced that LOC is a coarse review aid, not a reliable standalone semantic
complexity classification. The skill's 50/250 bands are judgment-based
guidance, not validated predictive cutoffs.
The research artifacts are local preparation material and are not included
in this PR. The proposed skill was handwritten and iteratively refined by
the human author with AI-assisted research and review.
Duplicate-work checks
Open-PR searches were run on September 15, 2026:
The broad searches returned unrelated PRs mentioning review or checklist
usage. Targeted title searches found checklists for tool-calling models and
server hardening, and skills for CI and installation; none appeared to
duplicate this general contribution-review workflow. No existing open PR
was found for this branch. No associated issue was supplied, so issue-specific
duplicate checks were not applicable.
AI assistance
AI assistance was used for GitHub review-history collection, qualitative
categorization, exploratory LOC analysis, draft review, behavioral smoke
checks, and preparation of this description. The human submitter remains
responsible for reviewing and understanding every changed line.
Pull Request Checklist
/pr-checklistskill.