[.ai] add self-review skill by yiyixuxu · Pull Request #13917 · huggingface/diffusers

yiyixuxu · 2026-06-11T00:38:43Z

No description provided.

… 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>

…t 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>

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>

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>

yiyixuxu · 2026-06-11T01:21:51Z

@@ -1,172 +0,0 @@
---
-name: testing-parity


i removed this skill for now since it is a WIP

yiyixuxu · 2026-06-11T01:28:48Z

@@ -0,0 +1,48 @@
+---
+name: self-review


added a self-review skill here
mostly doing the samething our claude CI do, but for self-review I think they should focus on blocking issues & clean up dead code
we may ask contributor to provide the self-review summary in PR submission as well

@claude

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>

HuggingFaceDocBuilderDev · 2026-06-11T04:14:57Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

sayakpaul · 2026-06-11T04:16:54Z

+- [ ] **Scheduler** — reuse an existing scheduler, or add a custom one
+- [ ] **Pipeline**
+ - [ ] Implement the pipeline — see [modular.md](../../modular.md) for modular pipeline, or [pipelines.md](../../pipelines.md) for standard pipeline
+ - [ ] Add a LoRA mixin if applicable


Maybe better if this is skipped in the initial implementation. Easier to review and test.

Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>

sergereview

🤗 Serge says:

This PR adds a self-review skill, consolidates parity-testing content into the model-integration skill, and restructures several .ai/ guide files. The changes are mostly documentation reorganization. Two issues found:

Correctness

Broken cross-reference in SKILL.md: Line 77 references pitfalls.md #10 for the "never use /tmp/" rule, but in the new pitfalls.md, item #10 is "RoPE position dtype" — the storage-location pitfall was dropped during the consolidation. Either re-add the storage-location pitfall to the new pitfalls.md (and renumber), or inline the advice directly in the SKILL.md sentence and drop the #10 reference.

Minor

Scope claim mismatch in self-review skill: The skill header (lines 5-6) says it "Runs the same review the @claude CI runs", but line 17 restricts scope to src/diffusers/ and .ai/ only. The CI reviewer reviews the full diff (tests, docs, scripts, etc.). Either widen the scope or soften the claim (e.g. "Runs the same rubric" rather than "the same review").

model: claude-opus-4-6 · 17 LLM turns · 21 tool calls · 145.8s · 595532 in / 5054 out tokens

sergereview · 2026-06-11T17:42:52Z

-See [../../models.md](../../models.md) for the attention pattern, implementation rules, common conventions, dependencies, and gotchas. These apply to all model work.
+1. Map the original state-dict keys to the diffusers module names (renames + any tensor surgery — see patterns below).
+2. Instantiate the diffusers model from its config and load the converted state dict.
+3. `save_pretrained(...)` to a **persistent** path (never `/tmp/` — see [pitfalls.md](pitfalls.md) #10), then load it back with `from_pretrained` to confirm it round-trips.


Bug — broken #10 reference. The parenthetical says see [pitfalls.md](pitfalls.md) #10, but in the new pitfalls.md item #10 is "RoPE position dtype", not the storage-location rule. The original pitfall #10 ("NEVER store converted models in /tmp/") was dropped during the consolidation from parity-testing/pitfalls.md.
Suggested fix — inline the advice and drop the dangling anchor:
Suggested change
3.`save_pretrained(...)` to a **persistent** path (never `/tmp/` — see [pitfalls.md](pitfalls.md)#10), then load it back with `from_pretrained` to confirm it round-trips.
3.`save_pretrained(...)` to a **persistent** path (never `/tmp/` — temporary directories get wiped on restart), then load it back with `from_pretrained` to confirm it round-trips.

sergereview · 2026-06-11T17:42:53Z

+The same pass the `@claude` CI reviewer runs, so you can catch issues before a
+maintainer does. You're already on the branch with the conventions loaded, so:
+get the diff → review it against the rubric → report. **Review only changes under
+`src/diffusers/` and `.ai/`** (skip everything else, like the CI does).


Scope mismatch with the claim on lines 5-6 ("Runs the same review the @claude CI runs"). The CI reviewer reviews the full diff — including tests/, docs/, and scripts/ — not just src/diffusers/ and .ai/. Either widen the scope here or soften the header claim to something like "Runs the same rubric" rather than "the same review".

- 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>

dg845

Thanks!

@yiyixuxu

* [.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>

github-actionsBot added the size/L PR with diff > 200 LOC label Jun 11, 2026

yiyixuxuand others added 3 commits June 11, 2026 00:47

yiyixuxu commented Jun 11, 2026
View reviewed changes

Comment thread.ai/skills/model-integration/pitfalls.md Outdated

Apply suggestion from@yiyixuxu
c2b8bd2

yiyixuxu commented Jun 11, 2026
View reviewed changes

Comment thread.ai/skills/model-integration/SKILL.md Outdated

Apply suggestion from@yiyixuxu
ae6513b

yiyixuxu commented Jun 11, 2026
View reviewed changes

yiyixuxu requested review from dg845 and sayakpaul June 11, 2026 01:29

github-actionsBot added the documentation Improvements or additions to documentation label Jun 11, 2026

sayakpaul approved these changes Jun 11, 2026
View reviewed changes

Apply suggestions from code review
1f93b63
Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>

sergereviewBot reviewed Jun 11, 2026
View reviewed changes

dg845 approved these changes Jun 12, 2026
View reviewed changes

yiyixuxu merged commit 141852f into mainJun 12, 2026
7 checks passed

yiyixuxu deleted the self-review-skill branch June 12, 2026 17:01

Navigation Menu

Uh oh!

[.ai] add self-review skill - #13917

[.ai] add self-review skill#13917
yiyixuxu merged 9 commits into
mainfrom
self-review-skill

yiyixuxu commented Jun 11, 2026

Uh oh!

yiyixuxuJun 11, 2026

Uh oh!

Uh oh!

Uh oh!

yiyixuxuJun 11, 2026

Uh oh!

HuggingFaceDocBuilderDev commented Jun 11, 2026

Uh oh!

Uh oh!

sayakpaulJun 11, 2026

Uh oh!

Uh oh!

sergereviewBot left a comment

Uh oh!

sergereviewBotJun 11, 2026

Uh oh!

sergereviewBotJun 11, 2026

Uh oh!

dg845 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	3.`save_pretrained(...)` to a persistent path (never `/tmp/` — see [pitfalls.md](pitfalls.md)#10), then load it back with `from_pretrained` to confirm it round-trips.
	3.`save_pretrained(...)` to a persistent path (never `/tmp/` — temporary directories get wiped on restart), then load it back with `from_pretrained` to confirm it round-trips.

Uh oh!

Conversation

yiyixuxu commented Jun 11, 2026

Uh oh!

yiyixuxuJun 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yiyixuxuJun 11, 2026

Choose a reason for hiding this comment

Uh oh!

HuggingFaceDocBuilderDev commented Jun 11, 2026

Uh oh!

Uh oh!

sayakpaulJun 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sergereviewBot left a comment

Choose a reason for hiding this comment

Correctness

Minor

Uh oh!

sergereviewBotJun 11, 2026

Choose a reason for hiding this comment

Uh oh!

sergereviewBotJun 11, 2026

Choose a reason for hiding this comment

Uh oh!

dg845 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants