From 49a13e63592cd44697d15f07f387fbc5080785db Mon Sep 17 00:00:00 2001 From: SolitaryThinker Date: Sun, 26 Apr 2026 17:14:49 -0700 Subject: [PATCH] [skill] Add `add-model` and `review-add-model-pr` skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two skills split out of the will/stable-audio PR (#1260) so the model work and the agent infra evolve on independent review cycles. `add-model/SKILL.md` (~1600 lines) — the canonical procedure for porting a new diffusion model into FastVideo, distilled from the Wan, LTX-2, Hunyuan, Cosmos, Stable Audio, and MagiHuman ports. Covers the 17-row Files-table, 16-step procedure, FastVideo layer/attention selection rules, parity-test pattern, weight conversion recipe, and the I2V variant pattern. `add-model/REVIEW.md` (~1900 lines) — 36 documented failure modes discovered while porting / re-reviewing those families. Each item is a self-contained "what we did wrong, what to do instead" entry with a suggested action on the skill text. Items 1-22 from the Wan / MagiHuman porting passes; items 23-36 from the Stable Audio port (audio as first-class workload, hard ban on `from diffusers import` for model classes at runtime, user-story example docstrings, single-class kwargs-driven variants, examples accept paths not user-written decode glue, etc.). `add-model/add_model_split.md` — plan for breaking the monolithic 1600-line SKILL.md into ~12 satellite docs (deferred until after the next porting pass validates which sections are most-referenced). `review-add-model-pr/SKILL.md` (~480 lines, NEW) — reviewer-facing dual of `add-model`. Walks a PR reviewer through the canonical surface (per-component checklist for DiT / VAE / encoder / PipelineConfig / pipeline class / stages / presets+registry / tests / example), indexes the 36 REVIEW failure modes by where they show up in a diff (the "Pitfall map"), and produces a structured verdict (block / nit / follow-up) with a copy-pasteable comment skeleton. Key design choice: the reviewer's job is NOT to redesign the port, just to verify the porter followed the canonical procedure honestly and exercised the parity gate — REVIEW item 16 ("skip-on-missing parity is a silent no-op trap") gets disproportionate weight because it's the single most expensive failure mode in past ports. Both skills registered in `.agents/skills/index.jsonl` as `status: draft, trust: low` per existing convention. --- .agents/skills/add-model/REVIEW.md | 1667 +++++++++++++++++++ .agents/skills/add-model/SKILL.md | 1568 +++++++++++++++++ .agents/skills/add-model/add_model_split.md | 230 +++ .agents/skills/index.jsonl | 1 + .agents/skills/review-add-model-pr/SKILL.md | 482 ++++++ 5 files changed, 3948 insertions(+) create mode 100644 .agents/skills/add-model/REVIEW.md create mode 100644 .agents/skills/add-model/SKILL.md create mode 100644 .agents/skills/add-model/add_model_split.md create mode 100644 .agents/skills/review-add-model-pr/SKILL.md diff --git a/.agents/skills/add-model/REVIEW.md b/.agents/skills/add-model/REVIEW.md new file mode 100644 index 000000000..387358810 --- /dev/null +++ b/.agents/skills/add-model/REVIEW.md @@ -0,0 +1,1667 @@ +# add-model skill — open items for review + +Generated during the Wan2.1 / Wan2.2 verification pass on 2026-04-24. + +Each item below is something the skill currently says or implies that I +could not fully resolve from the Wan code alone. Please triage. The +skill has been updated with what was verifiable; these are the +judgment calls I deferred. + +--- + +## 1. No Wan parity or smoke tests exist in the repo + +**Where in the skill:** Step 12 (smoke-test), step 13(a) (mandatory +pipeline parity test), step 6 (per-component parity tests). + +**What I found:** `tests/local_tests/` has parity tests for LTX-2, +GameCraft, Gen3C, Kandinsky5 — but **none for Wan**. No +`test_wan_pipeline_smoke.py`, no `test_wan_pipeline_parity.py`, no +`tests/local_tests/transformers/test_wan*.py`, +`tests/local_tests/vaes/test_wan*.py`, or +`tests/local_tests/encoders/test_wan*.py`. + +**Why this matters:** The skill calls Wan out as the canonical +reference family, but the "every ported family ships pipeline + +component parity tests" convention the skill enforces is not actually +observed for Wan in the current tree. + +**Options:** +- (a) Keep the skill strict for *new* ports — Wan predates the + convention, call it out explicitly as grandfathered. +- (b) Relax the skill — make parity tests strongly recommended but not + a hard gate. +- (c) Add Wan parity tests retroactively (separate work item, not + covered by the skill). + +**My guess:** (a) is closest to your intent given how firmly the skill +currently phrases step 13(a) as a handoff gate — but flagging it so +you decide. + +--- + +## 2. `wan_to_diffusers.py` is a legacy conversion-script shape + +**Where in the skill:** "Weight conversion" section — "Conversion +script conventions" and the reference-scripts table. + +**What I found:** `scripts/checkpoint_conversion/wan_to_diffusers.py` +has **no argparse CLI**. It's a module of regex mapping dicts +(`_param_names_mapping`, +`_self_forcing_to_diffusers_param_names_mapping`) imported by other +code, not a runnable script. The modern pattern +(`convert_ltx2_weights.py`) is clearly different: argparse CLI, +`PARAM_NAME_MAP` + `COMPONENT_PREFIXES`, synthesized `model_index.json`. + +**What I did in the skill:** Marked `wan_to_diffusers.py` explicitly +as "legacy pattern — do not copy the module shape" and continued to +point at `convert_ltx2_weights.py` as the template. + +**Open question:** Is `wan_to_diffusers.py` still imported somewhere, +or is it actually dead? If dead, consider deleting it. If live, +document where it's invoked (e.g. which caller imports those dicts). + +--- + +## 3. `ConditioningStage` — optional or always-present? + +**Where in the skill:** "Standard stages — subclass targets" and +(previously) the canonical-order diagram in "FastVideo's single +architecture". + +**What I found:** Both `WanPipeline` (T2V) and +`WanImageToVideoPipeline` (I2V) always add `ConditioningStage` +unconditionally. No Wan variant skips it. Haven't audited every +other family. + +**What I did in the skill:** Changed the canonical-order diagram to +call it a default rather than optional. + +**Open question:** Is `ConditioningStage` skippable for any existing +family (LTX-2, SD3.5, Cosmos2.5, etc.), or is it universal? If +universal, the skill could go further and remove "optional" framing +everywhere. If sometimes-skippable, we should list which families +skip it. + +--- + +## 4. Image-encoder selection for I2V + +**Where in the skill:** New "Adding an I2V variant" section, "Config +delta" subsection. + +**What I found:** Wan I2V 480P/720P use stock `CLIPVisionConfig`. Wan +V2V swaps to `WAN2_1ControlCLIPVisionConfig` (a Wan-specific variant). +I didn't verify what other I2V families in the repo use (Cosmos25, +MatrixGame) — some likely use non-CLIP image encoders. + +**What I did in the skill:** Defaulted the I2V config example to +`CLIPVisionConfig` and noted that non-standard variants swap the +`image_encoder_config` default (with WANV2V as example). + +**Open question:** Should the skill provide a selection rule +("prefer CLIP unless the reference repo uses X")? I don't have enough +signal across families to write one confidently. + +--- + +## 5. Wan2.2 MoE / dual-guidance fields not covered in the skill + +**Where in the skill:** No dedicated section. + +**What I found:** Wan2.2 A14B configs and presets introduce: +- `boundary_ratio` (MoE routing parameter). +- `dmd_denoising_steps` list (for DMD variants). +- `guidance_scale_2` (second guidance scale for dual-CFG). +- `transformer_2` module — referenced in both T2V and I2V pipelines + via `self.get_module("transformer_2", None)` (T2V) or + `self.get_module("transformer_2")` (I2V, no default). + +**What I did in the skill:** Mentioned `transformer_2` in the pipeline +class example comments but did not add a dedicated MoE section. + +**Open question:** How broad is MoE / dual-guidance adoption going +forward? If mostly a Wan2.2 peculiarity, leaving it out keeps the +skill focused. If a generalizable pattern, it deserves a section on +par with I2V. My guess is Wan-specific for now, but flag. + +Sub-question: The I2V Wan pipeline calls +`self.get_module("transformer_2")` without a default, while T2V uses +`self.get_module("transformer_2", None)`. That inconsistency looks +like a real bug in `wan_i2v_pipeline.py:66` that would hard-error for +non-MoE I2V families. Not in scope for the skill, but worth noting +separately. + +--- + +## 6. `WanT2VCrossAttention` / `WanI2VCrossAttention` pattern name + +**Where in the skill:** New "Adding an I2V variant" section, "DiT +branching" subsection. + +**What I found:** Wan names its two cross-attention variants +`Wan{T2V,I2V}CrossAttention` and switches based on +`added_kv_proj_dim is None`. I used `{T2V,I2V}CrossAttention` +as a placeholder in the skill. + +**Open question:** Should the skill prescribe this naming convention +for new families, or leave it to author preference? Other families +(Cosmos, Hunyuan) may use different naming. + +--- + +## 7. Missing CI coverage for the pipeline-parity gate + +**Where in the skill:** Step 13(a) insists on a green pipeline parity +test before handoff. + +**What I found:** No CI job I can identify that runs +`tests/local_tests/pipelines/test_*_pipeline_parity.py`. These tests +are local-only (hence `local_tests/`). + +**Implication:** The gate the skill enforces is author-run, not +CI-enforced. That's fine, but worth being explicit — once the author +hands off, only the SSIM tests provide ongoing regression coverage. + +**Open question:** Should the skill say "local-only gate" explicitly? +Or is that obvious from the directory name? I left it implicit. + +--- + +## 8. Files-table coverage of variant pipelines + +**Where in the skill:** "Files you will create or touch" table +(rows 8, 13). + +**What I found:** A full Wan port touches multiple sibling pipeline +files (`wan_pipeline.py`, `wan_i2v_pipeline.py`, `wan_v2v_pipeline.py`, +`wan_dmd_pipeline.py`, `wan_causal_pipeline.py`, +`wan_causal_dmd_pipeline.py`, `wan_i2v_dmd_pipeline.py`) — seven in +total for Wan. The skill's table implies one pipeline file per family. + +**What I did in the skill:** Added a note on row 8 that variants get +sibling files. Did not enumerate every possible variant kind. + +**Open question:** Should the skill enumerate the common variant +buckets (T2V, I2V, V2V, DMD, Causal, Causal-DMD, SR/Refine) up front, +or keep that implicit? If enumerated, it makes the skill longer but +much more concrete for a porter staring at Wan. + +--- + +## 9. Conversion-script publish step assumes internet + HF write + +**Where in the skill:** "Weight conversion" recipe step 10 +("Publish to HF via create_hf_repo.py"). + +**What I found:** Works, but requires the HF token collected in +step 1 to have both read (to `snapshot_download` the base diffusers +repo) and write (to `push_to_hub`) scope. Step 1 currently asks only +about write scope. + +**What I did in the skill:** Skill step 1 already asks for +write-enabled; HF write tokens typically include read. Not changed. + +**Open question:** Is there a scenario in the Anthropic-style +organization where write tokens are scoped narrowly enough to block +`snapshot_download`? If so, step 1 should mention "read + write" to +be unambiguous. + +--- + +## 10. How strict is the "no raw nn.Linear" rule? + +**Where in the skill:** Common pitfalls #11. + +**What I found:** A handful of DiTs still have raw `nn.Linear` calls: +- `wanvideo.py:608` (`proj_out`). +- `causal_wanvideo.py:382` (`proj_out`). +- `cosmos.py:28, 50, 52, 90` (patch embed, modulation). +- `hunyuanvideo15.py:141-162, 574` (timestep/context embedders). +- `lingbotworld/model.py:42-43, 294` (cam layers, proj_out). + +**What I did in the skill:** Described these as "legacy, do not copy" +and told new code to default to `ReplicatedLinear`. + +**Open question:** Is there an explicit policy that new code must +use `ReplicatedLinear` everywhere, or is the occasional `nn.Linear` +(for a single final projection) accepted? If accepted in some cases, +the skill should say where. + +--- + +--- + +## 11. Packed-expert linears (MoE / multi-modality DiTs) + +**Where in the skill:** "Linear layers" section — "DiTs use ReplicatedLinear +everywhere" rule. + +**What I found (during the MagiHuman port, 2026-04-24):** MagiHuman's DiT +packs per-modality weight chunks along the output axis of every Linear in +its "mm-layers": `weight.shape = [out_features * num_experts, in_features]`, +with a runtime `ModalityDispatcher` that permutes tokens so each chunk sees +a contiguous slice before running `F.linear` per expert. `ReplicatedLinear` +does not model this layout — its quant-method dispatch path assumes a +single `[out, in]` weight. I had to implement a custom `PackedExpertLinear` +using raw `nn.Parameter` (documented deviation) to keep weight loading +byte-exact against the upstream checkpoint. + +The skill currently says (pitfall #11, pitfall #12) "always use +`ReplicatedLinear` in DiTs" with no named exceptions. This is genuinely +infeasible for MoE-style weight layouts where the output axis is +multi-expert-packed. + +**Options:** +- (a) Add a named exception to the linear rule: "packed-expert / + per-modality / MoE-along-output weight layouts are an accepted + deviation — use raw `nn.Parameter` + a small wrapper. Document the + deviation in the DiT module docstring." Applies to MagiHuman; may apply + to future MoE DiTs. +- (b) Extend `ReplicatedLinear` to support `num_experts > 1` directly + (bigger change; rewires the quant path). +- (c) Leave as-is and let each new porter rediscover the workaround. + +**My guess:** (a). The exception is rare but real; naming it saves the +next porter the same debugging loop. + +--- + +## 12. VAE reuse: prefer the `-Diffusers` suffix variant + +**Where in the skill:** "Weight conversion" — `_bundle_*_vae` helper +pattern and the "when to reuse a VAE" advice. + +**What I found (during the MagiHuman port):** I defaulted the +`_bundle_wan_vae` helper to `Wan-AI/Wan2.2-TI2V-5B` (the canonical repo +id) and it failed with `No vae/ subdir in Wan-AI/Wan2.2-TI2V-5B`. That +repo ships the official non-Diffusers layout: + + Wan2.2_VAE.pth # monolithic .pth, no subdir + diffusion_pytorch_model-0000{1,2,3}-of-00003.safetensors # DiT, not VAE + google/umt5-xxl/... # text encoder stuff + +The Diffusers-layout repo is the *separately-named* +`Wan-AI/Wan2.2-TI2V-5B-Diffusers`, which has the expected +`vae/config.json` + `vae/diffusion_pytorch_model.safetensors`. This +`-Diffusers` suffix convention is consistent across Wan (`Wan2.1-T2V-14B` +vs `Wan2.1-T2V-14B-Diffusers`, etc.) but the skill doesn't surface it. + +**Options:** +- (a) Add a one-liner to the "Weight conversion" recipe: + *"When bundling a VAE from another family, point at the `-Diffusers` + suffix variant (e.g. `Wan-AI/Wan2.2-TI2V-5B-Diffusers`), not the + canonical repo — the canonical repo often ships a monolithic `.pth` + at the root, not a `vae/` subdir."* +- (b) Add a lookup helper / `_find_vae_repo(family)` that tries + `{repo}-Diffusers` first. + +**My guess:** (a). One sentence, prevents a 10-min debugging loop. + +--- + +## 13. HF token needed at conversion time, not just publish time + +**Where in the skill:** Step 1 prerequisite — HF write token. + +**What I found (during the MagiHuman port):** Step 1 phrases the HF +token requirement as: *"write-scoped, used for `create_hf_repo.py +--push_to_hub` and SSIM reference uploads."* But T5-Gemma (MagiHuman's +text encoder, gated Google repo) requires the token for *read-only* +`snapshot_download` during conversion. Same for Gemma3 (LTX-2), +Llama-family encoders, and any future gated encoder. Without the token, +conversion step fails at `401 Unauthorized` on `text_encoder/` bundling +or at first-forward lazy-load. + +Write tokens usually include read, so this hasn't actually broken +anything — but the skill's framing is misleading. A porter who only +sees "write for publish" might think the token is needed only at the +very end. + +**Options:** +- (a) Reword step 1: *"read-scoped is strictly required at conversion + time (for gated text encoders). Write-scoped is required at publish + time. In practice, a single write token satisfies both."* +- (b) No change — trust that write tokens always include read. + +**My guess:** (a). Small rewording, much clearer to a first-time porter. + +--- + +## 14. Cross-modality flat-stream self-attention + +**Where in the skill:** "FastVideo layers and attention" section — +attention-layer selection table. + +**What I found (during the MagiHuman port):** MagiHuman's DiT performs +joint self-attention over a *flat concatenated stream* of video + audio +- text tokens in every layer, with a `ModalityDispatcher` permute/unpermute +around each linear. There is no clean "spatial sequence" to SP-shard the +way `DistributedAttention` expects — the attention needs to see all three +modalities at once. The skill's selection rule ("DiT full-sequence +self-attention → `DistributedAttention`") doesn't cleanly apply. + +For the first port I fell back to `F.scaled_dot_product_attention` +directly and left multi-GPU SP as a follow-up. But this raises the +question: is there a principled way to combine `DistributedAttention` +with a flat concat stream? + +**Options:** +- (a) Extend the attention table with a third row: + *"Cross-modality flat-stream self-attention (video + audio + text + concatenated in one sequence) — use raw SDPA for single-GPU; SP + integration is a known gap."* +- (b) Write an adapter that teaches `DistributedAttention` about + modality-aware sharding (bigger project, not skill scope). + +**My guess:** (a). Naming the gap is more useful than pretending the +existing table covers this case. + +--- + +## 15a. ArchConfig scope = the DiT's `transformer/config.json` only + +**Where in the skill:** Step 2 ("Study the reference implementation"), the +DiT config section, and the "Weight conversion" recipe. + +**What I found (during the MagiHuman port, after direct user +correction):** The FastVideo arch config class (e.g. `MagiHumanArchConfig`) +and the `transformer/config.json` emitted by the conversion script **must +match key-for-key**. FastVideo's `TransformerLoader.load` reads +`transformer/config.json`, pops `_class_name`/`_name_or_path`/`_diffusers_version`, +then calls `ArchConfig.update_model_arch(remaining_dict)` which hard-errors +on any key that isn't a declared arch-config field. + +I initially stuffed pipeline-level defaults (`num_inference_steps`, +`video_txt_guidance_scale`, `cfg_number`, `flow_shift`), data-proxy knobs +(`vae_stride`, `frame_receptive_field`, `coords_style`, `ref_audio_offset`, +`text_offset`), and eval knobs (`fps`, `t5_gemma_target_length`) onto both +the ArchConfig *and* the emitted `transformer/config.json`. Load failed +with `AttributeError: ArchConfig has no field 'tread_config'` (the nested +dict from the official source) and later would have failed on every +pipeline-level knob. + +**Principle (now consistent with Wan):** ArchConfig fields = exactly what +the `transformer/config.json` shipped in the Diffusers repo carries. Everything +else (sampling schedule, CFG scales, VAE stride, FPS, text-embed target +length, data-proxy knobs) lives on the `PipelineConfig` subclass +(`MagiHumanT2VConfig` here). + +**Where does the arch source-of-truth come from?** Two tiers: + +1. **Preferred:** the `transformer/config.json` inside the Diffusers-format + HF repo (e.g. `Wan-AI/Wan2.1-T2V-1.3B-Diffusers/transformer/config.json`). + Use its keys verbatim as the arch config fields. +2. **Fallback:** when the HF repo is not in Diffusers format and the + root `config.json` is empty / missing (as with `GAIR/daVinci-MagiHuman` + — `config.json` is literally `{}` and the per-subfolder files are just + `model.safetensors.index.json`), use the **official Python reference** + as source of truth. For MagiHuman that is + `inference/common/config.py`'s `ModelConfig` (not `DataProxyConfig`, not + `EvaluationConfig`). + + The conversion script then emits a synthesized `transformer/config.json` + mirroring only the `ModelConfig` fields; anything in `DataProxyConfig` / + `EvaluationConfig` lives on the pipeline config, never the arch. + +**Options:** +- (a) Add a paragraph to step 2 ("Study the reference"): *"Before writing + the arch config, locate the canonical per-subfolder `transformer/config.json` + — either the HF Diffusers repo or, when the HF repo is raw/empty, the + official Python `ModelConfig` equivalent. Arch config fields must + match this 1:1; pipeline-level knobs (steps, CFG, flow_shift, sampling + defaults) go on `PipelineConfig` instead."* +- (b) Add a hard pitfall: *"Pitfall: pipeline-level fields leaking onto + the arch config. Every field on the arch config must appear in the + emitted `transformer/config.json` and vice versa. If it's not a + transformer-architecture field (steps, CFG scales, VAE stride, fps, + data-proxy knobs), put it on the `PipelineConfig` instead."* + +**My guess:** both. Plus a short example cross-referencing Wan's +`transformer/config.json` (17 keys, all DiT-arch) next to +`WanVideoArchConfig` (same 17 keys + `boundary_ratio` / `exclude_lora_layers` +FastVideo additions). + +--- + +## 15. Scoping a port when upstream has multiple variants + +**Where in the skill:** No dedicated section. + +**What I found (during the MagiHuman port):** Upstream had four +checkpoints (`base`, `distill`, `540p_sr`, `1080p_sr`) plus an audio +VAE and a turbo VAE decoder. The user said *"start with base model +ONLY"* and I had to interpret what that meant — keep the DiT's audio +path (since joint denoise is the trained behavior) but skip audio +decoding? Skip SR entirely? Write a single pipeline config or one per +variant? No guidance from the skill. + +**Options:** +- (a) Add a "Scoping a port" paragraph to the "When to use" section: + *"If upstream ships multiple variants (distill, SR, I2V, audio), + agree with the user up front on which belong in the first PR. The + common narrow scope is **base T2V only** — one pipeline class, one + preset, no audio decode, no SR, no distill. Variants are added in + follow-up PRs using the I2V pattern (sibling pipeline file + config + subclass + second preset + second register_configs call)."* +- (b) Leave as-is; scoping is a per-port conversation. + +**My guess:** (a). One paragraph avoids a common source of scope +ambiguity at port-start. + +--- + +## 16. The "skip-on-missing" parity pattern is a silent no-op trap + +**Where in the skill:** "Parity test pattern" section — "Skip-on-missing, +never fail-on-missing" convention. Also step 6 (component parity) and +step 13(a) (pipeline parity). + +**What happened (during the MagiHuman port, user-corrected):** I produced +the full scaffold — model code, configs, pipeline, presets, registry, +example script — and then wrote parity test files that *looked* +rigorous but never actually loaded real weights: + +- `tests/local_tests/pipelines/test_magi_human_pipeline_smoke.py` — + only a preflight that builds the DiT on `meta` device and cross- + checks shapes against the HF index. No forward call. No numerics. +- `tests/local_tests/pipelines/test_magi_human_pipeline_parity.py` — + a skeleton that gated on an upstream module that's hard to construct + in-process, then called `pytest.skip("... is a follow-up milestone")` + unconditionally. Zero assertions ever executed. +- **No per-component parity tests** under `tests/local_tests/transformers/`, + `tests/local_tests/encoders/`, or `tests/local_tests/vaes/` at all. + Step 6's "each subagent produces a component parity test" was simply + skipped. + +From pytest's perspective, the smoke test PASSED and the parity skeleton +SKIPPED. I reported the port as "scaffold complete with parity tests" — +but in reality no numerical parity was ever verified anywhere. + +The user caught this with the observation: *"the fact that weights were +not converted at all makes me believe that these parity test are not +accurate right now"* — which turned out to be exactly correct. I had +never run the conversion, so the tests would have skipped anyway; when +I later ran the conversion and wrote **real** parity tests, the DiT +showed a measurable 1% mean drift that was completely invisible in the +skeleton's "all green" output. + +**Why this happened (root causes in the skill):** + +1. **Pytest-skip is visually indistinguishable from pytest-pass** in CI + output and summary lines. A test that skips every time is a liability, + not an asset — the skill says "skip-on-missing" but doesn't warn + that a skip-only test provides zero ongoing regression coverage. +2. **The skill doesn't prescribe a local verification checklist** tying + "conversion (step 4)" → "component parity (step 6)" → "pipeline + parity (step 13(a))" into a linear ordering that forces non-skip + passes at each stage before proceeding. Steps 4 and 6 read as + parallel; they are not. +3. **Step 6's subagent prompt template** mentions "Run: `pytest ...` — + the test must produce a non-skip pass", but the skill's main-agent + instructions around step 6 don't make the non-skip gate a blocker. + Easy to miss when the main agent isn't dispatching subagents (I did + the component work myself). +4. **Step 13(a) is the ONLY place the skill explicitly says "non-skip + pass required before user handoff"**. A conscientious porter who + only reads step 13 (because step 6 says "optional, subagents do this") + can and will ship without component parity. + +**Options:** + +- (a) **Add a blocking pre-handoff checklist** at the end of the skill: + > Before telling the user the port is ready, run all of these and + > confirm **non-skip passes** on each: + > 1. `pytest tests/local_tests//test__*parity*.py` + > for each non-reused component (DiT + any new VAE + any new + > encoder). Skips here mean you didn't convert weights first. + > 2. `pytest tests/local_tests/pipelines/test__pipeline_parity.py` + > (the step-13(a) gate). + > 3. `python examples/inference/basic/basic_.py` produces a + > non-corrupt mp4. + > If any of the above skips, STOP and fix — a skipping test is a + > missing test. + +- (b) **Reorder the steps** so conversion is a hard prerequisite for + step 6: move step 4 ("Convert weights") above the parity work and + note: "component parity tests in step 6 load from the output of + step 4; if step 4 hasn't been run, step 6 cannot be completed." + +- (c) **Strengthen the "Skip-on-missing" language** to add: *"But if + YOU, the porter, get a skip on your local machine, that means you + haven't done the conversion or clone. Fix that before proceeding."* + +- (d) **Add a grep-able "DONE" criterion** to step 6: the main agent + must verify `pytest tests/local_tests//test__parity.py + -v` output contains `PASSED`, not `SKIPPED` or `xfailed`, before + marking the step complete. + +**My guess:** all four. This is the single biggest gap the port +exposed. A skeleton test that silently skips is worse than no test, +because it creates false confidence. + +--- + +## 17. Upstream private-DSL dependencies (`magi_compiler`, etc.) + +**Where in the skill:** Nowhere currently. + +**What I found (during the MagiHuman parity round):** The upstream +`daVinci-MagiHuman/inference/model/dit/dit_module.py` hard-imports +three modules from `magi_compiler` (SandAI's internal graph compiler), +none of which are on PyPI. This makes `from inference.model.dit import +DiTModel` fail at module-load with `ModuleNotFoundError: No module named +'magi_compiler'` — so you cannot run the official DiT side-by-side with +your FastVideo port for parity testing without stubbing. + +This is not unique to MagiHuman. Other upstream research codebases are +likely to ship their own internal compilers / ops (TorchTitan, +`megatron_ops`, proprietary FlashAttention variants, custom op +registries). The skill currently treats "clone official repo + pip +install -r requirements.txt" as sufficient for parity; it's not when +the upstream has private deps. + +**What worked for MagiHuman (tests/local_tests/helpers/magi_human_upstream.py):** + +A small `install_stubs()` helper that before importing `inference.*`: +1. Registers a fake `magi_compiler` module in `sys.modules` with + identity decorators for `magi_compile` and `magi_register_custom_op`. +2. Monkey-patches `inference.infra.distributed.get_cp_world_size` to + return 1 (so Ulysses scatter/gather are no-ops). +3. Replaces `scatter_to_context_parallel_region` / + `gather_from_context_parallel_region` with identity functions. +4. Registers the `torch.ops.infra.*` custom ops that upstream calls + via `torch.ops.infra.flash_attn_func(q, k, v)` — routing them + through `F.scaled_dot_product_attention` so attention kernels + align between FastVideo and upstream (see item 19). + +**Options:** + +- (a) Add a "**Upstream private deps**" subsection to step 5 ("Clone + official repo") and/or the parity-test pattern: *"If the upstream + module imports fail with `ModuleNotFoundError` on non-PyPI packages + (private compilers, custom op registries), write a small stub helper + under `tests/local_tests/helpers/_upstream.py`. Typical stubs + needed: (1) identity decorators for compile/op-registration macros; + (2) `get_cp_world_size → 1` + identity scatter/gather; (3) + `torch.ops..*` registration preservation."* Point at the + MagiHuman helper as the template. + +- (b) Leave as-is; assume each porter figures it out per-family. + +**My guess:** (a). Stubs are a common enough reality in research +codebases that documenting the pattern saves every future porter half +a day. + +--- + +## 18. `torch.ops..` registration is a side-effect you can't stub away + +**Where in the skill:** Nowhere. + +**What I found (during the MagiHuman parity round):** Upstream's +`dit_module.py` uses `@magi_register_custom_op(name="infra::flash_attn_func", ...)` +as a decorator, then calls the op elsewhere as +`torch.ops.infra.flash_attn_func(q, k, v)`. The decorator's real +implementation registers the op with `torch.library`, which makes the +`torch.ops.infra.flash_attn_func` attribute resolvable. + +My first stub made `magi_register_custom_op` an identity decorator — +it returned the function unchanged but never registered it. Module +import succeeded; the call site later failed with `'_OpNamespace' 'infra' +object has no attribute 'flash_attn_func'`. That error is delayed from +import time to runtime, making it harder to debug. + +**Fix (in the helper):** the stub decorator must still call +`torch.library.Library("infra", "FRAGMENT").define(...)` and +`torch.library.impl(...)` so `torch.ops.infra.*` resolves — just with +a test-friendly backing implementation (SDPA in our case). + +**Options:** + +- (a) Mention this as a **pitfall** in the new "Upstream private + deps" subsection (see item 17): *"Identity-decorator stubs silently + lose custom-op registration side-effects. If the upstream decorator + registers with `torch.library`, your stub must too — otherwise call + sites using `torch.ops..(...)` will fail at runtime with + attribute errors that point at the wrong module."* + +**My guess:** (a). Short callout, prevents a 30-min head-scratch. + +--- + +## 19. GQA kernel alignment in cross-kernel parity tests + +**Where in the skill:** Partially in the "FastVideo layers and +attention" section's attention selection rules, but not specifically +for parity testing. + +**What I found (during the MagiHuman DiT parity):** Upstream calls +`flash_attn_func` directly with GQA-shaped inputs — `q: [B, L, 40, 128]`, +`k/v: [B, L, 8, 128]`. flash_attn handles GQA natively. When I routed +the stubbed `torch.ops.infra.flash_attn_func` through +`F.scaled_dot_product_attention`, SDPA errored with shape mismatch on +dim 1 because SDPA requires `num_heads_k == num_heads_q`. + +The fix is `k.repeat_interleave(num_heads_q // num_heads_kv, dim=2)` +before calling SDPA — but only one side (SDPA / parity stub) needs it. + +The broader point: **parity tests that align two attention kernels +(flash_attn vs SDPA vs VMoba vs Sage) must explicitly handle GQA +expansion on the non-native side.** The skill's attention section +mentions GQA in passing but doesn't name this pitfall. + +**Options:** + +- (a) Add a pitfall to the "Parity test pattern" section: + *"When your parity test routes upstream attention through a different + kernel than upstream uses (e.g. upstream=flash_attn, your stub=SDPA), + remember to expand KV heads manually on the SDPA side — + `k = k.repeat_interleave(num_heads_q // num_heads_kv, dim=head_axis)`. + flash_attn handles GQA natively; SDPA does not."* + +**My guess:** (a). Adds 3 lines to the parity pattern, saves the next +porter from a confusing shape error mid-test. + +--- + +## 20. VAE parity: normalization-convention mismatches across wrappers + +**Where in the skill:** Nowhere explicitly. + +**What I found (during the MagiHuman VAE parity):** The upstream +`Wan2_2_VAE.decode(z)` does: + + # Inside _video_vae.decode (inference/model/vae2_2/vae2_2_module.py:874) + z = z / scale[1] + scale[0] # where scale = [mean, 1/std] + # = z * std + mean # denormalizes BEFORE feeding the decoder + +— i.e. the upstream API takes latents in *normalized diffusion space* +and denormalizes internally. + +Diffusers' `AutoencoderKLWan.decode(z)` is the opposite: it expects +the caller to have **already applied** `z = z * std + mean`. In +FastVideo, this denormalization lives in the `DecodingStage`, not in +the VAE class. + +My first VAE parity test fed `z = randn(...)` to both and got 86% of +elements failing tolerance (max diff 1.38). After applying the +denormalization only on the Diffusers side — `z_denorm = z * std + mean` +then `fv_vae.decode(z_denorm)` — parity dropped to `diff_max=8e-4, +diff_mean=5e-5` (near fp32 noise). The VAEs agree; the wrappers just +place the normalization step in different layers of the call stack. + +This pattern is common when comparing **"research-code" wrappers that +bundle normalization inside `decode()`** vs. **"library-code" wrappers +that expect pre-normalized input** (the Diffusers convention). + +**Options:** + +- (a) Add a pitfall to the "Parity test pattern" section or to VAE + reuse advice: *"When the upstream VAE's `decode()` bundles the + `z → z*std + mean` denormalization but the Diffusers-format VAE + expects pre-denormalized input (the FastVideo/Diffusers convention + — the DecodingStage applies `std/mean` before calling `vae.decode()`), + a naive parity test that feeds the same `z` to both will diverge + drastically. Apply the denormalization on the Diffusers side + explicitly."* + +**My guess:** (a). One paragraph, gives the next porter the exact +fix. + +--- + +## 21. Realistic parity tolerances for bf16 through a deep DiT + +**Where in the skill:** "Parity test pattern" — the `atol=1e-4, rtol=1e-4` +default with "loosen deliberately, never silently". + +**What I found (during the MagiHuman DiT parity):** The 40-layer bf16 +DiT with upstream-flash_attn vs FastVideo-SDPA kernel crossing showed: + +- diff_max: 0.057 (on abs-mean 0.68 signal — ~8% tail drift) +- diff_mean: 0.005 (~0.75%) +- diff_median: 0.003 +- abs_mean match: 0.6833 vs 0.6825 (< 0.2%) + +At `atol=1e-4, rtol=1e-4` this is RED with 99% of elements failing — +but the structure is clearly correct (median < 5e-3, abs_mean matches, +text zero-padding exact). The drift is cross-kernel bf16 accumulation +through 40 layers, not a structural bug. + +`atol=1e-4` is the right default for **single-block, single-kernel, +shallow** parity tests (e.g. comparing one Wan DiT block in isolation). +For **full-DiT, multi-kernel, deep** parity the realistic tolerance is +~2 orders of magnitude larger, and element-wise assertion alone is the +wrong metric. What I ended up with that's actually informative: + +- `assert_close(text_rows, ref_text_rows, atol=1e-6, rtol=1e-6)` — + text rows are zero-padded on both sides; any drift here is a bug. +- `assert_close(fv_full, ref_full, atol=0.1, rtol=0.1)` — catches + structural bugs (sign flips, missing layers, bad permutations) + while tolerating realistic bf16 drift. +- `assert abs(ref_abs_mean - fv_abs_mean) / ref_abs_mean < 0.05` — + sanity check that mean magnitudes agree to 5% — catches gross + dropouts (e.g. a modality branch silently zeroed out). +- Printed per-modality breakdown (video / audio / text abs_mean, + diff_max, diff_mean) as a diagnostic for future iteration. + +**Options:** + +- (a) Replace the skill's "default `atol=1e-4, rtol=1e-4`" advice + with a table: + | Scope | `atol` | `rtol` | Notes | + |---|---|---|---| + | Single block, single kernel | 1e-4 | 1e-4 | tight; flags any real bug | + | Full DiT, aligned kernels | 1e-2 | 1e-2 | cross-layer accumulation | + | Full DiT, cross-kernel (flash_attn vs SDPA) | 0.1 | 0.1 | needs abs_mean drift check below 5% alongside | + | VAE decode, fp32 | 5e-2 | 5e-2 | after normalization alignment | + | Encoder wrapper (both wrap same HF class) | 1e-3 | 1e-3 | should be near-zero | + +- (b) Add a note: *"Element-wise `assert_close` alone is not enough + for full-DiT parity. Always combine with (1) a global abs_mean + drift check (<5%) and (2) per-modality diagnostic prints so + failures are debuggable."* + +**My guess:** both. The skill currently sets porters up for a false +red and then implicit silent loosening; explicit tolerance-by-scope +guidance is more honest. + +--- + +## 22b. "T2V-only" is not a valid scope interpretation of "base model" + +**Where in the skill:** Step 1 (inputs), "Scoping a port" open item (15 +above), `When to use` / `When not to use`. + +**What happened (during the MagiHuman audio round, user-corrected):** +The user said *"start with the base model ONLY"*. I interpreted that as +two axes at once: + + 1. Variant axis — base, distill, 540p_sr, 1080p_sr → **base only** ✅ + 2. Output-modality axis — audio + video vs video-only → **video-only** ❌ + +Axis (2) was my reading; the user had only meant axis (1). "Base" in +MagiHuman's README is a model *variant* (undistilled full-quality +joint-AV output), not a modality subset. Shipping a silent-mp4 pipeline +and labeling it `MagiHumanT2VConfig` with workload `"t2v"` was a +semantically wrong scope. + +The downstream damage: the DiT's `final_linear_audio` head was trained +to be consumed, not discarded. Dropping the audio output subtly changes +the trained-intended pipeline purpose, even though the DiT forward path +itself kept the audio token stream (so the video quality was unchanged). + +**Options:** + +- (a) **In the "Scoping a port" paragraph** (item 15): add an explicit + sentence about modality-axis scope: *"'Base model' refers to a + checkpoint variant (base vs distill vs SR vs I2V), NOT to an output + modality. If the upstream README lists audio, face, pose, or + additional output channels as core features of the base variant, + those belong in the first PR. Ask the user to spell out BOTH axes + explicitly at step 1 if in doubt."* +- (b) **Add a README-first checklist to step 2 ('Study the reference')**: + *"Before implementing anything, list all **output modalities** the + base checkpoint produces (video, audio, pose, segmentation, depth, + …). Every modality with a dedicated output head in the DiT must be + either decoded + emitted by the pipeline, or explicitly documented + as dropped with the user's agreement."* +- (c) **Pitfall to pipeline-config section**: *"If the upstream DiT + has N output heads (`final_linear_*`) but your `_required_config_modules` + only covers M`), (c) if +both miss, consider porting locally — many upstream Apache-licensed +modules are small enough (`AutoencoderOobleck` is ~450 lines) to +port cleanly without the third-party runtime dependency."* + +**Options:** + +- (a) Add a **reuse priority order** to the "Decide what to reuse" + step (step 3 in the skill): + 1. Existing FastVideo native class (`fastvideo/models/...`). + 2. Open PR adding a FastVideo native class (check with `gh pr list`). + 3. Local port of the upstream / Diffusers / Transformers class + under `fastvideo/models/`. + 4. Last resort: runtime `from diffusers/transformers import ...` — + document why and flag for follow-up. +- (b) Add a pitfall: *"A `from diffusers import AutoencoderX` in a + FastVideo pipeline class signals 'finish-porting-later'. It + freezes you to Diffusers' release cadence, opts you out of + FastVideo's FSDP/CP/loader infrastructure, and forks you from + any in-flight first-class port."* + +**My guess:** (a) + (b). The user explicitly called this out as +first-class is the expected bar; the skill doesn't currently reinforce +that expectation strongly enough. + +--- + +## 22. Custom VAE `scaling_factor` / `shift_factor` are per-channel vectors, not scalars + +**Where in the skill:** Weight-conversion section + VAE reuse notes +mention `scaling_factor` but don't warn about the per-channel case. + +**What I found (during the VAE parity):** The Wan 2.2 TI2V-5B VAE +has `latents_mean` + `latents_std` as **per-channel vectors of length +z_dim=48** (not the more common scalar `scaling_factor` that Stable +Diffusion uses). My initial parity test tried + + z_norm = (z - latents_mean) / latents_std + +without the per-channel `.view(1, z_dim, 1, 1, 1)` broadcast, which +would produce silently-wrong shapes for a 5D latent tensor (it might +broadcast along time or spatial dims instead of channel). + +For ported pipelines using a Wan-family VAE (or any modern video VAE +that ships per-channel normalization), the normalization must be +applied with explicit `.view(1, -1, 1, 1, 1)` reshape on the +mean/std vectors. + +**Options:** + +- (a) Add a pitfall in the VAE reuse section: *"Modern video VAEs + (Wan, AutoencoderKLWan, HunyuanVAE) ship **per-channel** + `latents_mean` / `latents_std` vectors. When applying the + normalization in a `DecodingStage` or parity test, always reshape + with `.view(1, z_dim, 1, 1, 1)` to broadcast along C, not along + T/H/W."* + +**My guess:** (a). One-liner prevents a silent broadcast bug that +produces nonsense video output. + +--- + +## 23. Skill assumes a "full model port"; "first-class component contribution" has no slot + +**Where in the skill:** "Files you will create or touch" table, +"Outputs" section, every Steps mention of "the pipeline". + +**What I found (during the will/stable-audio audit, 2026-04-25):** +The skill's 17-row Files-table assumes a porter is adding an entire +model family — DiT + VAE + encoder + pipeline + presets + registry + +tests + example. Stable Audio's VAE (Oobleck) was added as a +**first-class component contribution** (just rows 4–6 + 14 of the +table) so it could be reused by multiple downstream pipelines +(daVinci-MagiHuman audio decode, the in-flight T2A pipeline in +PR #1080, etc.) without each pipeline depending on `from diffusers +import AutoencoderOobleck`. The skill has no framing for this kind of +contribution: every reference to "the pipeline" implicitly assumes +the contribution includes a pipeline. + +Component-only contributions are common in practice and arguably +under-encouraged. They de-risk shared infrastructure (one PR landing +the VAE, then independent PRs landing each pipeline that uses it) +and let multiple ports share validation work. + +**Options:** + +- (a) Add a **"When the contribution is a component, not a model"** + subsection to "When to use" / "When not to use", with a reduced + Files-table specifying only rows 4–6 (or analogous: 1–3 for a + DiT-only contribution; encoder rows for an encoder-only contribution) + - row 14 (component parity test). Explicitly note the contribution + *will* skip the pipeline / preset / registry / smoke / pipeline-parity + rows and that's OK as long as a downstream pipeline PR is in flight + or planned. +- (b) Leave the skill strictly model-port-focused; treat + component-only contributions as an unrelated workflow with its own + skill (`add-component`?). +- (c) No change; let porters figure it out. + +**My guess:** (a). The pattern is real and recurring (Stable Audio +VAE here, future Stable Audio T5 conditioner, future shared video +VAEs across families). Naming the workflow saves the next porter from +guessing whether "skip the pipeline" is acceptable. + +--- + +## 24. Component-bucket inheritance is implicit + +**Where in the skill:** "Linear layers" table picks bucket-by-bucket, +but the skill never explicitly enumerates "config base class for each +bucket". + +**What I found (during the will/stable-audio audit):** Initially I put +`SAAudioVAEConfig(EncoderConfig)` — the wrong base class. The +underlying model is a VAE, not a text/image encoder, but the file +landed in `fastvideo/configs/models/encoders/` because it wraps a +"text-encoder-style" lazy loader. This was a real bug: it meant the +config didn't have `load_encoder` / `load_decoder` / tiling fields, +and no caller could feed an `OobleckVAEConfig` into a `vae_config` +slot that expects a `VAEConfig` subclass. + +The skill's existing tables list the right bucket per *model*: +- `fastvideo/models/dits/.py` → DiT bucket +- `fastvideo/models/vaes/vae.py` → VAE bucket +- `fastvideo/models/encoders/.py` → encoder bucket + +But it doesn't tie config inheritance back: per bucket, the new +config *must* inherit from a bucket-specific base. + +| Bucket | Config base | Arch base | +|---|---|---| +| `dits/` | `DiTConfig` | `DiTArchConfig` | +| `vaes/` | `VAEConfig` | `VAEArchConfig` | +| `encoders/` (text) | `TextEncoderConfig` | `TextEncoderArchConfig` | +| `encoders/` (image) | `ImageEncoderConfig` | `ImageEncoderArchConfig` | + +Choosing the wrong base typechecks fine but blows up at pipeline +wire-up time when the slot expects the bucket's specific base. + +**Options:** + +- (a) Add a "Bucket → config base" table to step 6 (component porting) + and to the "Quick reuse audit" subsection, so the right base is + picked the first time. +- (b) Add a runtime check / lint that flags model-bucket vs config- + inheritance mismatches. + +**My guess:** (a). One table, one row per bucket. + +--- + +## 25. Audio is a first-class workload but the skill is video-shaped + +**Where in the skill:** Many places — `SSIM regression test (row 15)` +assumes video frames; `fps`, `height`, `width`, `num_frames` are baked +into preset / pipeline-config defaults; the I2V section assumes spatial +conditioning; example scripts all save mp4. + +**What I found (during the will/stable-audio audit):** Stable Audio +Open is text-to-audio. None of the video-shaped conventions apply: + +- **No SSIM** — audio similarity needs different metrics (mel-scale + spectrogram L1, multi-resolution STFT, learned audio metrics like + CLAP cosine). +- **No `height` / `width`** — replaced by `audio_seconds` / + `sampling_rate`. +- **No `fps`** — replaced by sample rate. +- **No mp4 output** — `.wav` (or muxed audio track on an existing mp4 + for AV pipelines). +- **No "preset workload_type t2v / i2v"** — `t2a` (text-to-audio), + `a2a` (audio-to-audio), `av` (combined). + +The skill doesn't acknowledge audio as a workload at all. The MagiHuman +port worked around this by inheriting video conventions and bolting +audio on (random audio latent in latent prep, audio decode in a +separate stage that writes to `batch.extra["audio"]` for FastVideo's +existing audio-mux path). PR #1080's standalone Stable Audio T2A +pipeline will need to invent its own per-stage conventions because +nothing in the skill guides them. + +**Options:** + +- (a) Add a **"Modalities other than video"** section near the + "FastVideo's single architecture" intro with a small table: + + | Field | Video (default) | Audio | Joint AV | + |---|---|---|---| + | preset workload_type | "t2v" / "i2v" | "t2a" / "a2a" | "av" | + | shape coords | `(height, width, num_frames, fps)` | `(seconds, sampling_rate)` | both | + | regression metric | SSIM | mel-spectrogram L1 / CLAP | both | + | example output | `.mp4` | `.wav` | `.mp4` (muxed) | + +- (b) Generalize the canonical-stage-order diagram to mention audio: + add an "Audio T2A" variant alongside the existing T2V and I2V + diagrams. + +- (c) Cross-reference PR #1080's stable_audio pipeline (once merged) + as the canonical T2A example. + +**My guess:** all three. Audio is a real workload and likely to grow. + +--- + +## 26. Pipeline-glue wrappers vs. plain VAE classes + +**Where in the skill:** Files-table rows 4–6 (VAE + config) but no +mention of where wrapper / loader files belong. + +**What I found (during the will/stable-audio audit):** I added two +files for the Stable Audio VAE: + +- `fastvideo/models/vaes/oobleck.py` — the plain VAE class with + `from_pretrained`. Standalone-usable. +- `fastvideo/models/vaes/sa_audio.py` — a thin lazy-loader nn.Module + wrapper used by pipeline-component-loaded contexts (lazy fetch on + first forward, hide weights from `named_parameters`). + +The wrapper exists because FastVideo's `ComposedPipelineBase.load_modules` +walks `named_parameters` to match against the host pipeline's +converted-repo state dict; for VAEs/encoders that are externally +fetched (gated repo, not bundled), the wrapper is the standard pattern +to skip that match. `T5GemmaEncoderModel` does the same thing. + +The skill's Files-table has one row per (model, config) but no row for +this wrapper — porters either (a) discover the pattern by reading +existing encoder files, (b) inline the wrapper into the model file +(messy), or (c) skip the wrapper and crash at pipeline load. + +**Options:** + +- (a) Add a row 5b to the Files-table: + *"`fastvideo/models//_loader.py` (optional) — thin + lazy-loader nn.Module wrapper around the model class. Required when + the model is fetched from an external HF repo (gated, not bundled + in the converted family directory) and is consumed by a pipeline + that calls `ComposedPipelineBase.load_modules`."* +- (b) Document the wrapper pattern in the "Parallel component porting" + subagent prompt template + a small template snippet. + +**My guess:** (a) + (b). Naming the file pattern + showing the +template avoids the rediscovery cost. + +--- + +## 27. "Official repo" can be plural; pick the one that matches the published weights + +**Where in the skill:** Step 1 prerequisite ("Official reference +repository"); step 5 ("Clone the official reference repo locally"); +"Parity test pattern" section. + +**What I found (during the will/stable-audio rerun, 2026-04-25):** The +skill assumes a single "official repo" the porter clones for parity. +Stable Audio Open 1.0 has *two* first-party Python implementations: + + 1. `Stability-AI/stable-audio-tools` (the canonical training/inference + repo from Stability) — `OobleckEncoder`/`OobleckDecoder` classes + in `stable_audio_tools/models/autoencoders.py`. Defaults are + `c_mults=[1,2,4,8]`, `strides=[2,4,8,8]`, `latent_dim=32`, + `final_tanh=True`. Highly configurable (dozens of + `c_mults`/`strides`/`use_snake`/`final_tanh` permutations). + 2. `huggingface/diffusers::AutoencoderOobleck` (Stability-blessed + port to the diffusers library). Defaults are + `downsampling_ratios=[2,4,4,8,8]`, `channel_multiples=[1,2,4,8,16]`, + `decoder_input_channels=64`, no tanh. Fixed architecture per + class. + +These are **structurally different VAEs** with the same name. The +published Stable Audio Open 1.0 HF weights +(`stabilityai/stable-audio-open-1.0/vae/`) were trained with the +diffusers-shape variant (5 downsampling stages, latent_dim=64, no +tanh). So: + +- Comparing against `diffusers.AutoencoderOobleck` parity-tests the + published weights through the architecture they were trained for. +- Comparing against `stable-audio-tools.OobleckEncoder` would require + instantiating it with the right configurable defaults (5 stages, + 64-dim, etc.) — a non-trivial setup that just rebuilds what + diffusers already exposes as a fixed class. + +The will/stable-audio branch chose the diffusers route (parity is +exact, fp32 diff=0 on encode + decode). Defensible — but the skill's +literal text says "compare against the official repo" and we didn't. + +**Options:** + +- (a) Reword step 1 / step 5 to acknowledge "official repo can be + plural — pick the one whose architecture matches the published + weights you're loading. If the canonical research-code repo and a + Stability/Google/Meta-blessed library port both exist, prefer the + library port for parity (less per-config setup, more deterministic) + and document the choice in the parity test docstring." +- (b) Require both: parity vs library port + spot-check vs research + repo. + +**My guess:** (a). (b) doubles maintenance for a tiny coverage gain. + +--- + +## 28. `WorkloadType` enum has no audio variants + +**Where in the skill:** Step 11 ("Register in `fastvideo/registry.py`") +- "Inputs" table (says workload types are `T2V`/`I2V`/`V2V`/`T2I`). + +**What I found (during the will/stable-audio rerun):** The skill's +`workload_types` parameter only enumerates video variants. For Stable +Audio Open 1.0 (text-to-audio), there's no defined `WorkloadType.T2A` +or analogous enum value, so a downstream PR adding the full T2A +pipeline (PR #1080) will need either: + + 1. Extend `WorkloadType` enum with `T2A`, `A2A`, `AV`, etc. + 2. Use `()` (empty tuple) per the skill's note "Use `()` for + configs not exposed as a public workload." + +Option (2) is what the skill currently allows but it has a real cost: +the pipeline is then invisible in any UI / discovery layer that +filters by workload type. That's wrong for a first-class T2A pipeline. + +This is companion to REVIEW item 25 (audio is video-shaped throughout +the skill) — more specifically calls out the enum. + +**Options:** + +- (a) Pre-extend `WorkloadType` with the audio variants (`T2A`, `A2A`, + `AV`) + document them in the skill's "Inputs" table. Makes the + enum future-proof for both the in-flight T2A PR and any AV + pipeline (MagiHuman base needs `AV`). +- (b) Leave to the first audio-PR author to add them. + +**My guess:** (a). The skill's "Inputs" table should at minimum +mention "audio variants are TODO; for now, use `()` and explain +in the PR description." + +--- + +## 36. Strip narrative comments from production code; keep only WHY + +**Where in the skill:** Throughout (no current guidance on comment style). + +**What we found (during the will/stable-audio comment-trim pass, +2026-04-26):** A pattern emerged of comments narrating *provenance* +("Vendored from upstream X", "Mirrors upstream Y"), *change history* +("we removed Z because..."), and *recapitulating upstream class names* +that don't help anyone reading the code in isolation. Roughly 295 net +lines came out of 11 SA pipeline files without removing any +*useful* signal. + +**The rule (now codified):** + +> Comments in production code (`fastvideo/...`) explain **why**, not +> **what** or **where it came from**. Strip: +> - "Vendored from " / "Mirrors upstream X" +> - "Matches upstream Y" / restatements of upstream class names +> - Session-history narration ("Previously this was X, we moved +> it because Y", "removed in commit ...") +> - Cross-reference comments to REVIEW item N (the relevance lives +> in REVIEW.md, not in the code) +> - Section-divider banner comments (`# ---- Foo ---- #`) +> - Restating the function/class name in the docstring +> +> Keep: +> - Non-obvious WHY (hidden constraints, surprising defaults, +> workaround citations) +> - One-line module/class summaries +> - Pitfalls a future reader would hit (e.g. "model_max_length=128 +> is unusual for T5", "this depends on k_diffusion's mutate-state +> contract") + +**Action on the skill:** Add a "Comment style" pitfall in step 6 + +step 17 — `examples/...` lead with user-story docstrings (item 31), +production code stays comment-thin and WHY-focused. + +--- + +## 35. Examples must accept paths, not require user-written decode glue + +**Where in the skill:** Files-table row 17 (basic example scripts); +no current guidance on what *kind* of inputs the pipeline should +accept directly vs. preprocess at the example layer. + +**What happened (during the will/stable-audio polish, 2026-04-26):** +The first cut of `basic_stable_audio_a2a.py` exposed `init_audio` as +a `torch.Tensor` and made the example carry a 25-line `_load_reference` +helper that called `torchaudio.load(...)`. A user pointed at an mp4 of +a previous T2A run as the reference clip; the example crashed because +`torchaudio.load` on container formats (`.mp4` / `.m4a`) routes through +`torchcodec`, which pulls in the CUDA NVRTC stack we don't otherwise +need. Worse: the example required the user to know about resampling, +batch-dim normalization, channel layout — boilerplate that has nothing +to do with the use case. + +**Lesson:** the basic example is the *entry-point UX*. Anything more +than 5–10 lines of glue means we've pushed pipeline responsibilities +out to the user. Decode + resample + shape-norm belong inside the +pipeline (or a small shared helper it owns), not in +`examples/inference/basic/`. + +**Concrete rules now codified for this family (and the pattern to +copy for other media-input pipelines):** + +- Wherever the pipeline accepts a media tensor (audio waveform, + image, init video, mask), it should also accept a **file path** + pointing at any container/codec the standard FastVideo deps can + decode (PyAV is already a dep — it's used for muxing). The + pipeline does the resample + shape-norm internally. +- Where a structured input has a common "fill in the obvious" form + (e.g. inpaint mask = "keep first K seconds, regenerate the + rest"), accept a small DSL form (a tuple here) alongside the + raw-tensor form. Saves the user from building a `[samples]` + tensor by hand. +- Examples become 5–15 lines of constants + one `generate_video(...)` + call. The user-story docstring (item 31) is the only narrative. + +**Concrete bug avoided / fixed:** `torchaudio.load` ↔ `torchcodec` ↔ +NVRTC dependency chain — surfaced as `OSError: libnvrtc.so.13: cannot +open shared object file`. PyAV (already a FastVideo dep) handles all +the same formats without that chain. + +**Action on the skill:** + +- (a) Add a Files-table note on row 17: examples should be **5–15 + LOC of constants + a single pipeline call**. If the example needs + more than that, the missing convenience belongs *in the pipeline*. +- (b) Add a pitfall: *"`torchaudio.load` on container formats (mp4, + m4a) routes through `torchcodec` and needs the CUDA NVRTC runtime. + For audio-bearing files use PyAV (already a FastVideo dep — used + for muxing). For image inputs, use `imageio` (also a dep). Don't + reach for `torchaudio` / `torchvision` IO unless the file is + guaranteed to be a wav / png with no exotic container."* +- (c) Add a small reusable file-decode helper (e.g. + `fastvideo/utils/audio_io.py`) once a second pipeline needs it — + for now Stable Audio owns its `_decode_audio_file` inline in the + latent-prep stage. + +--- + +## 34. Single-class kwargs-driven pipeline shape (when modes share weights) + +**Where in the skill:** "Adding an I2V variant" section frames variants +as separate pipeline files. No guidance for the case where modes share +*everything* and only differ in kwargs. + +**What we found (during the will/stable-audio Tier-1 build, +2026-04-26):** Stable Audio's three end-user modes (T2A, A2A, +RePaint inpainting) share the same DiT, conditioner, VAE, and sampler +— the per-mode branching is ~30 LOC of stage-internal `if/else`. The +upstream API is also single-function kwargs-driven. Splitting into +three pipeline classes would force `from_pretrained` re-loads of 1B+ +params per mode switch, duplicate ~600 LOC of `load_modules` / stage +wiring / preset / registry, and require the user to instantiate a +different `VideoGenerator` per mode — none of which match the user's +mental model ("one model, different requests"). + +**The rule (codified in the SA pipeline class docstring):** keep one +pipeline class for modes that share weights / components / stage +chain; mode-select via call kwargs. Document the kwargs contract in +the class docstring + add loud-fail validation (e.g. `inpaint_mask` +without `inpaint_audio` should raise, not silently fall through to +T2A). + +**Triggers to revisit (split into per-mode classes):** + 1. A new published checkpoint genuinely needs different + `_required_config_modules` (e.g. Stability publishes an + inpaint-trained `diffusion_cond_inpaint` variant with extra + conditioning). + 2. `WorkloadType` enum gains audio variants (item 28); each preset + wants its own workload tag. + 3. Per-mode forward signatures diverge enough that kwarg-overloading + hides bugs (rather than just looking inelegant). + 4. The branches inside stages stop being light if/else and become + full alternative flows. + +**Contrast with Wan I2V split** (which IS justified): I2V there has +a different `image_encoder` module in `_required_config_modules`, a +different HF repo (`Wan2.1-I2V-14B-720P-Diffusers`), and a required +`image_path` argument. None of that applies to SA modes today. + +**Action on the skill:** + +- (a) Add a "Single-class kwargs-driven variants" subsection to + "Adding an I2V variant" with the four split-triggers above. +- (b) Add a pitfall: *"Don't split into per-mode pipeline classes + reflexively — splitting forces param reloads per mode switch and + duplicates ~600 LOC of glue. Split when components or + `_required_config_modules` actually differ across modes, not just + the call signature."* + +--- + +## 33. Hot-path/kwarg refactors after the first parity-clean cut + +**Where in the skill:** Step 13 (handoff). No mention of revisiting +hot-path allocations once parity is locked in. + +**What we found (during the will/stable-audio /simplify pass, +2026-04-26):** A 3-agent code review surfaced several issues that +all stemmed from the same pattern — **the first parity-clean cut +prioritized *correctness verification* over *per-call efficiency***, +and small things compounded: + +- `null_embed = torch.zeros_like(cross_attn_cond)` allocated **per + sampler step** (~100 fresh allocs per `generate_video()` call on + a `[B, L, D]` tensor). Side-effect: hoisting it outside the loop + improved A2A diff_max from 0.37 → 0.030 — the per-step + recomputation was apparently introducing *more variance* than + just allocations. +- `torch.backends.{cuda.matmul.allow_tf32, cudnn.allow_tf32, + allow_fp16_reduced_precision_reduction, cudnn.benchmark}` set in + a *per-call* stage instead of one-shot in `load_modules`. Mid-run + flips can invalidate the cuDNN algorithm cache. +- `torch.randn_like(reference_latent)` called per RePaint step + (~25 MB alloc churn / call). Pre-allocating one + `noise_buf = torch.empty_like(...)` + `noise_buf.normal_()` saves + the churn AND avoids ~100 RNG kernel launches. +- Dead writes to `batch.extra` (`seed`, `downsampling_ratio`, + `sample_size`, `init_noise_level` round-trip) that nothing + consumed — easy to leave behind once you're chasing parity, but + they accumulate as ghost contracts. + +**Why this happens:** when you're chasing parity drift, you'll add +intermediate tensors / extra context-dict writes / per-step +recomputations defensively. Once parity is green you can — and +should — go back through with a hot-path-only review pass. + +**The rule (now codified):** after parity is locked in, **run a +hot-path review pass before handoff**. Specific things to look for: + + 1. Tensor allocations inside `for step in sampler:` or per-DiT-call + adapter forwards — hoist constants outward. + 2. RNG calls inside callbacks — pre-alloc the noise buffer with + `empty_like + .normal_()` instead of `randn_like`. + 3. `torch.backends.*` flags set per-call — move to `load_modules`. + 4. `batch.extra` writes that nothing reads — delete. + 5. Hardcoded magic constants that ARE derivable from a model + config (e.g. VAE `hop_length`) — derive instead, so variant + checkpoints don't silently break. + +**Action on the skill:** + +- (a) Add a "Post-parity hot-path pass" sub-step to step 13 (right + before handoff): a checklist of the five items above. +- (b) Add a pitfall to the parity-test pattern: *"Once parity is + green, run a 3-agent reuse / quality / efficiency review (or + equivalent) before merging — defensive intermediates added during + parity hunting often hide perf wins that are also accuracy wins + (per-step recomputation can introduce more variance than the + allocation costs)."* + +--- + +## 32. Drop unused stub helpers when the deps they bypassed become real + +**Where in the skill:** Item 17 ("Upstream private-DSL stubs") added +a stub-helper template; nothing tells the porter when to *delete* it. + +**What we found (during the will/stable-audio polish, 2026-04-26):** +`tests/local_tests/helpers/stable_audio_upstream.py` started life as +a `sys.modules` stub installer to bypass uninstalled transitive deps +(`k_diffusion`, `einops_exts`, `alias_free_torch`). Once the test +suite started requiring those deps as real installs, the stub became +a no-op kept "for back-compat" with two test call sites. A user +called this out as dead weight; deleting the helper + its two +invocations was a 3-file diff with zero behavior change. + +**The pattern:** stubs are scaffolding. Once the real dep lands, +delete the stub *immediately*; don't leave it as a no-op shim. A +back-compat shim with no real callers is just dead code with a +plausible-looking name that confuses the next reader. + +**Action on the skill:** + +- (a) Add a one-line sentence to item 17's stub-helper template: + *"Delete the helper + every `install_stubs()` call site as soon as + the deps it bypassed are required-installs in the test suite. A + no-op shim is dead code, not a feature."* + +--- + +## 31. `examples/inference/basic/basic_*.py` should lead with user-story docstrings + +**Where in the skill:** Files-table row 17 ("`examples/inference/basic/basic_.py` +— User-runnable example for manual quality verification"); no guidance on +*how* to document the example. + +**What we found (during the will/stable-audio Tier-1 build, 2026-04-26):** +The user explicitly requested that each variant example (T2A, audio-to- +audio, inpainting, small-variant) lead with a docstring written in +**user-story form** — concrete who/why scenarios that motivate why the +variant exists, not just a what-the-code-does summary. This converts +each example into a self-contained explainer that: + +- answers "why would someone reach for this entry point" up front; +- names the personas (musician, sound designer, UI builder, …) and + their pain; +- documents the tunable knobs in their narrative role ("creative + dials"), not as a parameter table; +- cross-references the upstream API call the variant mirrors so a + reader can move between FastVideo and the upstream repo without + a translation step. + +The result reads like product documentation aimed at the *next porter +or end user*, not like API docs aimed at the implementer. This is +unusually high-leverage for variants whose purpose is non-obvious (e.g. +"inpainting" means very different things to different audiences) and +for examples that exist primarily to communicate intent (a pipeline +variant has zero discoverability if the example just says +"`generator.generate_video(...)`"). + +Concrete pattern (3-section docstring): + + 1. **One or more "User story ():"** blocks — first-person + ("I have…", "I want…") quoted scenarios. 2–4 sentences each. + Cover at least one professional and one tinkering use case. + 2. **"How it works:"** — short prose explanation of what the + variant does in pipeline terms (encode, mask, blend, sample), + plus a 1-line cross-reference to the upstream entry point or + algorithm name (RePaint, dpmpp-3m-sde, etc.). + 3. **"Tunable knobs (the 'creative dials'):"** — for variants with + a sensitivity knob (init_noise_level, mask coverage, sampler_type, + etc.), a 2-column micro-table mapping value → effect in narrative + terms ("0.5 — keep most of the reference; 5.0 — heavy reroll; + 500.0 — equivalent to no reference"). + + Plus a closing **"Prerequisites:"** line that names the access / + install asks (HF gated repo, optional inference deps). + +**Examples in-tree** (will/stable-audio branch as of commit ``): +- `examples/inference/basic/basic_stable_audio.py` (T2A baseline) +- `examples/inference/basic/basic_stable_audio_a2a.py` (variation) +- `examples/inference/basic/basic_stable_audio_inpaint.py` (RePaint + inpainting / loop extension) +- `examples/inference/basic/basic_stable_audio_small.py` (size variant) + +**Action on the skill:** + +- (a) Update Files-table row 17 wording: *"`examples/inference/basic/basic_*.py` + — User-runnable example **with a user-story-shaped docstring** + (persona + how-it-works + tunable-knobs sections). One per + pipeline variant the family supports."* +- (b) Add a new pitfall in step 17 / final-checklist: *"An example + whose docstring only restates the code is a missed opportunity. + Lead with at least one 'User story ():' block — concrete + scenario in the persona's voice — so the next porter / end user + can decide whether this is the entry point they want without + reading the source."* +- (c) Cross-reference the will/stable-audio examples as the canonical + shape (concrete, narrative, knob-aware). + +--- + +## 30. Hard ban on `from diffusers import` and `from transformers import` (model classes) at runtime + +**Where in the skill:** "Decide what to reuse" (step 3); "Files you will +create or touch" (rows 1, 4); pitfall around shortcuts. + +**What we found (during the will/stable-audio rerun, 2026-04-25):** The +first cut of `StableAudioPipeline.load_modules` imported three +diffusers components at runtime as a "temporary scaffold": + +- `diffusers.StableAudioDiTModel` +- `diffusers.pipelines.stable_audio.modeling_stable_audio.StableAudioProjectionModel` +- `diffusers.CosineDPMSolverMultistepScheduler` + +This was meant to be a fast first-pass that proved the orchestration +worked while the heavy DiT/projection/scheduler ports were deferred. +The cost was real and concrete: + + 1. **Pipeline parity vs the official upstream repo drifted to 71% + abs_mean** (see `test_stable_audio_pipeline_official_parity` in + `tests/local_tests/pipelines/`). All of that drift came from + diffusers↔upstream divergence in the components we re-imported — + none of it was FastVideo orchestration drift. + 2. The pipeline froze to diffusers' release cadence (a diffusers + bump could silently change Stable Audio behavior). + 3. Two parallel test suites had to exist: a tight diffusers-vs-FV + test (~0.1% drift, the meaningful gate) and a loose + official-vs-FV test (~70% drift, the documented-as-aspirational + gate). Confusing for any future reader. + 4. Future ports would copy the same shortcut, compounding the debt. + +The user's directive was unambiguous: *"remove all use of diffusers +version. directly port from official repo into fastvideo natively for +all pipeline components."* + +**The rule (now codified):** + +> **Pipeline component classes (DiT, VAE, projection / conditioner, +> scheduler) MUST be first-class FastVideo ports.** Runtime imports +> from `diffusers` or `transformers` (model classes) inside a +> `Pipeline.load_modules`, stage `forward`, or any production code +> path are forbidden. The only permitted runtime third-party imports +> are: +> - `transformers.` for tokenizers (these are pure +> data-conversion utilities, not model weights). +> - `diffusers` / `transformers` *inside test files* for parity +> references — never in `fastvideo/`. +> +> If a component isn't yet ported, **don't ship the pipeline that +> needs it**. A scaffold pipeline with diffusers imports as +> placeholders is worse than no pipeline at all: it sets the wrong +> precedent, masks parity drift, and forks the project from upstream. + +**Action on the skill:** + +- (a) Add the rule above as a top-level **Hard rule** in step 3 + ("Decide what to reuse") and again as pitfall #N in the parity + pattern. +- (b) Update step 6's subagent prompt template: every component + subagent must produce a FastVideo-native class under + `fastvideo/models//`. No subagent should ever return + `from diffusers import X` in a model file. +- (c) Update step 13(a): the pipeline-parity gate is **vs the + official upstream**, not vs diffusers. Drop the "comparing + diffusers as an interim gate" framing entirely. +- (d) Pre-extend the Files-table with separate rows for any DiT / + scheduler / conditioner ports needed for first-class status + (currently row 1 is DiT-only). + +**Exception:** the only acceptable runtime import from `diffusers` or +`transformers` (model classes) is when the component is already a +shared FastVideo dependency at the package boundary (e.g. T5 +*tokenizer* — pure data utility, no weights). Anything that owns +weights or numerical behavior must be FastVideo-native. + +--- + +## 29. VAE file naming: arch name vs family name is ambiguous + +**Where in the skill:** Files-table rows 4 + 5 say `vae.py`. + +**What I found (during the will/stable-audio rerun):** Existing VAEs +are named after the **family**: + +- `fastvideo/models/vaes/wanvae.py` +- `fastvideo/models/vaes/hunyuanvae.py` +- `fastvideo/models/vaes/ltx2vae.py` +- `fastvideo/models/vaes/cosmos2_5vae.py` +- `fastvideo/models/vaes/gen3cvae.py` + +For Stable Audio, "family-name VAE" would be `stable_audiovae.py` +(awkward read, multi-word + suffix). The VAE has its own published +arch name — "Oobleck" — so I went with `fastvideo/models/vaes/oobleck.py`. +Cleaner read, but inconsistent with the family-name convention. + +The right rule depends on whether the VAE is family-specific (one +family ever uses it → name after family) or arch-shared (multiple +families could use it → name after arch). Oobleck is the latter case +(currently used by Stable Audio Open 1.0; could be reused by Stable +Audio v2, AudioLDM-derivatives, etc.). + +**Options:** + +- (a) Update the skill's row 4/5 wording to: + *"`fastvideo/models/vaes/.py` — name after the VAE's + arch when it has one (e.g. `oobleck.py` for Stability's audio VAE, + `autoencoder_kl.py` for the SD-family KL-VAE), otherwise after the + family (e.g. `wanvae.py`)."* +- (b) Standardize all VAE files on the family name; rename `oobleck.py` + → `stable_audiovae.py` for consistency. (Costly retrofit.) + +**My guess:** (a). The arch-name pattern already exists in +`fastvideo/models/vaes/autoencoder_kl.py`; codifying it lets new +contributions pick the right name without trying to reverse-engineer +the convention. + +--- + +## Summary + +| # | Topic | Severity | Action | +|---|-------|----------|--------| +| 1 | No Wan tests exist | Medium | User decides: grandfather Wan, or retrofit tests. | +| 2 | `wan_to_diffusers.py` legacy | Low | Consider deleting if unused. | +| 3 | `ConditioningStage` universality | Low | Audit other families; finalize framing. | +| 4 | I2V image-encoder selection rule | Low | Add rule if one exists. | +| 5 | Wan2.2 MoE / dual-guidance fields | Medium | Decide if they deserve a dedicated section. | +| 6 | T2V/I2V cross-attn naming | Low | Decide if skill should prescribe. | +| 7 | Pipeline-parity gate is local-only | Low | Consider explicit callout. | +| 8 | Variant-pipeline enumeration | Low | Decide whether to enumerate. | +| 9 | HF token scope phrasing | Low | Consider "read + write". | +| 10 | Raw `nn.Linear` policy | Low | Clarify in pitfall #11 if you have a position. | +| 11 | Packed-expert linears (MoE / multi-modality) | Medium | Name as accepted exception to the ReplicatedLinear rule. (From MagiHuman port.) | +| 12 | VAE reuse — `-Diffusers` suffix | Low | One-line note in the conversion recipe. (From MagiHuman port.) | +| 13 | HF token at conversion, not just publish | Low | Reword step 1 prerequisite. (From MagiHuman port.) | +| 14 | Cross-modality flat-stream attention | Low | Extend the attention selection table. (From MagiHuman port.) | +| 15a | ArchConfig scope = DiT-only; fallback to official Python when HF config is empty | High | Rewrite step 2 + add hard pitfall. (From MagiHuman port, user-corrected.) | +| 15 | Scoping a port with multiple variants | Low | Add a "Scoping a port" paragraph. (From MagiHuman port.) | +| 16 | **Skip-on-missing parity is a silent no-op trap** | **Critical** | **Pre-handoff checklist + reorder conversion before parity + "skip = missing, not pass" language.** (From MagiHuman port, user-corrected — the single biggest gap.) | +| 17 | Upstream private-DSL stubs (`magi_compiler` etc.) | Medium | Add a "Upstream private deps" subsection with stub-helper template. (From MagiHuman port.) | +| 18 | `torch.ops..` registration side-effect | Low | Pitfall callout in the private-deps subsection. (From MagiHuman port.) | +| 19 | GQA kernel alignment in parity tests | Low | 3-line pitfall in "Parity test pattern". (From MagiHuman port.) | +| 20 | VAE normalization convention mismatches | Medium | Pitfall paragraph in VAE reuse or parity pattern. (From MagiHuman port.) | +| 21 | Realistic parity tolerances by scope | Medium | Replace the scalar `atol=1e-4` default with a scope-indexed table. (From MagiHuman port.) | +| 22 | Per-channel VAE scaling vectors | Low | One-liner pitfall in VAE reuse. (From MagiHuman port.) | +| 22b | **"Base" is a checkpoint variant, not a modality subset** | **High** | Add modality-axis scope clarifier to step 1 + README-first checklist in step 2 + output-head pitfall in pipeline-config. (From MagiHuman audio round, user-corrected.) | +| 22c | First-class over `from diffusers import` | Medium | Add explicit reuse priority order (FastVideo-native > in-flight PR > local port > diffusers import as last resort) + pitfall. (From MagiHuman audio round, user-corrected.) | +| 23 | **No "first-class component" workflow** | **High** | Add a "When the contribution is a component, not a model" subsection + reduced Files-table. (From will/stable-audio audit.) | +| 24 | Component-bucket → config-base inheritance is implicit | Medium | Add a "Bucket → config base" table to step 6 + Quick reuse audit. (From will/stable-audio audit.) | +| 25 | **Audio is a first-class workload but skill is video-shaped** | **High** | Add a "Modalities other than video" section + audio variant of the canonical stage diagram + cross-ref PR #1080. (From will/stable-audio audit.) | +| 26 | Pipeline-glue wrappers vs. plain VAE/encoder classes | Medium | Add Files-table row 5b (`_loader.py`) + template snippet in the subagent prompt. (From will/stable-audio audit.) | +| 27 | "Official repo" can be plural — pick the one matching published weights | Medium | Reword step 1 / step 5 to acknowledge multi-implementation case. (From will/stable-audio rerun.) | +| 28 | `WorkloadType` enum has no audio variants | High | Pre-extend with T2A/A2A/AV + document in Inputs table. (From will/stable-audio rerun.) | +| 29 | VAE file naming: arch name vs family name is ambiguous | Low | Update row 4/5 wording: name after arch when shared, family when family-specific. (From will/stable-audio rerun.) | +| 30 | **Hard ban on `from diffusers/transformers import` of model classes at runtime** | **Critical** | Codify the rule, update step 3 / step 6 / step 13(a), drop the "interim diffusers comparison" framing. (From will/stable-audio rerun, user-corrected — direct quote: "remove all use of diffusers version".) | +| 31 | User-story-shaped docstrings on every `examples/inference/basic/basic_*.py` | Medium | Update Files-table row 17 + add a pitfall: docstring leads with `User story ():` blocks → How it works → Tunable knobs ("creative dials"). (From will/stable-audio Tier-1 build, user request: "I love the user story style ... document each in docstring at top of corresponding basic example script".) | +| 32 | Drop stub helpers when the deps they bypassed become real | Low | One-line addendum to item 17's stub-helper template: stubs are scaffolding, not features. (From will/stable-audio polish.) | +| 33 | Post-parity hot-path pass before handoff | High | Add a 5-item checklist sub-step to step 13: per-step allocs hoisted, RNG buffers pre-alloc'd, `torch.backends.*` flags one-shot, dead `batch.extra` writes deleted, magic constants derived from configs. (From will/stable-audio /simplify pass — also surfaced an A2A diff_max regression that vanished once per-step `zeros_like` was hoisted.) | +| 34 | Single-class kwargs-driven variants (when modes share weights) | Medium | New "Single-class variants" subsection: split into per-mode classes only when components / `_required_config_modules` actually differ, not just call signature. List 4 split-triggers. (From will/stable-audio Tier-1 design discussion.) | +| 35 | **Examples must accept paths, not require user-written decode glue** | **High** | Files-table row 17 budget: examples are 5–15 LOC of constants + one pipeline call. Decode/resample/shape-norm belong inside the pipeline. Pitfall: `torchaudio.load` on container formats pulls in torchcodec → NVRTC; PyAV (already a dep for muxing) handles the same formats without that chain. (From will/stable-audio polish, user-corrected — direct quote: "we cannot expect user to provide this code".) | +| 36 | Strip narrative comments from production code; keep only WHY | Medium | Add a "Comment style" pitfall in step 6 + step 17: examples lead with user-story docstrings (item 31), production code stays comment-thin and WHY-focused. ~295 net lines came out of 11 SA pipeline files in the trim pass without losing useful signal. (From will/stable-audio comment-trim pass.) | + +None of these block a new porter from using the skill today; they're +polish / consistency items that need a codebase-owner call. + +**Exception: item 16 is load-bearing.** The skill as currently written +lets a port ship without any non-skip parity coverage, because the +"skip-on-missing" convention means green pytest output does not +actually mean parity was verified. Without a pre-handoff checklist +that explicitly bans skips on the porter's local machine, this will +keep happening. diff --git a/.agents/skills/add-model/SKILL.md b/.agents/skills/add-model/SKILL.md new file mode 100644 index 000000000..53161cf0e --- /dev/null +++ b/.agents/skills/add-model/SKILL.md @@ -0,0 +1,1568 @@ +--- +name: add-model +description: Use when adding a new model (or new model variant) to FastVideo — a text/image/video diffusion pipeline under `fastvideo/pipelines/basic/`. Walks through the component-by-component integration sequence, pointing at exact files, base classes, and registration hooks. FastVideo has one pipeline architecture (stage-based composition), so this skill does not cover style choices — only the canonical flow. +--- + +# Add a Model to FastVideo + +## Purpose + +Port a new diffusion model into FastVideo so it runs end-to-end through +`VideoGenerator.from_pretrained(...)` with the same public API as existing +models (Wan, LTX-2, Hunyuan, Cosmos, etc.). + +This skill is the **procedural index**. For the narrative version — why each +step exists, prompting examples, and a Wan2.1 case study — read +`docs/contributing/coding_agents.md` first, then return here. + +## When to use + +- A new model family is being ported (e.g., "add FooVideo"). +- A new variant of an existing family needs its own pipeline config / preset + (e.g., a distilled or I2V variant that doesn't fit the existing wiring). +- The official code or HF repo already exists and you have weights locally. + +## When not to use + +- Tweaking sampling defaults only → edit the relevant preset in + `fastvideo/pipelines/basic//presets.py`. No new pipeline needed. +- Reconfiguring an existing pipeline's CLI → use the API skill track + (`fastvideo/api/*` + `fastvideo/configs/pipelines/*`), not this. +- Seeding SSIM reference videos for a *just-added* test → that is a separate + skill, `.agents/skills/seed-ssim-references/`. + +## Prerequisites + +**Do not start any work until you have explicitly collected all three of +the following from the user.** If any is missing, stop and ask — don't +guess, don't scrape, don't assume. + +1. **Official reference repository.** GitHub URL or Diffusers pipeline + source. This is the numerical ground-truth you'll parity-test against. +2. **Hugging Face weight path.** The HF model ID (or local directory) where + the weights live. Confirm whether the repo is **Diffusers-format** (has + `model_index.json` at the root with `_class_name`, plus per-component + subdirs like `transformer/`, `vae/`, `text_encoder/`) or **raw official + format** (monolithic `.safetensors` / `.pt`, no `model_index.json`). + This dictates whether step 4 (weight conversion) is needed. +3. **Hugging Face write-enabled API key** — always ask at the start, even + if you think conversion/publishing won't be needed. Must have **write + scope** (a read-only token will not pass + `create_hf_repo.py --push_to_hub` nor the reference-video upload used + by `seed-ssim-references`). Ask the user to export it as one of + `HF_TOKEN` / `HUGGINGFACE_HUB_TOKEN` / `HF_API_KEY` in the shell where + the skill will run. **Never** accept the raw token pasted in chat and + **never** echo it back, log it, or commit it. If no env var is set, + stop and wait. + +Ask exactly: *"Before we start: (1) what's the official reference +repo/pipeline URL, (2) what's the HF model path or local weights +directory, and is it already in Diffusers format, and (3) is a +write-enabled HF API key exported in the environment (which env var — +`HF_TOKEN`, `HUGGINGFACE_HUB_TOKEN`, or `HF_API_KEY`)?"* + +Additional environment prerequisites: + +- Editable install working (`uv pip install -e .[dev]`) and pre-commit + hooks installed. +- Ability to run single-GPU inference (for parity + smoke tests). Multi-GPU + optional. +- If weights aren't yet downloaded, stage them under + `official_weights//` (see the + `scripts/huggingface/download_hf.py` helper). + +## Inputs + +| Parameter | Required | Description | +|-----------|----------|-------------| +| `model_family` | Yes | snake_case identifier for the family (e.g. `ltx2`, `wan`, `foovideo`). Used for directory, registry, and preset names. | +| `pipeline_class` | Yes | PascalCase pipeline class name (e.g. `LTX2Pipeline`). Must match the Diffusers `model_index.json` `_class_name` when one exists. | +| `hf_model_paths` | Yes | List of HF model IDs this pipeline should claim (e.g. `["Org/Model-Diffusers"]`). | +| `workload_types` | Yes | Tuple of `WorkloadType` values the pipeline supports (`T2V`, `I2V`, `V2V`, `T2I`, ...). | +| `official_ref` | Yes | Pointer to the reference implementation (repo URL, pipeline file, or HF model id). **Must be collected from the user before any code action.** | +| `hf_weights_path` | Yes | HF model ID or local directory where weights live, plus whether it is already in Diffusers format. **Must be collected from the user before any code action.** Drives whether step 4 (conversion) is needed. | +| `hf_write_api_key` | Yes | Confirmation that a write-enabled HF token is exported (`HF_TOKEN` / `HUGGINGFACE_HUB_TOKEN` / `HF_API_KEY`). **Must be collected at the start of the skill**, before any work. Used for publishing converted Diffusers repos and for SSIM reference uploads. Never accept the raw token in chat. | +| `reuse_candidates` | No | Existing families whose DiT/VAE/encoders you expect to reuse — surface these up front to avoid duplicate implementations. | + +## FastVideo's single architecture + +Every pipeline in FastVideo is a **stage-based composition** that inherits +from `ComposedPipelineBase`. There is **no** hybrid-vs-modular choice like +other frameworks have — all models use the same pattern. Variation happens +three ways: + +1. **Reuse standard stages** when the model fits them (most common). +2. **Subclass a standard stage** when one step diverges (e.g. + `LTX2DenoisingStage(DenoisingStage)` for audio+video fusion). +3. **Add a model-specific stage** when no standard stage fits. + +The canonical stage order is: + +``` +# T2V (verified against fastvideo/pipelines/basic/wan/wan_pipeline.py): +InputValidationStage + → TextEncodingStage + → ConditioningStage # present in Wan T2V + I2V; treat as default, not optional + → TimestepPreparationStage + → LatentPreparationStage + → DenoisingStage # receives transformer (+ transformer_2 for MoE) + scheduler + vae + → DecodingStage + +# I2V (verified against fastvideo/pipelines/basic/wan/wan_i2v_pipeline.py): +InputValidationStage + → TextEncodingStage + → ImageEncodingStage # NEW — CLIP-style image embedding; guarded on image_encoder being loaded + → ConditioningStage + → TimestepPreparationStage + → LatentPreparationStage + → ImageVAEEncodingStage # NEW — encodes the reference image into VAE latent space + → DenoisingStage # no vae kwarg here; it was consumed by ImageVAEEncodingStage + → DecodingStage +``` + +Reference: `fastvideo/pipelines/basic/wan/wan_pipeline.py` is the cleanest +"standard" example. Model-specific stages and overrides live in +`fastvideo/pipelines/basic//stages/`, not in the shared +`fastvideo/pipelines/stages/` tree. + +## Files you will create or touch + +Minimum set for a new model family. Paths are relative to the repo root. + +| # | Path | Role | +|---|------|------| +| 1 | `fastvideo/models/dits/.py` | DiT implementation (reuses FastVideo layers + attention). | +| 2 | `fastvideo/configs/models/dits/.py` | DiT arch config + `param_names_mapping`. | +| 3 | `fastvideo/configs/models/dits/__init__.py` | Export the new DiT config. | +| 4 | `fastvideo/models/vaes/vae.py` | VAE — skip if reusing an existing one. | +| 5 | `fastvideo/configs/models/vaes/vae.py` | VAE config — skip if reusing. | +| 6 | `fastvideo/configs/models/vaes/__init__.py` | Export the new VAE config (if added). | +| 7 | `fastvideo/configs/pipelines/.py` **or** `fastvideo/pipelines/basic//pipeline_configs.py` | `PipelineConfig` subclass wiring DiT + VAE + encoders. | +| 8 | `fastvideo/pipelines/basic//_pipeline.py` | Pipeline class with `EntryClass = `. **I2V / V2V / DMD variants get their own sibling file** (e.g. `wan_i2v_pipeline.py`, `wan_dmd_pipeline.py`, `wan_v2v_pipeline.py`), each with its own `EntryClass`. See [Adding an I2V variant](#adding-an-i2v-variant). | +| 9 | `fastvideo/pipelines/basic//presets.py` | `InferencePreset`s + `ALL_PRESETS` tuple. | +| 10 | `fastvideo/pipelines/basic//stages/` (optional) | Model-specific stage subclasses. If added, import them in `fastvideo/pipelines/stages/__init__.py`. | +| 11 | `fastvideo/registry.py` | Add `register_configs(...)` call inside `_register_configs()` **and** import + add to the group tuple inside `_register_presets()`. | +| 12 | `tests/local_tests/pipelines/test__pipeline_smoke.py` | End-to-end smoke test (in-process, fast). | +| 13 | `tests/local_tests/pipelines/test__pipeline_parity.py` | **Mandatory** end-to-end parity test against the cloned official pipeline. Must be green before handing off to the user. Template: `tests/local_tests/pipelines/test_gamecraft_pipeline_parity.py`. | +| 14 | `tests/local_tests//test__*.py` | Per-component numerical parity tests (DiT, VAE, encoder). | +| 15 | `fastvideo/tests/ssim/test__similarity.py` | SSIM regression test (reference videos seeded via `.agents/skills/seed-ssim-references/`). | +| 16 | `scripts/checkpoint_conversion/_to_diffusers.py` (or `convert__to_diffusers.py`) | **Only if source weights are not Diffusers-format.** Rewrites keys + splits by component into `converted_weights//`. See the [Weight conversion](#weight-conversion) section. | +| 17 | `examples/inference/basic/basic_.py` | User-runnable example script for manual quality verification. Mirrors existing `basic_.py` examples (e.g. `basic_ltx2.py`, `basic_wan2_2.py`). | + +## Steps + +1. **Gather inputs from the user (blocking).** + - Before any file read, grep, or implementation, ask the user for + **all three** of the following and wait for answers: + 1. **Official reference repo/pipeline URL.** + 2. **HF model path or local weights directory**, plus whether it is + already Diffusers-format (has `model_index.json`). + 3. **Write-enabled HF API key** — confirmation that one of `HF_TOKEN` + / `HUGGINGFACE_HUB_TOKEN` / `HF_API_KEY` is exported in the shell. + You need this for (a) publishing the converted Diffusers repo via + `create_hf_repo.py --push_to_hub`, and (b) uploading SSIM reference + videos later. Do not accept the raw token in chat. Verify it's set + with `[ -n "$HF_TOKEN$HUGGINGFACE_HUB_TOKEN$HF_API_KEY" ] && echo ok`. + - Also confirm: the target `model_family` name (snake_case), supported + workload types (`T2V`/`I2V`/`V2V`/`T2I`/…), and whether the user + wants LoRA support. + - If any of the three are missing, stop and let the user resolve. A + wrong HF path silently cascades into wrong `param_names_mapping` and + wasted parity debugging; a missing write token fails step 4 and + step 14 after the costly work is already done. + +2. **Study the reference implementation.** + - Open the official repo / Diffusers pipeline source. Locate + `model_index.json` (or the equivalent config) and list the required + modules (`text_encoder`, `tokenizer`, `vae`, `transformer`, + `scheduler`, plus any extras like `audio_vae`, `vocoder`, + `image_encoder`). + - Read `pipeline.__call__` end-to-end. Note: text encoding flow, latent + shape/dtype/scaling, timestep schedule, DiT forward kwargs, + CFG/guidance mechanism, VAE scale/shift, any post-decoding processing. + +3. **Decide what to reuse.** Before creating any file, grep existing pipelines: + - Does an existing family use the same text encoder? (CLIP, T5, Gemma, ...) + - Is the VAE identical to an existing one? Reuse `AutoencoderKL` / + `WanVAE` / `LTX2VAE` / etc. where possible. + - Does an existing `DenoisingStage` (or its subclasses: + `CosmosDenoisingStage`, `LTX2DenoisingStage`, `DmdDenoisingStage`, + `CausalDenoisingStage`, ...) match the denoising math? If 80%+ of a + step matches, subclass rather than duplicate. + - If the reference implementation is already in + [SGLang's multimodal stack](https://github.com/sgl-project/sglang/tree/main/python/sglang/multimodal_gen), + much of the logic can be ported (interfaces align with FastVideo's). + - Record the reuse decisions in the PR description. + +4. **Convert weights to Diffusers format — only if the HF path is not + already Diffusers-format.** See the + [Weight conversion](#weight-conversion) section below for the detailed + recipe. Summary decision tree: + - HF repo has `model_index.json` and per-component subdirs → + **skip conversion**, download via `scripts/huggingface/download_hf.py`. + You still need `param_names_mapping` in the DiT config. + - HF repo / official release has a monolithic `.safetensors` (or `.pt`) + with no `model_index.json` → **write a conversion script** under + `scripts/checkpoint_conversion/convert__to_diffusers.py` (or + `_to_diffusers.py`), modeled on the closest existing script. + Stage the output under `converted_weights//`. Optionally + publish to HF via `create_hf_repo.py`. + - Unsure → inspect the HF path's file listing before deciding: + + ```bash + python - <<'PY' + from huggingface_hub import list_repo_files + for f in sorted(list_repo_files("Org/Model")): + print(f) + PY + ``` + + Presence of `model_index.json` at the root means Diffusers-format. + +5. **Clone the official reference repo locally (for parity testing).** + - Existing parity tests under `tests/local_tests/` (see e.g. + `tests/local_tests/transformers/test_ltx2.py:13-16`) expect the + reference repo to be checked out **at the FastVideo repo root** + (`FastVideo/LTX-2/`, `FastVideo/gen3c/`, …). They push its module + path onto `sys.path` and skip if the directory is absent. + - Clone under the repo root, for example: + + ```bash + git clone --depth 1 + ``` + + - Add the clone to `.gitignore` so it's not accidentally committed: + + ```bash + echo "//" >> .gitignore + ``` + + - Install its dependencies **inside the same FastVideo environment** — + do not create a new venv, do not let it override FastVideo-pinned + versions. Typical recipe: + + ```bash + uv pip install --no-deps -e ./ # if editable + uv pip install -r .//requirements.txt --no-deps + ``` + + Use `--no-deps` first so you can review what extra transitive deps + the official repo wants. If any conflict with FastVideo's pins (torch, + transformers, diffusers, flash-attn, triton), stop and ask the user + before changing versions — a mismatched torch is the single most + common source of phantom parity failures. + - If the official implementation is already reachable on PyPI (e.g. the + Diffusers library itself), skip the clone and `pip install` the pinned + version instead. + - This directory is **temporary**. Step 14 removes it after SSIM refs + are seeded. + +6. **Port each component in parallel using subagents.** This is where the + bulk of the implementation happens. Launch one subagent per component + (DiT, VAE, text encoder(s), any extras — audio_vae, vocoder, + image_encoder, upsampler, …) in a **single message with multiple Agent + tool calls** so they run concurrently. See the + [Parallel component porting](#parallel-component-porting) section below + for the exact subagent prompt template and the + [Parity test pattern](#parity-test-pattern) section for the test + template each subagent must produce. + + Each subagent is responsible for producing, for its one component: + - The FastVideo model class under `fastvideo/models//.py` + (where `` is `dits`, `vaes`, `encoders`, `schedulers`, or + `upsamplers`). + - The config class under `fastvideo/configs/models//.py`, + subclassing the bucket's base config and defining + `param_names_mapping` (regex map from official keys → FastVideo keys). + - An export entry in the bucket's `__init__.py`. + - A component parity test under `tests/local_tests//test__*.py` + that loads FastVideo and official models in-process, runs them on + identical inputs, and asserts closeness with `torch.testing.assert_close`. + - Hard rule: **reuse `fastvideo/layers/` and `fastvideo/attention/` + primitives.** Do not introduce raw `nn.Linear`, raw SDPA, or hand-rolled + RMSNorm/RoPE. See the [FastVideo layers and attention](#fastvideo-layers-and-attention) + section for the catalog + selection rules. + + The main agent's job during this step is: + - Draft the per-component subagent prompts (one each), reference the + closest existing component file, and dispatch them in parallel. + - As each subagent returns, inspect the diff (do not trust the + summary). Verify the parity test actually compares forward outputs + (not just weight shapes) and that `assert_close` tolerances are + sensible (`atol=1e-4, rtol=1e-4` default; loosen deliberately, never + silently). + - Collect unresolved `param_names_mapping` mismatches; resolve them + centrally before moving on. + - Open a **DRAFT PR** once the DiT is parity-clean. Remaining + components can land incrementally. + +7. **Create the `PipelineConfig`.** + - For one-off families, the file can live in + `fastvideo/configs/pipelines/.py`. For families that introduce + multiple related configs (like LTX-2), use + `fastvideo/pipelines/basic//pipeline_configs.py`. Both + patterns exist — pick the one that matches the family you are closest + to structurally. + - Subclass `PipelineConfig` (see + `fastvideo/configs/pipelines/base.py`). Wire `dit_config`, + `vae_config`, `text_encoder_configs`, precisions, `flow_shift`, + `boundary_ratio`, `vae_tiling`, etc. Override `__post_init__` only to + toggle `load_encoder` / `load_decoder` or adjust encoder + `output_hidden_states` for downstream needs. + +8. **Build or pick the stages.** + - Start by trying to compose only standard stages from + `fastvideo/pipelines/stages/`. If that works, skip to step 9. + - If a step diverges, subclass the matching standard stage and put it + under `fastvideo/pipelines/basic//stages/.py`. + - Keep overrides small: a new stage usually just wraps a different + scheduler math path, an extra conditioning kwarg, or a different + latent packing. + - If you add new stage classes, re-export them from + `fastvideo/pipelines/basic//stages/__init__.py` and add the + imports to `fastvideo/pipelines/stages/__init__.py` so downstream + code can still do `from fastvideo.pipelines.stages import ...`. + +9. **Write the pipeline class.** + + ```python + # fastvideo/pipelines/basic//_pipeline.py + from fastvideo.fastvideo_args import FastVideoArgs + from fastvideo.pipelines import ComposedPipelineBase, LoRAPipeline + from fastvideo.pipelines.stages import ( + DecodingStage, DenoisingStage, InputValidationStage, + LatentPreparationStage, TextEncodingStage, TimestepPreparationStage, + ) + + + class FooVideoPipeline(LoRAPipeline, ComposedPipelineBase): # drop LoRAPipeline if unsupported + _required_config_modules = [ + "text_encoder", "tokenizer", "vae", "transformer", "scheduler", + ] + + def initialize_pipeline(self, args: FastVideoArgs) -> None: + # Optional hook: swap scheduler, preload custom modules, etc. + ... + + def create_pipeline_stages(self, args: FastVideoArgs) -> None: + self.add_stage(stage_name="input_validation_stage", stage=InputValidationStage()) + self.add_stage( + stage_name="prompt_encoding_stage", + stage=TextEncodingStage( + text_encoders=[self.get_module("text_encoder")], + tokenizers=[self.get_module("tokenizer")], + ), + ) + self.add_stage(stage_name="timestep_preparation_stage", + stage=TimestepPreparationStage(scheduler=self.get_module("scheduler"))) + self.add_stage(stage_name="latent_preparation_stage", + stage=LatentPreparationStage(scheduler=self.get_module("scheduler"), + transformer=self.get_module("transformer", None))) + self.add_stage(stage_name="denoising_stage", + stage=DenoisingStage(transformer=self.get_module("transformer"), + scheduler=self.get_module("scheduler"), + vae=self.get_module("vae"), + pipeline=self)) + self.add_stage(stage_name="decoding_stage", + stage=DecodingStage(vae=self.get_module("vae"), pipeline=self)) + + + EntryClass = FooVideoPipeline # single class, NOT a list + ``` + + - Module-level `EntryClass` is how `fastvideo/pipelines/pipeline_registry.py` + discovers pipelines. No manual pipeline registration required. + - `_required_config_modules` must list every key the loader will read from + `model_index.json`. Missing keys cause silent degradation. + +10. **Define presets.** + + ```python + # fastvideo/pipelines/basic//presets.py + from fastvideo.api.presets import InferencePreset, PresetStageSpec + + _DENOISE_STAGE = PresetStageSpec( + name="denoise", + kind="denoising", + description="Main denoising pass", + allowed_overrides=frozenset({"num_inference_steps", "guidance_scale"}), + ) + + FOO_BASE = InferencePreset( + name="foovideo_base", + version=1, + model_family="foovideo", + description="FooVideo base at 512x768", + workload_type="t2v", + stage_schemas=(_DENOISE_STAGE,), + defaults={ + "seed": 0, + "height": 512, + "width": 768, + "num_frames": 121, + "fps": 24, + "guidance_scale": 3.0, + "num_inference_steps": 40, + "negative_prompt": "", + }, + ) + + ALL_PRESETS = (FOO_BASE,) + ``` + + `ALL_PRESETS` is the export expected by `_register_presets()`. Every + preset listed there becomes reachable via + `SamplingParam.from_pretrained(model_path)` or the public preset API. + Multi-stage pipelines (e.g. LTX-2's two-stage refine) declare multiple + `PresetStageSpec` entries and provide `stage_defaults`. + +11. **Register in `fastvideo/registry.py`.** Two edits required: + + a. Add a `register_configs(...)` block inside `_register_configs()`: + + ```python + register_configs( + sampling_param_cls=None, # use the default SamplingParam unless you subclass it + pipeline_config_cls=FooVideoT2VConfig, + workload_types=(WorkloadType.T2V,), + hf_model_paths=[ + "Org/FooVideo-Diffusers", + ], + model_detectors=[ + lambda path: "foovideo" in path.lower(), + ], + model_family="foovideo", + default_preset="foovideo_base", + ) + ``` + + b. Inside `_register_presets()`, import the family's presets and add + them to `all_preset_groups`: + + ```python + from fastvideo.pipelines.basic.foovideo.presets import ( + ALL_PRESETS as FOOVIDEO_PRESETS, + ) + # ... + all_preset_groups = ( + ..., + FOOVIDEO_PRESETS, + ) + ``` + + Discovery order during `_get_config_info`: exact HF path → short name → + detector fallback (which also matches against the `_class_name` pulled + from `model_index.json`). Make sure your detector accepts both the HF + path and the pipeline class name. + +12. **Smoke-test the pipeline.** + + ```python + from fastvideo import VideoGenerator + from fastvideo.api.sampling_param import SamplingParam + + gen = VideoGenerator.from_pretrained("Org/FooVideo-Diffusers", num_gpus=1) + sp = SamplingParam.from_pretrained("Org/FooVideo-Diffusers") + video = gen.generate_video( + "A vibrant city street at sunset.", + sampling_param=sp, + output_path="video_samples", + save_video=True, + ) + ``` + + Add this as `tests/local_tests/pipelines/test__pipeline_smoke.py` + mirroring `tests/local_tests/pipelines/test_ltx2_pipeline_smoke.py`. + +13. **Full-pipeline parity + user-runnable example (gated; iterate until + green before any user hand-off).** This is the final verification + gate. Do **not** tell the user the port is ready until step (a) passes. + + **(a) Pipeline parity test — mandatory, iterate until green.** Write + `tests/local_tests/pipelines/test__pipeline_parity.py` modeled + on `tests/local_tests/pipelines/test_gamecraft_pipeline_parity.py`. + The test must push the cloned official repo onto `sys.path` and + import its pipeline (matching the pattern used by the component parity + tests, see [Parity test pattern](#parity-test-pattern)); instantiate + both pipelines in the same process with matched weights, prompts, + seed, resolution, and `num_inference_steps`; compare denoised latents + (faster than decoded video) with `torch.testing.assert_close` starting + at `atol=1e-3, rtol=1e-3` and chasing down the first divergent step + before loosening bounds; set `DISABLE_SP=1` / + `FASTVIDEO_ATTENTION_BACKEND=TORCH_SDPA` in env defaults to match the + official backend; and `pytest.skip` cleanly when the official clone + or converted weights are missing — never fail. + Run with `DISABLE_SP=1 pytest -v -s tests/local_tests/pipelines/test__pipeline_parity.py`. + If it fails, iterate — the most common causes are sigma / timestep + schedule drift (first denoising step diverges), VAE scale/shift wrong + in `PipelineConfig`, CFG kwargs not matching the DiT's `forward()` + signature, rotary embedding style (neox vs interleaved) flipped, + prompt embeds passed as tensor where the DiT expects a list (or vice + versa), or `vae_precision` causing overflow on long sequences. Do not + move to (b) until this test passes with a non-skip pass. + + **(b) Create the user-facing example script.** Once (a) is green, add + `examples/inference/basic/basic_.py` modeled on the closest + existing example (e.g. `basic_ltx2.py`, `basic_wan2_2.py`, + `basic_longcat_t2v.py`). Keep it minimal and deterministic: import + `VideoGenerator`, call `from_pretrained(, num_gpus=1)`, invoke + `generate_video(prompt=..., output_path=..., save_video=True)`, then + `shutdown()`. Leave sampling params blank to use the registered preset; + override only when a specific resolution / frame count is meaningful + for manual QA. Run the example once yourself to confirm it writes a + non-corrupted mp4. Do not tune sampling params beyond the preset + unless the preset defaults are clearly wrong — in that case, fix the + preset, not the example. + + **(c) Only now, ask the user to manually verify quality.** Point + them at `examples/inference/basic/basic_.py` and the passing + pipeline parity test output. The pipeline parity test is the + objective gate; the example script is for the user's subjective + eyeball check. + +14. **Add SSIM regression + seed references.** + - Copy `fastvideo/tests/ssim/test_ltx2_similarity.py` and adapt for the + new model: parametrize prompts, attention backends, and model_ids; + set `min_acceptable_ssim` (start at 0.60 unless the model is known + to be stable). + - The first run will fail on missing reference videos — that is + expected. Hand off to the + [`seed-ssim-references`](../seed-ssim-references/SKILL.md) skill to + generate the refs on Modal L40S and upload them to + `FastVideo/ssim-reference-videos`. + +15. **Clean up the locally cloned official repo.** Once the DiT, VAE, + encoder, and pipeline parity tests are all green (or have been + converted into permanent, skip-on-missing tests), remove the + `FastVideo//` clone introduced in step 5: + + ```bash + rm -rf + git check-ignore -v || true # confirm it was gitignored + ``` + + Leave the `.gitignore` line in place so future parity runs (which + re-clone on demand) stay untracked. If your parity tests need the + clone permanently, instead document the expected directory name in + `tests/local_tests/README.md` and leave the clone untouched — but + still never commit it. + +16. **Ask about tests and performance data.** Once quality is verified and + SSIM is green, ask the user whether to add: + - Additional unit/integration tests (per-component parity tests under + `tests/local_tests//`). + - Performance benchmarks (single-GPU latency, TP/SP scaling numbers). + + Do not skip — the scope varies per model and per contributor goal. + +## Standard stages — subclass targets + +All live under `fastvideo/pipelines/stages/` unless noted. When an existing +subclass is listed, that is a strong signal others have hit the same +divergence — read it before writing your own. + +| Stage | File | Existing subclasses | +|-------|------|---------------------| +| `PipelineStage` (base) | `base.py` | — | +| `InputValidationStage` | `input_validation.py` | (shared, rarely subclassed) | +| `TextEncodingStage` | `text_encoding.py` | `Cosmos25TextEncodingStage`, `LTX2TextEncodingStage` | +| `ImageEncodingStage` et al. | `image_encoding.py`, `gamecraft_image_encoding.py` | `MatrixGame...`, `Hy15...`, `HYWorld...`, `RefImage...`, `ImageVAE...`, `VideoVAE...`, `GameCraftImageVAE...` | +| `ConditioningStage` | `conditioning.py` | `Gen3CConditioningStage`, `SD35ConditioningStage` | +| `TimestepPreparationStage` | `timestep_preparation.py` | `Cosmos25TimestepPreparationStage` | +| `LatentPreparationStage` | `latent_preparation.py` | `CosmosLatentPreparationStage`, `Cosmos25...`, `Gen3CLatentPreparationStage`, `LTX2LatentPreparationStage` | +| `DenoisingStage` | `denoising.py` | `CosmosDenoisingStage`, `Cosmos25(...)DenoisingStage`, `DmdDenoisingStage`, plus causal (`causal_denoising.py`), `MatrixGameCausalDenoisingStage`, `HYWorldDenoisingStage`, `GameCraftDenoisingStage`, `Gen3CDenoisingStage`, `LTX2DenoisingStage`, `SRDenoisingStage`, `LongCatVCDenoisingStage` | +| `DecodingStage` | `decoding.py` | `LTX2AudioDecodingStage` | +| `EncodingStage` | `encoding.py` | — | + +## FastVideo layers and attention + +Every component you port **must** build on the primitives in +`fastvideo/layers/` and `fastvideo/attention/`. Do not introduce raw +`torch.nn.Linear` in hot paths, hand-rolled RMSNorm, custom SDPA calls, +or ad-hoc RoPE implementations. The shared primitives handle: attention +backend selection, TP/SP sharding, torch.compile compatibility, +bf16/fp32 dispatch, and quantization. Bypassing them breaks all of those +invariants silently. + +### Linear layers (`fastvideo/layers/linear.py`) + +FastVideo has two linear regimes, and mixing them up is a common porting +mistake. **Verified across every DiT in `fastvideo/models/dits/` as of +2026-04-24:** + +- **DiTs and VAEs use `ReplicatedLinear` everywhere** — QKV projections, + output projection, MLP up/down projections, adaLN modulation heads, + timestep/context embedders, patch projection. There is **zero** usage + of `ColumnParallelLinear` / `RowParallelLinear` / `QKVParallelLinear` / + `MergedColumnParallelLinear` in any DiT or VAE. Multi-GPU scaling in + DiTs comes from **sequence parallelism via `DistributedAttention`**, + not from TP-sharding the linear weights. +- **LLM-style text encoders use TP linears.** Files like + `fastvideo/models/encoders/llama.py`, `qwen2_5.py`, `siglip.py`, and + `t5.py` shard their QKV / MLP across TP ranks using + `QKVParallelLinear`, `MergedColumnParallelLinear`, and + `RowParallelLinear`. Copy the pattern from these files when your new + text encoder is LLM-structured. + +Summary rule: + +| Component bucket | Default linear | Why | +|------------------|---------------|-----| +| DiT (`fastvideo/models/dits/`) | `ReplicatedLinear` everywhere. | DiTs scale via SP on the long spatio-temporal sequence, not via TP on linear weights. `DistributedAttention` handles the sequence redistribution. | +| VAE (`fastvideo/models/vaes/`) | `ReplicatedLinear` (plus `nn.Conv*` / conv primitives). | Same reason — no TP in VAEs. | +| LLM-style text encoder (`fastvideo/models/encoders/`) | `QKVParallelLinear` for QKV, `MergedColumnParallelLinear` for gate/up MLP, `RowParallelLinear` for output/down projections. | Proven win on transformer encoders; existing encoders (llama, qwen2_5, siglip, t5) already do this. | +| Small pre/post heads inside any bucket | `ReplicatedLinear`. | Too small for TP to help. | + +Why `ReplicatedLinear` instead of raw `nn.Linear` in DiTs/VAEs? It plugs +into the quantization and weight-loading stacks the same way the TP +variants do, so swapping it in later (if TP ever makes sense for a +specific DiT) is a one-line change rather than a rewrite. A handful of +legacy `nn.Linear` calls remain in some DiT files (e.g. `proj_out`) — +treat those as legacy, not as a pattern to copy. For new code, default +to `ReplicatedLinear`. + +Reference reading before writing any linear layer: + +| Reading for… | Open | +|--------------|------| +| DiT linear pattern | `fastvideo/models/dits/wanvideo.py`, `fastvideo/models/dits/ltx2.py`, `fastvideo/models/dits/hunyuanvideo.py` — all `ReplicatedLinear` throughout. | +| LLM text encoder TP pattern | `fastvideo/models/encoders/llama.py:39-141`, `fastvideo/models/encoders/qwen2_5.py`. | +| SigLIP vision-encoder TP pattern | `fastvideo/models/encoders/siglip.py:96-168`. | +| T5 encoder TP pattern | `fastvideo/models/encoders/t5.py`. | + +### Attention layers (`fastvideo/attention/layer.py`) + +There are **three** attention layers. Picking the wrong one is one of +the most common porting bugs — it parity-passes on single GPU and then +breaks silently under SP. + +| Class | Use for | Do not use for | +|-------|---------|----------------| +| `DistributedAttention` | The DiT's **full-sequence self-attention** layers — the long spatial/temporal sequences that get SP-sharded. Handles all-to-all head/seq redistribution, RoPE application, padding trim/pad. | Cross-attention. Short-sequence attention inside encoders. | +| `DistributedAttention_VSA` | Video Sparse Attention variant of the above — only when the reference model uses VSA-style attention. | Dense attention. | +| `LocalAttention` | Cross-attention blocks, short-seq attention inside text/image encoders, and any attention that should **not** be SP-sharded. | Full-sequence DiT self-attention (it will recompute full attention on every rank). | + +Rule of thumb: +- "Does this attention need to see the whole spatio-temporal sequence, + and will that sequence be sharded across SP ranks?" → `DistributedAttention`. +- Everything else (cross-attn, encoder self-attn on small token counts, + fusion heads) → `LocalAttention`. + +See `fastvideo/models/dits/wanvideo.py` and +`fastvideo/models/dits/ltx2.py` for canonical usage. Do not reach down +into the backend directly via `fastvideo/attention/selector.py` from a +model file — always go through `DistributedAttention` / `LocalAttention`. + +### Other layer primitives + +| Module | Use for | +|--------|---------| +| `fastvideo/layers/layernorm.py` — `RMSNorm` | Canonical RMSNorm with optional fused scale/shift. | +| `fastvideo/layers/layernorm.py` — `FP32LayerNorm` | When the reference model forces fp32 LayerNorm (common at modulation boundaries). | +| `fastvideo/layers/layernorm.py` — `ScaleResidual`, `ScaleResidualLayerNormScaleShift`, `LayerNormScaleShift` | Fused scale/residual/shift combinations used by most DiT blocks. Prefer over manual `x = x + alpha * residual; norm(x)`. | +| `fastvideo/layers/activation.py` — `SiluAndMul`, `GeluAndMul`, `NewGELU`, `QuickGELU` | Fused activation + multiply for gated MLPs. | +| `fastvideo/layers/mlp.py` — `MLP` | Generic 2-layer MLP; prefer this over writing one inline. | +| `fastvideo/layers/rotary_embedding.py` — `RotaryEmbedding`, `_apply_rotary_emb` | Standard RoPE (1D). Respect the `is_neox_style` flag — neox = split-half, non-neox = interleaved. | +| `fastvideo/layers/rotary_embedding_3d.py` — `RotaryPositionalEmbedding3D` | 3D RoPE used by video DiTs. | +| `fastvideo/layers/visual_embedding.py` — `PatchEmbed`, `TimestepEmbedder`, `ModulateProjection`, `Timesteps` | Standard patch + timestep + modulation embeds. | +| `fastvideo/layers/vocab_parallel_embedding.py` — `VocabParallelEmbedding` | Shardable embedding tables (text encoders). | + +### Quick reuse audit before any new layer + +When a subagent (or you) is about to write a new nn.Module, check in +this order: + +1. Is it an attention? → pick from the attention table above. +2. Is it a Linear? → pick from the linear table above. +3. Is it a norm, activation, RoPE, or embedding? → check the layer + primitive table; re-export if needed. +4. Is it a block/module composition (e.g. a DiT transformer block)? → + grep existing DiTs (`wanvideo.py`, `ltx2.py`, `hunyuanvideo.py`, + `cosmos.py`) for the same pattern and subclass/inline adapt. +5. None of the above? Only then, implement custom and document why. + +## Parity test pattern + +Every component subagent's output must include a parity test under +`tests/local_tests//` that compares FastVideo against the +locally-cloned official reference. The pattern below is distilled from +`tests/local_tests/transformers/test_ltx2.py`, +`tests/local_tests/vaes/test_ltx2_vae.py`, and +`tests/local_tests/encoders/test_ltx2_gemma_parity.py`. + +### Conventions (all parity tests share these) + +- **Repo-root resolution**: + + ```python + from pathlib import Path + import sys + repo_root = Path(__file__).resolve().parents[3] + official_src = repo_root / "" / "src" # adjust to the repo's layout + if official_src.exists() and str(official_src) not in sys.path: + sys.path.insert(0, str(official_src)) + ``` + + Tests under `tests/local_tests//test_*.py` live three levels + below repo root — `.parents[3]` resolves correctly. Push the reference + package's source path onto `sys.path` *before* importing from it. + +- **Skip-on-missing, never fail-on-missing.** If the clone, converted + weights, or official safetensors are absent, `pytest.skip(...)` so + CI does not have to carry the reference code: + + ```python + if not official_path.exists(): + pytest.skip(f"Official weights not found at {official_path}") + ``` + +- **Paths via env vars with sensible defaults** — lets different + contributors run the same test with different local layouts: + + ```python + official_path = Path(os.getenv( + "FOOVIDEO_OFFICIAL_PATH", + "official_weights/foovideo/foovideo-v1.safetensors", + )) + converted_path = Path(os.getenv( + "FOOVIDEO_DIFFUSERS_PATH", + "converted_weights/foovideo", + )) + ``` + +- **Load matched weights into both models.** Use the conversion + script's mapping so the same tensor content lands in both models; a + freshly-initialized FastVideo model compared against a pre-trained + official model is not a parity test. + +- **Identical inputs**, fixed seed: + + ```python + torch.manual_seed(0) + x = torch.randn(batch, ..., dtype=precision, device=device) + ``` + +- **`torch.testing.assert_close(fastvideo_out, official_out, atol=1e-4, rtol=1e-4)`** + as the default tolerance. If the test needs looser bounds, write a + comment explaining *why* (e.g. "flash-attn fp16 accumulation drift — + aligned with TORCH_SDPA shows 1e-5"). Never loosen silently. + +- **Align attention backend** to avoid ghost drift: + + ```python + os.environ.setdefault("FASTVIDEO_ATTENTION_BACKEND", "TORCH_SDPA") + ``` + +- **Debug logging hooks** — `tests/local_tests/transformers/test_ltx2.py` + shows the per-block forward-hook pattern that logs activation sums to + a file. Copy it when the scalar assertion fails and you need to locate + the first divergent layer. + +### Pipeline-level parity test (the gate for user handoff) + +Per step 13(a), every ported family must ship a pipeline-level parity +test at `tests/local_tests/pipelines/test__pipeline_parity.py`. +This is the **final objective gate** before telling the user to eyeball +the example script — if this test is red, the port is not done. + +Use `tests/local_tests/pipelines/test_gamecraft_pipeline_parity.py` as +the template. Key differences from component tests: + +- Compares on **denoised latents**, not decoded video — decoding is + expensive and adds VAE error on top. Run the denoising loop in both + pipelines with matched `num_inference_steps`, snapshot the latent at + the end of the loop, and compare. +- Tolerances are looser by construction (the denoising loop compounds + tiny per-step deltas). Start at `atol=1e-3, rtol=1e-3`; if you need + looser, first log per-step latent sums from both pipelines to prove + divergence is bounded and steady (not runaway). +- Set `DISABLE_SP=1` in env defaults to avoid cross-pipeline SP + disagreement when running on multi-GPU hosts. +- `pytest.skip` if the official clone is missing, if converted weights + are missing, or if GPU memory is below what the pipeline needs. + +### Component-specific template skeleton + +```python +# tests/local_tests//test__parity.py +# SPDX-License-Identifier: Apache-2.0 +import os +import sys +from pathlib import Path + +import pytest +import torch +from torch.testing import assert_close + +os.environ.setdefault("MASTER_ADDR", "localhost") +os.environ.setdefault("MASTER_PORT", "29513") +os.environ.setdefault("FASTVIDEO_ATTENTION_BACKEND", "TORCH_SDPA") + +repo_root = Path(__file__).resolve().parents[3] +official_src = repo_root / "" +if official_src.exists() and str(official_src) not in sys.path: + sys.path.insert(0, str(official_src)) + +from fastvideo.models.. import FooVideoComponent +from fastvideo.configs.models. import FooVideoComponentConfig + + +def test_foovideo__parity(): + official_weights = Path(os.getenv( + "FOOVIDEO_OFFICIAL_PATH", + "official_weights/foovideo/foovideo-v1.safetensors", + )) + converted_weights = Path(os.getenv( + "FOOVIDEO_DIFFUSERS_PATH", + "converted_weights/foovideo", + )) + if not official_weights.exists(): + pytest.skip(f"Official weights missing: {official_weights}") + if not converted_weights.exists(): + pytest.skip(f"Converted weights missing: {converted_weights}") + try: + from foovideo_official. import OfficialFooComponent + except ImportError as exc: + pytest.skip(f"Official reference not importable: {exc}") + + device = torch.device("cuda:0" if torch.cuda.is_available() else "cpu") + dtype = torch.bfloat16 if torch.cuda.is_available() else torch.float32 + + # Build both models + config = FooVideoComponentConfig() + fv_model = FooVideoComponent(config).to(device=device, dtype=dtype).eval() + ref_model = OfficialFooComponent().to(device=device, dtype=dtype).eval() + + # Load matched weights + # ... use the conversion mapping to route tensors into both models ... + + # Identical inputs + torch.manual_seed(0) + inputs = + + with torch.no_grad(): + fv_out = fv_model(**inputs) + ref_out = ref_model(**inputs) + + assert_close(fv_out, ref_out, atol=1e-4, rtol=1e-4) +``` + +## Parallel component porting + +Step 6 launches one subagent per component in parallel. They have no +shared state, so each prompt must be fully self-contained — the agent +doesn't see this conversation. + +### Dispatch pattern + +Send a **single message with multiple `Agent` tool calls**. Typical +component list: + +- DiT (`bucket=dits`) +- VAE (`bucket=vaes`) +- Text encoder(s) (`bucket=encoders`) — one per distinct encoder +- Any extras: audio VAE, vocoder, image encoder, upsampler, scheduler + +Skip components that step 3 ("decide what to reuse") marked as reused. + +### Subagent prompt template + +``` +Port the component of into FastVideo. + +Context: + - Official reference repo is cloned at FastVideo//. + Relevant source: . + - Converted (Diffusers-format) weights are at + converted_weights///. + - Closest existing FastVideo component to model on: + fastvideo/models//.py + fastvideo/configs/models//.py + - Closest existing parity test to model on: + tests/local_tests//test__parity.py + +Hard requirements: + - Reuse primitives from fastvideo/layers/ and fastvideo/attention/. + Do NOT introduce raw nn.Linear in hot paths, hand-rolled RMSNorm, + custom SDPA, or ad-hoc RoPE. Selection rules (verified against + every existing DiT / VAE / encoder in the repo): + * Attention: + - DiT full-sequence self-attention → DistributedAttention + - DiT Video-Sparse-Attention variant → DistributedAttention_VSA + - Cross-attention, encoder self-attention, any short-seq attn + → LocalAttention + * Linear layers: + - If this is a DiT or a VAE → ReplicatedLinear everywhere + (QKV, output proj, MLP, adaLN heads, embedders). No + ColumnParallel/RowParallel/QKVParallel here — DiTs scale via + SP inside DistributedAttention, not TP. + - If this is an LLM-style text encoder → QKVParallelLinear for + QKV, MergedColumnParallelLinear for gate/up MLP, + RowParallelLinear for output/down projections (mirror + fastvideo/models/encoders/llama.py, qwen2_5.py, siglip.py, + t5.py). + * Use fused norms/activations from fastvideo/layers/{layernorm, + activation,mlp,visual_embedding,rotary_embedding{,_3d}}.py. + - Name parameters to minimize the regex map in + fastvideo/configs/models//.py :: param_names_mapping. + - Keep torch.compile-compatible: no Python-side control flow that + branches on tensor values. + +Deliverables (in order): + 1. fastvideo/models//.py — the model class. + 2. fastvideo/configs/models//.py — config + regex map. + 3. Add export in fastvideo/configs/models//__init__.py. + 4. tests/local_tests//test__parity.py — loads + FastVideo and official in-process, asserts closeness with + torch.testing.assert_close (atol=1e-4, rtol=1e-4 default). + +Validation before you return: + - Run: `pytest tests/local_tests//test__parity.py -vs` + - The test must produce a non-skip pass (not xfail, not skip). If it + legitimately must skip (e.g. weights missing), explain which env var + must be set. + - Report any unresolved param_names_mapping mismatches verbatim. + +Do NOT: + - Touch the pipeline class, presets, registry, or any other component. + - Create new files outside the four listed above. + - Loosen atol/rtol silently. If a looser tolerance is needed, add a + comment explaining why and quantifying the drift. +``` + +Notes for the main agent: + +- Give each subagent only its component's scope. Don't bundle (e.g. "do + the DiT and the VAE") — subagents perform worse as scope expands. +- Run them concurrently — **one message with N Agent calls** — they + have no dependencies on each other. +- When each returns, verify by reading the diff, not by trusting the + summary. +- Consolidate shared issues centrally (e.g. if every subagent hit a + missing `param_names_mapping` rule that comes from the conversion + script, fix the conversion, not each component). + +## Weight conversion + +When the source weights are **not** in Diffusers format (no +`model_index.json`, single monolithic checkpoint, or custom layout from the +official repo), you must convert them before FastVideo can load the model. +All conversion scripts live under `scripts/checkpoint_conversion/`. + +### Decide whether conversion is needed + +| Source layout | Action | +|---------------|--------| +| HF repo with `model_index.json` + `transformer/`, `vae/`, `text_encoder/` subdirs, each with `config.json` + `.safetensors` | **No conversion.** Download via `scripts/huggingface/download_hf.py`, rely on `param_names_mapping` in the DiT config to reconcile any module-name drift. | +| HF repo or release with one big `.safetensors` (all components fused, e.g. LTX-2 `ltx-2-19b-dev.safetensors`) | **Write a conversion script.** Split by component prefix, write per-component directories, add a synthetic `model_index.json`. | +| Official `.pt` / `.ckpt` files | First convert to `.safetensors` with `scripts/checkpoint_conversion/pt_to_safetensors.py`, then treat as the previous row. | +| Component weights scattered across separate files with non-Diffusers prefixes | Write a conversion script that loads each, remaps, and lays them out in Diffusers form. See `convert_gen3c_to_fastvideo.py` for a dense example. | + +### Conversion script conventions + +All scripts in `scripts/checkpoint_conversion/` follow the same shape: + +- A CLI with `argparse`, typically: + - `--source` — path to the official `.safetensors` file or directory. + - `--output` — target directory, by convention + `converted_weights//`. + - `--class-name` — the Diffusers DiT class name that will appear in + `model_index.json` (e.g. `"LTX2Transformer3DModel"`). + - `--pipeline-class-name` — the pipeline class name that FastVideo will + match on (e.g. `"LTX2Pipeline"`). + - `--diffusers-version` — version string baked into the output config. + - Component-specific flags (e.g. `--gemma-path` for LTX-2, image-encoder + paths, VAE paths, etc.). +- A `PARAM_NAME_MAP: dict[str, str]` of regex patterns: official key → + FastVideo/Diffusers key. Applied in-order via `re.sub`. +- A `COMPONENT_PREFIXES: dict[str, tuple[str, ...]]` that groups keys by + destination subdir (`transformer`, `vae`, `audio_vae`, `vocoder`, + `text_encoder`, …). Each key in the final state dict is routed into + `//diffusion_pytorch_model.safetensors`. +- Per-component `config.json` generation so `from_pretrained` can load each + piece without the original repo. +- A top-level `model_index.json` that pins the `_class_name`, + `_diffusers_version`, and the list of component sub-paths. + +### Reference scripts — pick the closest one and copy its shape + +| Script | When to model on it | +|--------|--------------------| +| `convert_ltx2_weights.py` | **Default starting point** for a new model. Modern pattern: shard-safe loading, regex mapping, component splitting (`transformer`/`vae`/`audio_vae`/`vocoder`), synthesized `model_index.json`, optional extra text-encoder side-load. 484 lines, clean. | +| `wan_to_diffusers.py` | **Legacy pattern** — no argparse, just a module of regex dicts (`_param_names_mapping`, `_self_forcing_to_diffusers_param_names_mapping`). Useful as a reference for *how to write regex rules* for self-/cross-attention, FFN, modulation, and image-embedding projections, and for handling variant checkpoints (`self_forcing_`, etc.). **Do not copy the module shape** — follow the `convert_ltx2_weights.py` CLI template instead. | +| `convert_turbodiffusion_to_diffusers.py` / `convert_turbodiffusion_i2v_to_diffusers.py` | Shows T2V vs I2V variants sharing most logic with small deltas — copy when you're adding I2V support for an existing family. | +| `convert_gamecraft_weights.py` + `convert_gamecraft_vae.py` + `convert_gamecraft_full.py` | Pattern for families where the DiT, VAE, and end-to-end conversion live in separate scripts. Use when a single family needs piecewise conversion (different source formats per component). | +| `convert_gen3c_to_fastvideo.py` | Heaviest example (724 lines). Use as reference when the model has many components and non-trivial per-component logic; skim rather than copy wholesale. | +| `longcat_to_fastvideo.py` | Similar: large, component-heavy. Pair with `validate_longcat_weights.py` for the sanity-check pattern. | +| `extract_llava_text_encoder.py` | Template for pulling a single component out of a larger multimodal checkpoint (here, the text encoder half of a LLaVA checkpoint). | +| `pt_to_safetensors.py` | Utility — convert legacy `.pt` / `.ckpt` to `.safetensors` before running any of the above. | + +### Conversion recipe + +1. **Inspect source keys** before touching code: + + ```bash + python - <<'PY' + import safetensors.torch as st + keys = list(st.load_file( + "official_weights//.safetensors" + ).keys()) + print(len(keys), "keys") + for k in keys[:40]: + print(k) + PY + ``` + +2. **Pick the closest reference script** from the table above. +3. **Copy it to** `scripts/checkpoint_conversion/_to_diffusers.py` + and rename the family identifier throughout. +4. **Edit `PARAM_NAME_MAP`** — replace with the regex rules that translate + the official keys to FastVideo / Diffusers naming. Rules apply in + iteration order; put more-specific patterns first. +5. **Edit `COMPONENT_PREFIXES`** — list every component your pipeline needs + (transformer, vae, text_encoder, audio_vae, vocoder, upsampler, …) with + the set of official prefixes that route into each. +6. **Run the conversion** into `converted_weights//`: + + ```bash + python scripts/checkpoint_conversion/_to_diffusers.py \ + --source official_weights//.safetensors \ + --output converted_weights/ \ + --class-name \ + --pipeline-class-name \ + --diffusers-version "0.33.0" + ``` + +7. **Validate** the output layout: + + ```bash + ls converted_weights/ + # model_index.json + # transformer/{config.json, diffusion_pytorch_model.safetensors} + # vae/{config.json, diffusion_pytorch_model.safetensors} + # text_encoder/{config.json, ...} + ``` + +8. **Parity-sanity-check converted weights.** Load one component both ways + and diff a tensor sum. See `validate_longcat_weights.py` for a + worked-through validator. Do this before writing any FastVideo model + code — if conversion silently drops keys, all downstream parity work + will fail mysteriously. +9. **Feed the converted directory into `from_pretrained`** during parity + testing: + + ```python + gen = VideoGenerator.from_pretrained("converted_weights/", num_gpus=1) + ``` + +10. **(Optional) Publish to HF** so teammates and CI don't need to rerun + conversion. Use: + + ```bash + python scripts/checkpoint_conversion/create_hf_repo.py \ + --repo_id "FastVideo/-Diffusers" \ + --local_dir "converted_weights/" \ + --checkpoint_dir "converted_weights//transformer" \ + --component_name "transformer" \ + --push_to_hub \ + --upload_repo_id "FastVideo/-Diffusers" + ``` + + `create_hf_repo.py` downloads the base diffusers repo, injects the + converted component weights, and (with `--push_to_hub`) uploads the + result. Once the HF repo exists, add its ID to the `hf_model_paths` + list in your `register_configs(...)` call — future users skip + conversion entirely. + +### Conversion gotchas + +- **Regex iteration order matters.** A loose pattern placed before a + specific one swallows the specific one. Always test on a dry-run dict. +- **Check for unmapped keys.** After applying `PARAM_NAME_MAP`, diff the + post-mapping key set against the FastVideo module's `state_dict().keys()`. + Unmapped keys silently fall through to `load_state_dict(strict=False)` + and produce noisy output. +- **Sharded checkpoints.** Use `*.safetensors.index.json` + per-shard + loading (see `_find_shards` in `convert_ltx2_weights.py`) when the + source is split across multiple files. Don't assume a single file. +- **Dtype preservation.** Many conversion scripts call `.to(torch.bfloat16)` + on save. Match the DiT's configured `dit_precision` in the pipeline + config, or you'll get silent upcasts during load. +- **Shape checks.** Some regex rewrites hide shape-mismatch bugs (e.g. a + fused QKV reshaped wrong). After conversion, load into the FastVideo + model with `strict=True` at least once and inspect the error message + carefully — missing/unexpected keys are the only reliable conversion + verification. + +## `register_configs` cheatsheet + +Required fields: + +- `pipeline_config_cls` — your `PipelineConfig` subclass. +- `workload_types` — tuple of `WorkloadType` values (`T2V`, `I2V`, `V2V`, + `T2I`, etc.). Use `()` for configs that should not appear as a public + workload. + +Optional but strongly recommended: + +- `hf_model_paths` — exact HF IDs this config claims. First match wins. +- `model_detectors` — list of `Callable[[str], bool]` run against the HF + path AND the `_class_name` from `model_index.json`. Use these for + prefix/substring patterns when you don't want to enumerate every variant. +- `model_family` — groups presets together. +- `default_preset` — name of the preset used by default when the user does + not specify one. Must exist in your `ALL_PRESETS`. +- `sampling_param_cls` — only when the family adds new sampling fields. + Default is the base `SamplingParam` in `fastvideo/api/sampling_param.py`. + +## Adding an I2V variant + +This section covers adding image-to-video (I2V) support to a family, +based directly on how Wan I2V is structured in the repo. The same +pattern also underlies V2V variants (`WANV2VConfig` just swaps the +image encoder). + +**Decision point: same family or new family?** + +I2V is a *variant of an existing T2V family*, not a new family. Add it +on top of the T2V port — do not start from scratch. If you have not +yet completed the T2V port per steps 1–16 above, do that first. + +**Verified against** (as of 2026-04-24): +- `fastvideo/configs/pipelines/wan.py:73-94` (config inheritance). +- `fastvideo/pipelines/basic/wan/wan_i2v_pipeline.py` (stage wiring). +- `fastvideo/models/dits/wanvideo.py:246-296, 406-457, 83-98, 595` + (DiT branching on `added_kv_proj_dim`). +- `fastvideo/pipelines/basic/wan/presets.py:87-103` + (`WAN_I2V_14B_480P` preset). +- `fastvideo/registry.py` (I2V registrations). +- `examples/inference/basic/basic_wan2_2_i2v.py` (user-facing example). + +### Files touched for I2V (delta on top of the T2V port) + +| Path | Delta | +|------|-------| +| `fastvideo/pipelines/basic//_i2v_pipeline.py` | **New file** with its own `EntryClass`. Wires the I2V-specific stage order. | +| `fastvideo/configs/pipelines/.py` | **Add** a new `I2V...Config` class that **inherits from** the corresponding T2V config. | +| `fastvideo/pipelines/basic//presets.py` | **Add** one or more `InferencePreset`s with `workload_type="i2v"`. Append to `ALL_PRESETS`. | +| `fastvideo/registry.py` | **Add** a second `register_configs(...)` call with `workload_types=(WorkloadType.I2V,)` and the I2V `pipeline_config_cls`. | +| `examples/inference/basic/basic__i2v.py` | **New example** that passes `image_path=...` to `generate_video(...)`. | +| `fastvideo/models/dits/.py` | **Usually no new file** — the T2V DiT is reused by branching on the config. See "DiT branching" below. | +| `fastvideo/models/vaes/...` | No change. Same VAE serves T2V and I2V. | +| `fastvideo/models/encoders/...` | No new text encoder. Image encoder is typically stock CLIP (`CLIPVisionConfig`) already present in the repo. | + +### Config delta — inherit, add image encoder, flip VAE encoder flag + +```python +# fastvideo/configs/pipelines/.py +from fastvideo.configs.models.encoders import CLIPVisionConfig + +@dataclass +class I2V480PConfig(T2V480PConfig): + """I2V variant: inherits every T2V field, adds CLIP image encoder, + flips VAE load_encoder on so we can encode the reference image.""" + + image_encoder_config: EncoderConfig = field(default_factory=CLIPVisionConfig) + image_encoder_precision: str = "fp32" + + def __post_init__(self) -> None: + self.vae_config.load_encoder = True # I2V NEEDS the encoder + self.vae_config.load_decoder = True +``` + +Why `load_encoder=True`: T2V only decodes generated latents at the end, +so it saves memory by skipping the VAE encoder. I2V must encode the +**reference image** into latent space for conditioning, so the encoder +is required. Setting this wrong is a common silent failure — the +pipeline will still run but the image conditioning is empty. + +For non-standard image encoders (e.g. Wan's V2V control variant), swap +the field's default: + +```python +@dataclass +class V2VConfig(I2V480PConfig): + image_encoder_config: EncoderConfig = field(default_factory=) + image_encoder_precision: str = "bf16" +``` + +### Pipeline class delta — new file, two extra stages + +Create `fastvideo/pipelines/basic//_i2v_pipeline.py`. +Copy the T2V pipeline file and apply these deltas: + +1. `_required_config_modules` adds `"image_encoder"` and + `"image_processor"`: + + ```python + _required_config_modules = [ + "text_encoder", "tokenizer", "vae", "transformer", "scheduler", + "image_encoder", "image_processor", # NEW + ] + ``` + +2. Add `ImageEncodingStage` after `TextEncodingStage`, guarded on both + modules being loaded (so the same pipeline class can still run if + the user uses a no-image-encoder variant of the family): + + ```python + if (self.get_module("image_encoder") is not None + and self.get_module("image_processor") is not None): + self.add_stage( + stage_name="image_encoding_stage", + stage=ImageEncodingStage( + image_encoder=self.get_module("image_encoder"), + image_processor=self.get_module("image_processor"), + ), + ) + ``` + +3. Add `ImageVAEEncodingStage` **between** `LatentPreparationStage` + and `DenoisingStage`: + + ```python + self.add_stage( + stage_name="image_latent_preparation_stage", + stage=ImageVAEEncodingStage(vae=self.get_module("vae")), + ) + ``` + +4. **Remove** the `vae=...` kwarg from `DenoisingStage` — the VAE was + consumed upstream by `ImageVAEEncodingStage`: + + ```python + # T2V version (keep vae here): + # DenoisingStage(transformer=..., scheduler=..., vae=...) + # I2V version (drop vae): + DenoisingStage(transformer=self.get_module("transformer"), + transformer_2=self.get_module("transformer_2", None), + scheduler=self.get_module("scheduler")) + ``` + +5. `DecodingStage` is unchanged. + +6. `EntryClass = ImageToVideoPipeline` at module level. + +### DiT branching — one DiT, two cross-attention branches + +In most families, a single DiT file serves both T2V and I2V. The split +happens in the DiT block constructor based on an arch-config flag: + +```python +# fastvideo/models/dits/.py (pattern from wanvideo.py:288-297) +if added_kv_proj_dim is not None: + self.attn2 = I2VCrossAttention(dim, ...) # concatenates image K/V +else: + self.attn2 = T2VCrossAttention(dim, ...) # text-only cross-attn +``` + +The DiT's `forward()` gains an optional `encoder_hidden_states_image` +kwarg; it stays `None` for T2V runs and carries the image embedding +tensor for I2V runs (see `wanvideo.py:83-98` for the canonical +`condition_embedder` pattern that fuses timestep + text + image). + +Flag: make `added_kv_proj_dim: int | None = None` default in the DiT +arch config; the I2V variant's DiT config sets it to the CLIP hidden +size (e.g. 1024). This keeps T2V and I2V loading from the same model +class with no code fork. + +### Preset — `workload_type="i2v"`, fewer default steps + +```python +# fastvideo/pipelines/basic//presets.py +_I2V_14B_480P = InferencePreset( + name="_i2v_14b_480p", + version=1, + model_family="", + description=" I2V 14B at 480p", + workload_type="i2v", # NOT "t2v" + stage_schemas=(_DENOISE_STAGE,), + defaults={ + "height": 480, + "width": 832, + "num_frames": 81, + "fps": 16, + "guidance_scale": 5.0, + "num_inference_steps": 40, # Wan I2V defaults to 40; T2V uses 50 + "negative_prompt": _NEGATIVE_PROMPT, + # Do NOT put image_path in defaults — it's a per-call input. + }, +) +``` + +Add the new preset(s) to the `ALL_PRESETS` tuple at the bottom of the +file. + +### Registry — second `register_configs` call with I2V workload + +```python +# fastvideo/registry.py — inside _register_configs() +register_configs( + sampling_param_cls=None, + pipeline_config_cls=I2V480PConfig, + workload_types=(WorkloadType.I2V,), # NOT T2V + hf_model_paths=["Org/-I2V-Diffusers"], + model_detectors=[ + lambda path: "" in path.lower() and "i2v" in path.lower(), + ], + model_family="", + default_preset="_i2v_14b_480p", +) +``` + +If the HF repo's `model_index.json` `_class_name` already uniquely +distinguishes I2V (e.g. `"WanImageToVideoPipeline"`), prefer a detector +that matches on that class name — it's robust against repo-renaming. + +Unified T2V+I2V variants (e.g. `Wan2_2_TI2V_5B`) pass both workload +types: + +```python +workload_types=(WorkloadType.T2V, WorkloadType.I2V), +``` + +### User-facing example — `image_path` kwarg + +```python +# examples/inference/basic/basic__i2v.py +from fastvideo import VideoGenerator + +OUTPUT_PATH = "video_samples__i2v" + + +def main() -> None: + generator = VideoGenerator.from_pretrained( + "Org/-I2V-Diffusers", + num_gpus=1, + ) + + prompt = "" + image_path = "" + + generator.generate_video( + prompt, + image_path=image_path, # <-- the only API delta vs T2V + output_path=OUTPUT_PATH, + save_video=True, + height=480, + width=832, + num_frames=81, + ) + + +if __name__ == "__main__": + main() +``` + +The `image_path` kwarg is resolved to a PIL image internally and +flows through the stage chain (`ImageEncodingStage` → +`ImageVAEEncodingStage`). + +### Pipeline parity test for I2V + +Follow the [Pipeline-level parity test](#pipeline-level-parity-test-the-gate-for-user-handoff) +pattern with the I2V-specific extras: + +- Feed both pipelines the same `(prompt, image_path, seed, height, + width, num_frames, num_inference_steps)`. +- Compare denoised latents as usual; the image-encoding path will + contribute its own drift budget, so start at `atol=1e-3` and loosen + deliberately if needed (log per-step latent sums to prove bounded + drift first). +- Gate user handoff on a green I2V parity test *and* a green T2V + parity test — porting I2V without re-running T2V is how silent T2V + regressions slip in. + +## Distributed support (SP for DiTs, TP for LLM encoders) + +FastVideo splits the distributed story by component, and the skill's +advice reflects what actually ships in the codebase: + +- **DiTs: sequence parallelism (SP) only.** Every DiT in + `fastvideo/models/dits/` scales across multiple GPUs via SP on the + spatio-temporal sequence dimension. The SP wiring is already inside + `DistributedAttention`; your DiT just needs to assert head count is + divisible by `get_sp_world_size()` and to shard the input sequence + before the first block / all-gather it after the last. Reference: + `fastvideo/models/dits/wanvideo.py` (see `sequence_model_parallel_shard` + near line 657 and `sequence_model_parallel_all_gather_with_unpad` near + line 718). No `ColumnParallelLinear` / `RowParallelLinear` in DiTs. +- **LLM-style text encoders: tensor parallelism (TP).** `llama.py`, + `qwen2_5.py`, `siglip.py`, and `t5.py` all use `QKVParallelLinear` + + `MergedColumnParallelLinear` + `RowParallelLinear`. Copy that + pattern when your new text encoder is LLM-structured. +- **VAEs: neither, typically.** Parallelism comes from VAE tiling + (`vae_tiling=True` in `PipelineConfig`) plus optional VAE sequence + parallelism (`vae_sp=True`). No TP linears, no sequence sharding in + the VAE model class itself. +- **Ship single-GPU first.** If the DiT is >1-GPU capable, verify SP + parity in a follow-up; don't block the initial port on it. + +Key imports (used by all DiTs): + +```python +from fastvideo.distributed import ( + sequence_model_parallel_all_gather_with_unpad, + sequence_model_parallel_shard, +) +from fastvideo.distributed.parallel_state import get_sp_world_size +``` + +## Common pitfalls + +1. **`EntryClass` missing or wrong.** Must be a single class at module + level (not a list). Without it, pipeline discovery silently skips the + file. +2. **`_required_config_modules` drift.** If it doesn't match + `model_index.json`, the loader either misses a module or errors on load. +3. **`param_names_mapping` incomplete.** Silent weight-load misses mean + the model runs but outputs noise. Always diff `state_dict().keys()`. +4. **Scheduler / sigma schedule off-by-one.** Produces subtly wrong videos, + not obvious errors. Parity-test the schedule itself, not just the DiT. +5. **Attention backend mismatch.** Official repos often use SDPA; align + backends (`FASTVIDEO_ATTENTION_BACKEND=TORCH_SDPA`) before chasing + numerical drift. +6. **Wrong `DistributedAttention` vs `LocalAttention` choice.** + `DistributedAttention` only for full-sequence self-attention in the + DiT; everything else (cross-attn, short-seq) is `LocalAttention`. +7. **Forgetting to add the preset import to `_register_presets`.** The + preset file exists, the pipeline loads, but `SamplingParam.from_pretrained` + falls back to a generic default — hard to notice without a test. +8. **VAE precision.** Many VAEs need fp32 or bf16 for numerical stability. + Set `vae_precision` in `PipelineConfig` explicitly. +9. **Detector regex too broad.** `lambda p: "video" in p.lower()` will + claim every model. Anchor on the family prefix. +10. **Missing `__init__.py` exports.** A new DiT/VAE config that isn't + re-exported from `fastvideo/configs/models/dits/__init__.py` (or the + VAE equivalent) breaks imports elsewhere in the codebase. +11. **Raw `torch.nn.Linear` in a hot path.** Skips the quantization / + weight-loading hooks and drifts from the codebase convention. Use + `ReplicatedLinear` in DiTs and VAEs, and the TP variants + (`QKVParallelLinear` / `MergedColumnParallelLinear` / + `RowParallelLinear`) in LLM-style text encoders — see the + [Linear layers](#linear-layers-fastvideolayerslinearpy) section for + the bucket-specific rule. Same "use the fastvideo primitive" + discipline applies to RMSNorm, GELU, SiLU-and-mul, MLP, RoPE. +12. **Using TP linears in a DiT or VAE.** The opposite mistake: reaching + for `ColumnParallelLinear` / `QKVParallelLinear` in a DiT because + it sounded fancier. No DiT or VAE in the repo uses TP linears; + doing so breaks weight loading for the existing converted HF repos. + `ReplicatedLinear` is the DiT/VAE default — period. +13. **Official-repo deps overriding FastVideo's pins.** When `pip install`-ing + the cloned reference repo, a stray `torch==` or `transformers==` + requirement can silently downgrade FastVideo's environment and produce + phantom parity failures. Use `--no-deps` and review each extra + dependency before accepting it. +14. **Subagents widening their scope.** A DiT subagent that also edits the + pipeline class or the registry breaks step 6's parallelism and + creates merge headaches. Check every subagent's diff and reject any + out-of-scope edit. + +## Outputs + +- A new pipeline reachable via `VideoGenerator.from_pretrained()`. +- Registry entries (config + preset) committed to `fastvideo/registry.py`. +- Per-component parity tests under `tests/local_tests//` (DiT, + VAE, encoder(s)). +- **Pipeline-level parity test** at + `tests/local_tests/pipelines/test__pipeline_parity.py`, passing + green (non-skip) against the cloned official repo. +- User-runnable example at `examples/inference/basic/basic_.py`. +- SSIM regression test + seeded reference videos on HF. +- PR description summarizing: reuse decisions, parity deltas, GPU + requirements, known caveats. + +## Example prompt snippet + +``` +Add FooVideo to FastVideo as a T2V family. + +Inputs (required before any work): + - Official reference repo: github.com/Example/FooVideo + - HF weights path: Org/FooVideo-Release (NOT Diffusers-format — single + foovideo-v1.safetensors at root, no model_index.json. Conversion + required.) + - Target model_family: foovideo + - Workload types: (T2V,) + - LoRA support: no + +The DiT is a flow-matching transformer close to Wan's structure; the VAE +is a custom 3D VAE (unique to this model). Reuse T5 text encoder. + +Reference files: + - Closest existing pipeline: fastvideo/pipelines/basic/wan/wan_pipeline.py + - Closest existing DiT: fastvideo/models/dits/wanvideo.py + - Closest existing conversion script: scripts/checkpoint_conversion/convert_ltx2_weights.py + - SGLang port (if helpful): + +Acceptance: + - Conversion produces converted_weights/foovideo/ that loads via + VideoGenerator.from_pretrained without missing keys. + - Per-component parity tests under tests/local_tests/{transformers, + vaes,encoders}/ pass at atol=1e-4. + - Pipeline parity test + tests/local_tests/pipelines/test_foovideo_pipeline_parity.py passes + (non-skip) against the cloned official repo. + - examples/inference/basic/basic_foovideo.py runs end-to-end and + produces a clean mp4. + - SSIM test registered (references seeded separately via + seed-ssim-references). +``` + +## References + +- `docs/contributing/coding_agents.md` — narrative guide, LTX-2 case study, + Wan2.1 worked example. Read this alongside the skill. +- `docs/design/overview.md` — pipeline architecture, configuration system, + Diffusers weight layout. +- `fastvideo/pipelines/basic/wan/wan_pipeline.py` — minimal standard pipeline. +- `fastvideo/pipelines/basic/ltx2/` — non-standard family with custom + stages, audio synthesis, multi-stage presets; copy patterns here when + your model diverges from standard. +- `fastvideo/registry.py` — registration entry points + (`_register_configs`, `_register_presets`). +- `fastvideo/configs/pipelines/base.py` — `PipelineConfig` base class. +- `fastvideo/pipelines/composed_pipeline_base.py` — pipeline base. +- `fastvideo/pipelines/stages/__init__.py` — catalog of standard stages. +- `fastvideo/layers/` — linears, norms, activations, MLPs, RoPE, visual + embeddings. See the [FastVideo layers and attention](#fastvideo-layers-and-attention) + section for selection rules. +- `fastvideo/attention/layer.py` — `DistributedAttention`, + `DistributedAttention_VSA`, `LocalAttention`. Selection rules in the + section above. +- `fastvideo/attention/selector.py` — backend selection (never call + directly from a model file; go through `DistributedAttention` / + `LocalAttention`). +- `tests/local_tests/transformers/test_ltx2.py`, + `tests/local_tests/vaes/test_ltx2_vae.py`, + `tests/local_tests/encoders/test_ltx2_gemma_parity.py` — canonical + component parity-test templates; see the [Parity test pattern](#parity-test-pattern) + section. +- `tests/local_tests/pipelines/test_gamecraft_pipeline_parity.py` — + canonical **pipeline-level** parity-test template (the gate in step 13). +- `tests/local_tests/pipelines/test_ltx2_pipeline_smoke.py` — smoke-test + template for step 12. +- `tests/local_tests/README.md` — existing convention doc for local parity tests. +- `examples/inference/basic/` — directory of user-runnable example + scripts. Model on `basic_ltx2.py`, `basic_wan2_2.py`, or the closest + existing family when creating `basic_.py` in step 13(b). +- `scripts/checkpoint_conversion/` — every conversion script. Start with + `convert_ltx2_weights.py` as the template; see the + [Weight conversion](#weight-conversion) section for the full map. +- `scripts/checkpoint_conversion/create_hf_repo.py` — publish a converted + directory as a Diffusers-format HF repo. +- `scripts/huggingface/download_hf.py` — stage existing HF repos locally + when no conversion is needed. +- `.agents/skills/seed-ssim-references/SKILL.md` — sibling skill for + seeding SSIM reference videos once the test file exists. + +## Changelog + +| Date | Change | +|------|--------| +| 2026-04-24 | Initial draft. Adapted from SGLang's `sglang-diffusion-add-model` skill, rewritten around FastVideo's single stage-based architecture and its registry + preset model. | +| 2026-04-24 | Require official repo URL + HF weights path as explicit gather-inputs step 1. Add "Weight conversion" section mapping `scripts/checkpoint_conversion/` and the conversion recipe. | +| 2026-04-24 | Add HF write-enabled API key as required input (asked at start, never pasted in chat). New step 5 "Clone official repo locally". New step 6 "Port components in parallel via subagents", consolidating former DiT + VAE + encoder steps. New reference sections: "FastVideo layers and attention" (enforces reuse of `fastvideo/layers/` + correct attention-layer selection), "Parity test pattern" (distilled from `tests/local_tests//test_ltx2_*`), "Parallel component porting" (subagent prompt template). Added step 15 cleanup for the temporary clone. Added pitfalls for raw `nn.Linear`, dep overrides, and subagent scope creep. | +| 2026-04-24 | Step 13 is now a gated "full-pipeline parity + example script" checkpoint: (a) mandatory pipeline parity test against the cloned official repo — iterate until green, (b) create user-runnable `examples/inference/basic/basic_.py`, (c) only then hand off to the user. Added pipeline parity test template section. Added example-script + pipeline-parity-test rows to the Files table. | +| 2026-04-24 | **Correction** based on code audit: DiTs and VAEs use `ReplicatedLinear` exclusively; TP linears (`ColumnParallelLinear` / `RowParallelLinear` / `QKVParallelLinear` / `MergedColumnParallelLinear`) appear only in LLM-style text encoders (`llama.py`, `qwen2_5.py`, `siglip.py`, `t5.py`). Prior draft incorrectly recommended TP linears for DiT QKV/MLP. Linear-layers table rewritten with a per-bucket rule, "Distributed support" section rewritten to match (SP for DiTs, TP for LLM encoders), subagent prompt template and Common pitfalls updated, added pitfall #12 for the inverse mistake (using TP linears in a DiT/VAE). | +| 2026-04-24 | Full-pass verification against Wan2.1 / Wan2.2 code. Canonical stage-order diagram now shows verified T2V and I2V orders side-by-side; ConditioningStage reframed as default-present (not optional). Files-table row 8 notes I2V/V2V/DMD variants get sibling pipeline files. Conversion reference table flags `wan_to_diffusers.py` as a legacy pattern (use `convert_ltx2_weights.py` as the template instead). New "Adding an I2V variant" section added — covers config inheritance, pipeline-file delta (`ImageEncodingStage` + `ImageVAEEncodingStage`, drop `vae` from DenoisingStage), DiT branching on `added_kv_proj_dim`, preset `workload_type="i2v"`, registry delta, example with `image_path`, and I2V pipeline parity test. Uncertainties captured in sibling `REVIEW.md`. | diff --git a/.agents/skills/add-model/add_model_split.md b/.agents/skills/add-model/add_model_split.md new file mode 100644 index 000000000..0ba6b0e28 --- /dev/null +++ b/.agents/skills/add-model/add_model_split.md @@ -0,0 +1,230 @@ +# add-model skill — split plan + +The current `SKILL.md` is **1605 lines** in a single file. This document +proposes splitting it into ~12 satellite docs with a much shorter +top-level index, following the idiomatic Anthropic-skills pattern of +"short procedural index + targeted satellites." + +## Why split + +1. **Cold-load cost.** The whole 1605-line file is loaded into the + model's context every time the skill fires. Most porting sessions + only need a fraction of that — e.g. a VAE-only contribution doesn't + need the I2V-variant section or the weight-conversion recipe. A + shorter `SKILL.md` index + lazy-loaded satellites keeps the live + context lean. + +2. **Reviewability.** A single 1605-line markdown file is hard to + diff. Splits keep changes scoped — adding the "audio workload" + section (REVIEW item 25) becomes a new file in `how-to/` rather + than a 100-line insert in the middle of an existing megafile. + +3. **Findability.** Section anchors in a single file are hard to + navigate; per-topic files surface in `ls .agents/skills/add-model/` + and `grep -r` returns clean per-file matches. + +4. **Component-only contributions** (REVIEW item 23) become a + first-class workflow document instead of an awkward "but skip + half the steps" exception inside the main file. + +## Current section inventory + +| Section | Approx lines | Stays in SKILL.md? | Target file | +|---|---|---|---| +| Purpose | 8 | Yes | SKILL.md | +| When to use / not to use | 25 | Yes | SKILL.md | +| Prerequisites | 40 | Yes | SKILL.md | +| Inputs (table) | 15 | Yes | SKILL.md | +| FastVideo's single architecture | 40 | No | how-to/architecture.md | +| Files you will create or touch (table) | 25 | Yes (link to how-to) | SKILL.md + how-to/files_table.md | +| Steps (1–16) | 440 | **Index only** in SKILL.md (3-line per step + link) | how-to/steps_.md ×4 | +| Standard stages — subclass targets | 20 | No | how-to/architecture.md | +| FastVideo layers and attention | 110 | No | how-to/layers_and_attention.md | +| Parity test pattern | 165 | No | how-to/parity_testing.md | +| Parallel component porting | 100 | No | how-to/parity_testing.md | +| Weight conversion | 155 | No | how-to/weight_conversion.md | +| `register_configs` cheatsheet | 25 | No | how-to/registry_cheatsheet.md | +| Adding an I2V variant | 270 | No | how-to/i2v_variant.md | +| Distributed support | 35 | No | how-to/distributed_support.md | +| Common pitfalls | 50 | No | reference/pitfalls.md | +| Outputs | 15 | Yes | SKILL.md | +| Example prompt snippet | 35 | No | reference/example_prompt.md | +| References | 50 | No | reference/links.md | +| Changelog | 20 | No | reference/changelog.md | + +## Target tree + +``` +.agents/skills/add-model/ +├── SKILL.md ~250 lines — procedural index +├── REVIEW.md unchanged +├── add_model_split.md this doc +├── how-to/ +│ ├── architecture.md ~70 lines — single-architecture diagram + standard stages +│ ├── files_table.md ~50 lines — annotated 17-row Files table +│ ├── steps_1_setup.md ~80 lines — steps 1–5 (gather, study, reuse, convert, clone) +│ ├── steps_2_components.md ~120 lines — step 6 + parallel component porting +│ ├── steps_3_pipeline.md ~100 lines — steps 7–11 (config, stages, pipeline class, presets, registry) +│ ├── steps_4_validation.md ~120 lines — steps 12–16 (smoke, pipeline parity, SSIM, cleanup, ask) +│ ├── parity_testing.md ~250 lines — conventions + component template + pipeline-level gate + subagent prompt +│ ├── weight_conversion.md ~200 lines — decision tree + recipe + reference scripts + gotchas +│ ├── layers_and_attention.md ~150 lines — linear/attention/primitive selection rules +│ ├── i2v_variant.md ~270 lines — full I2V add section verbatim (cleanly extractable) +│ ├── distributed_support.md ~50 lines — SP/TP/VAE-tiling rules +│ ├── registry_cheatsheet.md ~30 lines — register_configs fields +│ ├── component_only_contributions.md ~120 lines — NEW (REVIEW #23): VAE-only / encoder-only PR shape +│ └── audio_workload.md ~150 lines — NEW (REVIEW #25): audio output, T2A workload, no-SSIM metrics +├── reference/ +│ ├── pitfalls.md ~80 lines — the 14-item pitfalls list +│ ├── example_prompt.md ~40 lines — the example user prompt snippet +│ ├── links.md ~50 lines — file/repo references +│ └── changelog.md ~30 lines — change history table +└── seed-ssim-references/ unchanged +``` + +Total: ~2200 lines across 17 files (vs current 1605 lines × 1 file). +The line growth is intentional — each file gets a short "Purpose + +Status + Prerequisites" header so it can be loaded without context +from the others. + +## SKILL.md target shape (~250 lines) + +```markdown +# Add a Model to FastVideo + +## Purpose +[8 lines, unchanged] + +## When to use / When not to use +[25 lines, unchanged] + +## Prerequisites — gather inputs (blocking) +[40 lines, unchanged] + +## Inputs +[15-line table, unchanged] + +## Files you will create or touch +The full table lives in `how-to/files_table.md`. Quick sketch: +- Component files (model + config + __init__ export): rows 1–6. +- Pipeline files (config + class + stages + presets): rows 7–10. +- Registry: row 11. +- Tests (smoke + pipeline parity + per-component parity + SSIM): rows 12–15. +- Conversion script (only if not Diffusers format): row 16. +- Example: row 17. + +For component-only contributions (just a VAE / encoder), see +`how-to/component_only_contributions.md` — you can skip rows 7–13 + 15 ++ 17. + +## Steps (procedural index) + +1. **Gather inputs (blocking).** See `how-to/steps_1_setup.md`. +2. **Study the reference implementation.** See `how-to/steps_1_setup.md`. +3. **Decide what to reuse.** See `how-to/steps_1_setup.md`. +4. **Convert weights to Diffusers format.** Only if not Diffusers-format. + See `how-to/weight_conversion.md`. +5. **Clone the official repo for parity testing.** See `how-to/steps_1_setup.md`. +6. **Port components in parallel via subagents.** See + `how-to/steps_2_components.md` + `how-to/parity_testing.md`. +7. **Create the PipelineConfig.** See `how-to/steps_3_pipeline.md`. +8. **Build or pick the stages.** See `how-to/architecture.md` (standard + stages catalog) + `how-to/steps_3_pipeline.md`. +9. **Write the pipeline class.** See `how-to/steps_3_pipeline.md`. +10. **Define presets.** See `how-to/steps_3_pipeline.md`. +11. **Register in `fastvideo/registry.py`.** See `how-to/registry_cheatsheet.md`. +12. **Smoke-test the pipeline.** See `how-to/steps_4_validation.md`. +13. **Full-pipeline parity + example (gated).** See + `how-to/parity_testing.md` (the pipeline-level section is the + handoff gate). +14. **Add SSIM regression.** See `how-to/steps_4_validation.md`. For + audio, see `how-to/audio_workload.md` (no SSIM analog). +15. **Clean up the cloned reference repo.** See `how-to/steps_4_validation.md`. +16. **Ask about tests + perf data.** See `how-to/steps_4_validation.md`. + +## Pre-handoff checklist +[New section addressing REVIEW item 16 — bans skip-only parity at handoff] +- [ ] `pytest tests/local_tests//test__*parity*.py -v` produces non-skip PASS for each non-reused component. +- [ ] `pytest tests/local_tests/pipelines/test__pipeline_parity.py -v` produces non-skip PASS. +- [ ] `python examples/inference/basic/basic_.py` writes a non-corrupt mp4 (or .wav for audio). +- [ ] Conversion has actually been run (the parity tests skip if not). + +## Outputs +[15 lines, unchanged] + +## Common pitfalls +See `reference/pitfalls.md` for the full 14-item list. Most-cited: +- #1: `EntryClass` missing → pipeline silently invisible. +- #11: raw `nn.Linear` in DiT/VAE hot paths → use `ReplicatedLinear`. +- #16: skip-only parity → see pre-handoff checklist above. + +## See also +- `reference/example_prompt.md` — example user prompt for invoking this skill. +- `reference/links.md` — file/repo references. +- `reference/changelog.md` — change history. +``` + +## Migration steps + +1. **Create the new files** with content extracted from current + `SKILL.md` (no semantic edits — just relocation + cross-link edits). +2. **Add per-file headers** of the form: + + ```markdown + # + + **Part of:** add-model skill + **When to read:** + **Prerequisites:** + + --- + + + ``` + +3. **Rewrite SKILL.md** as the index. Each step gets the new 3-line + format: title + 2-line summary + link to `how-to/.md`. +4. **Add the pre-handoff checklist** (addresses REVIEW item 16) — the + single load-bearing addition this split enables. +5. **Add `how-to/component_only_contributions.md`** (addresses REVIEW + item 23). +6. **Add `how-to/audio_workload.md`** (addresses REVIEW items 25 + 28). +7. **Update REVIEW.md** to mark items 16, 23, 25, 28 as resolved by + the split. +8. **Re-run a sample skill invocation** (e.g. on a hypothetical new + port) to validate the split — check that the model only loads + the index + 2-3 satellite files for a typical port. + +## Risks / open questions + +- **Breaks existing prompts.** Anyone with an in-flight skill + invocation may have memorized section anchors that move. Mitigate + by leaving anchor stubs in SKILL.md for one cycle (forwarding + comments). +- **Cross-link maintenance.** Each rename / move requires updating + cross-refs. Mitigate by keeping the file tree shallow (one + `how-to/` and one `reference/` directory only). +- **Discoverability of new files.** A porter who only reads + `SKILL.md` may not realize `audio_workload.md` exists. Mitigate by + having the index's step-3 line for "audio variant of step X" link + explicitly to the audio doc, and by keeping the "See also" section + visible. +- **What counts as "idiomatic"?** Anthropic skills tend toward + ~150-300 line single-file or ~3-5 file splits. 17 files is on the + large side. We might collapse further if some satellites are + always-loaded-together (e.g. merge `architecture.md` + + `registry_cheatsheet.md` if they're never read independently). + Decide post-prototype. + +## Next step + +Implement the split as a separate PR (don't bundle with the +will/stable-audio first-class VAE work or the will/magi MagiHuman +port). Sequence: + +1. Land the split as-is (mechanical relocation, zero semantic + change). Verify that running the skill against a known port + produces equivalent guidance. +2. Then layer in the REVIEW-item edits (16, 23, 25, 28) as content + changes inside the new structure. diff --git a/.agents/skills/index.jsonl b/.agents/skills/index.jsonl index fae93d913..72ac73a2e 100644 --- a/.agents/skills/index.jsonl +++ b/.agents/skills/index.jsonl @@ -6,3 +6,4 @@ {"name": "index-related-work", "description": "Ingest a paper or repository into the related work index", "path": "index-related-work/SKILL.md", "status": "draft", "trust": "low"} {"name": "search-related-work", "description": "Query the related work index for relevant papers, repos, or comparisons", "path": "search-related-work/SKILL.md", "status": "draft", "trust": "low"} {"name": "seed-ssim-references", "description": "Run a new or updated fastvideo/tests/ssim/ test on Modal, pull generated videos, and upload them to FastVideo/ssim-reference-videos so the test has a regression baseline", "path": "seed-ssim-references/SKILL.md", "status": "draft", "trust": "low"} +{"name": "review-add-model-pr", "description": "Review a PR that adds a new model / pipeline (or major variant) to FastVideo. Walks the canonical surface, indexes the 36 documented add-model failure modes by where they show up in a diff, and produces a structured verdict (block / nit / follow-up).", "path": "review-add-model-pr/SKILL.md", "status": "draft", "trust": "low"} diff --git a/.agents/skills/review-add-model-pr/SKILL.md b/.agents/skills/review-add-model-pr/SKILL.md new file mode 100644 index 000000000..b6951b336 --- /dev/null +++ b/.agents/skills/review-add-model-pr/SKILL.md @@ -0,0 +1,482 @@ +--- +name: review-add-model-pr +description: Use when reviewing a PR that adds a new model / pipeline (or a major variant like I2V/V2V/DMD) to FastVideo under `fastvideo/pipelines/basic//`. Walks a reviewer through the canonical surface the porter should have touched, the parity bar, and the 36 documented failure modes from prior ports. Returns a structured review verdict (block / nit / follow-up). +--- + +# Review an add-model PR + +## Purpose + +Catch the failure modes that have actually shipped in past add-model PRs +(documented in `.agents/skills/add-model/REVIEW.md`) **before merge**, +without re-litigating design choices that the `add-model` skill has +already settled. The reviewer's job is *not* to redesign the port — it +is to verify the porter did the things the skill prescribes, exercised +the parity gate honestly, and surfaced the right knobs to end users. + +This skill assumes the PR claims to add a model. For PRs adding only +sampling-preset tweaks, a single pipeline kwarg, or a new test against +an existing pipeline, this skill is overkill — comment scoped to those +specific changes instead. + +## When to use + +- A new pipeline directory under `fastvideo/pipelines/basic//`. +- A new variant pipeline (e.g. `_i2v_pipeline.py`, + `_dmd_pipeline.py`) sibling-added to an existing family. +- A "first-class component" PR that adds a new DiT, VAE, or encoder + port without (yet) wiring a pipeline (REVIEW item 23). +- Re-review of a port that previously skipped the post-parity hot-path + pass (REVIEW item 33) or shipped with placeholder diffusers imports + (REVIEW item 30). + +## When not to use + +- PR only edits sampling defaults in an existing + `/presets.py` → review the values against the model card + inline, no skill needed. +- PR only adds a new SSIM reference → use the + `seed-ssim-references` skill instead. +- PR refactors shared infra (`fastvideo/layers/`, `fastvideo/attention/`, + `fastvideo/pipelines/stages/` base classes) without touching a + family directory → that's not an add-model PR. + +## Required reading before starting the review + +1. **The PR description.** What family is being added, what variants + (T2V / I2V / V2V / DMD / T2A / …), what the porter claims is parity- + verified, and which of the four prereqs (official repo URL, HF + weights path, HF token, target `model_family`) they collected. +2. **`.agents/skills/add-model/SKILL.md` Files-table** (rows 1–17) — the + canonical surface a model port touches. You'll cross-reference this + against the diff in step 2 below. +3. **`.agents/skills/add-model/REVIEW.md` summary table** — the 36 + failure modes. The "Pitfall map" section below indexes them by where + they typically show up in a diff. + +## Inputs + +| Parameter | Required | Description | +|-----------|----------|-------------| +| `pr` | Yes | GitHub PR number, URL, or local branch name. | +| `model_family` | Recommended | The snake_case family slug (e.g. `stable_audio`, `magi_human`). Lets you grep-scope the review. | +| `official_repo` | Recommended | URL of the upstream reference, so you can sanity-check the parity test references it correctly. | +| `parity_run_log` | Optional | If the porter attached a parity run log, read it before reading code. | + +## Steps + +### 1. Verify the PR scope is shaped like an add-model PR + +Run `gh pr view --json files | jq -r '.files[].path'` (or +`git diff --name-only origin/main...HEAD`). Confirm the diff touches +**at least 6 of the 17 Files-table rows** in `add-model/SKILL.md`. +Typical shape: + +| If you see... | Expect... | If missing... | +|---|---|---| +| `fastvideo/models/dits/.py` | `fastvideo/configs/models/dits/.py` + export in `__init__.py` | Block: DiT config + export missing (Files-table rows 2 + 3). | +| `fastvideo/pipelines/basic//_pipeline.py` | `presets.py`, `registry.py` edit, smoke test, parity test, example | Block on whichever is missing — the porter will be told the same by the skill. | +| `_i2v_pipeline.py` (sibling-added) | A new preset for the I2V workload + the registry entry pointing at the I2V HF repo | Block: I2V variants get their own preset + workload tag (REVIEW item 8). | +| Standalone DiT/VAE/encoder under `fastvideo/models//` with no pipeline | A "first-class component" PR (REVIEW item 23) | Confirm with the author that the consuming pipeline PR is in flight or planned. Don't block. | +| Edits under `fastvideo/configs/pipelines/base.py`, `fastvideo/api/sampling_param.py`, `fastvideo/pipelines/pipeline_batch_info.py` | New pipeline-call kwargs the family needs | Sanity-check the field's default doesn't change behavior for other families. Often a footgun (e.g. `0 or default` Python truthiness). | + +### 2. Walk the diff in dependency order, not file order + +Components have a strict dependency order — review them in this order +so you can spot mismatches as they cascade: + +1. **DiT** (`fastvideo/models/dits/.py` + config) — foundational. +2. **VAE** — only one usually exists; if not, this is the second-biggest + review surface. +3. **Encoder(s) / conditioner** — text encoder is usually shared; new + conditioners (e.g. `MultiConditioner`-style) need careful look. +4. **`PipelineConfig`** subclass. +5. **Pipeline class** — wires modules + stages. +6. **Stages** — most should be standard; mod-specific subclasses + warrant careful review. +7. **Presets + registry** — usually mechanical; check workload type + selection against REVIEW item 28. +8. **Tests** — both component parity and pipeline parity. +9. **Example script** — last, but the bar is high (REVIEW items 31, 35). + +For each component, run the Per-component checklist (next section). + +### 3. Apply the Per-component checklist + +For each new file in `fastvideo/models//.py`: + +#### DiT review (`fastvideo/models/dits/.py`) + +- [ ] **No raw `nn.Linear`** in QKV/MLP/proj/embedder paths. All + projections should be `ReplicatedLinear` from + `fastvideo.layers.linear`. Exceptions are the legacy MoE-packed + case (REVIEW item 11) — must be flagged in a comment with the + weight-layout justification. +- [ ] **No raw SDPA / `flash_attn_func`** calls. Self-attention should + be `DistributedAttention` (or `LocalAttention` for cross-attention). + See `add-model/SKILL.md` "Attention layers" table. +- [ ] **No raw `nn.LayerNorm` in modulation paths.** Should be + `FP32LayerNorm` / `RMSNorm` / `LayerNormScaleShift` from + `fastvideo.layers.layernorm`. +- [ ] **No `from diffusers import` or `from transformers import + `** anywhere outside of test files (REVIEW item 30, the + hard ban). Tokenizers are the *only* allowed `from transformers` + runtime import in production code. +- [ ] **`from_official_state_dict()` (or equivalent)** is provided so + the consuming pipeline can load the published checkpoint without + going through `diffusers.from_pretrained`. +- [ ] **Param-mapping deviations from upstream are localized** — e.g. + if the model uses `gamma`/`beta` and FastVideo's `FP32LayerNorm` + uses `weight`/`bias`, the remap happens *in the loader*, not by + renaming the layers. +- [ ] **Partial / non-standard rotary** (e.g. halves-swap vs + interleaved-pair) is kept local with a one-line WHY comment, not a + copy of FastVideo's `_apply_rotary_emb` with edits. + +#### VAE review (`fastvideo/models/vaes/.py`) + +- [ ] **File name matches the convention** (REVIEW item 29): name + after the *arch* if the VAE is shared across families + (`oobleck.py`, `autoencoder_kl.py`); name after the *family* if it's + specific (`wanvae.py`). +- [ ] **`from_pretrained()` (or `from_official_state_dict`)** loads + weights from the published HF path *without* a Diffusers + intermediary (REVIEW item 30 again). +- [ ] **Per-channel `latents_mean`/`latents_std` are reshaped with + explicit `.view(1, -1, 1, 1, 1)`** when applied (REVIEW item 22) — + silent broadcasting along the wrong dim is a stealth bug. +- [ ] **Normalization-convention mismatches with the wrapper** + (REVIEW item 20) — if upstream `decode()` does + `z = z*std + mean` *internally* but the FastVideo wrapper expects + pre-denormalized input, the parity test must compensate or the + decode parity will look broken. +- [ ] **Pipeline-glue wrapper** (e.g. `fastvideo/models/vaes/_audio.py` + for lazy-load semantics) exists if the pipeline needs lazy-load / + hide-from-named_parameters semantics (REVIEW item 26). + +#### Conditioner / encoder review + +- [ ] If the conditioner mixes text + numeric (e.g. duration), it + produces the **DiT-ready (cross_attn_cond, cross_attn_mask, + global_embed) triple** in a single helper, not via inline cat-ing + in the conditioning stage. +- [ ] **T5 / Llama / SigLIP TP wiring** uses the encoder bucket's TP + primitives (`QKVParallelLinear`, `MergedColumnParallelLinear`, + `RowParallelLinear`) — not `ReplicatedLinear`. +- [ ] If the conditioner intentionally hides its T5 from + `named_parameters()` (so the SA-style checkpoint loader doesn't try + to match upstream-absent T5 keys), that exclusion is documented in + a comment. + +#### `PipelineConfig` review (`fastvideo/configs/pipelines/.py` +or `fastvideo/pipelines/basic//pipeline_configs.py`) + +- [ ] **Subclasses `PipelineConfig`** from + `fastvideo.configs.pipelines.base`. +- [ ] **`vae_config`, `dit_config`, `text_encoder_configs`** are + defaulted via `field(default_factory=...)` (mutable-default-arg + Python rule). +- [ ] **The text-encoder slot** matches what the pipeline actually + uses. If the pipeline owns its own conditioner (no FastVideo-loaded + text encoder), the text-encoder tuples should be `tuple()` and the + parent's length-equality validator should still pass — the porter + must zero out *all four* of `text_encoder_configs`, + `text_encoder_precisions`, `preprocess_text_funcs`, + `postprocess_text_funcs` together (we've seen this break before). +- [ ] **Component-bucket inheritance** (REVIEW item 24) — if the new + config goes under `vaes/`, it must subclass `VAEConfig` not + `EncoderConfig`. The bucket directory determines the base. +- [ ] **`__post_init__`** is used only to flip `load_encoder` / + `load_decoder` / etc. on the child configs, not to do any heavy + build. + +#### Pipeline class review (`fastvideo/pipelines/basic//_pipeline.py`) + +- [ ] **`EntryClass` is a single class**, not a list. +- [ ] **`_required_config_modules`** lists exactly what the loader + reads from `model_index.json`. Missing keys cause silent loading + degradation. +- [ ] **`load_modules()`** does not have any `from diffusers import` + / `from transformers import ` for production + components. If the porter explicitly opted into a temporary + diffusers shim, REJECT — the right move is to ship the native port + or hold the pipeline back (REVIEW item 30). +- [ ] **`torch.backends.*` flags** (TF32, cuDNN benchmark) — if set, + they're set **once in `load_modules`**, not per-call inside a stage + (REVIEW item 33). Mid-run flips invalidate the cuDNN algorithm + cache and amplify A2A SDE drift. +- [ ] **`create_pipeline_stages()`** uses standard stages from + `fastvideo.pipelines.stages` where possible, only subclassing when + the math diverges. New stage classes live in + `fastvideo/pipelines/basic//stages/` and are re-exported in + `fastvideo/pipelines/stages/__init__.py`. +- [ ] **One pipeline class for kwargs-driven variants** (REVIEW item + 34) — T2A/A2A/inpaint that share weights/components shouldn't be + split into three classes. Triggers to split: separate + `_required_config_modules`, separate HF repo, divergent forward + signatures, separate `WorkloadType`. Reject the split unless one + of those applies. + +#### Stages review (`fastvideo/pipelines/basic//stages/*.py`) + +- [ ] **No `init_audio_strength = 0` / "0 or default" footguns** — + Python truthy-or fallbacks (`x or default`) silently swallow `0`, + `0.0`, `""`. Use `x if x is not None else default`. +- [ ] **Loud-fail on malformed kwarg combos** — e.g. inpaint without + mask should raise `ValueError`, not silently fall through to T2A. +- [ ] **Hot-path discipline** (REVIEW item 33): no per-step + `torch.zeros_like(...)` or `torch.randn_like(...)` inside the + sampler loop; pre-allocate buffers + reuse via `.normal_()` / + `.copy_()`. +- [ ] **No dead `batch.extra` writes** — every key written should be + read by a downstream stage. +- [ ] **Stage docstring** is one or two lines. No "Mirrors upstream X" + / "Vendored from Y" provenance (REVIEW item 36); upstream + comparison belongs in the parity test, not the production docstring. + +#### Presets + registry review + +- [ ] **Sampling defaults** match the published model card example + block. Track the *model* defaults, not the upstream library's + *generic* defaults (this distinction shipped a 100% drift before + for Stable Audio). +- [ ] **`workload_type`** matches what the model actually does. If + the family is audio (T2A/A2A/AV) and `WorkloadType` doesn't yet + have those values (REVIEW item 28), accept `"t2v"` as a placeholder + but require a TODO comment + the porter to file a follow-up. +- [ ] **Registry detector** matches both the HF path and the + pipeline class name (`_class_name` from `model_index.json`). +- [ ] **`ALL_PRESETS`** export exists and is added to + `_register_presets()`'s group tuple in `fastvideo/registry.py`. + +#### Tests review — **the most load-bearing review surface** + +REVIEW item 16 is the single most expensive failure mode. A skipped +test reads as green in CI. Confirm explicitly: + +- [ ] **Component parity tests exist** for every non-reused + component: DiT, VAE (if new), encoder (if new). Find them under + `tests/local_tests//test__*.py`. +- [ ] **Each component parity test produces a non-skip pass on the + reviewer's machine** if at all possible. If the porter says "I ran + it locally", ask for the diff numbers in the PR description. +- [ ] **Pipeline parity test exists** at + `tests/local_tests/pipelines/test__pipeline_parity.py` AND + has a non-skip pass — *not* a "skipped because the official clone + is missing" pass. +- [ ] **Smoke test exists** at `test__pipeline_smoke.py` — no + GPU, just import + registry + preset wiring. CI can run this even + if local-only parity tests skip. +- [ ] **Parity reference is the official upstream**, not diffusers + (REVIEW item 30). Diffusers parity is acceptable as a *secondary* + test only when (a) the published weights load through both and (b) + the official repo is also imported and compared. +- [ ] **Tolerances are scope-appropriate** (REVIEW item 21): single- + block + single-kernel = `atol=1e-4`; full-DiT cross-kernel = `0.1` + with a complementary `abs_mean drift < 5%` check; bare `assert_close` + alone with a loose tolerance is a smell. +- [ ] **Stub helpers for upstream private DSL deps** (REVIEW items + 17, 18, 32) — if the upstream has `magi_compiler`-style imports + that aren't on PyPI, a small `tests/local_tests/helpers/_upstream.py` + shim is fine. But if the deps it bypasses become real installs, + the shim must be deleted (item 32 — no zombie no-op shims). +- [ ] **GQA-aware kernel routing in stub paths** (REVIEW item 19) — + if the parity test routes upstream's flash_attn through SDPA, KV + heads must be `repeat_interleave`'d explicitly. +- [ ] **VAE normalization symmetry** (REVIEW item 20) — if upstream + bundles `z = z*std + mean` in `decode()` and FastVideo expects + pre-denormalized, the test must compensate explicitly. + +#### Example script review (`examples/inference/basic/basic_*.py`) + +The bar here is high — the example is the user's entry point, not the +porter's debugging script. + +- [ ] **User-story-shaped docstring** (REVIEW item 31) — at least one + `User story ():` block, then a "How it works" / "Picking + the dial" / "Tunable knobs" section. Not a code-narration docstring. +- [ ] **5-15 LOC of constants + one `generate_video()` call** + (REVIEW item 35). If the example carries a 25-line `_load_reference` + / shape-norm / resample helper, that glue belongs *in the + pipeline*, not the example. +- [ ] **Pipeline accepts file paths** for any media-input kwarg + (`init_audio`, `inpaint_audio`, `image_path`). The example just + passes the path; pipeline does decode + resample internally. +- [ ] **No `torchaudio.load` on container formats** (mp4, m4a) — + routes through `torchcodec` → CUDA NVRTC. PyAV (already a + FastVideo dep, used by `_mux_audio`) handles all formats. +- [ ] **Tunable-knob defaults** match the model's published sweet + spot, not the upstream library's generic defaults. + +#### Local repro doc review (optional but recommended) + +If the family ships a `tests/local_tests/.md`: + +- [ ] **Setup section** covers HF gated access, optional inference + deps, upstream clone instructions, model cache pre-warm. +- [ ] **Per-test table** explains what each test compares against + with expected drift numbers. +- [ ] **Troubleshooting section** covers gated-skip behavior, batch- + vs-single-run flag interactions (cuDNN benchmark!), first-call + cache download blowup. + +### 4. Cross-check the "first-class component" rule + +Run `grep -rn "from diffusers import\|from transformers import" +fastvideo/pipelines/basic// fastvideo/models/dits/* +fastvideo/models/vaes/* fastvideo/models/encoders/*`. + +The **only** acceptable hits are `from transformers import +` (data-utility, no weights) and `from transformers +import T5EncoderModel` *only if* (a) it's loaded via HF and (b) the +T5 weights are absent from the model's checkpoint by design (e.g. +SA conditioner). + +Anything else — `StableAudioDiTModel`, `AutoencoderKL`, +`UnetXxx`, `T2VPipeline` — is a REVIEW item 30 violation. Block +the PR with a pointer to that item. + +### 5. Run the parity tests yourself if budget permits + +The porter said it passes. Confirm: + +```bash +# DiT/VAE/encoder component parity +pytest tests/local_tests//test__*.py -v -s + +# Pipeline parity (the gate per add-model step 13(a)) +pytest tests/local_tests/pipelines/test__pipeline_parity.py -v -s + +# Smoke (no GPU) +pytest tests/local_tests/pipelines/test__pipeline_smoke.py -v +``` + +If any *parity* test SKIPs rather than PASSes on your machine and +you have the prerequisites set up, that means the porter never +actually verified parity (REVIEW item 16). Block. + +### 6. Read REVIEW.md for any new failure modes the porter didn't address + +The PR may also amend REVIEW.md with newly-discovered failure modes. +Skim those — they're typically the most accurate signal of what to +look for in *this specific* port. If the porter added a REVIEW item +and the linked code in the PR doesn't actually mitigate that item, +that's a contradiction — flag it. + +### 7. Write the verdict + +Structure your review comment as three buckets: + +- **Block (must fix before merge)** — REVIEW item 30 violations, + skipped parity tests, missing required Files-table rows, dead + `batch.extra` writes that hide caller bugs, footguns like `0 or + default`. +- **Nit (would be better)** — naming convention drift, narrative + comments per REVIEW item 36, missing user-story docstrings, + hot-path allocations that don't change correctness. +- **Follow-up (not this PR)** — extracting test boilerplate to a + shared helper, future variant pipelines, performance benchmarks. + +Always link each item back to the `add-model/REVIEW.md` item number +when applicable; that's the institutional memory and lets the porter +fix the issue with full context. + +## Pitfall map (REVIEW.md items by where they show up in a diff) + +Use this when you've spotted something off and want to find the +documented failure mode it corresponds to. + +| If the diff has... | Suspect REVIEW item(s) | +|---|---| +| `from diffusers import ` in `fastvideo/...` | **30** (hard ban) | +| Raw `nn.Linear` in DiT projections | 10, 11 (only acceptable for MoE-packed weights with comment) | +| Custom RoPE that's *not* `_apply_rotary_emb` | Probably fine if the convention differs (e.g. halves-swap), but require a one-line WHY comment + a parity-test row | +| `parity_test.py` that calls `pytest.skip` unconditionally | **16** (silent no-op trap) — block | +| Component parity tests missing | **16** — block | +| New pipeline file with no `register_configs` call in registry.py | Files-table row 11 missing | +| `_required_config_modules` that doesn't match `model_index.json` keys | Silent loading degradation; block | +| `text_encoder_configs=tuple()` but other text-encoder tuples non-empty | Length-equality validator will fail at runtime | +| ArchConfig has `num_inference_steps` / `guidance_scale` / `flow_shift` | **15a** — pipeline-level fields leaking into ArchConfig; block | +| `vae.py` for an arch-shared VAE (e.g. Oobleck) | **29** — should be `.py` (e.g. `oobleck.py`) | +| Pipeline imports `from transformers import T5EncoderModel` | OK only if the conditioner intentionally hides T5 from `named_parameters()` and the checkpoint omits T5 keys; document the exception | +| `torch.backends.*` set in a stage's `forward()` | **33** — should be one-shot in `load_modules` | +| `init_X = 0` silently treated as default via `or` | **33-adjacent** — Python truthy footgun; flag | +| Per-step `torch.zeros_like` / `randn_like` in sampler callback | **33** — pre-allocate; can also fix accuracy regressions | +| Magic `_DOWNSAMPLING_RATIO = 2048` in stage code | **33** — derive from the VAE config | +| `examples/.../basic_*.py` >30 LOC | **35** — decode/resample/shape-norm belongs in the pipeline | +| Example uses `torchaudio.load(...)` on mp4 | **35** — torchcodec → NVRTC dep chain; use PyAV | +| Example docstring narrates code | **31** — needs `User story ():` block | +| Pipeline class docstring narrates upstream provenance | **36** — strip "Vendored from X" / "Mirrors upstream Y" | +| Comment says "previously this was X, we moved it because Y" | **36** — strip; goes in commit message, not code | +| Diff adds `tests/local_tests/helpers/_upstream.py` that's a no-op | **32** — delete the no-op shim + its call sites | +| Diff adds `init_audio` / `inpaint_audio` / similar to `SamplingParam` | OK; verify they default to `None` and other families ignore | +| Multiple pipeline classes that share weights/components | **34** — should be one class with kwargs-driven modes | +| Config under `fastvideo/configs/models/encoders/` for a VAE | **24** — wrong bucket; should be under `vaes/` with `VAEConfig` base | +| New pipeline that depends on a not-yet-ported component (placeholder import) | **30** — hold the pipeline back until the component is ported | + +## Outputs + +A structured review comment on the PR with: + +1. **Verdict** — `approve` / `request-changes` / `comment-only`. +2. **Block list** — REVIEW-item-linked issues that must be fixed. +3. **Nit list** — style / convention drift. +4. **Follow-up list** — items the porter should know about but + shouldn't fix in this PR. +5. **Parity numbers you confirmed** — diff_max / diff_mean / drift / + element-wise bound, per parity test you ran. Lets the next reviewer + skip re-running. + +## Example review comment skeleton + +``` +## Review summary + +**Verdict:** request-changes + +I ran the smoke + DiT-component parity tests and read the full diff. +The native ports look right; two REVIEW-30 violations in the pipeline +file need resolving before merge, plus a few smaller nits. + +### Block (must fix) + +- `fastvideo/pipelines/basic//_pipeline.py:NN` — `from + diffusers import ` violates REVIEW item 30 (hard ban). + Either ship the first-class port now or hold this pipeline back + until the component is ported. +- `tests/local_tests/pipelines/test__pipeline_parity.py` skips + on my machine even with HF token set (the upstream-clone path check + fails). REVIEW item 16: a parity test that always skips is worse + than no test. Update the path resolution or document it in + `tests/local_tests/.md`. + +### Nit + +- `examples/inference/basic/basic_.py` docstring is code-narration; + REVIEW item 31 wants a `User story ():` block. +- `_pipeline.py:NN` has a per-call `torch.backends.cuda.matmul.allow_tf32 = False`; + REVIEW item 33 says move to one-shot in `load_modules`. + +### Follow-up + +- HF-token boilerplate is duplicated across 4 parity files. REVIEW + item 1-tier work, but doesn't need to land in this PR. + +### Parity numbers I confirmed locally + +| Test | diff_max | diff_mean | drift | +|---|---|---|---| +| DiT component parity | 0.0 | 0.0 | bit-identical | +| VAE decode parity | 0.0 | 0.0 | bit-identical | +| Pipeline parity (T2V, 25 steps) | 0.012 | 0.0009 | 0.31% | +``` + +## References + +- `.agents/skills/add-model/SKILL.md` — the canonical procedure; + every "should be there" claim in this skill is a row in that + skill's Files-table. +- `.agents/skills/add-model/REVIEW.md` — 36 documented failure modes; + the Pitfall map above indexes them by where they show up in a diff. +- `.agents/skills/seed-ssim-references/SKILL.md` — for SSIM regression + add-on PRs (separate from this skill's scope).