From 3fe9c0c70ddc66f0f9fda44aea8fbd322862e54c Mon Sep 17 00:00:00 2001 From: Aryan Kumar <156166681+aryan5v@users.noreply.github.com> Date: Mon, 2 Nov 2026 23:32:40 -0800 Subject: [PATCH] [bugfix]: get the full CI suite green on main (pytest 9 flag teardown, Qwen golden env) (#2059) Co-authored-by: Aryan Kumar --- ..._pytest9-monkeypatch-torch-flag-proxies.md | 37 ++++++++++++++ .github/workflows/ci-trigger-full-suite.yml | 5 +- .github/workflows/publish-comfyui.yml | 48 +++++++++++++++++-- fastvideo/attention/utils/fa4_env_cache.py | 26 ++++++++-- .../tests/attention/test_cudnn_sdpa_dense.py | 13 ++--- .../tests/attention/test_fa4_env_cache.py | 29 +++++++++++ fastvideo/tests/conftest.py | 19 ++++++++ .../tests/contract/test_ci_test_collection.py | 3 ++ .../tests/golden_gate/test_qwen_image21.py | 10 ++-- .../tests/layers/test_lora_merge_precision.py | 4 +- .../tests/layers/test_lora_side_branch.py | 31 +++++++----- .../tests/transformers/test_hunyuan_batch.py | 5 +- .../test_qwen_image21_attention.py | 5 +- 13 files changed, 197 insertions(+), 38 deletions(-) create mode 100644 .agents/lessons/2026-11-02_pytest9-monkeypatch-torch-flag-proxies.md diff --git a/.agents/lessons/2026-11-02_pytest9-monkeypatch-torch-flag-proxies.md b/.agents/lessons/2026-11-02_pytest9-monkeypatch-torch-flag-proxies.md new file mode 100644 index 000000000..a12ea19fe --- /dev/null +++ b/.agents/lessons/2026-11-02_pytest9-monkeypatch-torch-flag-proxies.md @@ -0,0 +1,37 @@ +--- +date: 2026-11-02 +experiment: FastVideo CI repair on main (Transformer and Unit lanes) +category: infrastructure +severity: important +--- + +# pytest 9 monkeypatch cannot restore torch flag proxies + +## What Happened + +After the CI image picked up pytest 9, the Transformer lane reported 22 +teardown errors (`'cuBLASModule' object has no attribute 'allow_tf32'`) and +the Unit lane reported 28 setup errors in `test_lora_side_branch.py` +(`torch._dynamo.config ... has no attribute 'disable'`). The test bodies +still passed. + +## Root Cause + +pytest 9's `monkeypatch.setattr` reads the previous value from the target's +instance `__dict__`. `torch.backends.cuda.matmul` and `torch._dynamo.config` +serve their flags through `__getattr__`, so pytest records the value as +missing. Teardown then calls `delattr`: on cuBLAS that raises and leaves TF32 +changed, and on the dynamo config it deletes the `disable` entry, so every +later `setattr` on it fails. pytest 8.4 restores both correctly. + +## Fix / Workaround + +Use the owner's own scoping instead of monkeypatch: +`torch._dynamo.config.patch(name=value)` for dynamo/inductor config, and the +`tf32_disabled` fixture in `fastvideo/tests/conftest.py` for the TF32 flags. + +## Prevention + +Do not `monkeypatch.setattr` attributes of `torch.backends.cuda.matmul`, +`torch._dynamo.config`, `torch._inductor.config` or other torch config +proxies. Module functions such as `torch.compiler.is_compiling` are safe. diff --git a/.github/workflows/ci-trigger-full-suite.yml b/.github/workflows/ci-trigger-full-suite.yml index bc8bfcc48..0d724290e 100644 --- a/.github/workflows/ci-trigger-full-suite.yml +++ b/.github/workflows/ci-trigger-full-suite.yml @@ -22,8 +22,9 @@ jobs: trigger: if: >- inputs.pr_number > 0 - || (github.event.action == 'labeled' && github.event.label.name == 'ready') - || github.event.action == 'synchronize' + || (github.event.pull_request.base.ref == github.event.repository.default_branch + && ((github.event.action == 'labeled' && github.event.label.name == 'ready') + || github.event.action == 'synchronize')) runs-on: ubuntu-latest # Job-level concurrency: only this guarded job acquires the group, so an # unrelated `labeled` event (which skips the job) cannot cancel an in-flight diff --git a/.github/workflows/publish-comfyui.yml b/.github/workflows/publish-comfyui.yml index 031d52b3d..921c0bc6b 100644 --- a/.github/workflows/publish-comfyui.yml +++ b/.github/workflows/publish-comfyui.yml @@ -12,10 +12,52 @@ permissions: issues: write jobs: - publish-node: - name: Publish Custom Node to registry + # Any pyproject.toml edit triggers this workflow, but the registry rejects a + # version it already has ("The node version already exists"). Publish only + # when [project].version changed, as the PyPI workflow does. + check-version-change: runs-on: ubuntu-latest if: ${{ github.repository_owner == 'hao-ai-lab' }} + outputs: + changed: ${{ steps.check.outputs.changed }} + steps: + - name: Check out code + uses: actions/checkout@v4 + with: + fetch-depth: 2 + - name: Compare [project].version with the version before this push + id: check + shell: python + env: + BEFORE_SHA: ${{ github.event.before }} + run: | + import os + import subprocess + import tomllib + + def version(text: str) -> str | None: + return tomllib.loads(text).get("project", {}).get("version") + + def git(*args: str) -> subprocess.CompletedProcess: + return subprocess.run(["git", *args], capture_output=True, text=True) + + # A push can carry several commits; compare with the tip before the push, not HEAD~1. + base = os.environ.get("BEFORE_SHA", "").strip() + if not base or set(base) == {"0"} or git("fetch", "--no-tags", "--depth=1", "origin", base).returncode: + base = "HEAD~1" + new = version(open("pyproject.toml", encoding="utf-8").read()) + previous = git("show", f"{base}:pyproject.toml") + old = version(previous.stdout) if previous.returncode == 0 else None + changed = new is not None and new != old + print(f"base: {base}, previous version: {old}, current version: {new}, changed: {changed}") + with open(os.environ["GITHUB_OUTPUT"], "a", encoding="utf-8") as output: + output.write(f"changed={str(changed).lower()}\n") + + publish-node: + name: Publish Custom Node to registry + needs: check-version-change + runs-on: ubuntu-latest + if: ${{ github.repository_owner == 'hao-ai-lab' && (needs.check-version-change.outputs.changed == 'true' || github.event_name == 'workflow_dispatch') }} steps: - name: Check out code uses: actions/checkout@v4 @@ -25,4 +67,4 @@ jobs: uses: Comfy-Org/publish-node-action@v1 with: ## Add your own personal access token to your Github Repository secrets and reference it here. - personal_access_token: ${{ secrets.REGISTRY_ACCESS_TOKEN }} \ No newline at end of file + personal_access_token: ${{ secrets.REGISTRY_ACCESS_TOKEN }} diff --git a/fastvideo/attention/utils/fa4_env_cache.py b/fastvideo/attention/utils/fa4_env_cache.py index 218d9be18..3a8253a27 100644 --- a/fastvideo/attention/utils/fa4_env_cache.py +++ b/fastvideo/attention/utils/fa4_env_cache.py @@ -28,14 +28,34 @@ def fa4_env_fingerprint(environ: Mapping[str, str] | None = None, ) -> tuple[tup return tuple(pairs) +def _cached_entries(cache: object) -> dict | None: + """The in-memory entries of an FA4 JIT cache, or None if ``cache`` is not one. + + Older FA4 builds keep a plain dict; the pinned ref wraps it in ``cache_utils.JITCache`` + (entries under ``.cache``), which is not a dict, so checking only for dicts cleared nothing. + """ + if isinstance(cache, dict): + return cache + entries = getattr(cache, "cache", None) + if isinstance(entries, dict) and callable(getattr(cache, "clear", None)): + return entries + return None + + def clear_fa4_jit_caches(kernel_objects: Iterable[object]) -> int: - """Drop every FA4 JIT cache on ``kernel_objects``; return entries dropped.""" + """Drop every FA4 JIT cache on ``kernel_objects``; return entries dropped. + + Clearing goes through the cache's own ``clear()``. For FA4's opt-in persistent cache + (FLASH_ATTENTION_CUTE_DSL_CACHE_ENABLED=1) that also purges its on-disk kernels, whose keys + omit the tuning env as well; it only happens when compiled kernels are held in memory. + """ cleared = 0 for kernel_object in kernel_objects: for attribute in _JIT_CACHE_ATTRS: cache = getattr(kernel_object, attribute, None) - if isinstance(cache, dict) and cache: - cleared += len(cache) + entries = _cached_entries(cache) + if entries: + cleared += len(entries) cache.clear() return cleared diff --git a/fastvideo/tests/attention/test_cudnn_sdpa_dense.py b/fastvideo/tests/attention/test_cudnn_sdpa_dense.py index d3c369bbe..92f232cff 100644 --- a/fastvideo/tests/attention/test_cudnn_sdpa_dense.py +++ b/fastvideo/tests/attention/test_cudnn_sdpa_dense.py @@ -49,14 +49,11 @@ def test_cudnn_route_presents_bhsd_strided_views(): def test_cudnn_route_is_bit_identical_to_cudnn_dispatched_sdpa(): query, key, value = _qkv(2048) scale = 1.0 / math.sqrt(query.shape[-1]) - previous = torch.backends.cuda.cudnn_sdp_enabled() - torch.backends.cuda.enable_cudnn_sdp(True) - try: - with torch.no_grad(): - reference = torch.nn.functional.scaled_dot_product_attention( - query.transpose(1, 2), key.transpose(1, 2), value.transpose(1, 2), scale=scale).transpose(1, 2) - finally: - torch.backends.cuda.enable_cudnn_sdp(previous) + # Pin the reference to cuDNN: enabling it only makes it eligible, and the dispatcher + # prefers another backend on some GPUs (GB10), which is not bit-identical. + with torch.no_grad(), torch.nn.attention.sdpa_kernel(torch.nn.attention.SDPBackend.CUDNN_ATTENTION): + reference = torch.nn.functional.scaled_dot_product_attention( + query.transpose(1, 2), key.transpose(1, 2), value.transpose(1, 2), scale=scale).transpose(1, 2) with torch.no_grad(): output = cudnn_sdpa_bshd(query, key, value, scale=scale) diff --git a/fastvideo/tests/attention/test_fa4_env_cache.py b/fastvideo/tests/attention/test_fa4_env_cache.py index aebcce918..20c36ac59 100644 --- a/fastvideo/tests/attention/test_fa4_env_cache.py +++ b/fastvideo/tests/attention/test_fa4_env_cache.py @@ -98,6 +98,35 @@ def test_unrelated_environment_changes_do_not_drop_kernels() -> None: assert list(fwd.compile_cache) == ["a"] +class _JITCacheLike: + """Shape of ``flash_attn.cute.cache_utils.JITCache`` at the pinned FA4 ref: not a dict.""" + + def __init__(self, entries: dict) -> None: + self.cache = dict(entries) + + def __contains__(self, key) -> bool: + return key in self.cache + + def clear(self) -> None: + self.cache.clear() + + +def test_jit_cache_objects_are_cleared_through_their_own_clear() -> None: + """The pinned FA4 wraps its compile cache in a JITCache; a dict-only check cleared nothing.""" + fwd = SimpleNamespace(compile_cache=_JITCacheLike({"a": object(), "b": object()})) + bwd = SimpleNamespace(compile_cache=_JITCacheLike({}), compile_cache_pre=_JITCacheLike({"c": object()})) + + assert fa4_env_cache.clear_fa4_jit_caches((fwd, bwd)) == 3 + assert fwd.compile_cache.cache == {} + assert bwd.compile_cache_pre.cache == {} + + fwd.compile_cache.cache["a"] = object() + assert fa4_env_cache.invalidate_fa4_jit_caches_on_env_change((fwd, ), {"FA_X": "1"}) is True + fwd.compile_cache.cache["a"] = object() + assert fa4_env_cache.invalidate_fa4_jit_caches_on_env_change((fwd, ), {"FA_X": "0"}) is True + assert "a" not in fwd.compile_cache + + def test_missing_or_non_dict_caches_are_ignored() -> None: bare = SimpleNamespace() non_dict = SimpleNamespace(compile_cache=["not", "a", "dict"]) diff --git a/fastvideo/tests/conftest.py b/fastvideo/tests/conftest.py index dd5d65764..42fc6ecce 100644 --- a/fastvideo/tests/conftest.py +++ b/fastvideo/tests/conftest.py @@ -45,3 +45,22 @@ def env_overrides(): """ with contextlib.ExitStack() as stack: yield stack + + +@pytest.fixture +def tf32_disabled(): + """Disable TF32 for CUDA matmuls and cuDNN during the test, then restore the previous flags. + + ``torch.backends.cuda.matmul`` serves ``allow_tf32`` through ``__getattr__``. pytest 9's + ``monkeypatch.setattr`` reads the old value from the instance ``__dict__``, finds nothing, and + its teardown ``delattr`` raises instead of restoring the flag. Restore through the same legacy + flags: mixing them with the per-backend ``fp32_precision`` API makes later ``allow_tf32`` reads raise. + """ + matmul, cudnn = torch.backends.cuda.matmul, torch.backends.cudnn + previous = (matmul.allow_tf32, cudnn.allow_tf32) + matmul.allow_tf32 = False + cudnn.allow_tf32 = False + try: + yield + finally: + matmul.allow_tf32, cudnn.allow_tf32 = previous diff --git a/fastvideo/tests/contract/test_ci_test_collection.py b/fastvideo/tests/contract/test_ci_test_collection.py index c0c11d88b..dcb31111d 100644 --- a/fastvideo/tests/contract/test_ci_test_collection.py +++ b/fastvideo/tests/contract/test_ci_test_collection.py @@ -187,6 +187,9 @@ def test_merge_comment_has_one_change_aware_trigger_path(): assert "pr.state !== 'open'" in ready_workflow assert "pr.base.repo.full_name !== context.payload.repository.full_name" in ready_workflow assert "pr.base.ref !== context.payload.repository.default_branch" in ready_workflow + trigger_if = yaml.safe_load(ready_workflow)["jobs"]["trigger"]["if"] + assert "github.event.pull_request.base.ref == github.event.repository.default_branch" in trigger_if + assert trigger_if.startswith("inputs.pr_number > 0 ||") assert "core.setOutput('head_sha', pr.head.sha)" in ready_workflow assert "core.setOutput('base_sha', pr.base.sha)" in ready_workflow assert "api.buildkite.com" in ready_workflow diff --git a/fastvideo/tests/golden_gate/test_qwen_image21.py b/fastvideo/tests/golden_gate/test_qwen_image21.py index f32ec4a42..9ec559843 100644 --- a/fastvideo/tests/golden_gate/test_qwen_image21.py +++ b/fastvideo/tests/golden_gate/test_qwen_image21.py @@ -6,10 +6,12 @@ the target image. Causal text segments use masked SDPA; reference and target segments use FlashAttention, including their unequal query/key lengths. Complex RoPE and the two modulation rows match the causal-conditioning block interface. -Requires a BF16-capable CUDA GPU and FlashAttention. Select FA4 with -FASTVIDEO_FA4=1 before starting Python. The shared harness downloads -only checkpoint shards containing block 0 and compares a device/runtime-matched -golden exactly; it never loads the text encoder or VAE. +Requires a BF16-capable CUDA GPU and FlashAttention. Like the other block +goldens, the published references match the golden-gate CI lane: FA2 with +FASTVIDEO_FA4 off. Running with FASTVIDEO_FA4=1 reports an environment +mismatch rather than drift. The shared harness downloads only checkpoint +shards containing block 0 and compares a device/runtime-matched golden +exactly; it never loads the text encoder or VAE. """ from __future__ import annotations diff --git a/fastvideo/tests/layers/test_lora_merge_precision.py b/fastvideo/tests/layers/test_lora_merge_precision.py index 6bbf39ca0..51d813ed8 100644 --- a/fastvideo/tests/layers/test_lora_merge_precision.py +++ b/fastvideo/tests/layers/test_lora_merge_precision.py @@ -15,8 +15,10 @@ IN, OUT, RANK = 32, 24, 16 @pytest.fixture(autouse=True) def _cpu_merge(monkeypatch: pytest.MonkeyPatch): - monkeypatch.setattr(torch._dynamo.config, "disable", True) + # config.patch restores the flag; a monkeypatch.setattr teardown would delete it (pytest 9). monkeypatch.setattr(lora_linear, "get_local_torch_device", lambda: torch.device("cpu")) + with torch._dynamo.config.patch(disable=True): + yield def _merged(adapter_dtype: torch.dtype) -> tuple[torch.Tensor, torch.Tensor, torch.Tensor, torch.Tensor]: diff --git a/fastvideo/tests/layers/test_lora_side_branch.py b/fastvideo/tests/layers/test_lora_side_branch.py index 04a804df4..3222fe19e 100644 --- a/fastvideo/tests/layers/test_lora_side_branch.py +++ b/fastvideo/tests/layers/test_lora_side_branch.py @@ -23,9 +23,21 @@ IN, OUT, RANK = 16, 12, 4 @pytest.fixture(autouse=True) def _eager_forward(monkeypatch: pytest.MonkeyPatch): - """``BaseLayerWithLoRA.forward`` is ``torch.compile``d; run it eagerly, and merge on CPU even on MPS hosts.""" - monkeypatch.setattr(torch._dynamo.config, "disable", True) + """``BaseLayerWithLoRA.forward`` is ``torch.compile``d; run it eagerly, and merge on CPU even on MPS hosts. + + Dynamo flags go through ``config.patch``: its module serves them via ``__getattr__``, so a + ``monkeypatch.setattr`` teardown deletes the flag instead of restoring it (pytest 9). + """ monkeypatch.setattr(lora_linear, "get_local_torch_device", lambda: torch.device("cpu")) + with torch._dynamo.config.patch(disable=True): + yield + + +@pytest.fixture +def dynamo_enabled(): + """Re-enable dynamo inside the autouse eager scope for tests that compile on purpose.""" + with torch._dynamo.config.patch(disable=False): + yield class _PackedMethod: @@ -84,14 +96,12 @@ def test_wrapped_layer_at_strength_zero_is_bitwise_the_base_layer(packed: bool) @pytest.mark.parametrize("backend", ["eager", "inductor"]) @pytest.mark.parametrize("adapter", [False, True]) -def test_wrapping_does_not_change_a_compiled_block(monkeypatch: pytest.MonkeyPatch, backend: str, - adapter: bool) -> None: +def test_wrapping_does_not_change_a_compiled_block(dynamo_enabled, backend: str, adapter: bool) -> None: """Under a caller's compile the base layer is traced with the block, as it is unwrapped; the wrapper adds no region of its own, so the compiled block is unchanged with no adapter or at strength 0.""" if backend == "inductor" and not torch.cuda.is_available(): pytest.skip("Inductor execution requires a CUDA GPU for this regression") device = torch.device("cuda" if backend == "inductor" else "cpu") - monkeypatch.setattr(torch._dynamo.config, "disable", False) torch._dynamo.reset() class Block(torch.nn.Module): @@ -117,9 +127,9 @@ def test_wrapping_does_not_change_a_compiled_block(monkeypatch: pytest.MonkeyPat torch._dynamo.reset() -def test_wrapper_adds_no_compiled_region_around_the_base_layer(monkeypatch: pytest.MonkeyPatch) -> None: +def test_wrapper_adds_no_compiled_region_around_the_base_layer(monkeypatch: pytest.MonkeyPatch, + dynamo_enabled) -> None: """Compiling a packed base layer on its own changes its numerics, so an uncompiled caller runs it eagerly.""" - monkeypatch.setattr(torch._dynamo.config, "disable", False) torch._dynamo.reset() base = _packed_linear() layer = get_lora_layer(base) @@ -179,11 +189,11 @@ def test_side_branch_chunked_update_matches_the_full_product(monkeypatch: pytest @pytest.mark.parametrize("backend", ["eager", "inductor"]) -def test_side_branch_chunked_update_compiles_fullgraph(monkeypatch: pytest.MonkeyPatch, backend: str) -> None: +def test_side_branch_chunked_update_compiles_fullgraph(monkeypatch: pytest.MonkeyPatch, dynamo_enabled, + backend: str) -> None: if backend == "inductor" and not torch.cuda.is_available(): pytest.skip("Inductor execution requires a CUDA GPU for this regression") device = torch.device("cuda" if backend == "inductor" else "cpu") - monkeypatch.setattr(torch._dynamo.config, "disable", False) monkeypatch.setattr(lora_linear, "_SIDE_BRANCH_CHUNK_BYTES", 64) torch._dynamo.reset() base = _packed_linear().to(device) @@ -221,9 +231,8 @@ def test_side_branch_chunked_update_compiles_fullgraph(monkeypatch: pytest.Monke torch._dynamo.reset() -def test_side_branch_strength_changes_do_not_recompile(monkeypatch: pytest.MonkeyPatch) -> None: +def test_side_branch_strength_changes_do_not_recompile(dynamo_enabled) -> None: """A float strength is guarded by value, so fullgraph would fail after ``recompile_limit`` distinct strengths.""" - monkeypatch.setattr(torch._dynamo.config, "disable", False) torch._dynamo.reset() base = _packed_linear() layer = get_lora_layer(base) diff --git a/fastvideo/tests/transformers/test_hunyuan_batch.py b/fastvideo/tests/transformers/test_hunyuan_batch.py index 77dff563e..01887a66e 100644 --- a/fastvideo/tests/transformers/test_hunyuan_batch.py +++ b/fastvideo/tests/transformers/test_hunyuan_batch.py @@ -25,7 +25,7 @@ pytestmark = pytest.mark.skipif(not torch.cuda.is_available(), reason="Requires @pytest.fixture -def hunyuan_setup(monkeypatch, request): +def hunyuan_setup(request): # Environment writes go through the registry helpers (docs/contributing/env_vars.md). with contextlib.ExitStack() as stack: # Respect the port reserved by the CI runner when sharing a GPU host. @@ -37,8 +37,7 @@ def hunyuan_setup(monkeypatch, request): if "MASTER_ADDR" not in os.environ: stack.enter_context(envs.override_external("MASTER_ADDR", "127.0.0.1")) stack.enter_context(envs.FASTVIDEO_ATTENTION_BACKEND.override("TORCH_SDPA")) - monkeypatch.setattr(torch.backends.cuda.matmul, "allow_tf32", False) - monkeypatch.setattr(torch.backends.cudnn, "allow_tf32", False) + request.getfixturevalue("tf32_disabled") request.getfixturevalue("distributed_setup") yield diff --git a/fastvideo/tests/transformers/test_qwen_image21_attention.py b/fastvideo/tests/transformers/test_qwen_image21_attention.py index 2fa32f919..0c1253c09 100644 --- a/fastvideo/tests/transformers/test_qwen_image21_attention.py +++ b/fastvideo/tests/transformers/test_qwen_image21_attention.py @@ -24,12 +24,11 @@ from fastvideo.layers.linear import ReplicatedLinear from fastvideo.platforms import AttentionBackendEnum @pytest.fixture -def qwen_setup(env_overrides, monkeypatch, request): +def qwen_setup(env_overrides, request): if not torch.cuda.is_available(): pytest.skip("Requires one CUDA GPU") env_overrides.enter_context(envs.FASTVIDEO_ATTENTION_BACKEND.override("TORCH_SDPA")) - monkeypatch.setattr(torch.backends.cuda.matmul, "allow_tf32", False) - monkeypatch.setattr(torch.backends.cudnn, "allow_tf32", False) + request.getfixturevalue("tf32_disabled") request.getfixturevalue("distributed_setup") with torch.no_grad(), sdpa_kernel(SDPBackend.MATH): yield torch.device("cuda")