diff --git a/.ai/references/models.md b/.ai/references/models.md index 559b445ba111..e6ecc2ed5066 100644 --- a/.ai/references/models.md +++ b/.ai/references/models.md @@ -14,6 +14,7 @@ Shared reference for model-related conventions, patterns, and gotchas. * 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. ## Single-file model layout @@ -226,4 +227,6 @@ Boolean gate. If `False` (default), calling that method raises `ValueError`. All 7. **Tensor contiguity.** - Non-contiguous tensors can degrade performance. Therefore, try to maintain contiguity of the tensors whenever possible. A non-contiguous tensor is usually produced because of the operations. A common -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. diff --git a/.ai/references/review-rules.md b/.ai/references/review-rules.md index 239f63aa5dad..1eddc5fe4e45 100644 --- a/.ai/references/review-rules.md +++ b/.ai/references/review-rules.md @@ -28,6 +28,6 @@ A PR can leave existing docs stale or surface a pattern worth recording. Scan th When reviewing a PR that adds a new model, trace how the model is actually called from the pipeline to identify likely dead code. Include the results as a **suggestions / additional info** section in your review (not as blocking comments — the findings are advisory). 1. **Trace the call path.** Read the pipeline's `__call__` and follow every call into the model — which arguments are passed, which branches are taken, which helper methods are invoked. -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). +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.