Conversation
yiyixuxu
left a comment
There was a problem hiding this comment.
thanks for working on this!
great start
| @@ -0,0 +1,34 @@ | |||
| # PR Review Rules | |||
There was a problem hiding this comment.
there are a lot of overlaps from https://github.com/huggingface/diffusers/blob/main/.ai/skills/model-integration/SKILL.md
I think maybe we have a separate subdoc for models.md that contains the gotchas, common conventions, rules, structures etc and we can link it from everywhere: for now both AGENTS.md and the model-integration skills
what do you think? cc @stevhliu here too
these contents that are already in AGENTS.md does not need to be included here again, I think claude CI would have read the AGENTS.md. anyways no?
There was a problem hiding this comment.
yeah i agree here, and i think it'd be cleaner and easier to maintain to provide a path to content that is already in AGENTS.md
|
|
||
| on: | ||
| issue_comment: | ||
| types: [created] |
There was a problem hiding this comment.
can we also be able to @claude on the pr review comment? (not just issue comments) this gives it more context for stuff like we want it to add a rule based on this pattern eetc
| with: | ||
| anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} | ||
| claude_args: | | ||
| --append-system-prompt "Review this PR against the rules in .ai/review-rules.md. Focus on correctness, not style (ruff handles style). Only review changes under src/diffusers/." |
There was a problem hiding this comment.
can we add something like
only commit changes when explicitly asked using words/phrase blah blah - then maybe we can come up with a safe word for it lol
|
|
||
| ## Code style | ||
| - Inline logic — minimize small helper/utility functions. A reader should follow the full flow without jumping between functions. | ||
| - No defensive code or unused code paths — no fallback paths, safety checks, or config options "just in case". |
There was a problem hiding this comment.
I wonder if this statement might be too strong. For example, it seems like pipeline check_input methods break the "no safety checks" rule if we interpret it literally or strictly. Maybe it would be better to break this into separate items for "fallback paths", "safety checks", "config options", etc. with specific guidance for each?
There was a problem hiding this comment.
I'm not sure what the actual behavior of Claude will be, but it seems possible to me that it could interpret "no safety checks" to include check_input pipeline methods, which are generally standard. (Alternatively, maybe I'm misunderstanding what is referred to as a "safety check".)
So my proposal would be a separate item for "safety checks" with more concrete guidance. A rough example could be something like
- Safety checks - for pipelines, all inputs to `__call__` should be validated in the `check_inputs` method. Refer to <good example> as a reference. Assert statements should not be used to check expressions "just in case"; unsupported cases should be caught early and raise a concise exception. See also Code Style #X on fallback paths.
We could also do something similar for the other things discussed in the point above such as "fallback paths" (the below example is based on the similar bullet point in AGENTS.md):
- Fallback paths - expected inputs should be made clear in the docstring for each function. When unexpected inputs are supplied, a concise error should be raised rather than implementing complex fallback logic. The code should not try to guess the user's intent and silently correct user behavior.
| Rules for Claude to check during PR reviews. Focus on correctness — style is handled by ruff. | ||
|
|
||
| ## Code style | ||
| - Inline logic — minimize small helper/utility functions. A reader should follow the full flow without jumping between functions. |
There was a problem hiding this comment.
I think numbering the items in each section would be useful if we want to refer to them elsewhere (e.g. "See Code Style (1) about inlining logic").
|
Thanks for the comments, all. I will work on them and push the updates. |
What does this PR do?
Use Claude for PR reviews. Some choices / comments:
anthropics/claude-code-action) that auto-reviews PRs touchingsrc/diffusers/**and responds to@claudementions from folks with write access to the repo..ai/review-rules.mdfile distilled from the existingAGENTS.md, model-integration skill, and parity-testing pitfalls. It currently covers: covering code style, dependencies, models, pipelines, and copied code.@claudeadd toreview-rules.mdto never useeinops) — Claude commits the rule update to the PR branch, so rules accumulate over time with zero friction. This is inspired by https://x.com/bcherny/status/2007179842928947333.@claude.src/diffusers/**only.How to use?
src/diffusers/**.@claudeon any PR to ask Claude about the code, the diff, or the review rules.@claude add to .ai/review-rules.md to never use numpy operations in forward methods, always use torch equivalents. Claude will update the rules file and commit it to the PR branch.