Conversation
yiyixuxu left a comment
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
.ai/review-rules.md Outdated
| | ||
| ## 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. There was a problem hiding this comment.
I have now explicitly asked it to review the rules we already have under skills and apply them during reviewing. I guess that should work?
.ai/review-rules.md Outdated
| 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. |
| @yiyixuxu @dg845 in the latest changes, I have made it so:
LMK if you have any thoughts. |
| Review-specific rules for Claude. Focus on correctness — style is handled by ruff. | ||
| | ||
| Before reviewing, read and apply the guidelines in: | ||
| - [AGENTS.md](AGENTS.md) — coding style, dependencies, copied code, model conventions |
There was a problem hiding this comment.
Should we refactor model information into a models.md file following #13297 (comment) in this PR?
There was a problem hiding this comment.
yeah would be better but ok to do it in a seperate PR
cc @stevhliu in case you're interested
| cc @DN6 in case if you want to review it |
4 participants
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.