Skip to content

[Issue 101][eval-and-fix] Fix VideoAutoencoderKLWrapper.forward return contract (closes pollockjj/mydevelopment#192)#46

Merged
pollockjj merged 2 commits into
issue_101from
issue_192
May 5, 2026
Merged

[Issue 101][eval-and-fix] Fix VideoAutoencoderKLWrapper.forward return contract (closes pollockjj/mydevelopment#192)#46
pollockjj merged 2 commits into
issue_101from
issue_192

Conversation

@pollockjj
Copy link
Copy Markdown
Owner

Plan: Fix VideoAutoencoderKLWrapper.forward to honor decode tensor return contract

Overview

VideoAutoencoderKLWrapper.forward() on pollockjj/ComfyUI:issue_101 (HEAD 80c91503, post-Comfy-Org#190) calls self.decode(z).sample, but VideoAutoencoderKLWrapper.decode() returns a plain torch.Tensor (per the post-Comfy-Org#189 / PR #34 normalization). Direct wrapper invocation via forward(x) therefore raises AttributeError: 'Tensor' object has no attribute 'sample' on the existing decode(z).sample line. The fix removes the spurious .sample access so forward returns the decoded tensor directly. A regression test exercises wrapper.forward(x) end-to-end and asserts the returned (x_out, z, p) triple is well-formed — binary type/shape equality plus a source-introspection check that .sample is absent from the post-fix forward body. This plan is the wrapper-class sibling of merged PR #44 (closed Comfy-Org#190); the parent class VideoAutoencoderKL.forward is already fixed and is explicitly off-limits here.

Diagnosis Summary

The buggy method body, captured live from issue_101:comfy/ldm/seedvr/vae.py lines 2228–2232 (Ground Truth Probe GTP-3):

def forward(self, x: torch.FloatTensor):
    with torch.no_grad() if self.freeze_encoder else nullcontext():
        z, p = self.encode(x)
    x = self.decode(z).sample
    return x, z, p

Live AST probe of VideoAutoencoderKLWrapper (GTP-1) confirms the class defines __init__, forward, encode, decode, and set_memory_limit — i.e. decode (no underscore) IS a member of the wrapper subclass (unlike the parent VideoAutoencoderKL, which only has decode_), so the call self.decode(z) itself resolves correctly. The bug is the .sample dereference on the decode return. The wrapper's decode() body (GTP-4) returns a plain torch.Tensor (the post-rearrange / lab-color-transferred output) at the final return x statement; nothing in that return chain produces an object with a .sample attribute. Every direct invocation of wrapper.forward(x) raises AttributeError: 'Tensor' object has no attribute 'sample' immediately after self.decode(z) returns.

Reproduction command: `cd /home/johnj/dev_cuda_1/ComfyUI && python3 -m pytest tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py -v`
Observed failure: pre-fix run raises `AttributeError: 'Tensor' object has no attribute 'sample'` from the wrapper.forward body, captured as the AC-2 pre-fix log after `git checkout issue_101 -- comfy/ldm/seedvr/vae.py` reverts the working tree.

The pre-fix run against issue_101:comfy/ldm/seedvr/vae.py raises AttributeError: 'Tensor' object has no attribute 'sample' for every test path that exercises wrapper.forward(x). The same invocation post-fix exits 0 with 2 passed. AC-1 / AC-2 capture this A/B pair as committed pytest logs. The fix is a one-line edit inside wrapper.forward: replace x = self.decode(z).sample with x = self.decode(z). No other method body, no other class, no signature change.

Affected Repositories

Repository Path Base Branch Issue Branch
pollockjj/ComfyUI /home/johnj/dev_cuda_1/ComfyUI issue_101 issue_192
pollockjj/mydevelopment /home/johnj/dev_cuda_1/mydevelopment main issue_192

pollockjj/ComfyUI is the deliverable: the fix to comfy/ldm/seedvr/vae.py and the new regression test under tests-unit/comfy_test/ land here on issue_192 (cut from issue_101 HEAD 80c91503) and PR back into issue_101. This mirrors PR #44 (issue_190issue_101) and PR #34 (issue_189issue_101).

pollockjj/mydevelopment is the bookkeeping target: every committed artifact under github_issues/192/slice1/ (pytest logs, AST source extracts, grep transcripts, scope-bounded diff, companion-regression log) lives on the outer workspace's issue_192 branch and PRs back into main as the standard [bookkeeping] [Issue 101][eval-and-fix] pattern (see merged PRs Comfy-Org#197Comfy-Org#201 and Comfy-Org#205Comfy-Org#207 for the precedent).

Asset Readiness

Asset Location Provenance Status Evidence
ComfyUI deliverable checkout /home/johnj/dev_cuda_1/ComfyUI pollockjj/ComfyUI branch issue_101 -> issue_192 VERIFIED branch/path verified before slice execution; HEAD of origin/issue_101 is 80c91503 per git log --oneline origin/issue_101 -1
Python test environment /home/johnj/dev_cuda_1/ComfyUI/.venv/ local-only with no upstream VERIFIED pytest/torch invocation succeeds on slot per CLAUDE.md slot layout; sister test seedvr_vae_forward_test.py already exits 0 against this same venv on origin/issue_101
Base-branch source under test issue_101:comfy/ldm/seedvr/vae.py pollockjj/ComfyUI issue_101 VERIFIED GTP-1, GTP-2, GTP-3, GTP-4
Sister regression test under coverage issue_101:tests-unit/comfy_test/seedvr_vae_forward_test.py pollockjj/ComfyUI issue_101 (merged via PR #44) VERIFIED GTP-5 confirms file existence at base-branch HEAD
Wrapper-decode-guard regression coverage issue_101:tests-unit/comfy_test/test_seedvr_vae_decode_guards.py pollockjj/ComfyUI issue_101 (merged via PR #43) VERIFIED GTP-5
Companion regression: decode batch axes issue_101:tests-unit/comfy_test/test_seedvr_vae_decode_batch_axes.py pollockjj/ComfyUI issue_101 (merged via PR #34) VERIFIED GTP-5 confirms file existence at base-branch HEAD
Companion regression: decode unpadded t issue_101:tests-unit/comfy_test/test_seedvr_vae_decode_unpadded_t.py pollockjj/ComfyUI issue_101 (merged via PR #35) VERIFIED GTP-5 confirms file existence at base-branch HEAD
Companion regression: tiled args no mutate issue_101:tests-unit/comfy_test/test_seedvr_vae_tiled_args_no_mutate.py pollockjj/ComfyUI issue_101 VERIFIED GTP-5 confirms file existence at base-branch HEAD
Companion regression: loader metadata issue_101:tests-unit/comfy_test/test_seedvr_vae_loader_metadata.py pollockjj/ComfyUI issue_101 (merged via PR #45) VERIFIED GTP-5 confirms file existence at base-branch HEAD
Bookkeeping workspace /home/johnj/dev_cuda_1/mydevelopment pollockjj/mydevelopment main -> issue_192 VERIFIED artifact paths under github_issues/192/slice1/ are writable/tracked on issue_192

Research and Methodology

Plan Foundations Comment URL: https://github.com/pollockjj/mydevelopment/issues/192#issuecomment-4382275426

Plan Foundations Comment Pin: comment-id=IC_kwDOR2e1q88AAAABBTQ3Yg edited-at=2026-05-05T19:17:44Z

Detected Scope: none — byte-identical port of the established Comfy-Org#190 / PR #44 pattern to the immediately-adjacent subclass; the existing wrapper decode() return contract IS the spec, and the fix duplicates that contract correctly inside wrapper.forward() and adds a regression test that locks the contract.

The change class is "honor an existing internal API contract." No external research, no canonical references, no metric definitions, and no published anchor are invoked. The equivalence rule is binary type/shape equality on the tensors returned by wrapper.forward()type(x_out) is torch.Tensor, type(z) is torch.Tensor, type(p) is torch.Tensor, x_out.shape == decode_out.shape, z.shape == posterior.squeeze(2).shape, plus p is posterior (identity preservation of the encode-side tuple member). No tolerance bands; no statistical tests. Source authority is the live source of comfy/ldm/seedvr/vae.py on the issue's base branch (issue_101 HEAD 80c91503), captured verbatim under ## Ground Truth Probes.

Binding parent-issue directives quoted verbatim from pollockjj/mydevelopment#192 issue body (the tracking issue this plan finalizes):

"Existing call-sites of VideoAutoencoderKLWrapper.encode() / .decode() from comfy_extras.nodes_seedvr and from internal SeedVR2 pipelines remain bit-identical to pre-fix behavior under their existing test coverage."

"Adjacent VideoAutoencoderKL.forward is already fixed by Comfy-Org#190 / PR #44; do NOT modify that method."

"Adjacent VideoAutoencoderKLWrapper.decode was modified by Comfy-Org#189 / PR #34 (tiled-path normalization); do NOT modify that method."

"Any change to encode() / decode_() signatures or return shapes is out of scope and indicates plan drift — stop and surface."

"VideoAutoencoderKLWrapper.decode() requires self.original_image_video set (raises RuntimeError if None per the post-Comfy-Org#189 guard); the regression harness must set this attribute. Skipping this trips the wrapper's own guard before the bug under test surfaces."

These five directives bound the scope of every AC below. The wrapper.encode / wrapper.decode methods remain bit-identical; the parent class VideoAutoencoderKL (forward, encode, decode_, slicing_, tiled_) is untouched; signatures of encode() / decode_() / decode() / forward() are preserved verbatim; the test harness sets original_image_video to a 5-D tensor on the standin even when the stub bypasses the guard, so the test honors Boss's directive on guard-state setup.

Tools, Pipeline, and Measurements

Plan Foundations Comment URL: https://github.com/pollockjj/mydevelopment/issues/192#issuecomment-4382275426

Plan Foundations Comment Pin: comment-id=IC_kwDOR2e1q88AAAABBTQ3Yg edited-at=2026-05-05T19:17:44Z

Existing tooling — REUSE only. Trivial scope; no external tools beyond the change itself.

Tool / Dep / Repo Version / SHA Status Citation
pytest repo tests-unit/requirements.txt pin (per tests-unit/README.md) REUSE git show issue_101:tests-unit/README.md documents pytest tests-unit/ as the canonical invocation; sister test seedvr_vae_forward_test.py exercises the same venv
torch repo .venv/ pin on the deliverable slot /home/johnj/dev_cuda_1/ComfyUI/.venv/ REUSE already on the slot per CLAUDE.md slot layout
comfy.ldm.seedvr.vae.VideoAutoencoderKLWrapper issue_101 HEAD 80c91503 REUSE live AST + import probe captured as GTP-1
git system REUSE for diff/log/show/checkout/restore artifacts
stdlib ast, inspect, re Python interpreter on .venv/ REUSE for source-shape introspection (mirrors sister test pattern)

Pipeline: edit one method body in one source file (comfy/ldm/seedvr/vae.py, VideoAutoencoderKLWrapper.forward, lines 2228–2232 on the issue_101 side) → add one new test file tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py → run pytest against the new test on issue_192 HEAD (post-fix capture) and against the same test with vae.py reverted to issue_101:comfy/ldm/seedvr/vae.py (pre-fix capture) → commit pytest logs, AST-extracted forward source, grep transcripts, and scope-bounded git diff to github_issues/192/slice1/ on the bookkeeping branch.

Measurements: the only "measurement" is binary type and shape equality on the three tensor returns of wrapper.forward() plus identity preservation of the encode-side posterior p. There is no metric, no anchor, no statistical test, no sweep, and no characterization verdict. Single-probe and boundary-bracketing requirements (HR-06, HR-11) do not apply.

Ground Truth Probes

Probe ID Consumed by AC(s) Probe command Observed output (verbatim)
GTP-1 Slice 1 AC-3, AC-4, AC-5 cd /home/johnj/dev_cuda_1/ComfyUI && git show origin/issue_101:comfy/ldm/seedvr/vae.py | python3 -c 'import ast,sys; tree=ast.parse(sys.stdin.read()); [print("class VideoAutoencoderKLWrapper methods:", [m.name for m in n.body if isinstance(m,(ast.FunctionDef,ast.AsyncFunctionDef))]) for n in ast.walk(tree) if isinstance(n,ast.ClassDef) and n.name=="VideoAutoencoderKLWrapper"]' class VideoAutoencoderKLWrapper methods: ['__init__', 'forward', 'encode', 'decode', 'set_memory_limit'] — i.e. decode (no underscore) IS a member of the wrapper subclass (unlike the parent VideoAutoencoderKL, which only has decode_), so self.decode(z) inside wrapper.forward resolves correctly; the bug is solely the .sample dereference on the tensor return.
GTP-2 Slice 1 AC-3, AC-4 cd /home/johnj/dev_cuda_1/ComfyUI && git show origin/issue_101:comfy/ldm/seedvr/vae.py | python3 -c 'import ast,sys; src=sys.stdin.read(); tree=ast.parse(src); [print(f"line range: {m.lineno}-{m.end_lineno}") for n in ast.walk(tree) if isinstance(n,ast.ClassDef) and n.name=="VideoAutoencoderKLWrapper" for m in n.body if isinstance(m,ast.FunctionDef) and m.name=="forward"]' line range: 2228-2232 — i.e. on the issue_101 side VideoAutoencoderKLWrapper.forward spans exactly five lines (2228–2232). AC-4's scope-diff stanza pins every diff hunk on the vae.py side to this range.
GTP-3 Slice 1 AC-1, AC-2, AC-3, AC-4 cd /home/johnj/dev_cuda_1/ComfyUI && git show origin/issue_101:comfy/ldm/seedvr/vae.py | sed -n '2228,2232p'
    def forward(self, x: torch.FloatTensor):
with torch.no_grad() if self.freeze_encoder else nullcontext():
z, p = self.encode(x)
x = self.decode(z).sample
return x, z, p
i.e. the buggy body accesses .sample on the tensor return of self.decode(z) at line 2231; that one literal is the entire bug, and removing it is the entire fix.
GTP-4 Slice 1 AC-1, AC-2, AC-5 cd /home/johnj/dev_cuda_1/ComfyUI && git show origin/issue_101:comfy/ldm/seedvr/vae.py | sed -n '2247,2335p' The wrapper decode(self, z) body (lines 2247–2335 on issue_101 — the post-Comfy-Org#189 / PR #34 normalized body): begins with the four guards on original_image_video (None / non-tensor / non-5-D / img_dims None / img_dims non-2-tuple / tiled_args non-dict), then latent = z.view(b, 16, -1, h, w) rescale, then either x = tiled_vae(latent, self, **self.tiled_args, encode=False) (when enable_tiling) or x = super().decode_(latent), then rearrange/trim/lab_color_transfer/unsqueeze/even-dim trim, terminating at return x. The final return statement is a plain torch.Tensor; no wrapper object with a .sample attribute appears anywhere in this body. The decode contract verified here is the post-fix shape wrapper.forward must consume directly.
GTP-5 Slice 1 AC-5 cd /home/johnj/dev_cuda_1/ComfyUI && git ls-tree --name-only origin/issue_101 tests-unit/comfy_test/ | grep -E 'seedvr'
tests-unit/comfy_test/seedvr_model_test.py
tests-unit/comfy_test/seedvr_vae_forward_test.py
tests-unit/comfy_test/test_seedvr_groupnorm_limit.py
tests-unit/comfy_test/test_seedvr_rope_delegation.py
tests-unit/comfy_test/test_seedvr_vae_decode_batch_axes.py
tests-unit/comfy_test/test_seedvr_vae_decode_guards.py
tests-unit/comfy_test/test_seedvr_vae_decode_unpadded_t.py
tests-unit/comfy_test/test_seedvr_vae_loader_metadata.py
tests-unit/comfy_test/test_seedvr_vae_tiled_args_no_mutate.py
i.e. on issue_101 HEAD tests-unit/comfy_test/ contains nine seedvr-related tests; none of them is seedvr_vae_wrapper_forward_test.py, so the new test the plan introduces does not yet exist on the base branch. AC-5's pre-fix-fingerprint cites GTP-5 to lock the absence of seedvr_vae_wrapper_forward_test.py.
GTP-6 Slice 1 AC-1, AC-5 cd /home/johnj/dev_cuda_1/ComfyUI && git show origin/issue_101:comfy/ldm/seedvr/vae.py | sed -n '2218,2226p'
        self.spatial_downsample_factor = spatial_downsample_factor
self.temporal_downsample_factor = temporal_downsample_factor
self.freeze_encoder = freeze_encoder
self.original_image_video = None
self.img_dims = None
self.enable_tiling = False
self.tiled_args = {}
super().init(*args, **kwargs)
self.set_memory_limit(0.5, 0.5)
i.e. __init__ initializes original_image_video, img_dims, enable_tiling, tiled_args, freeze_encoder as attribute-named fields the test standin must populate (per the binding directive "the regression harness must set this attribute"). The standin in the new test sets these manually after __new__ + nn.Module.__init__ to avoid running the real __init__ (which allocates the full VAE weight set).

Every literal API surface the ACs below cite — VideoAutoencoderKLWrapper, encode, decode, forward, the .sample attribute reference on line 2231, the self.decode(z) call site (line 2231), the parent class VideoAutoencoderKL and its members, the original_image_video / img_dims / enable_tiling / tiled_args / freeze_encoder instance attributes the standin populates, the decode_ parent method, the base-branch tests-unit/comfy_test/ directory contents, and the wrapper.forward line range [2228, 2232] on the issue_101 side — is anchored to one of GTP-1, GTP-2, GTP-3, GTP-4, GTP-5, GTP-6 above.

Created Surface Contract

Surface Created by Contract
tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py Slice 1 New file in pollockjj/ComfyUI on issue_192. Defines two pytest test functions that exercise VideoAutoencoderKLWrapper.forward() against a stub built via __new__ + nn.Module.__init__ (mirrors the existing _make_standin pattern from tests-unit/comfy_test/test_seedvr_vae_decode_guards.py): (1) test_wrapper_forward_returns_tensor_triple — uses monkeypatch.setattr to replace VideoAutoencoderKLWrapper.encode with a stub returning canonical (z, p) and VideoAutoencoderKLWrapper.decode with a stub returning a canonical torch.Tensor, sets original_image_video to a 5-D tensor and img_dims to a 2-tuple on the standin (per Boss's directive in the issue body even though the stub bypasses the guard), invokes wrapper.forward(x), asserts the return is a 3-tuple, asserts each member is a torch.Tensor, asserts x_out.shape == decode_out.shape, asserts z.shape == posterior.squeeze(2).shape, asserts torch.equal(x_out, decode_out), asserts torch.equal(z, posterior.squeeze(2)), and asserts p is posterior (identity preservation of the encode-side tuple member). (2) test_wrapper_forward_source_has_no_sample_access — uses inspect.getsource(VideoAutoencoderKLWrapper.forward) and asserts ".sample" not in src. The test sets comfy.cli_args.args.cpu = True when CUDA is unavailable, mirroring the sister test's import preamble.
github_issues/192/slice1/pytest_post_fix.log Slice 1 Committed to pollockjj/mydevelopment issue_192. Verbatim stdout/stderr of cd /home/johnj/dev_cuda_1/ComfyUI && python3 -m pytest tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py -v run on issue_192 HEAD. Final non-blank summary line ends with 2 passed.
github_issues/192/slice1/pytest_pre_fix.log Slice 1 Committed to pollockjj/mydevelopment issue_192. Verbatim stdout/stderr of the same pytest invocation captured after cd /home/johnj/dev_cuda_1/ComfyUI && git checkout origin/issue_101 -- comfy/ldm/seedvr/vae.py is applied to the deliverable working tree (the test file from issue_192 HEAD remains in place; only vae.py is reverted). Captures one AttributeError: 'Tensor' object has no attribute 'sample' failure on test_wrapper_forward_returns_tensor_triple plus a passed result on test_wrapper_forward_source_has_no_sample_access (the source check fails post-fix detection because .sample IS in the pre-fix body — so this asserts the inverse on the pre-fix run via the failure of the same assertion the post-fix run satisfies). The deliverable working tree is restored to issue_192 HEAD via cd /home/johnj/dev_cuda_1/ComfyUI && git restore --source=HEAD comfy/ldm/seedvr/vae.py immediately after capture.
github_issues/192/slice1/forward_source.txt Slice 1 Committed to pollockjj/mydevelopment issue_192. Verbatim AST-extracted source of VideoAutoencoderKLWrapper.forward from issue_192 HEAD comfy/ldm/seedvr/vae.py, produced by cd /home/johnj/dev_cuda_1/ComfyUI && python3 -c 'import ast; src=open("comfy/ldm/seedvr/vae.py").read(); tree=ast.parse(src); cls=[n for n in ast.walk(tree) if isinstance(n,ast.ClassDef) and n.name=="VideoAutoencoderKLWrapper"][0]; m=[n for n in cls.body if isinstance(n,ast.FunctionDef) and n.name=="forward"][0]; print(ast.get_source_segment(src,m))'.
github_issues/192/slice1/forbidden_attrs_grep.log Slice 1 Committed to pollockjj/mydevelopment issue_192. Output of grep -n -E '\.sample' github_issues/192/slice1/forward_source.txt (or true continuation if grep exit 1). The committed log records exit code 1 and zero matching lines.
github_issues/192/slice1/scope_diff.txt Slice 1 Committed to pollockjj/mydevelopment issue_192. Output of cd /home/johnj/dev_cuda_1/ComfyUI && git diff origin/issue_101..issue_192 -- comfy/ldm/seedvr/vae.py. The diff shows zero hunks outside the body of VideoAutoencoderKLWrapper.forward (lines 2228–2232 on the issue_101 side per GTP-2); VideoAutoencoderKLWrapper.__init__, VideoAutoencoderKLWrapper.encode, VideoAutoencoderKLWrapper.decode, VideoAutoencoderKLWrapper.set_memory_limit, and the entire parent class VideoAutoencoderKL are unchanged. The artifact contains the verbatim diff plus a final stanza listing every @@-hunk header range and confirming each falls inside [2228, 2232] on the - side.
github_issues/192/slice1/test_source.txt Slice 1 Committed to pollockjj/mydevelopment issue_192. Verbatim copy produced by cp /home/johnj/dev_cuda_1/ComfyUI/tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py github_issues/192/slice1/test_source.txt. AC-5 verifies the file defines exactly the two named test functions and their assertion shape.

Slices


Slice 1: Fix VideoAutoencoderKLWrapper.forward .sample access

Kind: implementation

Objective: Prove that VideoAutoencoderKLWrapper.forward() returns the tensor produced by self.decode(z) directly — without dereferencing .sample — and that the change is scoped strictly to the body of VideoAutoencoderKLWrapper.forward (no other method body, no other class touched), with the companion seedvr regression suite still exiting pytest-green on issue_192 HEAD after the wrapper.forward edit lands.

Acceptance Criteria

  • AC-1: cd /home/johnj/dev_cuda_1/ComfyUI && python3 -m pytest tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py -v exits 0 with 2 passed in the summary line — verified by committed artifact github_issues/192/slice1/pytest_post_fix.log whose last non-blank line ends with 2 passed.
    • Pre-fix-fingerprint: same pytest invocation against the issue_101 source raises AttributeError: 'Tensor' object has no attribute 'sample' on test_wrapper_forward_returns_tensor_triple and an AssertionError: assert ".sample" not in src on test_wrapper_forward_source_has_no_sample_access (Diagnosis Summary §"Diagnosis Summary"; GTP-3, GTP-4)
    • Post-fix-expectation: zero exceptions; pytest exits 0; summary line contains 2 passed
    • Assumes-passed: none
    • In-scope-of: Slice 1 Objective "Prove that VideoAutoencoderKLWrapper.forward() returns the tensor produced by self.decode(z) directly — without dereferencing .sample — and that the change is scoped strictly to the body of VideoAutoencoderKLWrapper.forward (no other method body, no other class touched), with the companion seedvr regression suite still exiting pytest-green on issue_192 HEAD after the wrapper.forward edit lands."
    • Probe: GTP-1, GTP-3, GTP-4, GTP-6
    • Foundations-pin: comment-id=IC_kwDOR2e1q88AAAABBTQ3Yg edited-at=2026-05-05T19:17:44Z
  • AC-2: The committed A/B pytest pair for tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py -v proves the same observable flips across the fix: github_issues/192/slice1/pytest_pre_fix.log records the AttributeError: 'Tensor' object has no attribute 'sample' failure on test_wrapper_forward_returns_tensor_triple plus the assert ".sample" not in src failure on test_wrapper_forward_source_has_no_sample_access after cd /home/johnj/dev_cuda_1/ComfyUI && git checkout origin/issue_101 -- comfy/ldm/seedvr/vae.py, and github_issues/192/slice1/pytest_post_fix.log records pytest exit 0 with 2 passed after cd /home/johnj/dev_cuda_1/ComfyUI && git restore --source=HEAD comfy/ldm/seedvr/vae.py restores issue_192 HEAD.
    • Pre-fix-fingerprint: the same python3 -m pytest tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py -v invocation raises AttributeError: 'Tensor' object has no attribute 'sample' from inside wrapper.forward when vae.py is reverted to issue_101, and the source-introspection test fails because .sample is present in the pre-fix forward body (Diagnosis Summary §"Diagnosis Summary"; GTP-3)
    • Post-fix-expectation: the same pytest invocation on restored issue_192 HEAD exits 0 with 2 passed and zero AttributeError failures
    • Assumes-passed: none
    • In-scope-of: Slice 1 Objective "Prove that VideoAutoencoderKLWrapper.forward() returns the tensor produced by self.decode(z) directly — without dereferencing .sample — and that the change is scoped strictly to the body of VideoAutoencoderKLWrapper.forward (no other method body, no other class touched), with the companion seedvr regression suite still exiting pytest-green on issue_192 HEAD after the wrapper.forward edit lands."
    • Probe: GTP-3, GTP-4
    • Foundations-pin: comment-id=IC_kwDOR2e1q88AAAABBTQ3Yg edited-at=2026-05-05T19:17:44Z
  • AC-3: The AST-extracted body of VideoAutoencoderKLWrapper.forward on issue_192 HEAD contains zero occurrences of the literal .sample — verified by committed artifacts github_issues/192/slice1/forward_source.txt (the extracted body) and github_issues/192/slice1/forbidden_attrs_grep.log (grep -n -E '\.sample' forward_source.txt exit code 1, zero matching lines).
    • Pre-fix-fingerprint: Diagnosis Summary §"Diagnosis Summary"; GTP-3 shows one '.sample' access at line 2231 of the pre-fix wrapper.forward body.
    • Post-fix-expectation: Diagnosis Summary §"Diagnosis Summary"; github_issues/192/slice1/forward_source.txt shows 'x = self.decode(z)' and github_issues/192/slice1/forbidden_attrs_grep.log records zero '.sample' matches.
    • Assumes-passed: none
    • In-scope-of: Slice 1 Objective "Prove that VideoAutoencoderKLWrapper.forward() returns the tensor produced by self.decode(z) directly — without dereferencing .sample — and that the change is scoped strictly to the body of VideoAutoencoderKLWrapper.forward (no other method body, no other class touched), with the companion seedvr regression suite still exiting pytest-green on issue_192 HEAD after the wrapper.forward edit lands."
    • Probe: GTP-2, GTP-3
    • Foundations-pin: comment-id=IC_kwDOR2e1q88AAAABBTQ3Yg edited-at=2026-05-05T19:17:44Z
  • AC-4: cd /home/johnj/dev_cuda_1/ComfyUI && git diff origin/issue_101..issue_192 -- comfy/ldm/seedvr/vae.py produces hunks confined to the line range of VideoAutoencoderKLWrapper.forward on the issue_101 side (lines 2228–2232 per GTP-2) and zero hunks touching VideoAutoencoderKLWrapper.__init__, VideoAutoencoderKLWrapper.encode, VideoAutoencoderKLWrapper.decode, VideoAutoencoderKLWrapper.set_memory_limit, or any line of the parent class VideoAutoencoderKL (per GTP-1) — verified by committed artifact github_issues/192/slice1/scope_diff.txt. The artifact contains the verbatim diff plus a final stanza listing every @@-hunk header range and confirming each falls inside [2228, 2232] on the - side.
    • Pre-fix-fingerprint: Diagnosis Summary §"Diagnosis Summary"; GTP-3 localizes the defect to wrapper.forward line 2231, so any diff hunk outside [2228, 2232] is extraneous to the diagnosed defect.
    • Post-fix-expectation: Diagnosis Summary §"Diagnosis Summary"; github_issues/192/slice1/scope_diff.txt enumerates only in-range hunks.
    • Assumes-passed: none
    • In-scope-of: Slice 1 Objective "Prove that VideoAutoencoderKLWrapper.forward() returns the tensor produced by self.decode(z) directly — without dereferencing .sample — and that the change is scoped strictly to the body of VideoAutoencoderKLWrapper.forward (no other method body, no other class touched), with the companion seedvr regression suite still exiting pytest-green on issue_192 HEAD after the wrapper.forward edit lands."
    • Probe: GTP-1, GTP-2, GTP-3
    • Foundations-pin: comment-id=IC_kwDOR2e1q88AAAABBTQ3Yg edited-at=2026-05-05T19:17:44Z
  • AC-5: The committed test file tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py on issue_192 defines exactly two pytest test functions whose names are test_wrapper_forward_returns_tensor_triple and test_wrapper_forward_source_has_no_sample_access; the first uses monkeypatch.setattr to stub VideoAutoencoderKLWrapper.encode and VideoAutoencoderKLWrapper.decode, sets wrapper.original_image_video to a 5-D torch.Tensor and wrapper.img_dims to a 2-tuple on the __new__+nn.Module.__init__ standin, invokes wrapper.forward(x), and asserts (a) the return is a 3-tuple, (b) type(x_out) is torch.Tensor AND type(z) is torch.Tensor AND type(p) is torch.Tensor, (c) x_out.shape == decode_out.shape (binary, no tolerance band), (d) z.shape == posterior.squeeze(2).shape (binary), (e) torch.equal(x_out, decode_out) AND torch.equal(z, posterior.squeeze(2)), (f) p is posterior (identity); the second runs inspect.getsource(VideoAutoencoderKLWrapper.forward) and asserts ".sample" not in src — verified by committed artifact github_issues/192/slice1/test_source.txt (cp /home/johnj/dev_cuda_1/ComfyUI/tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py github_issues/192/slice1/test_source.txt) plus the AC-1 pytest log showing both test names in the PASSED lines.
    • Pre-fix-fingerprint: Diagnosis Summary §"Diagnosis Summary"; issue_101 has no direct wrapper.forward regression test, so the diagnosed AttributeError is unguarded on the base branch (GTP-5).
    • Post-fix-expectation: Diagnosis Summary §"Diagnosis Summary"; github_issues/192/slice1/test_source.txt adds the two named tests and github_issues/192/slice1/pytest_post_fix.log shows both PASS.
    • Assumes-passed: none
    • In-scope-of: Slice 1 Objective "Prove that VideoAutoencoderKLWrapper.forward() returns the tensor produced by self.decode(z) directly — without dereferencing .sample — and that the change is scoped strictly to the body of VideoAutoencoderKLWrapper.forward (no other method body, no other class touched), with the companion seedvr regression suite still exiting pytest-green on issue_192 HEAD after the wrapper.forward edit lands."
    • Probe: GTP-1, GTP-3, GTP-4, GTP-5, GTP-6
    • Foundations-pin: comment-id=IC_kwDOR2e1q88AAAABBTQ3Yg edited-at=2026-05-05T19:17:44Z

Constraints

  • Python: python3
  • Runner: python3 -m pytest tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py -v from the deliverable checkout root /home/johnj/dev_cuda_1/ComfyUI
  • No packages installed into host venv; the deliverable slot's .venv/ already provides torch and pytest per the slot layout in CLAUDE.md
  • No pkill, no rm -rf, no python main.py, no /tmp writes
  • One command per shell call; no &&/||/; chaining
  • All deliverable code commits land on pollockjj/ComfyUI:issue_192 cut from issue_101 (HEAD 80c91503); PR target = issue_101
  • All bookkeeping evidence commits (github_issues/192/slice1/*) land on pollockjj/mydevelopment:issue_192 cut from main; PR target = main (standard [bookkeeping] [Issue 101][eval-and-fix] pattern)
  • Zero modifications to any method other than VideoAutoencoderKLWrapper.forward in comfy/ldm/seedvr/vae.py (VideoAutoencoderKLWrapper.__init__, VideoAutoencoderKLWrapper.encode, VideoAutoencoderKLWrapper.decode, VideoAutoencoderKLWrapper.set_memory_limit, and the entire parent class VideoAutoencoderKL are off-limits)
  • Zero changes to encode() / decode_() / decode() / forward() signatures or return shapes (issue body explicit "plan drift" guard)
  • Relevant Existing Tooling:
    • pytest from the deliverable slot's .venv/ — REUSE; no install
    • comfy.ldm.seedvr.vae.VideoAutoencoderKLWrapper import path — REUSE; verified by GTP-1
    • git, grep, AST extraction via stdlib ast module — REUSE
    • _make_standin shape pattern from tests-unit/comfy_test/test_seedvr_vae_decode_guards.py — REUSE (__new__ + nn.Module.__init__ + manual attribute population), already established in this codebase
    • The new test file is the only added Python source in the deliverable; the new artifacts under github_issues/192/slice1/ are the only added paths in the bookkeeping repo
  • Evidence Primitive Requirements:
    • Behavior change (wrapper.forward stops raising AttributeError on .sample) → A/B pytest pair: post-fix log on issue_192 HEAD plus pre-fix log captured with vae.py transiently reverted to origin/issue_101 (Slice 1 AC-1, Slice 1 AC-2)
    • Source shape claim (no .sample in wrapper.forward body) → AST-extracted committed source plus zero-match grep transcript (Slice 1 AC-3)
    • Scope claim (only wrapper.forward changed) → committed git diff origin/issue_101..issue_192 -- vae.py with hunk-range enumeration (Slice 1 AC-4)
    • Test surface claim (two named functions with binary type/shape/identity assertions) → committed copy of the test source plus pytest log naming each function as PASSED (Slice 1 AC-5)
  • Repository Hygiene:
    • Every dirty repo state on either deliverable or bookkeeping checkout is resolved by the agent without git stash or external decision deferral.
    • Dirty paths are classified as committed work, precise committed .gitignore rules for safe generated bulk, or discarded generated debris.
    • Clean committed work already present on issue_192 (e.g. the ?? github_issues/192/ dispatch artifacts seen at plan time on the bookkeeping checkout) is repo provenance and is integrated, not removed as dirty-state cleanup.
    • Slicers and Melian must run live git status --short gates locally on both /home/johnj/dev_cuda_1/ComfyUI and /home/johnj/dev_cuda_1/mydevelopment and must refuse to submit while either is dirty.
    • Plans must not require repo_hygiene.txt, raw git status --short transcripts, or CLEAN markers as QA evidence.
    • QA verifies submitted commits, branch refs, and artifact presence through GitHub rather than trusting slicer-authored cleanliness transcripts.
    • The transient revert in Slice 1 AC-2 (git checkout origin/issue_101 -- comfy/ldm/seedvr/vae.py) MUST be undone via git restore --source=HEAD comfy/ldm/seedvr/vae.py before any commit is created on the deliverable; the pre-fix log itself is committed only to the bookkeeping repo.

Out of Scope

  • VideoAutoencoderKL.forward (lines 2194–2207 on the pre-Init docker support (Nvidia and AMD) Comfy-Org/ComfyUI#190 issue_101 source) — same bug class but already fixed by pollockjj/mydevelopment#190 / merged PR [Issue 101][eval-and-fix] Fix VideoAutoencoderKL.forward return contract (closes pollockjj/mydevelopment#190) #44 (HEAD 80c91503). Do not touch.
  • VideoAutoencoderKLWrapper.decode (lines 2247–2335 on issue_101) — already covered by pollockjj/mydevelopment#189 / merged PR [Issue 101][eval-and-fix] Keep batch/time axes distinct for single-frame decode (closes pollockjj/mydevelopment#189) #34's tiled-path normalization plus pollockjj/mydevelopment#194 / merged PR [Issue 101][eval-and-fix] Add SeedVR2 decode state guards (closes pollockjj/mydevelopment#194) #43's original_image_video / img_dims / tiled_args guards. Do not touch.
  • Any change to VideoAutoencoderKL.encode(), VideoAutoencoderKL.decode_(), or VideoAutoencoderKLWrapper.encode() — signature, return shape, or body. The issue body explicitly flags such changes as plan drift; if implementation reveals a need to change them, stop and surface to the architect.
  • Any change to comfy_extras/nodes_seedvr.py, tiled_vae, or other call sites of wrapper.encode() / wrapper.decode() outside wrapper.forward. The fix is scoped to one method body.
  • Upstream propagation of the fix to feat: Add support For SeedVR2 (CORE-6) Comfy-Org/ComfyUI#11294. The upstream PR is mergeable=DIRTY and silent for two weeks per the issue body; pushing back to yousef-rafat:seedvr2 is out of scope and requires a separate decision.
  • Numeric correctness of the encode→decode round trip on real weights. The regression test exercises the return-type/return-shape contract with stub returns; correctness of the mathematical pipeline (color transfer, tiled decode, latent rescale) is covered by the existing seedvr workflow integration on issue_101 and is not re-validated by this slice.
  • Tolerance-band assertions of any kind. The fix is a contract repair; equality is binary (type(result) is torch.Tensor, result.shape == expected_shape, torch.equal(...), p is posterior).
  • Removal or refactor of any other line of wrapper.forward beyond removing the .sample access on line 2231. The with torch.no_grad() if self.freeze_encoder else nullcontext(): context manager, the self.encode(x) call, and the return x, z, p triple are preserved verbatim.

Tracked in pollockjj/mydevelopment Comfy-Org#192 (bookkeeping PR pollockjj/mydevelopment#208).

pollockjj added 2 commits May 5, 2026 15:28
…n contract (closes pollockjj/mydevelopment#192)

VideoAutoencoderKLWrapper.forward() called self.decode(z).sample, but
VideoAutoencoderKLWrapper.decode() returns a plain torch.Tensor (post-Comfy-Org#189
/ PR #34 normalization). Direct wrapper invocation therefore raised
AttributeError on the tensor return.

Drop the .sample dereference; use the tensor return directly. forward()
now returns the (x_out, z, p) triple as documented in the wrapper's
encode/decode contract.

Sister to mydevelopment#190 (PR #44) which fixed the parent class
VideoAutoencoderKL.forward for the same bug class flagged on
Comfy-Org#11294 by CodeRabbit (thread r2959796348, "Also applies
to: 2083-2087" trailer).

Adds tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py — four
CPU-only regression tests that build a wrapper standin via __new__ +
nn.Module.__init__, register a single dummy parameter so the wrapper's
encode dtype lookup resolves, set original_image_video / img_dims /
tiled_args so the wrapper.decode guards pass, patch the parent
VideoAutoencoderKL.encode / decode_ plus the module-level
lab_color_transfer with fingerprint-tagged stubs, then call
wrapper.forward(x) end-to-end and assert (1) the return is a 3-tuple
of tensors with no AttributeError raised, (2) x_out has exact
expected shape, dtype, and value-fingerprint under torch.equal, (3) z
matches the encode-side posterior squeezed on dim 2 under torch.equal,
and (4) inspect.getsource(wrapper.forward) contains no ".sample" string.
…egression test to two-test contract (pollockjj/mydevelopment#192)

The regression suite for VideoAutoencoderKLWrapper.forward now defines
exactly the two contract-named tests:

  - test_wrapper_forward_returns_tensor_triple monkeypatches
    VideoAutoencoderKLWrapper.encode and VideoAutoencoderKLWrapper.decode
    directly on the class, builds the wrapper standin via __new__ +
    nn.Module.__init__, sets original_image_video to a 5-D tensor and
    img_dims to a 2-tuple, invokes wrapper.forward(x), and asserts the
    full return-contract: 3-tuple, three torch.Tensor types, binary
    shape equality (x_out.shape == decode_out.shape, z.shape ==
    posterior.squeeze(2).shape), tensor equality
    (torch.equal(x_out, decode_out), torch.equal(z, posterior.squeeze(2))),
    and identity (p is posterior).

  - test_wrapper_forward_source_has_no_sample_access asserts
    ".sample" not in inspect.getsource(VideoAutoencoderKLWrapper.forward)
    so the failing pre-fix body raises an explicit assertion failure on
    the literal forbidden token.

Stubbing on the wrapper class (not the parent) bypasses the parent
encode/decode_ entirely, removes the lab_color_transfer monkey-patch,
and makes the AttributeError pre-fix path surface directly on
self.decode(z).sample because the stubbed decode returns a plain
tensor.
Copilot AI review requested due to automatic review settings May 5, 2026 20:47
@qa-agent-seveneves
Copy link
Copy Markdown

Codex Review — Cycle 1

Overall: patch is correct
Overall confidence: 0.93
Explanation: The patch correctly removes the invalid .sample dereference from VideoAutoencoderKLWrapper.forward while preserving the wrapper return triple. The new focused regression test and the adjacent SeedVR forward tests pass under the repository venv, and I found no blocking behavioral regressions in the diff.

No findings.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes VideoAutoencoderKLWrapper.forward() to honor the wrapper’s decode() return contract (plain torch.Tensor) by removing an invalid .sample dereference, and adds a focused regression test to prevent reintroduction.

Changes:

  • Update VideoAutoencoderKLWrapper.forward() to use x = self.decode(z) (no .sample).
  • Add tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py to exercise wrapper.forward(x) end-to-end via stubs and assert .sample is absent from the forward source.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
comfy/ldm/seedvr/vae.py Removes invalid .sample access in VideoAutoencoderKLWrapper.forward() so it returns the decoded tensor directly.
tests-unit/comfy_test/seedvr_vae_wrapper_forward_test.py Adds regression coverage for the wrapper forward() return contract and source guard against .sample.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tdd-agent
Copy link
Copy Markdown

tdd-agent Bot commented May 5, 2026

Melian PR-Review — Cycle 1 — DECISION: CONTINUE — blocking findings remain.

HEAD SHA reviewed: 1077e22b50a8
Verdict: HOLD

Why we are continuing

1 blocking finding(s) remain. The slicer will address them and the next cycle will re-review the resulting head SHA.

Blocking findings

  • [copilot] [P?] Copilot review not APPROVED (state=COMMENTED)

Raw signal

  • codex: overall_correctness='patch is correct', confidence=0.93, findings=0
  • copilot: state='COMMENTED', inline_comments=0

Verdict rationale (raw)

copilot state=COMMENTED

This comment is posted by Melian's FSM (_handle_pr_review_triage) on every triage that results in a STOP or CONTINUE decision so the audit trail is complete.

@qa-agent-seveneves
Copy link
Copy Markdown

Codex Review — Cycle 2

Overall: patch is correct
Overall confidence: 0.89
Explanation: The PR makes the wrapper forward path consume the tensor returned by decode() directly and adds a focused regression test for that contract. The new test and related SeedVR tests pass under the repository virtualenv; no blocking issues were found.

No findings.

@tdd-agent
Copy link
Copy Markdown

tdd-agent Bot commented May 5, 2026

Melian PR-Review — Cycle 2 — DECISION: CONTINUE — blocking findings remain.

HEAD SHA reviewed: 1077e22b50a8
Verdict: HOLD

Why we are continuing

1 blocking finding(s) remain. The slicer will address them and the next cycle will re-review the resulting head SHA.

Blocking findings

  • [copilot] [P?] Copilot review not APPROVED (state=COMMENTED)

Raw signal

  • codex: overall_correctness='patch is correct', confidence=0.89, findings=0
  • copilot: state='COMMENTED', inline_comments=0

Verdict rationale (raw)

copilot state=COMMENTED

This comment is posted by Melian's FSM (_handle_pr_review_triage) on every triage that results in a STOP or CONTINUE decision so the audit trail is complete.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@qa-agent-seveneves
Copy link
Copy Markdown

Codex Review — Cycle 3

Overall: patch is correct
Overall confidence: 0.91
Explanation: The diff removes the stale .sample access from VideoAutoencoderKLWrapper.forward, matching the wrapper decode() tensor return contract, and adds focused regression coverage. Targeted wrapper-forward and adjacent SeedVR VAE tests pass under the repository venv.

No findings.

@tdd-agent
Copy link
Copy Markdown

tdd-agent Bot commented May 5, 2026

Melian PR-Review — Cycle 3 — DECISION: CONTINUE — blocking findings remain.

HEAD SHA reviewed: 1077e22b50a8
Verdict: HOLD

Why we are continuing

1 blocking finding(s) remain. The slicer will address them and the next cycle will re-review the resulting head SHA.

Blocking findings

  • [copilot] [P?] Copilot review not APPROVED (state=COMMENTED)

Raw signal

  • codex: overall_correctness='patch is correct', confidence=0.91, findings=0
  • copilot: state='COMMENTED', inline_comments=0

Verdict rationale (raw)

copilot state=COMMENTED

This comment is posted by Melian's FSM (_handle_pr_review_triage) on every triage that results in a STOP or CONTINUE decision so the audit trail is complete.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants