Skip to content

Commit 141852f

Browse files
yiyixuxuclaudesayakpaul
authored
[.ai] add self-review skill (#13917)
* [.ai] add self-review skill, retire parity-testing skill, and tighten the agent guides - New `self-review` skill mirroring the `@claude` CI review (rubric from review-rules.md, call-path dead-code analysis), report-only, with the report flagging what to fix before submitting (blocking + dead code) vs what to leave for the actual review. - Remove the WIP `parity-testing` skill; preserve its pitfalls as `model-integration/pitfalls.md` (numerical-discrepancy reference). - model-integration: restructure around a grouped checklist, default-to-modular, an overall file-structure sketch (details deferred to the guides), a fresh-conversion `Model parity test` example (internal, not shipped), and a filled-in weight/checkpoint-conversion section. - Centralize the loading rule (from_pretrained / from_single_file, no custom loaders) in models.md; add per-folder File structure sections to models.md / pipelines.md; default-to-modular note in pipelines.md. - AGENTS.md: dedicated 'Self-review before a PR' and 'Reference guides' sections. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [.ai] simplify pitfalls #6 and drop the model-storage / injection-test entries Trim pitfall #6 to the essential point (small dtype diffs compound into a large final difference), remove the `/tmp` model-storage and incomplete-injection-test pitfalls, and renumber 1-16 with cross-references updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [.ai] drop parity-harness-specific pitfalls With the parity-testing skill gone, remove the stale-test-fixtures pitfall (saved tensors / cross-pipeline fixtures no longer apply) and de-jargon the noise-dtype detection note. Keeps the pitfalls list generic to numerical discrepancy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [.ai] trim pitfalls to a concise possible-causes reference Drop the variable-shadowing and decoder-config pitfalls and the noise-dtype 'Detection' aside, tighten the remaining entries, renumber 1-12 (cross-refs updated), and reframe the intro as a non-checklist reference list of possible causes to consult only when outputs don't match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Apply suggestion from @yiyixuxu * Apply suggestion from @yiyixuxu * [docs] update contributing guide for the self-review skill Replace the retired parity-testing skill with self-review in the skills list, and add a 'Self-review before opening' step to the AI-assisted contributions section: run the self-review skill / review-rules, fix blocking issues + dead code, and treat the @claude CI review as a non-authoritative helper (note any intentional skips in the PR for the reviewer). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Apply suggestions from code review Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> * [.ai] fix dangling pitfalls ref and broaden self-review scope - Drop the broken 'pitfalls.md #10' reference in the conversion step (the /tmp model-storage pitfall was removed); save to a local path instead. - Self-review now reviews the whole diff, not just src/diffusers/ and .ai/ — a contributor should review their own tests/docs/scripts too (the CI's scoping is a safety measure for untrusted PRs). Reword to 'same rubric as the CI'. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
1 parent a50ade4 commit 141852f

11 files changed

Lines changed: 213 additions & 465 deletions

File tree

.ai/AGENTS.md

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,36 +8,31 @@ Strive to write code as simple and explicit as possible.
88
- No defensive code, unused code paths, or legacy stubs — do not add fallback paths, safety checks, or configuration options "just in case"; do not carry unused method parameters "for API consistency", backwards-compatibility aliases for names that never shipped, or deprecation shims for code that was never released. When porting from a research repo, delete training-time code paths, experimental flags, and ablation branches entirely — only keep the inference path you are actually integrating.
99
- Do not guess user intent and silently correct behavior. Make the expected inputs clear in the docstring, and raise a concise error for unsupported cases rather than adding complex fallback logic.
1010

11-
Before opening the PR, self-review against [review-rules.md](review-rules.md), which collects the most common mistakes we catch in review.
12-
1311
---
1412

1513
## Code formatting
1614

17-
- `make style` and `make fix-copies` should be run as the final step before opening a PR
15+
- `make style` and `make fix-copies` should be run before opening a PR
1816

1917
### Copied Code
2018

2119
- Many classes are kept in sync with a source via a `# Copied from ...` header comment
2220
- Do not edit a `# Copied from` block directly — run `make fix-copies` to propagate changes from the source
2321
- Remove the header to intentionally break the link
2422

25-
### Models
26-
27-
- See [models.md](models.md) for model conventions, attention pattern, implementation rules, dependencies, and gotchas.
28-
- See the [model-integration](./skills/model-integration/SKILL.md) skill for the full integration workflow, file structure, test setup, and other details.
29-
30-
### Pipelines & Schedulers
31-
32-
- See [pipelines.md](pipelines.md) for pipeline conventions, patterns, and gotchas.
23+
## Reference guides
3324

34-
### Modular Pipelines
35-
36-
- See [modular.md](modular.md) for modular pipeline conventions, patterns, and gotchas.
25+
- **Models** — see [models.md](models.md) for model conventions, attention pattern, implementation rules, dependencies, and gotchas. For adding or converting a model, use the [model-integration](./skills/model-integration/SKILL.md) skill.
26+
- **Pipelines** — see [pipelines.md](pipelines.md) for pipeline conventions, patterns, and gotchas.
27+
- **Modular pipelines** — see [modular.md](modular.md) for modular pipeline conventions, patterns, and gotchas.
3728

3829
## Skills
3930

4031
Task-specific guides live in `.ai/skills/` and are loaded on demand by AI agents. Available skills include:
4132

4233
- [model-integration](./skills/model-integration/SKILL.md) (adding/converting pipelines)
43-
- [parity-testing](./skills/parity-testing/SKILL.md) (debugging numerical parity).
34+
- [self-review](./skills/self-review/SKILL.md) (pre-PR self-review against the project rules)
35+
36+
## Self-review before a PR
37+
38+
Before opening a PR, run self-review against [review-rules.md](review-rules.md). The [self-review skill](skills/self-review/SKILL.md) runs this as the same pass the `@claude` CI reviewer uses.

.ai/models.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ Linked from `AGENTS.md`, `skills/model-integration/SKILL.md`, and `review-rules.
1313

1414
* Models use `ModelMixin` with `register_to_config` for config serialization.
1515
* When adding a new transformer (or reviewing one), skim `src/diffusers/models/transformers/transformer_flux.py`, `src/diffusers/models/transformers/transformer_flux2.py`, `src/diffusers/models/transformers/transformer_qwenimage.py`, and `src/diffusers/models/transformers/transformer_wan.py` first to establish the pattern. Most conventions (mixin set, file structure, naming, gradient-checkpointing implementation, `_no_split_modules` settings, etc.) are easiest to internalize by comparison rather than from a fixed list.
16+
* **Loading goes through `from_pretrained` / `from_single_file`.** Weights and configs load through the standard paths — never fetched or imported out-of-band at runtime. Don't override or add a custom `from_pretrained`, and don't load weights manually (`load_file(...)`, `hf_hub_download(...)`, or `sys.path.insert(...)` to import a reference repo). For an original-format single checkpoint, add `from_single_file` support (mixin + weight-mapping).
1617

1718
## Attention pattern
1819

.ai/pipelines.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,22 @@
33
Shared reference for pipeline-related conventions, patterns, and gotchas.
44
Linked from `AGENTS.md`, `skills/model-integration/SKILL.md`, and `review-rules.md`.
55

6+
> **Prefer modular for new pipelines.** [Modular Diffusers](modular.md) is the preferred way to add a new pipeline; the standard `DiffusionPipeline` covered below is still supported but is no longer the default. We prefer modular especially for models that don't fit a fixed task-based structure (e.g. modality baked into the checkpoint) or that are actively evolving. The conventions below apply when you do build or review a standard pipeline.
7+
68
## Common pipeline conventions
79

810
When adding a new pipeline (or reviewing one), skim `pipeline_flux.py`, `pipeline_flux2.py`, `pipeline_qwenimage.py`, `pipeline_wan.py` first to establish the pattern. Most conventions (class structure, mixin set, `__call__` shape — input validation → encode prompt → timesteps → latent prep → denoise loop → decode — `encode_prompt` / `prepare_latents` shape, `output_type` / `generator` / `progress_bar` plumbing, `@torch.no_grad()` on `__call__`, LoRA mixin, `from_single_file` support, etc.) are easiest to internalize by comparison rather than from a fixed list.
911

12+
## File structure
13+
14+
```
15+
src/diffusers/pipelines/<model>/
16+
__init__.py # Lazy imports
17+
pipeline_<model>.py # Main pipeline (with __call__)
18+
pipeline_<model>_<variant>.py # Variant pipelines (e.g. img2img, inpaint) — one file/class each
19+
pipeline_output.py # Output dataclass
20+
```
21+
1022
## Gotchas
1123

1224
1. **Config-derived static values: prefer `__init__` attributes.** Values that come from a sub-component's config (e.g. `vae_scale_factor`) belong as `self.foo = ...` in `__init__` — not `@property`, not module-level constants. Note the `getattr(...)` fallback — sub-components may not be loaded when the pipeline is constructed (e.g. via `from_pretrained` on a partial config), so don't assume `self.vae` / `self.transformer` exists.

.ai/review-rules.md

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,7 @@ Before reviewing, read and apply the guidelines in:
77
- [models.md](models.md) — model conventions, attention pattern, implementation rules, dependencies, gotchas
88
- [pipelines.md](pipelines.md) — pipeline conventions, coding style, gotchas
99
- [modular.md](modular.md) — modular pipeline conventions, patterns, common mistakes
10-
- [skills/parity-testing/SKILL.md](skills/parity-testing/SKILL.md) — testing rules, comparison utilities
11-
- [skills/parity-testing/pitfalls.md](skills/parity-testing/pitfalls.md) — known pitfalls (dtype mismatches, config assumptions, etc.)
10+
- [skills/model-integration/pitfalls.md](skills/model-integration/pitfalls.md) — known pitfalls causing numerical discrepancies between the reference implementation and the diffusers port (dtype mismatches, config assumptions, etc.)
1211

1312
## Common mistakes
1413

0 commit comments

Comments
 (0)