From 41547469a8ef07a32374f6a28948912f14f06af0 Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Mon, 7 Sep 2026 04:10:21 +0300 Subject: [PATCH] fix: H3 query steering silently defeated by h3_av_decouple, and a misreported commit outcome Two real faults found by a second adversarial review round on 608a97a: - h3_av_decouple, installed after q_steering, becomes the OUTER optimized_attention_override and always calls its inner override with a non-None mask for the video-row partition -- exactly the case q_steering's own override treats as "already handled, back off". With both enabled, q_steering captured and injected nothing while still printing "applying" and paying the clone/hook cost. The two can't currently compose, so q_steering is now skipped entirely (with a declared reason) whenever h3_av_decouple is also active, rather than silently no-op. - conditioning.py's q_steer commit call was silent on no_pending/no_key, justified by a comment claiming REINS' own commit is silent there too -- that was wrong, REINS is loud on exactly those outcomes. Fixed to match. Also tightened the shape-mismatch warning's wording: it checks total element count, not the heads/head_dim split, so a same-total-different-split mismatch would still reshape onto the wrong head boundaries undetected -- documented as a known narrow gap rather than fixed, since a real fix would mean changing the persistence format REINS itself also depends on. Added regression tests for the av_decouple interaction (both directions: guard fires when it should, stays quiet when it shouldn't). --- conditioning.py | 18 +++++++++++------ samplers.py | 40 +++++++++++++++++++++++++++++-------- tests/test_h3_q_steering.py | 36 +++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 14 deletions(-) diff --git a/conditioning.py b/conditioning.py index 83f52e4d..cc870e78 100644 --- a/conditioning.py +++ b/conditioning.py @@ -10217,13 +10217,19 @@ class FunPackVideoRefinerV2(FunPackVideoRefiner): print(f"[FunPackRefiner] H3 representation steering: weight " f"{_rs_reward:+.3f} for rating '{rating_label}' — {_rs_note}") # Query steering rides the same rating, its own kind="q_steer" store — - # see h3_repr_steering.py / _install_h3_q_steering. Unconditional and - # silent-on-no-pending for the same reason as REINS above: a run with the - # sampler's h3_q_steer_block blank never captured anything to commit. + # see h3_repr_steering.py / _install_h3_q_steering. Loud on every outcome, + # same as REINS just above (REINS is NOT silent on no_pending/no_key -- + # an earlier version of this comment claimed it was, which was wrong). _rs_q_outcome = _rs.commit(refinement_key, _rs_reward, kind="q_steer") - if _rs_q_outcome == "recorded": - print(f"[FunPackRefiner] H3 query steering: weight " - f"{_rs_reward:+.3f} for rating '{rating_label}' — recorded") + _rs_q_note = { + "recorded": "recorded", + "no_pending": "NOT recorded — no captured generation was waiting " + "(the sampler's h3_q_steer_block was likely blank or " + "h3_av_decouple was also on when that run sampled)", + "no_key": "NOT recorded — no refinement key", + }.get(_rs_q_outcome, _rs_q_outcome) + print(f"[FunPackRefiner] H3 query steering: weight " + f"{_rs_reward:+.3f} for rating '{rating_label}' — {_rs_q_note}") # Block-influence probe rides the same rating. Measurement only -- it # steers nothing, so it is committed unconditionally alongside REINS # rather than behind its own admissibility gate. DELIBERATELY SILENT: diff --git a/samplers.py b/samplers.py index 7a164c36..ee58df75 100644 --- a/samplers.py +++ b/samplers.py @@ -3823,9 +3823,23 @@ class FunPackLTXAVSceneChainSampler: steer_block=h3_repr_steering_block) _q_steer_capture = [{}] if refinement_key and self._parse_block_spec(h3_q_steer_block): - model = self._install_h3_q_steering( - model, refinement_key, h3_q_steer_strength, _q_steer_capture, - steer_block=h3_q_steer_block) + # h3_av_decouple, if also enabled, becomes the OUTER optimized_attention_override + # (installed after this one) and always calls its inner override with a non-None + # mask for exactly the video-row partition -- which is the one thing q_steering's + # own override treats as "someone else already handled this call, back off". The + # two features cannot currently compose: q_steering would capture/inject nothing + # while still paying the clone+hook cost and printing a false "applying" line, so + # skip installing it entirely and say why, rather than let it silently no-op. + if float(h3_av_decouple or 0.0) > 0.0: + print("[FunPackSceneChain] H3 query steering: h3_av_decouple is also enabled " + "this run -- its attention masking defeats query steering's video-row " + "capture and injection entirely, so query steering is NOT installed " + "this run (nothing captured, nothing applied). Disable h3_av_decouple " + "to use query steering.") + else: + model = self._install_h3_q_steering( + model, refinement_key, h3_q_steer_strength, _q_steer_capture, + steer_block=h3_q_steer_block) model = self._install_h3_av_decouple(model, h3_av_decouple) model = self._install_h3_attn_temperature( model, h3_explore_temperature, h3_explore_temperature_block) @@ -7215,12 +7229,22 @@ class FunPackLTXAVSceneChainSampler: direction = directions.get(block) if direction is not None and _strength > 0.0: head_dim = q.shape[-1] + # Checks TOTAL element count only, not the heads/head_dim split -- a + # direction's flat vector carries no record of its original split, so a + # coincidental same-total, different-split mismatch (e.g. 8x16 vs 4x32, + # both 128) would pass this check and reshape onto the wrong head + # boundaries instead of being caught. Narrow scenario (a model swap that + # changes head count without changing hidden size, on a reused + # refinement_key) -- catching it would mean storing heads/head_dim + # alongside every captured descriptor, which touches the persistence + # format h3_repr_steering.direction() shares with REINS itself, so left + # unhandled rather than risking that for a case this narrow. if direction.numel() != heads * head_dim: - # A direction banked under a different head/head_dim layout (model - # swap, a different H3 variant on the same refinement_key) cannot be - # reshaped onto this run's Q at all -- .view() would hard-crash the - # generation instead of degrading, exactly what this file's other - # experimental knobs (batch>1 above, av_decouple's shape check) avoid. + # A direction banked under a different TOTAL size (model swap, a + # different H3 variant on the same refinement_key) cannot be reshaped + # onto this run's Q at all -- .view() would hard-crash the generation + # instead of degrading, exactly what this file's other experimental + # knobs (batch>1 above, av_decouple's shape check) avoid. if block not in _warned_shape: _warned_shape.add(block) print(f"[FunPackSceneChain] H3 query steering: block {block}'s " diff --git a/tests/test_h3_q_steering.py b/tests/test_h3_q_steering.py index b7d224e0..81c66350 100644 --- a/tests/test_h3_q_steering.py +++ b/tests/test_h3_q_steering.py @@ -262,5 +262,41 @@ def test_mismatched_direction_length_does_not_crash(monkeypatch, capsys): assert "learned direction has 3 values" in capsys.readouterr().out +def _run_chunk_with(refinement_key="fake_key", **kw): + """Runs _sample_chunk against a stubbed sample_custom (returns the input latent + unchanged) -- enough comfy surface for the install-time gating logic to execute for + real, without a real sampler or GPU.""" + sys.modules["comfy.sample"].prepare_noise = lambda samples, seed, **_: torch.zeros_like(samples) + sys.modules["comfy.sample"].sample_custom = lambda model, noise, cfg, smp, sigmas, pos, neg, samples, **k: samples + latent = {"samples": torch.zeros(1, 2, 2, 2, 2)} + sampler = type("F", (), {"extra_options": {}, "sampler_function": None})() + S()._sample_chunk(object(), sampler, torch.tensor([1.0, 0.0]), 0, 1.0, [], [], latent, + refinement_key=refinement_key, **kw) + + +def test_av_decouple_together_skips_q_steer_install_with_a_declared_reason(monkeypatch, capsys): + """h3_av_decouple, when also enabled, becomes the OUTER optimized_attention_override and + always hands its inner override a non-None mask for exactly the video-row partition -- + the one case q_steering's own override treats as 'someone else already handled this, + back off'. The two features cannot compose today, so q_steering must not be installed at + all when both are requested together (paying the clone/hook cost and printing 'applying' + while doing nothing would be worse than not installing).""" + monkeypatch.setattr(_rs, "save_pending", lambda *a, **k: (_ for _ in ()).throw( + AssertionError("q_steer must never capture anything when av_decouple is also on"))) + _run_chunk_with(h3_q_steer_block="0,1", h3_q_steer_strength=1.0, h3_av_decouple=1.0) + out = capsys.readouterr().out + assert "h3_av_decouple is also enabled" in out + assert "NOT installed this run" in out + + +def test_q_steer_alone_is_unaffected_by_the_av_decouple_guard(monkeypatch, capsys): + """Sanity check for the fix above: the guard must only fire when av_decouple is actually + on, not whenever q_steer is requested at all.""" + monkeypatch.setattr(_rs, "direction", lambda *a, **k: (None, 0, 0)) + _run_chunk_with(h3_q_steer_block="0,1", h3_q_steer_strength=1.0, h3_av_decouple=0.0) + out = capsys.readouterr().out + assert "h3_av_decouple is also enabled" not in out + + if __name__ == "__main__": print("run via pytest -- these tests need the monkeypatch/capsys fixtures")