SeedVR2 native PR regression tests (consolidates #21, #22, #24 onto issue_101_pi)#30
Merged
Conversation
…p print noise Follow-up to PR #20 review feedback and the new mydevelopment#114 plan (Production-Code Shape Independence rule): - stub comfy.model_management via unittest.mock.patch.dict instead of importing the real module, so test collection no longer triggers torch.cuda.is_available() or any GPU/server-side initialization. Live introspection confirmed comfy.model_management is the only heavy module transitively imported by comfy_extras.nodes_seedvr; nodes and server are not, so they are not stubbed. - pop and re-import comfy_extras.nodes_seedvr inside the patch context via importlib.import_module so the stubbed sys.modules entry is the one observed during attribute resolution. - replace the bare assert + print() pair with a single assert carrying a descriptive f-string failure message that includes both lists, so pytest's standard error report is the only failure surface and the passing-run stdout is quiet.
…rtol=0/atol=0, local Generator, non-default start_index/scale coverage, drop autouse params print
Adopt PR #20 Copilot review on e042ff2 verbatim: wrap the stubbed import in try/finally so the mocked comfy.model_management cannot leak into later tests via the cached comfy_extras.nodes_seedvr module. - Bind the MagicMock to a local mock_model_management so the finally block can identity-check before deleting. - Pop comfy_extras.nodes_seedvr from sys.modules in finally so the next test that imports it gets a fresh load with the real comfy.model_management. - If the comfy package object itself acquired model_management as an attribute pointing at the test mock, delete that attribute so the package state matches the unstubbed condition. The assertion logic is unchanged; the test still passes on issue_114 HEAD and still fails on the spatial_overlap/temporal_tile_size swap.
Two stylistic suggestions from Copilot's review of 5eb9257: 1. Reword the module docstring so it no longer claims the stub prevents GPU init at *test collection*; the stub/import happens inside the test body, so the wording now scopes the guarantee to the import performed inside the test. Adopted verbatim from the review. 2. Iterate inspect.signature(...).parameters.keys() instead of .parameters so it reads as 'list of names' rather than 'list of Parameter objects'. The runtime semantics are identical because .parameters yields keys when iterated, but .keys() makes the reader's job easier. Behaviour, assertion text, and stubbing strategy are unchanged. The test still passes on issue_114 HEAD and still fails on the spatial_overlap/temporal_tile_size swap.
Text-level pytest that reads comfy/ldm/seedvr/vae.py and asserts .tiled_args.pop( is absent and .tiled_args.get( is present. Catches drift back to dict-mutation semantics per upstream fix in Comfy-Org#11294 commit 3b418da. Refs: pollockjj/mydevelopment#116
…5/3150100250 — apply_rope1 spy, freqs-longer-than-seq case, exact-equality wording, eager msg string
When this test imports comfy_extras.nodes_seedvr under a stubbed comfy.model_management, Python caches the imported module on the comfy_extras package object as a 'nodes_seedvr' attribute as well as in sys.modules. The existing finally block only popped sys.modules, so a later 'from comfy_extras import nodes_seedvr' in the same process could resolve through the stale parent-package attribute and receive the mock-loaded module instead of forcing a fresh import. Add a parallel cleanup step in the finally block: when comfy_extras is in sys.modules and carries a nodes_seedvr attribute, delete it. This mirrors the existing comfy.model_management attribute cleanup so both halves of the cached-import path are cleared. References: #24 (comment) codex review round 1 (issue 114) [P2] finding
…management and comfy_extras.nodes_seedvr package attrs Capture the pre-test value of comfy.model_management on the comfy package and nodes_seedvr on the comfy_extras package before importing comfy_extras.nodes_seedvr under the mock, then in finally restore the attribute to its pre-test value if it had one, or delete it if it did not. This addresses the case where another test in the same pytest process had already legitimately imported comfy.model_management (or comfy_extras.nodes_seedvr) prior to this test running: the previous delete-on-mock cleanup left those package attributes unset even though the real modules were still in sys.modules, which would AttributeError on a subsequent comfy.model_management or comfy_extras.nodes_seedvr attribute access without re-import. The pattern is applied symmetrically to both attributes since the correctness concern Copilot flagged on comfy.model_management is the same correctness concern that motivated the codex P2 cleanup of comfy_extras.nodes_seedvr — both are package attributes set as a side-effect of import that we must restore rather than blindly delete. References: #24 (comment) Copilot review id 4184963495 (round 1, head e96447f, ComfyUI#24)
…odules['comfy_extras.nodes_seedvr'] Both reviewers on round 2 of the autonomous review loop independently flagged the same concern: the previous cleanup restored the comfy_extras.nodes_seedvr package attribute but did not explicitly restore the sys.modules['comfy_extras.nodes_seedvr'] entry. While patch.dict's clear-and-restore semantics would actually undo the finally-block pop on with-exit, that behavior is not obvious to a reader and the intent is better expressed as an explicit capture-before-patch / restore-in-finally pair, exactly mirroring the existing handling of the comfy.model_management and comfy_extras.nodes_seedvr package attributes. Capture prior_comfy_extras_seedvr_module = sys.modules.get(...) before the patch.dict block. In finally, if the prior value was the sentinel (absent before the test) pop the entry; otherwise restore the prior module object directly into sys.modules. The sys.modules pop inside finally is replaced by this explicit conditional restore; the sys.modules.pop before the try stays — that pop is functionally required so importlib.import_module re-imports under the mock instead of returning a cached real module. References: #24 (comment) (codex round 2 [P2]) #24 (comment) (Copilot id 3150906079)
…testing', replace per-comment-id docstring refs with stable issue link
… importing comfy.ldm.* on CPU-only hosts so import-time get_torch_device() does not probe a missing CUDA device (matches tests-unit/comfy_quant/test_mixed_precision.py pattern)
…nvention to test-local choice; cite image_stitch_test.py as a counter-example of stub usage in this repo
…h pytest.mark.skipif(not _cpu_trig_supported(dtype)) so PyTorch CPU wheels that don't implement torch.cos/sin for those dtypes skip cleanly instead of failing CI; cases preserved per plan Comfy-Org#120 Slice 2 AC-2
…se both:' -> 'each parametrized case does both:'
This was referenced May 1, 2026
Codex Review -- Round 1 -- KICKOFFRunner: Prompt sent to codex reviewAwaiting codex output. The result will be posted as a separate comment on this PR when codex exits. |
Codex Review -- Round 1 -- RESULTPR: #30 Prompt sent to codex reviewVerbatim codex review outputThis PR was reviewed using No merge-blocking or actionable P3 issues were found in the added regression tests. The targeted tests pass locally with the available torch environment. Codex runner stderr (non-evidence) |
There was a problem hiding this comment.
Pull request overview
This PR consolidates three SeedVR2-focused regression tests onto the issue_101_pi base to prevent future behavioral drift in key integration points (node schema/signature alignment, RoPE wrapper delegation, and VAE tiling arg handling).
Changes:
- Added a regression test asserting
SeedVR2InputProcessing.define_schema().inputs[*].idorder matchesexecute()positional parameter order. - Added a parametrized regression test locking
comfy.ldm.seedvr.model.apply_rotary_embdelegation toapply_rope1and exact output equivalence vs a direct reproduction. - Added a regression test that reads
comfy/ldm/seedvr/vae.pyand assertstiled_argsis accessed via.get(...)(not.pop(...)) to avoid mutation across calls.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests-unit/comfy_test/test_seedvr_vae_tiled_args_no_mutate.py | Source-level regression check preventing reintroduction of mutating tiled_args.pop(...) usage in SeedVR2 VAE decode path. |
| tests-unit/comfy_test/test_seedvr_rope_delegation.py | Parametrized regression coverage for SeedVR2 RoPE wrapper delegation and exact output matching against a direct reproduction. |
| tests-unit/comfy_extras_test/test_seedvr_node_signature.py | Ensures SeedVR2 node schema input-id order cannot drift from execute() positional parameter order. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+149
to
+156
| # Patch the apply_rope1 symbol as imported into seedvr.model with a wraps | ||
| # spy: a future change that inlines the math and stops calling the | ||
| # imported apply_rope1 makes spy.call_count == 0 and fails the test. | ||
| with patch.object( | ||
| seedvr_model, "apply_rope1", wraps=seedvr_model.apply_rope1 | ||
| ) as spy: | ||
| wrapper_out = apply_rotary_emb( | ||
| freqs, t, start_index=start_index, scale=scale |
There was a problem hiding this comment.
The delegation spy is attached to seedvr_model.apply_rope1, but the test’s stated invariant is delegation to comfy.ldm.flux.math.apply_rope1. If apply_rotary_emb is later refactored to call flux.math.apply_rope1 via a different reference (e.g., re-importing it or qualifying it differently), this test would fail even though the delegation invariant still holds. To align the test with the invariant, consider spying on comfy.ldm.flux.math.apply_rope1 (or patch both references) and update the assertion message accordingly.
Owner
Author
There was a problem hiding this comment.
REJECT — patching at comfy.ldm.seedvr.model.apply_rope1 is the correct Python mocking idiom: you patch the name where it is looked up, not where it is defined. comfy.ldm.seedvr.model does from comfy.ldm.flux.math import apply_rope1 at import time, which binds apply_rope1 in seedvr_model's namespace. Patching comfy.ldm.flux.math.apply_rope1 after that import has no effect on the lookup inside seedvr_model.apply_rotary_emb — the spy would never fire and the test would silently always pass.
The stated invariant is apply_rotary_emb delegates to the apply_rope1 it imported from comfy.ldm.flux.math. The current spy on seedvr_model.apply_rope1 is exactly the assertion that catches the regression where a refactor either (a) inlines the math, (b) re-imports under a different name, or (c) stops binding apply_rope1 in seedvr_model at all. Patching the source-side reference would mask all three.
Reference: unittest.mock docs, Where to patch — patch where the object is looked up.
Closing this thread; test stays as written.
Comment on lines
+1
to
+8
| from pathlib import Path | ||
|
|
||
|
|
||
| def test_seedvr_vae_tiled_args_uses_get_not_pop(): | ||
| path = Path(__file__).resolve().parents[2] / "comfy" / "ldm" / "seedvr" / "vae.py" | ||
| src = path.read_text(encoding="utf-8") | ||
| assert ".tiled_args.pop(" not in src, f"VideoAutoencoderKLWrapper.decode contains tiled_args.pop(...) which mutates self.tiled_args across calls; expected tiled_args.get(...) per the upstream fix in Comfy-Org/ComfyUI#11294 commit 3b418da. Source path: {path}" | ||
| assert ".tiled_args.get(" in src, f"VideoAutoencoderKLWrapper.decode does not read tiled_args via .get(); expected exactly one self.tiled_args.get(\"enable_tiling\", False) call per Slice 1 baseline. Source path: {path}" |
There was a problem hiding this comment.
The string checks are narrower than the stated intent and can miss a regression: this only asserts absence of the substring ".tiled_args.pop(", so a mutating call like tiled_args = self.tiled_args; tiled_args.pop(...) would not be detected. Consider checking for "tiled_args.pop(" (no leading dot) or using a regex/AST-based check that catches both self.tiled_args.pop and tiled_args.pop patterns. Also, the failure messages mention VideoAutoencoderKLWrapper.decode and "expected exactly one" .get(...) call, but the assertions only check the whole file and only check presence, not count—either tighten the assertions or adjust the messages to match what’s being asserted.
Suggested change
| from pathlib import Path | |
| def test_seedvr_vae_tiled_args_uses_get_not_pop(): | |
| path = Path(__file__).resolve().parents[2] / "comfy" / "ldm" / "seedvr" / "vae.py" | |
| src = path.read_text(encoding="utf-8") | |
| assert ".tiled_args.pop(" not in src, f"VideoAutoencoderKLWrapper.decode contains tiled_args.pop(...) which mutates self.tiled_args across calls; expected tiled_args.get(...) per the upstream fix in Comfy-Org/ComfyUI#11294 commit 3b418da. Source path: {path}" | |
| assert ".tiled_args.get(" in src, f"VideoAutoencoderKLWrapper.decode does not read tiled_args via .get(); expected exactly one self.tiled_args.get(\"enable_tiling\", False) call per Slice 1 baseline. Source path: {path}" | |
| import re | |
| from pathlib import Path | |
| def test_seedvr_vae_tiled_args_uses_get_not_pop(): | |
| path = Path(__file__).resolve().parents[2] / "comfy" / "ldm" / "seedvr" / "vae.py" | |
| src = path.read_text(encoding="utf-8") | |
| assert not re.search(r"(?:self\.)?tiled_args\.pop\s*\(", src), ( | |
| f"VideoAutoencoderKLWrapper.decode contains tiled_args.pop(...) which mutates tiled_args across calls; " | |
| f"expected reads via .get(...) only per the upstream fix in Comfy-Org/ComfyUI#11294 commit 3b418da. " | |
| f"Source path: {path}" | |
| ) | |
| enable_tiling_get_calls = re.findall( | |
| r"self\.tiled_args\.get\s*\(\s*[\"']enable_tiling[\"']\s*,\s*False\s*\)", | |
| src, | |
| ) | |
| assert len(enable_tiling_get_calls) == 1, ( | |
| f"VideoAutoencoderKLWrapper.decode should contain exactly one " | |
| f"self.tiled_args.get('enable_tiling', False) call per Slice 1 baseline; " | |
| f"found {len(enable_tiling_get_calls)}. Source path: {path}" | |
| ) |
…+ exact-count assertion on enable_tiling .get call (catches aliased mutation; locks count to 1)
Codex Review -- Round 2 -- KICKOFFRunner: Prompt sent to codex reviewAwaiting codex output. The result will be posted as a separate comment on this PR when codex exits. |
Codex Review -- Round 2 -- RESULTPR: #30 Prompt sent to codex reviewVerbatim codex review outputThis PR was reviewed using No introduced P0/P1/P2/P3 issues were identified in the added regression tests. The changed files are additive and align with the stated SeedVR2 invariants. Codex runner stderr (non-evidence)gh api /repos/{OWNER/REPO}/pulls/{PR_NUMBER}/comments gh api -X POST /repos/{OWNER/REPO}/pulls/{PR_NUMBER}/comments cycle = 0 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates three previously-open SeedVR2 regression-test PRs (#21, #22, #24) into a single PR against the clean `issue_101_pi` base. The original PRs all targeted `issue_101`, which has been hard-reset to match `issue_101_pi` (clean upstream-master pin + yousef-rafat/seedvr2 PR Comfy-Org#11294 + 3 SeedVR2 native fixes only). Their head branches were forked from the polluted ancestor of `issue_101` (which carried the entire pyisolate stack + canonical custom node + an older upstream master pin) and against the new base presented as 5,800-line spurious diffs.
Test files added (3, additive only)
Total: 3 files changed, 274 insertions(+), 0 deletions(-).
Commit history
16 commits cherry-picked in chronological order (initial test add + Copilot/codex review-address commits per file). Per-commit history preserved. All from 2026-04-27.
Supersedes