agent docs: support only what released checkpoints use - #14750
Open
yiyixuxu wants to merge 2 commits into
Open
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
stevhliu
approved these changes
Sep 10, 2026
stevhliu
left a comment
stevhliu
left a comment
Member
There was a problem hiding this comment.
thanks! should we also cite these new rules in the model-integration and self-review skills?
| 2. **Check the default model config.** Look at the default config values in the model's `__init__` (or any published config JSON). Identify code paths that are unreachable under those defaults — e.g. an `if self.config.use_foo:` branch where `use_foo` defaults to `False` and no published checkpoint sets it to `True`. | ||
| 3. **Flag unused parameters and methods.** Parameters declared in `forward` (or helper methods) but never passed by the pipeline, private methods never called, layers initialized but never used in `forward`. | ||
| 4. **Qualify findings.** The actual model config can differ from the defaults, so any dead code identified this way is *likely* dead — not certain. Frame findings accordingly: "Under the default config and the pipeline's call path, this code appears unreachable." The PR author may know of configs or use cases that exercise the path. | ||
| 2. **Check the released configs.** Search for (or ask the contributor for) the list of released checkpoints, and collect their configs. Remove all the `__init__` config options not used across those releases; the code paths only those options reach go with them (see "Support only what released checkpoints use" in models.md). |
Member
There was a problem hiding this comment.
Suggested change
| 2. **Check the released configs.** Search for (or ask the contributor for) the list of released checkpoints, and collect their configs. Remove all the `__init__` config options not used across those releases; the code paths only those options reach go with them (see "Support only what released checkpoints use" in models.md). | |
| 2. **Check the released configs.** Get the list of released checkpoints (search Hub/ask the contributor) and collect their configs. Flag `__init__` config options unused across those releases, and the code paths only those options reach, for removal (see "support only what released checkpoints use" in models.md). |
| 3. **Flag unused parameters and methods.** Parameters declared in `forward` (or helper methods) but never passed by the pipeline, private methods never called, layers initialized but never used in `forward`. | ||
| 4. **Qualify findings.** The actual model config can differ from the defaults, so any dead code identified this way is *likely* dead — not certain. Frame findings accordingly: "Under the default config and the pipeline's call path, this code appears unreachable." The PR author may know of configs or use cases that exercise the path. | ||
| 2. **Check the released configs.** Search for (or ask the contributor for) the list of released checkpoints, and collect their configs. Remove all the `__init__` config options not used across those releases; the code paths only those options reach go with them (see "Support only what released checkpoints use" in models.md). | ||
| 3. **Check the runtime arguments.** Same point for `forward` / `__call__` parameters: collect the ones the official examples and recipes never pass, and verify with the contributor that they are actually used — remove the ones that aren't, with the code paths only they reach. |
Member
There was a problem hiding this comment.
Suggested change
| 3. **Check the runtime arguments.** Same point for `forward` / `__call__` parameters: collect the ones the official examples and recipes never pass, and verify with the contributor that they are actually used — remove the ones that aren't, with the code paths only they reach. | |
| 3. **Check the runtime arguments.** Same for `forward` / `__call__` parameters the official examples and recipes never pass. Confirm with the contributor, then flag unused parameters and the paths only they reach for removal. |
| 4. **Qualify findings.** The actual model config can differ from the defaults, so any dead code identified this way is *likely* dead — not certain. Frame findings accordingly: "Under the default config and the pipeline's call path, this code appears unreachable." The PR author may know of configs or use cases that exercise the path. | ||
| 2. **Check the released configs.** Search for (or ask the contributor for) the list of released checkpoints, and collect their configs. Remove all the `__init__` config options not used across those releases; the code paths only those options reach go with them (see "Support only what released checkpoints use" in models.md). | ||
| 3. **Check the runtime arguments.** Same point for `forward` / `__call__` parameters: collect the ones the official examples and recipes never pass, and verify with the contributor that they are actually used — remove the ones that aren't, with the code paths only they reach. | ||
| 4. **Flag unused parameters, methods, and classes.** Parameters declared in `forward` (or helper methods) but never passed by the pipeline, private methods never called, layers initialized but never used in `forward` — and classes never instantiated: ablation-variant subclasses, intermediate base classes that exist only as inheritance rungs, and aliases giving one class two names. |
Member
There was a problem hiding this comment.
Suggested change
| 4. **Flag unused parameters, methods, and classes.** Parameters declared in `forward` (or helper methods) but never passed by the pipeline, private methods never called, layers initialized but never used in `forward` — and classes never instantiated: ablation-variant subclasses, intermediate base classes that exist only as inheritance rungs, and aliases giving one class two names. | |
| 4. **Flag unused parameters, methods, and classes.** Parameters declared in `forward` (or helper methods) but never passed by the pipeline, private methods never called, layers initialized but never used in `forward`, and classes never instantiated (ablation-variant subclasses, intermediate base classes that exist only as inheritance rungs, and dual-name aliases). |
| example is a `flatten()` followed by a `transpose()`. This sequence is known to produce non-contiguous layouts. So, prefer calling `contiguous()` on the output tensor to maintain performance. No newline at end of file | ||
| example is a `flatten()` followed by a `transpose()`. This sequence is known to produce non-contiguous layouts. So, prefer calling `contiguous()` on the output tensor to maintain performance. | ||
|
|
||
| 8. **Keeping dead keys or legacy names "because the checkpoint has them".** Diffusers checkpoints normally host a separate set of converted weights, and the conversion script owns the key remapping — so the original checkpoint format is not a constraint, unless explicitly discussed with the reviewer. |
Member
There was a problem hiding this comment.
Suggested change
| 8. **Keeping dead keys or legacy names "because the checkpoint has them".** Diffusers checkpoints normally host a separate set of converted weights, and the conversion script owns the key remapping — so the original checkpoint format is not a constraint, unless explicitly discussed with the reviewer. | |
| 8. **Keeping dead keys or legacy names "because the checkpoint has them".** Diffusers Hub checkpoints normally store converted weights. The conversion script (or `from_single_file` weight map) owns remapping. Don't leave unused key aliases or legacy branches in the model "for the original checkpoint" unless the reviewer explicitly agreed. Mapping belongs in conversion/single-file loaders, not as dead code in `forward`. |
| * Models use `ModelMixin` with `register_to_config` for config serialization. | ||
| * 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. | ||
| * **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). | ||
| * **Support only what released checkpoints use.** A new model only ships the configuration options, branches, and classes that published checkpoints use. Every config argument must be needed by a real config; if you plan to release more checkpoints and think a config option will be needed for a future one — still don't add it now: we will deal with it when that checkpoint is released, in the PR that adds it. The same goes for runtime arguments (`forward` / `__call__` parameters nothing passes). With both trimmed to what checkpoints actually use, every unreachable code path is dead code — remove it; it also makes the model much easier to review. |
Member
There was a problem hiding this comment.
Suggested change
| * **Support only what released checkpoints use.** A new model only ships the configuration options, branches, and classes that published checkpoints use. Every config argument must be needed by a real config; if you plan to release more checkpoints and think a config option will be needed for a future one — still don't add it now: we will deal with it when that checkpoint is released, in the PR that adds it. The same goes for runtime arguments (`forward` / `__call__` parameters nothing passes). With both trimmed to what checkpoints actually use, every unreachable code path is dead code — remove it; it also makes the model much easier to review. | |
| * **Support only what released checkpoints use.** Ship only the config options, branches, and classes that published checkpoints need. Every config argument must appear in a real released config. Don’t add options for a checkpoint you plan to release later. Add them in the PR that ships that checkpoint. Same bar for runtime arguments, if nothing in the official pipeline/examples/recipes passes a `forward` or `__call__` parameter, don’t add it. Trim those surfaces, then delete the unreachable paths. Smaller surface area is easier to review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
add a comment on keep only configurations for released checkpoint
I think it will make it easier to trim down unused code & speed up review process therefore!