Add SeedVR2 schema/signature regression test#24
Closed
pollockjj wants to merge 12 commits into
Closed
Conversation
Issue 130: Clean SeedVR2 native runtime fixes
…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.
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.
pollockjj
commented
Apr 27, 2026
| f"parameter order: schema_ids={schema_ids}, exec_params={exec_params}" | ||
| ) | ||
| finally: | ||
| sys.modules.pop("comfy_extras.nodes_seedvr", None) |
Owner
Author
There was a problem hiding this comment.
[P2] Clear the parent package submodule cache
When this test imports comfy_extras.nodes_seedvr under the mocked comfy.model_management, Python also leaves that module on the comfy_extras.nodes_seedvr parent package attribute. The finally block only removes sys.modules, so any later test in the same process that does from comfy_extras import nodes_seedvr can receive the stale module loaded with the mock instead of re-importing the real dependency.
There was a problem hiding this comment.
Pull request overview
Adds a unit-level regression test to prevent silent argument reordering bugs in the SeedVR2 node by asserting the declared schema input order matches the execute() positional parameter order.
Changes:
- Introduces a new pytest that compares
SeedVR2InputProcessing.define_schema().inputs[*].idordering to theSeedVR2InputProcessing.execute()signature order. - Stubs
comfy.model_managementduring import to avoid CUDA/GPU probing side effects and cleans up relevantsys.modulesentries afterward.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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: #24 Prompt sent to codex reviewVerbatim codex review outputThis PR was reviewed using The regression test leaves a parent-package submodule reference behind after importing under a mocked dependency, which can contaminate later tests in the same process. This P2 issue should block merge until cleanup is complete. Review comment:
Codex runner stderr (non-evidence) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+45
to
+48
| sys.modules.pop("comfy_extras.nodes_seedvr", None) | ||
| comfy_module = sys.modules.get("comfy") | ||
| if comfy_module is not None and getattr(comfy_module, "model_management", None) is mock_model_management: | ||
| delattr(comfy_module, "model_management") |
There was a problem hiding this comment.
The cleanup removes comfy.model_management from the comfy package via delattr when it points at the mock. If comfy.model_management was already imported earlier in the test session, this test overwrites that attribute with the mock and then deletes it, rather than restoring the original module reference. That can leak state and create order-dependent failures for code that expects comfy.model_management to remain attached to the comfy package.
Consider capturing the pre-test value of getattr(comfy_module, "model_management", sentinel) before importing comfy_extras.nodes_seedvr, then restore it in finally (set back to the original if it existed, otherwise delete).
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)
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: #24 Prompt sent to codex reviewVerbatim codex review outputThis PR was reviewed using The test still mutates global import state when the target module was already imported earlier in the same pytest process. This can cause order-dependent failures or unexpected GPU-dependent re-imports, so the patch should not be considered correct yet. Review comment:
Codex runner stderr (non-evidence) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+39
to
+75
| comfy_module_pre = sys.modules.get("comfy") | ||
| comfy_extras_module_pre = sys.modules.get("comfy_extras") | ||
| prior_comfy_mm_attr = ( | ||
| getattr(comfy_module_pre, "model_management", sentinel) | ||
| if comfy_module_pre is not None | ||
| else sentinel | ||
| ) | ||
| prior_comfy_extras_seedvr_attr = ( | ||
| getattr(comfy_extras_module_pre, "nodes_seedvr", sentinel) | ||
| if comfy_extras_module_pre is not None | ||
| else sentinel | ||
| ) | ||
|
|
||
| with patch.dict(sys.modules, {"comfy.model_management": mock_model_management}): | ||
| sys.modules.pop("comfy_extras.nodes_seedvr", None) | ||
| try: | ||
| nodes_seedvr = importlib.import_module("comfy_extras.nodes_seedvr") | ||
| schema_ids = [i.id for i in nodes_seedvr.SeedVR2InputProcessing.define_schema().inputs] | ||
| exec_params = [ | ||
| p | ||
| for p in inspect.signature(nodes_seedvr.SeedVR2InputProcessing.execute).parameters.keys() | ||
| if p != "cls" | ||
| ] | ||
| assert schema_ids == exec_params, ( | ||
| f"SeedVR2InputProcessing schema input ids do not match execute() " | ||
| f"parameter order: schema_ids={schema_ids}, exec_params={exec_params}" | ||
| ) | ||
| finally: | ||
| sys.modules.pop("comfy_extras.nodes_seedvr", None) | ||
| comfy_extras_module = sys.modules.get("comfy_extras") | ||
| if comfy_extras_module is not None: | ||
| if prior_comfy_extras_seedvr_attr is sentinel: | ||
| if hasattr(comfy_extras_module, "nodes_seedvr"): | ||
| delattr(comfy_extras_module, "nodes_seedvr") | ||
| else: | ||
| setattr(comfy_extras_module, "nodes_seedvr", prior_comfy_extras_seedvr_attr) | ||
| comfy_module = sys.modules.get("comfy") |
There was a problem hiding this comment.
The cleanup restores comfy_extras.nodes_seedvr / comfy.model_management attributes on the parent packages, but it doesn’t restore the original sys.modules["comfy_extras.nodes_seedvr"] entry if it existed before the test. If another test imported comfy_extras.nodes_seedvr earlier, this leaves the parent package attribute potentially pointing at a module that is no longer in sys.modules, and later imports will create a second module instance. Capture the pre-test sys.modules.get("comfy_extras.nodes_seedvr") value and restore/remove that entry in finally (similar to the attribute restoration), or wrap the whole import+pop logic in a patch.dict("sys.modules", ...) that restores prior state automatically.
…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)
Codex Review -- Round 3 -- 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 3 -- RESULTPR: #24 Prompt sent to codex reviewVerbatim codex review outputThis PR was reviewed using No P0/P1/P2/P3 issues were found in the latest PR state. Codex runner stderr (non-evidence) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 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.
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. |
Owner
Author
|
Superseded by #30 — consolidated into a single test PR against the clean |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 44 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+3
| """Regression test: SeedVR2InputProcessing schema input ids must match | ||
| execute() positional parameter order. Drift between the two would silently | ||
| swap arguments at runtime; this test fails loudly on any future drift. |
Comment on lines
758
to
761
Comment on lines
722
to
746
Comment on lines
1387
to
1393
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
Adds a unit regression test for
SeedVR2InputProcessingthat compares the schema input id order with theexecute()positional parameter order.Validation
.venv/bin/pytest tests-unit/comfy_extras_test/test_seedvr_node_signature.py -v