[bugfix]: build_eval_kwargs returns (T, C, H, W) per metric contract - #1412
[bugfix]: build_eval_kwargs returns (T, C, H, W) per metric contract#1412klhhhhh wants to merge 11 commits into
Conversation
![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 basic LTX2 audio evaluation script to conditionally generate the video only if it does not already exist, and improves the handling of both per-sample and corpus-level metrics. It also updates the documentation for the audio.desync metric. A critical issue was identified where evaluating corpus-level metrics like audio.frechet_distance with a single-sample call raises a ValueError, and using the list form causes per-sample metrics to be incorrectly reported as missing. A suggestion was provided to use the list-based evaluation format and check the first sample's results.
| results = evaluator.evaluate(audio=output_path, text_prompt=PROMPT) | ||
|
|
||
| print("\n=== Audio scores ===") | ||
| for name in METRICS: | ||
| r = results[name] | ||
| r = None | ||
|
|
||
| # per-sample metric | ||
| if hasattr(results, "__contains__") and name in results: | ||
| r = results[name] | ||
|
|
||
| # corpus-level metric (e.g. audio.frechet_distance) | ||
| elif hasattr(results, "corpus") and name in results.corpus: | ||
| r = results.corpus[name] |
There was a problem hiding this comment.
The current implementation has two issues when supporting both per-sample and corpus-level metrics (like audio.frechet_distance):
- ValueError on evaluation: If
audio.frechet_distanceis added toMETRICS, callingevaluator.evaluate(audio=...)(single-sample form) raises aValueErrorbecause set-vs-set metrics require the list/samples form. - Per-sample metrics reported as MISSING: If the list form
samples=[...]is used,resultsbecomes anEvalResults(list subclass). The checkname in resultswill evaluate toFalsebecausenameis a string and the list contains dictionaries, causing all per-sample metrics to be reported asMISSING.
Using the list form in evaluate and checking results[0] for per-sample metrics resolves both issues.
| results = evaluator.evaluate(audio=output_path, text_prompt=PROMPT) | |
| print("\n=== Audio scores ===") | |
| for name in METRICS: | |
| r = results[name] | |
| r = None | |
| # per-sample metric | |
| if hasattr(results, "__contains__") and name in results: | |
| r = results[name] | |
| # corpus-level metric (e.g. audio.frechet_distance) | |
| elif hasattr(results, "corpus") and name in results.corpus: | |
| r = results.corpus[name] | |
| results = evaluator.evaluate(samples=[{"audio": output_path, "text_prompt": PROMPT}]) | |
| print("\n=== Audio scores ===") | |
| for name in METRICS: | |
| r = None | |
| # corpus-level metric (e.g. audio.frechet_distance) | |
| if hasattr(results, "corpus") and name in results.corpus: | |
| r = results.corpus[name] | |
| # per-sample metric | |
| elif len(results) > 0 and name in results[0]: | |
| r = results[0][name] |
|
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI
🔴 PR merge requirementsWaiting for
This rule is failing.
|
|
This change fixes build_eval_kwargs to match the evaluator's expected The helper previously introduced an extra batch dimension on the video |
Wan2.1-T2V-1.3B VBench BaselineSingle-video sanity evaluation using the built-in VBench metrics on a Evaluation SetupModel: Metrics:
Results
Observations
ConclusionThe Wan2.1-T2V-1.3B baseline demonstrates strong temporal stability, |
1a7d4b3 to
fdc45b1
Compare
|
@SolitaryThinker @shaoxiongduan Hi Will and Shao, I've addressed the previous comments and pushed the latest updates. Could you please take a look when you have a chance? Thanks! |
|
Hi @klhhhhh — automated review from Gob, one of @SolitaryThinker's AI reviewers. Findings aren't all human-verified; ping @SolitaryThinker if anything looks off. TL;DRThe Verdict: ship-with-fixes
Findings (formatted for upload)[S1]
|
fdc45b1 to
b31f3ba
Compare
|
@SolitaryThinker @shaoxiongduan , all done, I delete the wan eval script and add some reference in ltx2 doc string. |
cf0ca0f to
d036569
Compare
SolitaryThinker
left a comment
SolitaryThinker
left a comment
There was a problem hiding this comment.
The core fix here is correct and needed: the metric contract is explicitly 4-D — BaseMetric.compute documents sample["video"] as (T, C, H, W) (fastvideo/eval/metrics/base.py:71) and nothing between the pool and the metrics strips a batch dim, so the old unsqueeze(0) was feeding every metric a shape it never expected. The only in-repo caller (basic_ltx2_eval.py) is updated consistently, and the README addition for audio.desync requiring fps checks out against the metric (desync/metric.py:203 skips without it).
Requesting changes for the items below (details inline):
paths.pydocstring documents backward compat that doesn't exist — there is no(1, T, C, H, W)auto-squeeze anywhere infastvideo/eval/. Either delete the claim or add the squeeze.- The audio example's corpus branch is unreachable, and the frechet claim in the PR description isn't delivered — the script still uses the kwargs form of
evaluate(), which returns a plain dict and raisesValueErrorif a set metric is registered. - PR description vs. diff mismatch — the description says this PR "adds a new Wan2.1-T2V-1.3B VBench evaluation example", but no such file is in the diff (only a docstring note that the LTX2 example can serve as a template). Please push the missing file or update the description.
- No test locks the new contract —
build_eval_kwargschanged its public output shape and was already wrong once. A small test infastvideo/tests/eval/assertingbuild_eval_kwargs({"prompt": "x"}, path)["video"].ndim == 4would have caught the original bug and prevents regression.
Nit: the title doesn't follow the repo's [tag]: convention — this is really [bugfix]: build_eval_kwargs returns (T,C,H,W) per metric contract rather than a refactor.
| The returned ``video`` tensor has shape ``(T, C, H, W)`` in ``[0, 1]``. | ||
| The eval pool still auto-squeezes legacy ``(1, T, C, H, W)`` callers | ||
| for backward compatibility. |
There was a problem hiding this comment.
This backward-compat claim doesn't match the code: there is no auto-squeeze anywhere in fastvideo/eval/. VideoPool._decode only populates Video.frames and passes raw tensors through untouched, EvalWorker.evaluate doesn't reshape, and load_video raises on ndim != 4. A legacy caller passing (1, T, C, H, W) gets garbage metric scores or a crash, not a squeeze.
Either drop the sentence or actually add the squeeze (e.g. an ndim == 5 check in EvalWorker.evaluate next to the Video unwrapping). Documenting compat that doesn't exist is worse than either option.
Nit: lines 47 and 52 state the same "(T, C, H, W) in [0, 1]" fact twice — one can go.
| for name in METRICS: | ||
| r = results[name] | ||
| r = None | ||
|
|
||
| # per-sample metric | ||
| if hasattr(results, "__contains__") and name in results: | ||
| r = results[name] | ||
|
|
||
| # corpus-level metric (e.g. audio.frechet_distance) | ||
| elif hasattr(results, "corpus") and name in results.corpus: | ||
| r = results.corpus[name] | ||
|
|
||
| if r is None: | ||
| print(f" {name}: MISSING") | ||
| continue |
There was a problem hiding this comment.
The corpus branch here is unreachable, and the frechet story in the PR description isn't actually delivered:
- This script calls the kwargs form
evaluator.evaluate(audio=..., text_prompt=...)(line 69), which returns a plaindict[str, MetricResult]— it never has a.corpusattribute..corpusonly exists onEvalResults, returned by thesamples=[...]form. - If
audio.frechet_distanceis added toMETRICS, the evaluator raisesValueErrorbefore any results exist —Evaluator.evaluaterejects set metrics in kwargs form (evaluator.py:169). So this branch can't save the crash the PR description says it avoids. hasattr(results, "__contains__")is always true for a dict.
Either switch the example to evaluate(samples=[...]) with ≥2 samples so audio.frechet_distance actually works, or drop the dead branch and keep the previous direct results[name] lookup.
| if r.score is None: | ||
| print(f" {name}: SKIPPED ({r.details.get('skipped', 'no score')})") | ||
| skipped = ( | ||
| r.details.get("skipped", "no score") | ||
| if isinstance(r.details, dict) | ||
| else "no score" | ||
| ) | ||
| print(f" {name}: SKIPPED ({skipped})") | ||
| else: | ||
| print(f" {name}: {r.score:.4f}") | ||
| if r.details: | ||
| for k, v in r.details.items(): | ||
| print(f" {k}: {v}") | ||
|
|
||
| if r.details: | ||
| for k, v in r.details.items(): | ||
| print(f" {k}: {v}") |
There was a problem hiding this comment.
Two things in this block:
isinstance(r.details, dict)is always true —MetricResult.detailsis a dataclass field withdefault_factory=dict, so it can't be anything else. The previousr.details.get("skipped", "no score")was already safe.- Moving the details loop out of the
elsemeans a skipped metric now prints its reason twice:SKIPPED (reason)on line 93, thenskipped: reasonagain in the details dump.
|
|
||
| from fastvideo import VideoGenerator | ||
| from fastvideo.eval import create_evaluator | ||
| from pathlib import Path |
There was a problem hiding this comment.
stdlib import — belongs in the first import block, above torch/fastvideo. (examples/ is pre-commit-excluded so CI won't flag it, but let's keep the ordering.)
| from fastvideo import VideoGenerator | ||
| from fastvideo.eval import Evaluator | ||
| from fastvideo.eval.io import build_eval_kwargs | ||
| from pathlib import Path |
There was a problem hiding this comment.
Same as the audio example — stdlib import goes in the first block, above torch/fastvideo.
| "Davids048/LTX2-Base-Diffusers", | ||
| num_gpus=1, | ||
| ) | ||
| if output_file.exists() and output_file.suffix.lower() == ".mp4": |
There was a problem hiding this comment.
output_file.suffix.lower() == ".mp4" is dead code — output_path is a hardcoded .mp4 literal three lines up. output_file.exists() is enough.
SolitaryThinker
changed the title
All findings addressed directly in 2e5d2ee: removed the false auto-squeeze docstring claim, dropped the dead corpus branch / always-true guards (fixing the double-printed skip reason), fixed import ordering and the dead .mp4 suffix check, added a contract test for the (T,C,H,W) shape, and updated the PR description to match the diff (the Wan example was removed in d036569).
Pre-commit checks failedHi @klhhhhh, the pre-commit checks have failed. To fix them locally: # Install pre-commit if you haven't already
uv pip install pre-commit
pre-commit install
# Run all checks and auto-fix what's possible
pre-commit run --all-filesCommon fixes:
After fixing, commit and push the changes. The checks will re-run automatically. For future commits, |
…add contract test - paths.py: remove the claim that the pool auto-squeezes legacy (1,T,C,H,W) input — no such code exists anywhere in fastvideo/eval/ — and dedupe the shape sentence. - basic_ltx2_audio_eval.py: drop the unreachable .corpus branch and always-true hasattr/isinstance guards (kwargs-form evaluate() returns a plain dict and rejects set metrics), restoring the direct results[name] lookup; fixes the double-printed skip reason. - both examples: stdlib import ordering; drop the dead .mp4 suffix check on a hardcoded .mp4 path. - new fastvideo/tests/eval/test_build_eval_kwargs.py locks the (T,C,H,W) no-batch-dim contract (CPU-only, PNG frame dir — no video decoder needed).
2e5d2ee to
f8d1ac8
Compare
3 participants
Summary
This PR updates the eval input helper
build_eval_kwargsto returnvideo tensors as
(T, C, H, W)instead of adding a batch dimension.This matches the one-sample input contract used by
Evaluator.evaluate(**sample)and documented on
BaseMetric.compute— the previous(1, T, C, H, W)shapewas feeding every metric a batch dim they never expected.
It also updates the LTX2 eval example to the frame-axis convention and
documents how to reuse it as a template for other T2V models (e.g.
Wan2.1-T2V-1.3B) by swapping the model name and generation parameters.
Documentation updates
Clarifies audio metric input contracts that were confusing during
real-world smoke testing:
1.
audio.frechet_distanceis corpus-level onlyUnlike most
audio.*metrics,audio.frechet_distanceis aset-vs-set metric and must be evaluated through
ev.evaluate(samples=[...])(the kwargs form raises for set metrics). The README now documents that
the result lives under
results.corpus["audio.frechet_distance"].2.
audio.desyncrequiresfpsThe docs previously only mentioned
videoandaudio, but the metricskips without
fps(orsrc_fps). The README now documents this.Example improvements
Both LTX2 eval examples now check whether the target output video
already exists before launching generation. If the mp4 is present, the
expensive generation step is skipped and the existing video is reused —
convenient for iterative metric debugging where metrics are re-run on
the same generated sample.
The audio example keeps to per-sample metrics (
audio.clap_score,audio.audiobox_aesthetics); set-vs-set metrics likeaudio.frechet_distanceneed thesamples=[...]form per the README.Review follow-up (2e5d2ee)
(1, T, C, H, W)input — no such compat path exists infastvideo/eval/..corpusbranch and always-true guards from theaudio example's result printing (kwargs-form
evaluate()returns a plaindict), which also fixes a double-printed skip reason.
.mp4suffixcheck on a hardcoded
.mp4path.fastvideo/tests/eval/test_build_eval_kwargs.pylocking the(T, C, H, W)no-batch-dim contract (CPU-only, PNG frame dir — no videodecoder or GPU needed).
Test evidence
fastvideo/eval/io/{paths,video}.pyin a CPU-only torch env):
test_build_eval_kwargs_video_is_4d_tchw,test_build_eval_kwargs_omits_absent_row_keys.pre-commit runpasses on the lint-covered touched files(
fastvideo/eval/io/paths.py, the new test).