testintel: add missing test for Decoder3d.forward (created by linxwang) - #1097
Conversation
Candidate cand:c45125ae274daf08773e3cdcf1dd30c1 at 2f01706.
…ocol Per review: import from vae2_2 (matches the actual Wan 2.2 production decode path), drop the no-cache full-sequence comparisons (not a valid oracle once temporal upsampling is involved), stop asserting 3 output channels (direct Decoder3d output is 12 channels; RGB projection happens later via unpatchify), and replay WanVAE_.decode's real per-frame protocol: one latent frame per call, one feat_cache list reused across calls, feat_idx reset to [0] per frame, first_chunk=True only on the first call, feat_idx[0] == count_conv3d(decoder) after every call, and the temporal-upsample cache slot's "Rep" sentinel transitioning to a real tensor after the second chunk. Also verifies first-chunk vs later chunk frame counts and deterministic replay after reinitializing the cache.
The file was committed as a single Base64-encoded line, which is not valid Python and raises a SyntaxError on collection (flagged by Copilot review). Restore the decoded, unencoded test source; the content itself is unchanged.
Copilot review (discussion_r3923254187): once a cache slot holds a tensor, `slot == "Rep"` raises TypeError (PyTorch tensor vs str comparison) instead of returning False. Gate the comparison on isinstance(slot, str) so it only fires for the sentinel value itself.
code-lint (3.12) failed because black wants the two "Rep" sentinel assertions collapsed to single lines (they fit within the line-length limit once written as one line). Applying the exact reformatting from the failed job's diff; no test logic changed.
Copilot flagged torch.equal as an overly strict/potentially flaky comparison for floating-point tensors across backends. Switch the replay-determinism assertion to torch.testing.assert_close with a tight tolerance, which still catches real cache/index regressions without risking backend-dependent bit-exactness flakiness.
The previous commit (fb3db94) that switched the replay-determinism assertion from torch.equal to torch.testing.assert_close was pushed with the file truncated mid-statement (missing the closing paren and the rest of the file), which black's parser correctly rejected with "Cannot parse: 113:0: EOF in multi-line statement" and failed code-lint (3.12). Restoring the intended, complete call - verified with `black --line-length 110 --target-version py38 --check` and `py_compile` locally before pushing.
Copilot flagged that torch.manual_seed(0) in _make_decoder() mutates global RNG state, which can leak into other tests run afterward in the same process. Wrap the seeding in torch.random.fork_rng() so the global generator is restored once the decoder's weights are initialized.
The previous commit's line-numbered patch miscounted and clobbered the blank lines plus the `def _decode_streaming(...)` header between _make_decoder and its docstring. Restoring them; no logic change.
The last two automated patches miscounted line ranges and clobbered the _decode_streaming body (dropped outputs=[]/for-loop/feat_idx=[0]). Restoring the full, correct file: the only intended change from the last known-good version (40f74e0) is wrapping _make_decoder's seeding in torch.random.fork_rng() per Copilot's global-RNG-mutation finding. No other logic changed.
There was a problem hiding this comment.
🟡 Changes recommended
The new tests don’t yet assert the PR’s stated numerical parity goal (streaming vs single-shot) and include an RNG forking pattern that may unnecessarily initialize GPU runtimes in CPU-only unit tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new unit test module to exercise primus.backends.diffusion.models.wan.vae2_2.Decoder3d.forward under Wan2.2’s streaming/chunked decode protocol, focusing on feat_cache/feat_idx behavior and cache sentinel handling.
Changes:
- Add a focused streaming decode test that validates cache-slot consumption (
feat_idx) and the"Rep"sentinel lifecycle. - Add a determinism test to ensure repeated streaming decodes match after cache reset.
File summaries
| File | Description |
|---|---|
tests/unit_tests/backends/diffusion/test_wan_vae_decoder.py |
New unit tests for Decoder3d.forward streaming/chunked decode cache bookkeeping and determinism. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| out = torch.cat(outputs, dim=2) | ||
| assert out.shape == (1, 12, 5, 8, 8) | ||
| assert torch.isfinite(out).all() |
Address Copilot review comment: torch.random.fork_rng() defaults to forking all CUDA devices when available, which can trigger GPU runtime initialization in this CPU-only unit test. Pass devices=[] to fork only CPU RNG state.
jiagaoxiang
left a comment
jiagaoxiang
left a comment
There was a problem hiding this comment.
TestIntel PR Steward: Replies to Copilot's two findings inline.
| # Seed only within a forked RNG scope so this doesn't mutate global RNG | ||
| # state and leak into other tests run later in the same process. | ||
| with torch.random.fork_rng(devices=[]): | ||
| torch.manual_seed(0) |
There was a problem hiding this comment.
TestIntel PR Steward: Good catch — fixed in 8204f01 by passing devices=[] to torch.random.fork_rng(), so the seeding scope no longer forks CUDA RNG state on GPU-enabled runners.
|
|
||
| out = torch.cat(outputs, dim=2) | ||
| assert out.shape == (1, 12, 5, 8, 8) | ||
| assert torch.isfinite(out).all() |
There was a problem hiding this comment.
TestIntel PR Steward: Declining this one after checking the architecture in vae2_2.py. Resample.forward (mode upsample3d) only performs the temporal 2x doubling inside the if feat_cache is not None: branch — when feat_cache is None it falls through to spatial-only upsampling and the time dimension is left unchanged. So there isn't a "single-shot, no-cache" decode path in this model that reproduces the streamed result: WanVAE_.decode itself always drives Decoder3d frame-by-frame through a shared feat_cache/feat_idx, exactly like this test does. A full-sequence call with a fresh cache and first_chunk=True also isn't equivalent, since the temporal-upsample branch only skips doubling on the very first cache slot access regardless of how many frames are in that call, so it wouldn't reproduce the expected 5-frame streamed output either. Given that, the cache-bookkeeping + determinism assertions already in this PR are the correct/only feasible way to pin down this behavior; a "no-cache reference" comparison would just be comparing against a different, non-equivalent computation. Happy to revisit if you see a concrete construction that sidesteps this.
|
TestIntel PR Steward: Addressed the Copilot review from this run (new head
This PR is still draft, so no merge action taken. Will keep following CI, reviews, and mergeability. |
There was a problem hiding this comment.
🔵 Needs a closer look
The new test suite does not currently assert that streaming/chunked decoding matches an equivalent larger-chunk/single-shot decode output as stated in the PR description, leaving the core acceptance criterion unverified.
Review details
Suppressed comments (1)
tests/unit_tests/backends/diffusion/test_wan_vae_decoder.py:99
- The PR description says this adds coverage that streaming/chunked decode reproduces a non-streaming (larger-chunk / single-shot) decode output, but this test currently only checks shapes and cache sentinel state. Consider adding an output-equality assertion by decoding the same input in fewer (larger) chunks and comparing against the per-frame protocol output; that will catch feat_idx drift/cache-shape regressions even if internal sentinels change.
out = torch.cat(outputs, dim=2)
assert out.shape == (1, 12, 5, 8, 8)
assert torch.isfinite(out).all()
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
The revised test code now follows the actual Wan 2.2 production protocol and no longer uses a no-cache/single-shot decode as an oracle, which is correct. However, the PR body still says the gap is that streaming/chunked decode does not verify reproduction of a single-shot output. That acceptance criterion is invalid for this decoder: the no-cache path does not execute the same temporal-upsample/cache protocol and can have different temporal behavior. Please update the PR description before marking it ready. It should instead say that the test covers the real per-frame |
|
TestIntel PR Steward: Updated the PR description per @WangLingxun's comment — it no longer claims streaming/chunked decode reproduces a single-shot decode output (that oracle is invalid for this decoder, as discussed in the review thread). The description now states the actual coverage: the per-frame Still draft, |
| @@ -0,0 +1,116 @@ | |||
| ########################################################################## | |||
| # Copyright (c) 2025, Advanced Micro Devices, Inc. | |||
There was a problem hiding this comment.
This is a newly added file, so the copyright year should be 2026.
Newly added file should use 2026, not 2025, per review comment.
|
TestIntel PR Steward: Addressed @WangLingxun's review comment on No other new findings from this poll. Still draft; |
There was a problem hiding this comment.
🔵 Needs a closer look
The new tests generate random inputs using the global RNG (outside the forked RNG scope), which can create cross-test RNG coupling and reduce reproducibility.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tests/unit_tests/backends/diffusion/test_wan_vae_decoder.py:63
- The test input
xis generated withtorch.randnoutside a forked RNG scope, which advances the global RNG state and undermines the earlier goal of avoiding RNG leakage into other tests; it also makes failures harder to reproduce.
This issue also appears on line 103 of the same file.
tests/unit_tests/backends/diffusion/test_wan_vae_decoder.py:106
- This test also constructs
xwithtorch.randnusing the global RNG state. Wrapping it infork_rng(optionally with a fixed seed) avoids cross-test RNG coupling and improves reproducibility.
decoder = _make_decoder()
conv_num = count_conv3d(decoder)
x = torch.randn(1, 4, 3, 4, 4)
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Copilot review (2026-09-07) flagged that both tests still draw the decoder input `x` with torch.randn() outside the forked RNG scope, which advances global RNG state and undermines the earlier fix that scoped decoder-init seeding to a forked RNG. Wrap the input draws in torch.random.fork_rng(devices=[]) with a fixed seed too, so runs are reproducible and isolated from other tests in the same process.
|
TestIntel PR Steward: Addressed the latest Copilot review (pullrequestreview-5128066056, on Still draft, no failing checks (only the Read the Docs status check, which is green). Will keep following CI, reviews, and mergeability. |
There was a problem hiding this comment.
🟡 Changes recommended
The new tests’ RNG scoping uses fork_rng(devices=[]) with manual_seed(), which can still leak CUDA/ROCm RNG state on GPU-capable builds and undermine test isolation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
tests/unit_tests/backends/diffusion/test_wan_vae_decoder.py:41
- Same RNG-scoping issue as in
_make_decoder():manual_seed()affects CUDA RNG, butfork_rng(devices=[])doesn't fork/restore it, so this can leak RNG state in GPU-capable test runs. Fork all CUDA devices here too.
# Same rationale as _make_decoder: keep the random draw inside a forked,
# seeded RNG scope so it is reproducible and never leaks into the global
# RNG state that other tests may depend on.
with torch.random.fork_rng(devices=[]):
torch.manual_seed(1)
return torch.randn(1, 4, 3, 4, 4)
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
| # count_conv3d(decoder)). | ||
| # Seed only within a forked RNG scope so this doesn't mutate global RNG | ||
| # state and leak into other tests run later in the same process. | ||
| with torch.random.fork_rng(devices=[]): |
Copilot flagged that torch.manual_seed() also seeds CUDA/ROCm RNG state when a GPU runtime is available, so fork_rng(devices=[]) doesn't restore that CUDA RNG afterward and can leak into other tests on GPU-capable runners. Fork torch.cuda.device_count() devices when CUDA is available, and no devices when it isn't (avoiding CUDA runtime init on CPU-only runners, per the earlier Copilot comment this replaces).
|
TestIntel PR Steward: Addressed the latest Copilot review comment on Fixed by adding a Still draft, no failing checks (only the Read the Docs status, which is green). |
There was a problem hiding this comment.
🟢 Approval recommended
The change is isolated to a new unit test file and the test logic aligns with the current WanVAE_.decode chunked-decode protocol without impacting production code.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Same-repo refile of #1084 so Primus-CI-TAS runs against
AMD-AGI/Primus(fork PRs do not receive Docker Hub credentials, sobuild-docker/ torch unit tests never ran).This PR was created by linxwang via Test Gap Resolver.
The feat_cache/feat_idx chunked-decode bookkeeping in Decoder3d.forward (and the CausalConv3d/Resample/ResidualBlock layers it drives) had no test coverage, leaving index-drift or cache-shape regressions undetected.
This PR adds a focused unit test for
Decoder3d.forwardcovering the real per-frameWanVAE_.decodeprotocol: a sharedfeat_cache,feat_idxreset per frame,first_chunkhandling, cache-slot/sentinel transitions (the Rep marker), temporal output counts, and deterministic replay after a cache reset. A no-cache/single-shot decode is not a valid oracle here:Resample.upsample3donly performs temporal doubling inside the feat_cache-is-not-None branch, so there is no equivalent non-streaming decode path to compare against (see the review-thread discussion). This PR does not change production code.