[skills] Add FLUX-port learnings to add-model + seed-ssim skills - #1338
[skills] Add FLUX-port learnings to add-model + seed-ssim skills#1338Mister-Raggs wants to merge 2 commits into
Conversation
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI
🔴 PR merge requirementsWaiting for
This rule is failing.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the documentation for model parity, pipeline schema parity, and SSIM reference seeding. It introduces guidelines for calibrating high atol values in deep DiT models using statistical distributions, mandates a multi-file update process for new generation kwargs to maintain schema consistency, and adds support for PNG ground-truth in T2I tests. Review feedback suggests using torch.quantile for more idiomatic percentile calculations and adding mkdir -p to manual file copy instructions to ensure destination directories exist.
| 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}") |
There was a problem hiding this comment.
Using torch.quantile is more idiomatic and readable for calculating the 99th percentile than kthvalue on a flattened tensor. It also avoids the manual calculation of the index and handles the distribution more naturally.
| 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}") | |
| 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.quantile(0.99):.4f}") |
| cp ./generated_videos_modal/default/generated_videos/L40S_reference_videos/<model_id>/<backend>/*.png \ | ||
| fastvideo/tests/ssim/reference_videos/default/L40S_reference_videos/<model_id>/<backend>/ |
There was a problem hiding this comment.
When seeding a new model, the destination directory in fastvideo/tests/ssim/reference_videos/ likely does not exist yet. Including mkdir -p in the instruction ensures the cp command doesn't fail due to a missing destination path.
| cp ./generated_videos_modal/default/generated_videos/L40S_reference_videos/<model_id>/<backend>/*.png \ | |
| fastvideo/tests/ssim/reference_videos/default/L40S_reference_videos/<model_id>/<backend>/ | |
| mkdir -p fastvideo/tests/ssim/reference_videos/default/L40S_reference_videos/<model_id>/<backend>/ | |
| cp ./generated_videos_modal/default/generated_videos/L40S_reference_videos/<model_id>/<backend>/*.png \ | |
| fastvideo/tests/ssim/reference_videos/default/L40S_reference_videos/<model_id>/<backend>/ |
There was a problem hiding this comment.
Pull request overview
Updates existing agent “skills” documentation to capture learnings from the FLUX port work (PR #1321), aiming to prevent repeat CI failures and improve reviewer-verifiable calibration guidance during future model ports.
Changes:
- Extend DiT parity tolerance guidance for very deep bf16 models and add a “calibrate atol > 0.1” diagnostic checklist.
- Expand pipeline checklist to enumerate the full schema-parity surface area when adding new
SamplingParamfields (schema, inventory, parser snapshot, etc.). - Document a seeding workflow gotcha for T2I artefacts and
.gitignoreinteractions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.agents/skills/add-model-02-parity/SKILL.md |
Adds guidance for deep DiT bf16 parity tolerances and diagnostic metrics for justified atol increases. |
.agents/skills/add-model-09-pipeline/SKILL.md |
Updates the pipeline checklist to cover all schema-parity “surfaces” that must be updated for new sampling fields. |
.agents/skills/seed-ssim-references/SKILL.md |
Documents additional SSIM reference artefact handling and related workflow caveats. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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}") |
| - **`.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. |
| 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). |
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 hao-ai-lab#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.
|
Hi @Mister-Raggs — automated review from Gob, one of @SolitaryThinker's AI reviewers. Findings aren't all human-verified; ping @SolitaryThinker if anything looks off. TL;DRVerdict is approve-with-followup. The skill docs themselves look well-scoped and the code-facing claims I checked line up, but the PR evidence list points the Deep-DiT bf16 tolerance learning at the wrong FLUX-port commit. Verdict: approve-with-followup
FindingsS2 — PR evidence list cites the RoPE device-fix commit for the bf16 tolerance learningThe PR body says `c82dd31a` is the "bf16 tolerance fix on FLUX DiT parity," and the initial commit message repeats `c82dd31a for atol`. The actual `c82dd31a` diff only changes `fastvideo/layers/rotary_embedding.py` to create `torch.arange(..., device=pos.device)`, so it is a RoPE CUDA device-mismatch fix, not the tolerance calibration evidence. A better evidence pointer exists in PR #1321: `5ca0b908eb` (`[bugfix] Fix DiT parity tolerance: atol=0.5 matches observed bf16 tail error (median=0, mean=0.04)`) changes `fastvideo/tests/transformers/test_flux.py` from `assert_close(..., atol=1e-4, rtol=1e-4)` to diagnostic `max/mean/median/p99` printing plus `assert_close(..., atol=0.5, rtol=0.0)`. Please update the PR body evidence table from `c82dd31a` to `5ca0b908eb`; I do not think this requires changing the skill docs themselves. — Gob (@SolitaryThinker's AI reviewer). Full review archived locally. |
|
This PR has merge conflicts with the base branch. Please rebase: git fetch origin main
git rebase origin/main
# Resolve any conflicts, then:
git push --force-with-lease |
|
Thanks @SolitaryThinker — confirmed locally:
Updated the PR body Verification table from (Leaving the original commit message as-is since the PR is squash-mergeable and history collapses on merge.) |
Three concrete, generalizable improvements derived from PR hao-ai-lab#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<<atol) from a real bug signature (mean_diff>>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 hao-ai-lab#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 hao-ai-lab#1321 (c82dd31 for atol, d1a63c8 for schema parity inventory, aabf179 for test_parser snapshot, 5b5fdcf for SSIM PNG copy).
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 hao-ai-lab#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.
a70ac31 to
4c13458
Compare
Summary
Three small in-place edits to existing
.agents/skills/*files, each derived directly from a failure we hit during PR #1321 (FLUX.1-dev port fixes) and verifiable against a specific commit in that PR. No new skills added, no skills removed.1.
add-model-02-parity— Deep-DiT bf16 toleranceThe tolerance guide capped at
Full DiT, cross-kernel bf16: 0.1 / 0.1. FLUX (57 transformer blocks) neededatol=0.5because per-GEMM bf16 epsilon (~7.8e-3) accumulates across all layers. Observed on A40:max=0.5, mean=0.04, median=0.Very deep DiT (50+ layers), bf16row.Calibrating atol > 0.1subsection that requires diagnostic prints (max/mean/median/p99) so reviewers can verify calibration without rerunning, and distinguishes the healthy bf16-tail signature (median≈0, mean<<atol) from a real bug signature (mean_diff >> 0.1).2.
add-model-09-pipeline— Complete schema-parity surface for new SamplingParam fieldsStep 4 listed two surfaces (
sampling_param.py+ CLI args). The current architecture has four, and each missed surface fails CI with a different error.Expanded to enumerate all four with the failure mode for each:
fastvideo/api/sampling_param.py— silent ignore of unknown keys.fastvideo/api/schema.pySamplingConfig—test_inventory_targets_exist_in_typed_schemawalksrequest.sampling.<field>.docs/design/inference_schema_parity_inventory.yaml—movedblock +expected_dests(under config-only CLI most fields skipexpected_dests).fastvideo/tests/api/test_parser.py—test_load_run_config_supports_yaml_roundtripcompares an exact hardcoded dict snapshot.We hit three separate CI failures from this during PR #1321 before getting green.
3.
seed-ssim-references— T2I PNG artefact handlingSkill documented
.mp4(pixel/video) and.pt(latent). T2I tests reuserun_text_to_video_similarity_testbut produce.pngwhennum_frames=1.reference_videos_cli.py copy-localsilently skips PNG (walks.mp4/.ptonly, reports0 copied files)..pngas a third artefact type in the Purpose section.cpworkaround..gitignorenegation pitfall (broad*.pngrule overrides thereference_videos/**allowlist unless the negation is explicitly added after the catch-all).Relevant for the upcoming Z-Image (#1236), SD3.5 (#1150), and FLUX.2 (#1133) ports.
Why each is generalizable
SamplingParamfieldVerification
Each item maps to a specific commit in PR #1321:
5ca0b908— bf16 tolerance fix on FLUX DiT parity (test_flux.py: atol=0.5 + diagnostic prints)d1a63c81— schema parity inventory registrationaabf179d—test_parser.pyroundtrip snapshot fix5b5fdcf9— manual PNG seeding for FLUX T2I SSIMTest plan