From 37c9c501bcce6f145acd4e2e31195232ec5c7b07 Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Tue, 22 Sep 2026 15:27:40 +0300 Subject: [PATCH] fix: shadow-negative compose remedy + explore_first_step silent failures Compose-mode message and Editor tooltip were stating a false remedy (turn REINS down to passive-capture-only/strength 0 to free blocks) -- REINS claims all 50 blocks the moment it's on at all, strength only gates injection not installation. Corrected to say only fully disabling REINS frees any blocks. _select_best_seed had three silent failure paths (no usable prediction, a scoring exception, zero candidates scored) that burned real GPU compute and reported nothing. Added prints to all three. Added _output_value_fn_sample_count so "not ready yet" messages show the actual banked sample count instead of leaving the user blind on how close they are to the threshold. Co-Authored-By: Claude Sonnet 5 --- movie_editor/frontend/engine_settings.js | 2 +- samplers.py | 134 +++++++++++++++++++---- tests/test_h3_shadow_negative_install.py | 47 +++++++- tests/test_scene_chain_sampler.py | 109 ++++++++++++++++++ 4 files changed, 269 insertions(+), 23 deletions(-) diff --git a/movie_editor/frontend/engine_settings.js b/movie_editor/frontend/engine_settings.js index 81f5aad6..5d22bfc1 100644 --- a/movie_editor/frontend/engine_settings.js +++ b/movie_editor/frontend/engine_settings.js @@ -357,7 +357,7 @@ hint: "Gives the negative prompt a job at CFG 1: runs it through the same blocks as a second stream and pushes the real output away from it. Heavier than 'Use the negative prompt' below (a per-step attention pass instead of a one-time edit) — needs a negative prompt connected to do anything.", detail: "MiniMax H3 runs at CFG 1.0 always, so the negative branch is never evaluated and a plain negative prompt does nothing. This runs the negative text through the SAME DiT blocks as a second 'shadow' stream, block by block, and repels the real stream's attention output from it (NAG-style, norm-capped so it cannot run away). A different, pricier lever than negative_erase's one-time embedding projection — roughly 15-30% slower while it's on. By default it does NOT compose with REINS / Query steering / Video-audio decoupling / Repeat block(s) / Randomizer: whichever of those is enabled loses to this one, and the console says so — turn on 'Allow when REINS/Q-steer are active' below if you want both running. Unvalidated — ported from a community node, never judged by eye on this project's own output yet." }, { name: "h3_shadow_negative_compose", label: "Allow when REINS/Q-steer are active", kind: "bool", default: false, dependsOn: "h3_shadow_negative", - hint: "Off: shadow negative takes over every block, which also stops REINS/Query steering/Repeat block(s)/Randomizer/Video-audio decoupling from capturing or steering — ratings made while shadow negative is on won't train them. On: shadow negative leaves whichever block(s) those already use completely alone, so they keep learning from your ratings as normal, and only pushes on the rest. Not the same as literally running both on one block at once — they just stop overwriting each other. The give-up is for the WHOLE run, not just the steps the other mechanism actually uses — e.g. a Repeat block(s) 'last steps' window still reserves its block for shadow negative every step, not only the steps it repeats." }, + hint: "Off: shadow negative takes over every block, which also stops REINS/Query steering/Repeat block(s)/Randomizer/Video-audio decoupling from capturing or steering — ratings made while shadow negative is on won't train them. On: shadow negative leaves whichever block(s) those already use completely alone, so they keep learning from your ratings as normal, and only pushes on the rest. Not the same as literally running both on one block at once — they just stop overwriting each other. The give-up is for the WHOLE run, not just the steps the other mechanism actually uses — e.g. a Repeat block(s) 'last steps' window still reserves its block for shadow negative every step, not only the steps it repeats. Does NOT help while REINS is running AT ALL, steering or just capturing: it claims all 50 blocks the moment it's on, for a technical reason (an even allocation pattern comfy's memory manager requires), not because of the one block you picked for it to learn from — its strength only decides whether it actually injects, not whether it claims the block, so turning strength down to 0 frees nothing. Only turning REINS fully off gives this checkbox any blocks to work with, and that also stops REINS learning for that run." }, { name: "h3_shadow_negative_video_scale", label: "Video push strength", kind: "float", default: 3.0, min: 1.0, max: 10.0, step: 0.1, dependsOn: "h3_shadow_negative", hint: "1 = off for video. Higher pushes the picture harder away from the negative." }, { name: "h3_shadow_negative_audio_scale", label: "Audio push strength", kind: "float", default: 1.0, min: 1.0, max: 10.0, step: 0.1, dependsOn: "h3_shadow_negative", diff --git a/samplers.py b/samplers.py index d925ed24..531586e9 100644 --- a/samplers.py +++ b/samplers.py @@ -971,6 +971,26 @@ def _strip_funpack_scene_wrappers(model): _FUNPACK_DIT_HOOK_TAG = "_funpack_dit_hook" +def _format_block_ranges(indices): + """[0,1,2,3,25,40,41,42] -> "0-3, 25, 40-42" -- a log line naming which blocks a + mechanism skipped is only useful if it stays short enough to read; a flat comma list + of every index in a 50-block span is exactly the kind of noise that buries the one + fact worth seeing (see H3 shadow negative's compose-mode log).""" + idx = sorted(set(int(i) for i in indices)) + if not idx: + return "" + runs = [] + start = prev = idx[0] + for i in idx[1:]: + if i == prev + 1: + prev = i + continue + runs.append((start, prev)) + start = prev = i + runs.append((start, prev)) + return ", ".join(str(a) if a == b else f"{a}-{b}" for a, b in runs) + + def _tag_dit_hook(hook, prev): """Mark a dit-patch hook or optimized_attention_override function (and what it chained through) so a later run can identify and unwind one leaked by a previous @@ -4884,6 +4904,32 @@ class FunPackLTXAVSceneChainSampler: except Exception: return None + def _output_value_fn_sample_count(self, refinement_key): + """How many samples the output-space value function has banked so far, or None if + it has never been trained at all. Separate from _load_output_value_function on + purpose: that method's None means "not ready to steer/select with" and every call + site trusts that gate directly (output_guidance's ascent wrapper in particular has + no readiness check of its own) — loosening it to also return an under-threshold vf + would silently start applying ascent from a near-empty value function. This is + read-only, for the "how many do I have" message (the user was "literally blind" on + this exact count before it existed).""" + try: + try: + from .value_function import LatentValueFunction + from .conditioning import refinement_state_path + except ImportError: + from value_function import LatentValueFunction + from conditioning import refinement_state_path + import os as _os + path = refinement_state_path(refinement_key, "value_fn_x0", prefix="refine_v2", extension="pt") + if not _os.path.exists(path): + return None + with torch.inference_mode(False): + vf = LatentValueFunction.load(path) + return len(vf.buffer_c) + except Exception: + return None + def _select_best_seed(self, model, sampler, sigmas, seed, cfg, positive, negative, latent, n_candidates, value_fn): """Explore-then-commit at step 1: score `n_candidates` seeds on ONE throwaway step @@ -4926,10 +4972,14 @@ class FunPackLTXAVSceneChainSampler: Returns `seed` UNCHANGED — not a fallback value, the literal seed the caller already has — whenever the value function isn't ready, `sigmas` can't provide a - one-step slice, or every candidate's own throwaway call fails. This is a real - limit (no trained scorer yet, or too short a schedule to branch), not a defect, so - it stays silent the same way every other "nothing to steer with yet" path in this - file does — the caller proceeds exactly as if this were never called.""" + one-step slice, `latent` can't be cloned, or every candidate's own throwaway call + fails. The first three bail BEFORE any compute is spent and stay silent, same as + every other "nothing to steer with yet" gate in this file. Once candidates actually + ran (real GPU time spent, visible in generation timing), a failure to produce a + usable score is reported per-candidate and, if none scored, as one final summary + line — silence there would have looked identical to "it worked and picked the + original seed," which a user cannot tell apart from "it silently did nothing" by + watching the render alone.""" if value_fn is None or not value_fn.is_ready() or int(n_candidates) < 2: return seed if not isinstance(sigmas, torch.Tensor) or sigmas.numel() < 2: @@ -4981,15 +5031,23 @@ class FunPackLTXAVSceneChainSampler: # must never leave its observer installed for the real run that follows. model.model_options["model_function_wrapper"] = old_wrapper if captured[0] is None: + print(f"[FunPackSceneChain] explore_first_step: candidate {i} produced no " + f"usable prediction (couldn't extract a video slice from this call), " + f"skipped") continue try: with torch.inference_mode(False), torch.no_grad(): score = float(value_fn.forward( value_fn.compress(captured[0].float())).item()) - except Exception: + except Exception as e: + print(f"[FunPackSceneChain] explore_first_step: candidate {i} failed to " + f"score ({e}), skipped") continue scored.append((score, cand_seed)) if not scored: + print(f"[FunPackSceneChain] explore_first_step: 0/{n_candidates} candidate(s) " + f"produced a usable score this run -- seed unchanged (original seed used, " + f"same as if this were off).") return seed scored.sort(key=lambda t: t[0], reverse=True) best_score, best_seed = scored[0] @@ -7794,11 +7852,24 @@ class FunPackLTXAVSceneChainSampler: _sample_chunk's dit-patch chain so an explicit request for this always wins. `compose=True`: installs on every block EXCEPT the ones another mechanism already - claimed this run -- REINS/Q-steer's named block, block-repeat's span, etc. keep - their own block entirely untouched (so they keep capturing/steering there exactly - as if shadow negative were off), while shadow negative's NAG push still runs on - every other block. Structural coexistence, not mathematical composition: no block - ever runs both mechanisms at once, they just no longer fight over which one wins. + claimed this run -- Q-steer's named block, block-repeat's span, etc. keep their own + block entirely untouched (so they keep capturing/steering there exactly as if + shadow negative were off), while shadow negative's NAG push still runs on every + other block. Structural coexistence, not mathematical composition: no block ever + runs both mechanisms at once, they just no longer fight over which one wins. + + REINS is the one exception where this genuinely cannot help: `_install_h3_repr_steering` + installs a hook on ALL 50 candidate blocks unconditionally, whenever it runs AT ALL + (steering OR passive-capture-only) -- not just its named steer block, and NOT gated + by strength. Strength only decides whether an installed hook actually injects + (comfy's aimdo memory prefetcher needs an identical allocation pattern on every + block in the pass whenever it steers anywhere, so the other 49 run a real, if + zero-valued, clone/inject too, on purpose, to avoid a crash it once had) -- it never + decides whether the hook exists. Dialing strength to 0, or flipping to + passive-capture-only, changes nothing here: both still claim all 50 blocks. The + ONLY way to free any block for compose is turning REINS off entirely (both + h3_repr_steering AND h3_repr_steering_passive_capture), which also means it stops + capturing/learning for that run. """ try: video_scale = float(video_scale) @@ -7866,13 +7937,32 @@ class FunPackLTXAVSceneChainSampler: to["patches_replace"] = patches_replace patched.model_options["transformer_options"] = to _installed = len(dm.blocks) - len(_skipped) - _extra = (f" -- left block(s) {', '.join(str(b) for b in _skipped)} untouched " - f"for REINS/Q-steer/av_decouple/block-repeat/attn_temperature to keep " - f"capturing/steering there") if _skipped else "" - print(f"[FunPackSceneChain] H3 shadow negative: installed on {_installed}/" - f"{len(dm.blocks)} blocks (video_scale={video_scale:g}, " - f"audio_scale={audio_scale:g}, tau={float(tau):g}, alpha={float(alpha):g}, " - f"window={_lo:.2f}-{_hi:.2f}){_extra}.") + _params = (f"video_scale={video_scale:g}, audio_scale={audio_scale:g}, " + f"tau={float(tau):g}, alpha={float(alpha):g}, window={_lo:.2f}-{_hi:.2f}") + if _skipped and _installed == 0: + # Every block was already claimed -- not a partial coexistence, a total + # no-op. Its own headline, not folded into the "installed on 0/50" line + # buried under a 50-entry index dump: that phrasing technically declared + # the limit (0/50 is right there) but a wall of block numbers is exactly + # the kind of noise that makes a real "did nothing" read as routine detail. + print(f"[FunPackSceneChain] H3 shadow negative: every block " + f"({_format_block_ranges(_skipped)}) is already claimed -- compose mode " + f"left NONE for shadow negative, so it had NO EFFECT this run " + f"({_params}). REINS claims all 50 whenever it runs at ALL (steering " + f"OR passive-capture-only), even though only its own named block learns " + f"from ratings -- strength only gates whether an installed hook injects, " + f"not whether it exists, so dialing strength to 0 frees nothing. The " + f"other 49 run the same no-op clone/inject so comfy's aimdo memory " + f"prefetcher sees a uniform allocation pattern across the whole pass (a " + f"real crash otherwise, not a style choice). The ONLY way to free a " + f"block for compose is turning REINS fully OFF (both the toggle and " + f"passive-capture), which also stops it capturing/learning this run.") + else: + _extra = (f" -- left block(s) {_format_block_ranges(_skipped)} untouched for " + f"REINS/Q-steer/av_decouple/block-repeat/attn_temperature to keep " + f"capturing/steering there") if _skipped else "" + print(f"[FunPackSceneChain] H3 shadow negative: installed on {_installed}/" + f"{len(dm.blocks)} blocks ({_params}){_extra}.") return patched except Exception as _e: # noqa: BLE001 _log.failed("FunPackSceneChain", "H3 shadow negative", _e, @@ -9020,8 +9110,9 @@ class FunPackLTXAVSceneChainSampler: if output_guidance and refinement_key_input: _output_value_fn = self._load_output_value_function(refinement_key_input) if _output_value_fn is None: - print("[FunPackSceneChain] output_guidance: value function not ready yet " - "(needs 10+ rated generations to reach MIN_SAMPLES)") + _n = self._output_value_fn_sample_count(refinement_key_input) + print(f"[FunPackSceneChain] output_guidance: value function not ready yet " + f"({_n if _n is not None else 0}/10 rated generations so far)") else: print(f"[FunPackSceneChain] output_guidance: active ({_output_value_fn.n_trained} samples), " f"strength={output_guidance_strength}") @@ -9036,8 +9127,9 @@ class FunPackLTXAVSceneChainSampler: if explore_first_step and not refinement_key_input: print("[FunPackSceneChain] explore_first_step: requires refinement_key_input — disabled") elif explore_first_step and _explore_value_fn is None: - print("[FunPackSceneChain] explore_first_step: value function not ready yet " - "(needs 10+ rated generations to reach MIN_SAMPLES)") + _n = self._output_value_fn_sample_count(refinement_key_input) + print(f"[FunPackSceneChain] explore_first_step: value function not ready yet " + f"({_n if _n is not None else 0}/10 rated generations so far)") # Per-bucket value functions for trajectory_guidance. Separate from the one above # because they answer a different question: that one asks "is this a good finish", diff --git a/tests/test_h3_shadow_negative_install.py b/tests/test_h3_shadow_negative_install.py index 09fc360b..826585c3 100644 --- a/tests/test_h3_shadow_negative_install.py +++ b/tests/test_h3_shadow_negative_install.py @@ -14,7 +14,20 @@ import torch sys.path.insert(0, str(Path(__file__).resolve().parents[1])) sys.path.insert(0, ".") import _comfy_stubs # noqa: E402, F401 -from samplers import FunPackLTXAVSceneChainSampler as S # noqa: E402 +from samplers import FunPackLTXAVSceneChainSampler as S, _format_block_ranges # noqa: E402 + + +def test_format_block_ranges_compresses_consecutive_runs(): + assert _format_block_ranges([0, 1, 2, 3, 25, 40, 41, 42]) == "0-3, 25, 40-42" + + +def test_format_block_ranges_handles_empty_and_single(): + assert _format_block_ranges([]) == "" + assert _format_block_ranges([7]) == "7" + + +def test_format_block_ranges_sorts_and_dedupes(): + assert _format_block_ranges([5, 3, 4, 3]) == "3-5" class _FakeDiffusionModel: @@ -229,6 +242,38 @@ def test_compose_with_nothing_claimed_hooks_everything_like_the_default(): assert {k for k in dit if k[0] == "double_block"} == {("double_block", i) for i in range(4)} +def test_compose_with_every_block_claimed_gets_its_own_clear_no_effect_message(capsys): + """REINS claims ALL 50 blocks unconditionally the moment it runs at all (steering OR + passive-capture-only) -- not just its named steer block, and NOT gated by strength -- + for comfy_aimdo allocation-uniformity reasons, so compose mode is left with nothing at + all whenever REINS is on in any form. This must read as a standalone "did nothing" + headline, not a generic N/50 line buried under a full block-index dump, and it must + give the CORRECT remedy: turning strength to 0 does not free anything (round-2 review + caught the first version of this message claiming otherwise), only turning REINS fully + off does.""" + model = _FakeModel(dm=_FakeDiffusionModel(n_blocks=3)) + model.model_options["transformer_options"] = {"patches_replace": {"dit": { + ("double_block", i): (lambda args, extra: {"img": args["img"]}) for i in range(3) + }}} + node = S() + patched = node._install_h3_shadow_negative(model, _negative(), 3.0, 1.0, 2.5, 0.35, 0.0, 0.6, + compose=True) + out = capsys.readouterr().out + assert "NO EFFECT" in out + assert "aimdo" in out + assert "strength only gates whether" in out, "must say strength controls injection, not installation" + assert "fully OFF" in out and "passive-capture" in out, \ + "the only real remedy (disable REINS entirely) must be stated, not 'lower strength'" + assert "down to passive-capture-only (strength 0)" not in out, \ + "this was the round-2-caught false remedy -- passive-capture ALSO claims all 50 blocks" + dit = patched.model_options["transformer_options"]["patches_replace"]["dit"] + assert all(k[0] != "double_block" or dit[k] is not None for k in dit) + img = torch.ones(3, 4) + result = dit[("double_block", 0)]({"img": img, "mod_segments": []}, + {"original_block": lambda a: {"img": a["img"] * 2.0}}) + assert torch.allclose(result["img"], img), "original per-block hook must survive untouched" + + if __name__ == "__main__": test_both_scales_at_one_is_a_true_noop() test_enabled_clones_and_hooks_every_block() diff --git a/tests/test_scene_chain_sampler.py b/tests/test_scene_chain_sampler.py index 461547aa..12184cf5 100644 --- a/tests/test_scene_chain_sampler.py +++ b/tests/test_scene_chain_sampler.py @@ -1379,3 +1379,112 @@ def test_output_value_snapshot_is_saved_on_h3(monkeypatch): ) assert snapshot_calls == ["testkey"] + + +def test_select_best_seed_reports_when_no_candidate_scores(monkeypatch, capsys): + """Regression (2026-09-22): explore_first_step's candidate loop used to `continue` + silently both when a candidate produced no usable prediction and when scoring itself + raised, then return the unchanged seed with NO console output at all if every + candidate failed this way -- indistinguishable from "it ran and picked the original + seed" to a user just watching generation timing. fake_sample_custom (below) never + invokes the model_function_wrapper, so every candidate here naturally produces no + capture -- exactly the silent path this test pins.""" + monkeypatch.setattr(sys.modules["comfy.sample"], "prepare_noise", + fake_prepare_noise, raising=False) + monkeypatch.setattr(sys.modules["comfy.sample"], "sample_custom", + fake_sample_custom, raising=False) + sample_calls.clear() + node = FunPackLTXAVSceneChainSampler() + + class _ReadyValueFn: + def is_ready(self): + return True + + latent = {"samples": torch.zeros(1, 2, 5, 3, 3)} + positive = [scene_cond(0)] + negative = [(torch.zeros(1, 2, 3), {})] + + result = node._select_best_seed( + model=FakeModel(), sampler=object(), sigmas=torch.tensor([1.0, 0.0]), seed=10, + cfg=1.5, positive=positive, negative=negative, latent=latent, n_candidates=2, + value_fn=_ReadyValueFn(), + ) + + assert result == 10, "seed must be unchanged when nothing scored" + out = capsys.readouterr().out + assert out.count("produced no usable prediction") == 2, "one line per failed candidate" + assert "0/2 candidate(s) produced a usable score" in out + assert "seed unchanged" in out + + +def test_select_best_seed_reports_a_scoring_exception_per_candidate(monkeypatch, capsys): + """The OTHER silent path this same regression covers: a candidate that DOES produce a + capture but whose value_fn.forward/.compress call raises must also say so, not just + the couldn't-extract-a-prediction case above.""" + class _RecordingWrapperModel: + def __init__(self): + self.model_options = {} + + def sample_custom_that_fires_the_wrapper(model, noise, cfg, sampler, sigmas, positive, + negative, latent_image, noise_mask=None, + callback=None, disable_pbar=False, seed=None): + wrapper = model.model_options.get("model_function_wrapper") + wrapper(lambda x, t, **c: latent_image, + {"input": noise, "timestep": torch.tensor([1.0]), "c": {}}) + return latent_image + + monkeypatch.setattr(sys.modules["comfy.sample"], "prepare_noise", + fake_prepare_noise, raising=False) + monkeypatch.setattr(sys.modules["comfy.sample"], "sample_custom", + sample_custom_that_fires_the_wrapper, raising=False) + node = FunPackLTXAVSceneChainSampler() + + class _BoomValueFn: + def is_ready(self): + return True + + def compress(self, x): + raise RuntimeError("boom") + + latent = {"samples": torch.zeros(1, 2, 5, 3, 3)} + positive = [scene_cond(0)] + negative = [(torch.zeros(1, 2, 3), {})] + + result = node._select_best_seed( + model=_RecordingWrapperModel(), sampler=object(), sigmas=torch.tensor([1.0, 0.0]), + seed=10, cfg=1.5, positive=positive, negative=negative, latent=latent, + n_candidates=2, value_fn=_BoomValueFn(), + ) + + assert result == 10 + out = capsys.readouterr().out + assert out.count("failed to score") == 2 + assert "0/2 candidate(s) produced a usable score" in out + + +def test_output_value_fn_sample_count_reads_an_under_threshold_file(monkeypatch, tmp_path): + """Regression (2026-09-22): 'value function not ready yet' used to give no indication + of how many rated generations it actually has banked -- the user called this out as + "literally blind" on the count. _load_output_value_function collapses an under- + threshold value function to None (correctly -- every other caller trusts that as + "not safe to steer/select with"), so the count has to come from a SEPARATE read.""" + import conditioning + from value_function import LatentValueFunction + + monkeypatch.setattr(conditioning, "refinement_state_path", + lambda key, mode, prefix="refine", extension="json": + str(tmp_path / f"{key}.{mode}.{extension}"), raising=True) + + node = FunPackLTXAVSceneChainSampler() + assert node._output_value_fn_sample_count("testkey") is None, \ + "no file at all -- never trained, not merely under threshold" + + vf = LatentValueFunction(hidden_dim=LatentValueFunction.DEFAULT_HIDDEN_DIM) + for i in range(4): + vf.train_on(torch.zeros(LatentValueFunction.DEFAULT_HIDDEN_DIM), float(i)) + vf.save(str(tmp_path / "testkey.value_fn_x0.pt")) + + assert vf.is_ready() is False, "fixture must actually be under MIN_SAMPLES (10)" + assert node._output_value_fn_sample_count("testkey") == 4 + assert node._load_output_value_function("testkey") is None, \ + "the gated loader must still treat this as not-ready"