Repository navigation
Conversation
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI
🔴 PR merge requirementsWaiting for
This rule is failing.
|
[Medium impact] The change affects MiniMax-H3 VAE decoding workflows. The remaining comment-rule violation must be addressed before merging; no new correctness issue was found. Findings
Fix with agent prompt### Issue 1
fastvideo/tests/vaes/test_minimax_h3_video_vae_streaming.py:undefined-188
This new docstring repeats the test name and assertion. The repository requires removing code comments outside its explicit keep-list; test narration and private helper docs do not qualify. Remove this docstring and the same narration in `test_batched_tile_decode_matches_per_tile` and `_tile_batch_size`. The expanded compile-test docstring has the same issue. This repository requirement must be satisfied before merging.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
|
|
||
|
|
||
| def test_tile_batch_defaults_to_eight(monkeypatch) -> None: | ||
| """The shipped default decodes eight spatial tiles per decoder call.""" |
There was a problem hiding this comment.
Comments violate repository rule
This new docstring repeats the test name and assertion. The repository requires removing code comments outside its explicit keep-list; test narration and private helper docs do not qualify. Remove this docstring and the same narration in test_batched_tile_decode_matches_per_tile and _tile_batch_size. The expanded compile-test docstring has the same issue. This repository requirement must be satisfied before merging.
Rule Used: What: Remove all unwanted code comments in the scoped diff except for a small keep-list (license headers, dependency/protocol-required non-obvious behavior, // prettier-ignore, public API doc comments, and issue/RFC constraint links). Why: Comment... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: fastvideo/tests/vaes/test_minimax_h3_video_vae_streaming.py
Line: 188
Comment:
**Comments violate repository rule**
This new docstring repeats the test name and assertion. The repository requires removing code comments outside its explicit keep-list; test narration and private helper docs do not qualify. Remove this docstring and the same narration in `test_batched_tile_decode_matches_per_tile` and `_tile_batch_size`. The expanded compile-test docstring has the same issue. This repository requirement must be satisfied before merging.
**Rule Used:** What: Remove all unwanted code comments in the scoped diff except for a small keep-list (license headers, dependency/protocol-required non-obvious behavior, `// prettier-ignore`, public API doc comments, and issue/RFC constraint links). Why: Comment... ([source](https://app.greptile.com/hao-ai-lab/-/custom-context?memory=856788fb-b8e1-47f8-b01b-d83ea205fc6b))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 78a83c8: removed the added test docstrings and the _tile_batch_size docstring, and reverted the compile-test docstring.
Summary
How this was found. These changes came out of an agent-driven kernel optimization run with MotionKernel, a tool we're developing that profiles a real generation, finds where the time goes, and only keeps changes that hold up end to end with the quality checks below.
Problem
FASTVIDEO_H3_VAE_TILE_BATCHalready groups the spatial tiles of one temporal chunk into asingle decoder call, but it defaulted to
1, i.e. one small ViT forward per tile: a 768x1344clip is an 18-tile grid over 7 temporal chunks, so the default issues 126 batch-1 decoder calls
per video. The H3 VAE decoder treats batch entries independently (no cross-tile interaction), and
32 heads x 64 dims is a much better shape for the GEMMs than 1 x 1797 tokens.
Fix
Change the default to 8 and document the interaction with the tile-split parallel decode. The env
override is unchanged (
FASTVIDEO_H3_VAE_TILE_BATCH=1restores per-tile decoding), and theexisting batched path (identical tile shapes only) is used automatically.
FASTVIDEO_H3_VAE_TILE_PARALLELreplays the per-tile serial forwards, so it now documents thatit requires
FASTVIDEO_H3_VAE_TILE_BATCH=1; its tests set that explicitly.Evidence
Decode-only, one loaded VAE, warm, interleaved arms, GB200 (768x1344, fp16 autocast, real latents
encoded by the shipped H3 VAE from
assets/videos/robot_pouring.mp4):0ddbf20b.../ad9a1b09...), and a separate fp32 comparison at the production shape findstorch.equal(tb1, tb8) == Truewithmax_abs = 0.0.Caveat: at other geometries a batched launch can select different attention/GEMM kernels, so the
bitwise property is a measured property of the production shape, not a theorem (the CPU unit test
uses a 1e-6 tolerance for that reason).
Tests
yapf -d/ruff check/codespellclean on the changed files (models/ and tests/ arepre-commit-excluded, formatted to match neighbors).
Verification on current main (d9e6c3e): the patch applies cleanly; pre-commit (yapf, ruff, codespell, PyMarkdown, mypy) passes on all changed files; the tests listed above were run on GB200.
Independent GB200 verification (after the review fixes)
An agent rechecked this PR on a separate GB200 with SDPA attention: