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).
This commit is contained in:
digital-garbage
+12
-6
@@ -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:
|
||||
|
||||
+32
-8
@@ -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 "
|
||||
|
||||
@@ -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")
|
||||
|
||||
Reference in New Issue
Block a user