From 850325c6cc0c1957f21c2a1e46f86035be24f02e Mon Sep 17 00:00:00 2001 From: Raghav Date: Mon, 11 May 2026 22:05:33 -0700 Subject: [PATCH 1/2] [skills] Add FLUX-port learnings to add-model + seed-ssim skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three concrete, generalizable improvements derived from PR #1321 (FLUX.1-dev port fixes) that the next model-port will hit: 1. add-model-02-parity — Tolerance guide stopped at "Full DiT, cross-kernel bf16: 0.1". FLUX (57 layers) needed atol=0.5 — observed max=0.5, mean=0.04, median=0 on A40. Added a "Very deep DiT (50+ layers), bf16" row and a "Calibrating atol > 0.1" subsection that requires diagnostic prints (max/mean/median/p99) so reviewers can verify the calibration without rerunning. Also distinguishes the healthy bf16-tail signature (median≈0, mean<>0.1). 2. add-model-09-pipeline — Step 4 listed two surfaces (sampling_param.py + CLI args) for adding new generation kwargs. The current architecture has four: sampling_param.py, api/schema.py SamplingConfig, the schema_parity_inventory YAML (moved + expected_dests), and the test_parser.py roundtrip dict snapshot. Missing any one fails CI with a different error; we hit three separate failures during PR #1321 before getting it green. Expanded Step 4 to enumerate all four with the failure mode for each. 3. seed-ssim-references — Skill documented .mp4 and .pt artefacts. T2I tests that reuse run_text_to_video_similarity_test produce .png when num_frames=1; reference_videos_cli.py copy-local silently skips PNG (walks .mp4/.pt only, reports "0 copied files"). Added .png as a third artefact type and a step-5 gotcha with the manual cp workaround plus the .gitignore negation pitfall. Each change is a small in-place edit to an existing skill file. No new skills added, no skill removed. Verifiable against the FLUX port: every failure mode called out here has a corresponding commit in PR #1321 (c82dd31a for atol, d1a63c81 for schema parity inventory, aabf179d for test_parser snapshot, 5b5fdcf9 for SSIM PNG copy). --- .agents/skills/add-model-02-parity/SKILL.md | 23 ++++++++++++++++ .agents/skills/add-model-09-pipeline/SKILL.md | 26 ++++++++++++++++--- .agents/skills/seed-ssim-references/SKILL.md | 21 +++++++++++++++ 3 files changed, 66 insertions(+), 4 deletions(-) diff --git a/.agents/skills/add-model-02-parity/SKILL.md b/.agents/skills/add-model-02-parity/SKILL.md index 5a96f34a56..2a637a14e6 100644 --- a/.agents/skills/add-model-02-parity/SKILL.md +++ b/.agents/skills/add-model-02-parity/SKILL.md @@ -191,6 +191,7 @@ Tolerance guide: | Single block, same kernel | `1e-4` / `1e-4` | Tight default. | | Full DiT, aligned kernels | `1e-2` / `1e-2` | Cross-layer accumulation. | | Full DiT, cross-kernel bf16 | `0.1` / `0.1` | Also require abs-mean drift below 5% and per-modality diagnostics. | +| Very deep DiT (50+ layers), bf16 | `0.5` / `0.0` | Tail errors from per-GEMM bf16 epsilon (~7.8e-3) accumulated across all layers. Must justify via diagnostic prints — see below. | | VAE decode fp32 | `5e-2` / `5e-2` | After normalization alignment. | | Encoder wrapper around same HF class | `1e-3` / `1e-3` | Should be near-zero. | @@ -203,6 +204,28 @@ hooks. Use `docs/contributing/activation_trace.md` to keep `FASTVIDEO_TRACE_LAYERS`, `FASTVIDEO_TRACE_STATS`, and `FASTVIDEO_TRACE_STEPS` identical across FastVideo and upstream traces. +### Calibrating atol > 0.1 + +When a deep DiT (e.g. FLUX with 57 transformer blocks) produces `max_diff > 0.1` +under bf16, do not silently bump `atol` to make the test pass. Print four metrics +and assert that the *distribution* — not just the max — looks healthy: + +```python +abs_diff = (hf_out.float().cpu() - fv_out.float().cpu()).abs() +print(f"max_diff={abs_diff.max():.4f} mean_diff={abs_diff.mean():.4f} " + f"median_diff={abs_diff.median():.4f} " + f"p99_diff={abs_diff.flatten().kthvalue(int(0.99 * abs_diff.numel())).values:.4f}") +``` + +Healthy bf16-tail signature (FLUX, 57 layers, observed on A40): +`max=0.5, mean=0.04, median=0, p99=0.25`. Median near zero and mean ≪ atol +prove the bulk of elements match — only the tail diverges due to accumulation. + +Real bug signature: `mean_diff >> 0.1` or `median_diff > 0.01`. Wrong weights, +swapped layers, or missing residuals push the mean up, not just the max. Keep +the diagnostic print in the committed test so reviewers can verify the +calibration without rerunning. + Useful local commands: ```bash diff --git a/.agents/skills/add-model-09-pipeline/SKILL.md b/.agents/skills/add-model-09-pipeline/SKILL.md index e91257b0a6..24ad0cfa53 100644 --- a/.agents/skills/add-model-09-pipeline/SKILL.md +++ b/.agents/skills/add-model-09-pipeline/SKILL.md @@ -69,10 +69,28 @@ Use this mode first. Runtime pipeline resolution is exact: `model_index.json["_class_name"]` must match a registered `EntryClass.__name__`, or a wrapper/alias class in `EntryClass`. Registry detectors do not select the executable pipeline class. -4. Add new public generation kwargs to `fastvideo/api/sampling_param.py` before - examples or presets use them. `SamplingParam.update()` ignores unknown keys - except for logging, and preset defaults apply only to declared fields. Add CLI - args when the option should be available from command-line entrypoints. +4. Add new public generation kwargs across the **full schema-parity surface** + before examples or presets use them. A missed surface fails CI in a different + way each time, so touching all four in one commit prevents three follow-ups: + - `fastvideo/api/sampling_param.py` — `SamplingParam.update()` ignores unknown + keys except for logging, and preset defaults apply only to declared fields. + Add CLI args here when the option should be available from command-line + entrypoints (note: the inference CLI is now config-only — most new fields + are reached via dotted overrides, not new flags). + - `fastvideo/api/schema.py` — add the field to `SamplingConfig` with the same + default. `test_inventory_targets_exist_in_typed_schema` walks + `request.sampling.` and will assert-fail if the dataclass is missing + the attribute. + - `docs/design/inference_schema_parity_inventory.yaml` — register the field + under `surfaces.sampling_param_base.moved` with target + `request.sampling.`. If the field has a live CLI dest (rare under + the config-only CLI), also add it to `cli.generate.expected_dests` + alphabetically, otherwise `test_cli_dest_inventory_matches_live_parsers` + fails. + - `fastvideo/tests/api/test_parser.py` — + `test_load_run_config_supports_yaml_roundtrip` compares an exact hardcoded + dict snapshot of every `SamplingConfig` field. Add the new field with its + default value in declaration order, or the dict-equality assertion fails. 5. Put loader-time changes in `load_modules()` or earlier, not `initialize_pipeline()`. `ComposedPipelineBase.__init__` loads modules before `post_init()` calls `initialize_pipeline()`, so process-global flags, loader diff --git a/.agents/skills/seed-ssim-references/SKILL.md b/.agents/skills/seed-ssim-references/SKILL.md index 527b76ef98..b18f61b3fc 100644 --- a/.agents/skills/seed-ssim-references/SKILL.md +++ b/.agents/skills/seed-ssim-references/SKILL.md @@ -19,6 +19,10 @@ side-by-side per `(model_id, backend, prompt)`: metadata + `slice_spec` + `format_version`) for tests that call `run_text_to_latent_similarity_test` in `latent_similarity_utils.py`. Compared via cosine distance on the slice and the full tensor. +- **`.png`** — pixel ground-truth for **T2I** tests (`num_frames=1`) that + reuse `run_text_to_video_similarity_test` but produce a single frame. The + helper writes a `.png` instead of a `.mp4` when the output has no time + dimension. Compared via SSIM the same way. This skill: @@ -111,6 +115,23 @@ and 6 are artefact-type-agnostic — `_iter_reference_files`, `copy_generated_to_reference`, and `upload_reference_videos` already walk both `.mp4` and `.pt` (see `reference_videos_cli.py`). +**T2I gotcha:** when the helper produces a `.png` (T2I test with +`num_frames=1`), `reference_videos_cli.py copy-local` currently walks +`.mp4`/`.pt` only and reports `0 copied files` without erroring. After step 5, +verify the destination contains the seeded file; if it's empty, copy the PNG +manually: + +```bash +cp ./generated_videos_modal/default/generated_videos/L40S_reference_videos///*.png \ + fastvideo/tests/ssim/reference_videos/default/L40S_reference_videos/// +``` + +Then `git add -f` the PNG: the repo `.gitignore` has a broad `*.png` rule, and +the `reference_videos/**` negation only applies to extensions explicitly +re-allowed *after* the catch-all (see `.gitignore` near the +`reference_videos/**` block — add `!fastvideo/tests/ssim/reference_videos/**/*.png` +there if the negation is missing). + If either check fails, stop and tell the user what's wrong. ### 2. Run the test on Modal L40S From 4c1345894b1211826343ecc446b3c7c6f3c6d91c Mon Sep 17 00:00:00 2001 From: Raghav Date: Mon, 11 May 2026 23:23:01 -0700 Subject: [PATCH 2/2] [skills]: address Gemini + Copilot review on PR #1338 add-model-02-parity: - Use .item() before f-string formatting so the snippet portably works on older PyTorch versions that lack `__format__` on 0-dim tensors. - Switch p99 to torch.quantile (cleaner, no off-by-one risk for small N vs. kthvalue(int(0.99 * N))). - Document why both changes matter. seed-ssim-references: - .png artefact: correct trigger is `workload_type.value.endswith("2i")` (per `VideoGenerator._is_image_workload`), not "num_frames=1". On current main the SSIM helpers (`_find_reference_video`, `output_video_name`) hardcode .mp4; consuming .png references requires the `media_extension` parameter introduced in PR #1321. - T2I gotcha: add `mkdir -p` before `cp` so the destination is created. - .gitignore: clarify that `reference_videos_cli.py upload` uses `huggingface_hub.upload_folder` (filesystem upload, not git), so `git add -f` only matters for local commits. Note that the `fastvideo/tests/ssim/reference_videos/**` catch-all near `.gitignore:94` currently overrides the earlier `*.mp4` negation, so every reference file requires `git add -f` regardless of extension. --- .agents/skills/add-model-02-parity/SKILL.md | 17 ++++++-- .agents/skills/seed-ssim-references/SKILL.md | 42 +++++++++++++------- 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/.agents/skills/add-model-02-parity/SKILL.md b/.agents/skills/add-model-02-parity/SKILL.md index 2a637a14e6..1bdace75d3 100644 --- a/.agents/skills/add-model-02-parity/SKILL.md +++ b/.agents/skills/add-model-02-parity/SKILL.md @@ -212,11 +212,22 @@ and assert that the *distribution* — not just the max — looks healthy: ```python abs_diff = (hf_out.float().cpu() - fv_out.float().cpu()).abs() -print(f"max_diff={abs_diff.max():.4f} mean_diff={abs_diff.mean():.4f} " - f"median_diff={abs_diff.median():.4f} " - f"p99_diff={abs_diff.flatten().kthvalue(int(0.99 * abs_diff.numel())).values:.4f}") +max_diff = abs_diff.max().item() +mean_diff = abs_diff.mean().item() +median_diff = abs_diff.median().item() +p99_diff = torch.quantile(abs_diff.flatten(), 0.99).item() +print( + f"max_diff={max_diff:.4f} mean_diff={mean_diff:.4f} " + f"median_diff={median_diff:.4f} p99_diff={p99_diff:.4f}" +) ``` +Call `.item()` before formatting (older PyTorch versions do not implement +`__format__` on 0-dim tensors), and prefer `torch.quantile` over +`kthvalue(int(0.99 * N))` — `kthvalue` is 1-indexed and degenerates for +very small `N`, while `quantile` handles the distribution boundary +cleanly. + Healthy bf16-tail signature (FLUX, 57 layers, observed on A40): `max=0.5, mean=0.04, median=0, p99=0.25`. Median near zero and mean ≪ atol prove the bulk of elements match — only the tail diverges due to accumulation. diff --git a/.agents/skills/seed-ssim-references/SKILL.md b/.agents/skills/seed-ssim-references/SKILL.md index b18f61b3fc..b1b8ab81d8 100644 --- a/.agents/skills/seed-ssim-references/SKILL.md +++ b/.agents/skills/seed-ssim-references/SKILL.md @@ -19,10 +19,15 @@ side-by-side per `(model_id, backend, prompt)`: metadata + `slice_spec` + `format_version`) for tests that call `run_text_to_latent_similarity_test` in `latent_similarity_utils.py`. Compared via cosine distance on the slice and the full tensor. -- **`.png`** — pixel ground-truth for **T2I** tests (`num_frames=1`) that - reuse `run_text_to_video_similarity_test` but produce a single frame. The - helper writes a `.png` instead of a `.mp4` when the output has no time - dimension. Compared via SSIM the same way. +- **`.png`** — pixel ground-truth for **T2I / I2I** tests. `VideoGenerator` + emits a `.png` (not a `.mp4`) whenever `workload_type.value.endswith("2i")` + is true (`fastvideo/entrypoints/video_generator.py::_is_image_workload`). + **The SSIM helpers in `inference_similarity_utils.py` on current main + hardcode `.mp4`** (`_find_reference_video` filter, `output_video_name` + format). To consume `.png` references through the SSIM path, the test + must call a helper variant that accepts a `media_extension` parameter + (added in PR #1321). Until that lands on main, T2I SSIM coverage is + blocked at the helper layer. This skill: @@ -115,22 +120,31 @@ and 6 are artefact-type-agnostic — `_iter_reference_files`, `copy_generated_to_reference`, and `upload_reference_videos` already walk both `.mp4` and `.pt` (see `reference_videos_cli.py`). -**T2I gotcha:** when the helper produces a `.png` (T2I test with -`num_frames=1`), `reference_videos_cli.py copy-local` currently walks -`.mp4`/`.pt` only and reports `0 copied files` without erroring. After step 5, -verify the destination contains the seeded file; if it's empty, copy the PNG -manually: +**T2I / I2I gotcha:** when the helper produces a `.png` (any workload +whose `workload_type.value` ends in `2i`), `reference_videos_cli.py copy-local` +currently walks `.mp4` / `.pt` only and reports `0 copied files` without +erroring. After step 5, verify the destination contains the seeded file; +if it's empty, copy the PNG manually: ```bash +mkdir -p fastvideo/tests/ssim/reference_videos/default/L40S_reference_videos/// cp ./generated_videos_modal/default/generated_videos/L40S_reference_videos///*.png \ fastvideo/tests/ssim/reference_videos/default/L40S_reference_videos/// ``` -Then `git add -f` the PNG: the repo `.gitignore` has a broad `*.png` rule, and -the `reference_videos/**` negation only applies to extensions explicitly -re-allowed *after* the catch-all (see `.gitignore` near the -`reference_videos/**` block — add `!fastvideo/tests/ssim/reference_videos/**/*.png` -there if the negation is missing). +`git add -f` is **not required for the HF upload path**: +`reference_videos_cli.py upload` calls `huggingface_hub.upload_folder` on +the filesystem directly (`reference_videos_cli.py:311`) — git tracking +status is irrelevant for `pytest fastvideo/tests/ssim/` reference sync. + +If you *also* want to commit the PNG locally, note that +`fastvideo/tests/ssim/reference_videos/**` (catch-all near `.gitignore:94`) +overrides the earlier `!fastvideo/tests/ssim/reference_videos/**/*.mp4` +negation at `.gitignore:73`. Every reference file in this dir currently +requires `git add -f`, regardless of extension. Fixing this properly +means moving the `!...reference_videos/**/*.{mp4,pt,png}` negations +*after* the catch-all (or removing the catch-all). Until then, `git add -f` +is the standing workaround for committing references locally. If either check fails, stop and tell the user what's wrong.