Skip to content

Commit 4c13458

Browse files
committed
[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.
1 parent 850325c commit 4c13458

2 files changed

Lines changed: 42 additions & 17 deletions

File tree

.agents/skills/add-model-02-parity/SKILL.md

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -212,11 +212,22 @@ and assert that the *distribution* — not just the max — looks healthy:
212212

213213
```python
214214
abs_diff = (hf_out.float().cpu() - fv_out.float().cpu()).abs()
215-
print(f"max_diff={abs_diff.max():.4f} mean_diff={abs_diff.mean():.4f} "
216-
f"median_diff={abs_diff.median():.4f} "
217-
f"p99_diff={abs_diff.flatten().kthvalue(int(0.99 * abs_diff.numel())).values:.4f}")
215+
max_diff = abs_diff.max().item()
216+
mean_diff = abs_diff.mean().item()
217+
median_diff = abs_diff.median().item()
218+
p99_diff = torch.quantile(abs_diff.flatten(), 0.99).item()
219+
print(
220+
f"max_diff={max_diff:.4f} mean_diff={mean_diff:.4f} "
221+
f"median_diff={median_diff:.4f} p99_diff={p99_diff:.4f}"
222+
)
218223
```
219224

225+
Call `.item()` before formatting (older PyTorch versions do not implement
226+
`__format__` on 0-dim tensors), and prefer `torch.quantile` over
227+
`kthvalue(int(0.99 * N))``kthvalue` is 1-indexed and degenerates for
228+
very small `N`, while `quantile` handles the distribution boundary
229+
cleanly.
230+
220231
Healthy bf16-tail signature (FLUX, 57 layers, observed on A40):
221232
`max=0.5, mean=0.04, median=0, p99=0.25`. Median near zero and mean ≪ atol
222233
prove the bulk of elements match — only the tail diverges due to accumulation.

.agents/skills/seed-ssim-references/SKILL.md

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,15 @@ side-by-side per `(model_id, backend, prompt)`:
1919
metadata + `slice_spec` + `format_version`) for tests that call
2020
`run_text_to_latent_similarity_test` in `latent_similarity_utils.py`.
2121
Compared via cosine distance on the slice and the full tensor.
22-
- **`.png`** — pixel ground-truth for **T2I** tests (`num_frames=1`) that
23-
reuse `run_text_to_video_similarity_test` but produce a single frame. The
24-
helper writes a `.png` instead of a `.mp4` when the output has no time
25-
dimension. Compared via SSIM the same way.
22+
- **`.png`** — pixel ground-truth for **T2I / I2I** tests. `VideoGenerator`
23+
emits a `.png` (not a `.mp4`) whenever `workload_type.value.endswith("2i")`
24+
is true (`fastvideo/entrypoints/video_generator.py::_is_image_workload`).
25+
**The SSIM helpers in `inference_similarity_utils.py` on current main
26+
hardcode `.mp4`** (`_find_reference_video` filter, `output_video_name`
27+
format). To consume `.png` references through the SSIM path, the test
28+
must call a helper variant that accepts a `media_extension` parameter
29+
(added in PR #1321). Until that lands on main, T2I SSIM coverage is
30+
blocked at the helper layer.
2631

2732
This skill:
2833

@@ -115,22 +120,31 @@ and 6 are artefact-type-agnostic — `_iter_reference_files`,
115120
`copy_generated_to_reference`, and `upload_reference_videos` already walk
116121
both `.mp4` and `.pt` (see `reference_videos_cli.py`).
117122

118-
**T2I gotcha:** when the helper produces a `.png` (T2I test with
119-
`num_frames=1`), `reference_videos_cli.py copy-local` currently walks
120-
`.mp4`/`.pt` only and reports `0 copied files` without erroring. After step 5,
121-
verify the destination contains the seeded file; if it's empty, copy the PNG
122-
manually:
123+
**T2I / I2I gotcha:** when the helper produces a `.png` (any workload
124+
whose `workload_type.value` ends in `2i`), `reference_videos_cli.py copy-local`
125+
currently walks `.mp4` / `.pt` only and reports `0 copied files` without
126+
erroring. After step 5, verify the destination contains the seeded file;
127+
if it's empty, copy the PNG manually:
123128

124129
```bash
130+
mkdir -p fastvideo/tests/ssim/reference_videos/default/L40S_reference_videos/<model_id>/<backend>/
125131
cp ./generated_videos_modal/default/generated_videos/L40S_reference_videos/<model_id>/<backend>/*.png \
126132
fastvideo/tests/ssim/reference_videos/default/L40S_reference_videos/<model_id>/<backend>/
127133
```
128134

129-
Then `git add -f` the PNG: the repo `.gitignore` has a broad `*.png` rule, and
130-
the `reference_videos/**` negation only applies to extensions explicitly
131-
re-allowed *after* the catch-all (see `.gitignore` near the
132-
`reference_videos/**` block — add `!fastvideo/tests/ssim/reference_videos/**/*.png`
133-
there if the negation is missing).
135+
`git add -f` is **not required for the HF upload path**:
136+
`reference_videos_cli.py upload` calls `huggingface_hub.upload_folder` on
137+
the filesystem directly (`reference_videos_cli.py:311`) — git tracking
138+
status is irrelevant for `pytest fastvideo/tests/ssim/` reference sync.
139+
140+
If you *also* want to commit the PNG locally, note that
141+
`fastvideo/tests/ssim/reference_videos/**` (catch-all near `.gitignore:94`)
142+
overrides the earlier `!fastvideo/tests/ssim/reference_videos/**/*.mp4`
143+
negation at `.gitignore:73`. Every reference file in this dir currently
144+
requires `git add -f`, regardless of extension. Fixing this properly
145+
means moving the `!...reference_videos/**/*.{mp4,pt,png}` negations
146+
*after* the catch-all (or removing the catch-all). Until then, `git add -f`
147+
is the standing workaround for committing references locally.
134148

135149
If either check fails, stop and tell the user what's wrong.
136150

0 commit comments

Comments
 (0)