From 4c44a8e275016f616afb765bbff2be3ffc340c79 Mon Sep 17 00:00:00 2001 From: WildAi <2853742+wildminder@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:19:46 +0300 Subject: [PATCH] plan(P2): address review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five review findings of the P2 phase, each fixed at its root: 1. (medium) The P2.6 guard could not see an undeclared third-party import: the closure's imports were filtered through a hard-coded 10-name map, so every module outside it was dropped silently (a module-level `import av` in a copy of the repo still gave "1 passed"). The map is now a *rename* map (cv2 -> opencv-python, PIL -> Pillow, yaml -> pyyaml); every other module is mapped to its own name, so an unknown module shows up as an undeclared dependency. The guard is split by import time: every third-party module imported at import time must be a declared distribution, every lazy (function-level) one must be allowlisted with a reason (dlib, gdown, mc, wandb - all unreachable from the node). 2. (low) The P2.1 (c) progression test was vacuous: it returned before its assertions and probed a second, top-level copy of the vendored tree (`sys.path.insert(modules/deps)`). It now imports the packaged architecture, asserts that FeedForward / AdaLayerNorm come from the vendored _diffusers_compat and that no top-level `basicsr` shadows an installed one. 3. (low) Two swallow-and-stub fallbacks remained in the vendored tree (facelib/detection, facelib/parsing - their stubs called an unimported `urlparse` and would have raised NameError). Both now import the vendored symbol directly; a guard forbids a fallback stub in the vendored tree. 4. (low) `basicsr/utils/file_client.py` still appended to `sys.path` (the optional `sys_path` of MemcachedBackend), which the plan's §3 rule forbids anywhere in the package. The mutation and the parameter are gone and guard (d) now covers the whole package (the vendored tree included). 5. (low) The unreachable body of `KEEPFaceProcessor.process_video` still called the deleted VideoReader / VideoWriter. The 141 dead lines are removed; an AST guard keeps the references out. Found while fixing the findings: the CI job did not install `requests`, which the vendored `basicsr/utils/download_util.py` imports at module level (P2.6 added it to requirements.txt but not to the workflow) - a clean CI runner died with "No module named 'requests'". The install list is now completed and a test derives it from requirements.txt. Gate: pytest -m "unit or mock_integration" -> 109 passed, 1 skipped, 4 xfailed; plan's local gate (+ headless) -> 109 passed, 3 skipped, 4 xfailed; tools/scan_compat.py -> finding_count 0 (exit 0); ruff check . -> All checks passed. A mutation check re-introduces six defects into a copy of the repository: the matching guard fails in 6/6 cases. --- .github/workflows/tests.yml | 5 +- modules/deps/basicsr/utils/file_client.py | 11 +- modules/deps/facelib/detection/__init__.py | 19 +- modules/deps/facelib/parsing/__init__.py | 17 +- modules/keep_processor.py | 151 +-------- tests/test_ci_workflow.py | 57 ++++ tests/test_structure.py | 25 +- tests/test_vendored_repair.py | 375 ++++++++++++++++++--- 8 files changed, 418 insertions(+), 242 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 9b29b64..cfcb91b 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -80,7 +80,10 @@ jobs: # PyYAML: the vendored tree imports it eagerly # (modules/deps/basicsr/utils/options.py:5 -> basicsr/utils/__init__.py:8), # so the unit tier fails with "No module named 'yaml'" without it. - python -m pip install opencv-python-headless einops scipy Pillow tqdm pytest pyyaml + # requests: the vendored download utility imports it eagerly + # (modules/deps/basicsr/utils/download_util.py:3 -> modules/utils.py:19), + # so the unit tier fails with "No module named 'requests'" without it. + python -m pip install opencv-python-headless einops scipy Pillow tqdm pytest pyyaml requests - name: Run the unit, mock_integration and headless tiers run: python -m pytest -q -m "unit or mock_integration or headless" diff --git a/modules/deps/basicsr/utils/file_client.py b/modules/deps/basicsr/utils/file_client.py index 62d4ea1..0416d96 100644 --- a/modules/deps/basicsr/utils/file_client.py +++ b/modules/deps/basicsr/utils/file_client.py @@ -1,4 +1,8 @@ # Modified from https://github.com/open-mmlab/mmcv/blob/master/mmcv/fileio/file_client.py # noqa: E501 +# +# The optional ``sys_path`` parameter of ``MemcachedBackend`` (it appended to +# ``sys.path``) is removed: the package must never mutate ``sys.path`` +# (plan §3, import rules). Nothing in the closure passes it. from abc import ABCMeta, abstractmethod @@ -25,14 +29,9 @@ class MemcachedBackend(BaseStorageBackend): Attributes: server_list_cfg (str): Config file for memcached server list. client_cfg (str): Config file for memcached client. - sys_path (str | None): Additional path to be appended to `sys.path`. - Default: None. """ - def __init__(self, server_list_cfg, client_cfg, sys_path=None): - if sys_path is not None: - import sys - sys.path.append(sys_path) + def __init__(self, server_list_cfg, client_cfg): try: import mc except ImportError: diff --git a/modules/deps/facelib/detection/__init__.py b/modules/deps/facelib/detection/__init__.py index 80af649..4d4b50d 100644 --- a/modules/deps/facelib/detection/__init__.py +++ b/modules/deps/facelib/detection/__init__.py @@ -2,16 +2,10 @@ import os import torch import pathlib -try: - from ...basicsr.utils.download_util import load_file_from_url -except ImportError: - # Minimal fallback if basicsr isn't in path when facelib is directly used (should not happen with our setup) - def load_file_from_url(url, model_dir, **kwargs): - print(f"Fallback load_file_from_url in facelib.detection for: {url} to {model_dir}") - filename = os.path.basename(urlparse(url).path) - path = os.path.join(model_dir, filename) - if not os.path.exists(path): raise FileNotFoundError(f"{path} not found.") - return path +# P2.7: the swallow-and-stub fallback is gone. The vendored download utility +# is imported **relatively** (P2.5), so a broken vendored dependency has to +# fail loudly instead of silently degrading into a local stub. +from ...basicsr.utils.download_util import load_file_from_url # Model URLs (these might be defined elsewhere in original facelib, ensure they are here or imported) URLS = { @@ -62,8 +56,9 @@ def init_detection_model(model_name, half=False, device='cuda', model_rootpath=N # This case should ideally be handled by the ComfyUI loader downloading it first. # If it still doesn't exist, it means the ComfyUI loader failed or paths are wrong. print(f"Warning (init_detection_model): Model file not found at {model_path_final}. Attempting download via facelib's URL.") - # This load_file_from_url is from basicsr, which should be in sys.path - # It will try to download to a generic 'weights/facelib' if model_dir is relative like this. + # This load_file_from_url is the vendored basicsr one (relative import + # above) - it will try to download to a generic 'weights/facelib' if + # model_dir is relative like this. # This is not ideal as it bypasses ComfyUI's model management. # Better to ensure load_file_from_url_comfy in our utils handles all downloads. model_path_final = load_file_from_url(url=URLS[model_name], model_dir=model_rootpath, file_name=model_file_name) diff --git a/modules/deps/facelib/parsing/__init__.py b/modules/deps/facelib/parsing/__init__.py index 5f491db..11c603b 100644 --- a/modules/deps/facelib/parsing/__init__.py +++ b/modules/deps/facelib/parsing/__init__.py @@ -1,17 +1,12 @@ # File: ComfyUI-KEEP/modules/deps/facelib/parsing/__init__.py import os import torch -# Assuming load_file_from_url is now globally accessible via sys.path or vendored -try: - from ...basicsr.utils.download_util import load_file_from_url -except ImportError: - def load_file_from_url(url, model_dir, **kwargs): - print(f"Fallback load_file_from_url in facelib.parsing for: {url} to {model_dir}") - filename = os.path.basename(urlparse(url).path) - path = os.path.join(model_dir, filename) - if not os.path.exists(path): raise FileNotFoundError(f"{path} not found.") - return path - + +# P2.7: the swallow-and-stub fallback is gone. The vendored download utility +# is imported **relatively** (P2.5), so a broken vendored dependency has to +# fail loudly instead of silently degrading into a local stub. +from ...basicsr.utils.download_util import load_file_from_url + URLS = { 'parsenet': 'https://github.com/jnjaby/KEEP/releases/download/v1.0.0/parsing_parsenet.pth' } diff --git a/modules/keep_processor.py b/modules/keep_processor.py index 1e63bdc..775d7b3 100644 --- a/modules/keep_processor.py +++ b/modules/keep_processor.py @@ -17,6 +17,10 @@ from comfy.utils import ProgressBar, tiled_scale # the package calls it; P4.7 removes it), so it now fails loudly instead of # silently importing a stub. The image / image-sequence paths are unaffected. # +# P2 review fix: the unreachable body of ``process_video`` (it still called +# the deleted ``VideoReader`` / ``VideoWriter``) is gone - the method is now +# the raise and nothing else. +# # P2.7: the dummy image helpers are gone - a broken vendored dependency must # fail loudly instead of returning ``None`` images. from .deps.basicsr.utils.img_util import img2tensor, tensor2img @@ -161,8 +165,10 @@ class KEEPFaceProcessor: The vendored ``basicsr.utils.video_util`` module executed a pip install at import time; it was deleted in P2.1 and this dead code - path was disabled with it. Use ``KEEP_ProcessImageSequence`` for - sequences (P4.7 removes this method entirely). + path was disabled with it. The unreachable body that still used the + deleted ``VideoReader`` / ``VideoWriter`` was removed in the P2 + review fix. Use ``KEEP_ProcessImageSequence`` for sequences (P4.7 + removes this method entirely). """ raise NotImplementedError( "process_video is not supported any more: the vendored " @@ -170,147 +176,6 @@ class KEEPFaceProcessor: "KEEP_ProcessImageSequence node for image sequences." ) - vidreader = VideoReader(video_path) - original_fps = vidreader.get_fps() - num_input_frames = len(vidreader) - - pbar_comfy = ProgressBar(num_input_frames * 5) - - input_frames_bgr = [] - for _ in tqdm(range(num_input_frames), desc="Reading video frames"): - img = vidreader.get_frame() - if img is None: break - input_frames_bgr.append(img) - pbar_comfy.update(1) - vidreader.close() - - if not input_frames_bgr: return None - - avg_landmarks_sequence = None - if not has_aligned: - raw_landmarks_list_video = [] - for i in tqdm(range(num_input_frames), desc="Detecting face landmarks"): - img = input_frames_bgr[i] - self.face_helper.clean_all() - self.face_helper.read_image(img) - num_det_faces = self.face_helper.get_face_landmarks_5( - only_center_face=only_center_face, resize=640, eye_dist_threshold=5, - only_keep_largest=True) - if num_det_faces >= 1: - raw_landmarks_list_video.append(self.face_helper.all_landmarks_5[0].reshape((10,))) - else: - raw_landmarks_list_video.append(np.array([np.nan] * 10)) - pbar_comfy.update(1) - raw_landmarks_np_video = np.array(raw_landmarks_list_video) - if np.all(np.isnan(raw_landmarks_np_video)): return None - for i_lm_coord in range(raw_landmarks_np_video.shape[1]): - raw_landmarks_np_video[:, i_lm_coord] = interpolate_sequence(raw_landmarks_np_video[:, i_lm_coord]) - avg_landmarks_sequence = gaussian_filter1d(raw_landmarks_np_video, sigma=5, axis=0).reshape(num_input_frames, 5, 2) - else: - pbar_comfy.update(num_input_frames) - - all_cropped_face_tensors = [] - for i in tqdm(range(num_input_frames), desc="Cropping and aligning faces"): - img_bgr = input_frames_bgr[i] - self.face_helper.clean_all() - self.face_helper.read_image(img_bgr) - if not has_aligned: - if avg_landmarks_sequence is None or i >= len(avg_landmarks_sequence) or np.isnan(avg_landmarks_sequence[i]).any(): - all_cropped_face_tensors.append(torch.zeros(3, 512, 512, dtype=torch.float32, device=self.device) * 0.5) - continue - self.face_helper.all_landmarks_5 = [avg_landmarks_sequence[i]] - self.face_helper.align_warp_face() - else: - img_resized = cv2.resize(img_bgr, (512, 512), interpolation=cv2.INTER_LINEAR) - self.face_helper.is_gray = is_gray(img_resized, threshold=10) - self.face_helper.cropped_faces = [img_resized] - if not self.face_helper.cropped_faces: - all_cropped_face_tensors.append(torch.zeros(3, 512, 512, dtype=torch.float32, device=self.device) * 0.5) - continue - cropped_face_cv2 = self.face_helper.cropped_faces[0] - cropped_face_t = img2tensor(cropped_face_cv2 / 255., bgr2rgb=True, float32=True) - normalize(cropped_face_t, (0.5, 0.5, 0.5), (0.5, 0.5, 0.5), inplace=True) - all_cropped_face_tensors.append(cropped_face_t) - pbar_comfy.update(1) - - if not all_cropped_face_tensors: return None - - batched_cropped_faces = torch.stack(all_cropped_face_tensors, dim=0).unsqueeze(0).to(self.device) - temp_restored_face_tensors = [] - for start_idx in tqdm(range(0, num_input_frames, max_clip_length), desc="Restoring faces with KEEP"): - end_idx = min(start_idx + max_clip_length, num_input_frames) - current_clip_faces = batched_cropped_faces[:, start_idx:end_idx, ...] - if current_clip_faces.shape[1] == 1: - current_clip_faces_for_net = torch.cat([current_clip_faces, current_clip_faces], dim=1) - clip_output_tensor = self.keep_net(current_clip_faces_for_net, need_upscale=False) - temp_restored_face_tensors.append(clip_output_tensor[:, 0:1, ...]) - elif current_clip_faces.shape[1] > 1 : - clip_output_tensor = self.keep_net(current_clip_faces, need_upscale=False) - temp_restored_face_tensors.append(clip_output_tensor) - pbar_comfy.update(end_idx - start_idx) - - if not temp_restored_face_tensors: return None - all_restored_face_tensors = torch.cat(temp_restored_face_tensors, dim=1).squeeze(0) - all_restored_faces_cv2 = [tensor2img(t, rgb2bgr=True, min_max=(-1, 1)) for t in all_restored_face_tensors] - del batched_cropped_faces, all_restored_face_tensors, temp_restored_face_tensors - torch.cuda.empty_cache() - - final_fps = target_fps if target_fps is not None else original_fps - h_orig, w_orig, _ = input_frames_bgr[0].shape - output_video_height, output_video_width = int(h_orig * final_upscale_factor), int(w_orig * final_upscale_factor) - - vidwriter = VideoWriter(output_video_path, output_video_height, output_video_width, final_fps) - - for i in tqdm(range(num_input_frames), desc="Pasting faces and writing video"): - original_frame_bgr = input_frames_bgr[i] - if i >= len(all_restored_faces_cv2): - final_frame_to_write = self._run_upscaler(self.bg_upscale_model, original_frame_bgr) - if final_frame_to_write.shape[0] != output_video_height or final_frame_to_write.shape[1] != output_video_width: - final_frame_to_write = cv2.resize(final_frame_to_write, (output_video_width, output_video_height)) - vidwriter.write_frame(final_frame_to_write) - continue - - restored_face_patch_cv2 = all_restored_faces_cv2[i] - self.face_helper.clean_all() - self.face_helper.read_image(original_frame_bgr) - self.face_helper.add_restored_face(restored_face_patch_cv2.astype('uint8')) - - if not has_aligned: - if avg_landmarks_sequence is None or i >= len(avg_landmarks_sequence) or np.isnan(avg_landmarks_sequence[i]).any(): - final_frame_to_write = self._run_upscaler(self.bg_upscale_model, original_frame_bgr) - if final_frame_to_write.shape[0] != output_video_height or final_frame_to_write.shape[1] != output_video_width: - final_frame_to_write = cv2.resize(final_frame_to_write, (output_video_width, output_video_height)) - vidwriter.write_frame(final_frame_to_write) - continue - - self.face_helper.upscale_factor = final_upscale_factor - self.face_helper.all_landmarks_5 = [avg_landmarks_sequence[i]] - self.face_helper.align_warp_face() - - bg_img_upscaled_by_model = self._run_upscaler(self.bg_upscale_model, original_frame_bgr) - if bg_img_upscaled_by_model.shape[0] != output_video_height or bg_img_upscaled_by_model.shape[1] != output_video_width: - bg_img_final = cv2.resize(bg_img_upscaled_by_model, (output_video_width, output_video_height), interpolation=cv2.INTER_LANCZOS4) - else: - bg_img_final = bg_img_upscaled_by_model - - self.face_helper.get_inverse_affine(None) - pasted_frame = self.face_helper.paste_faces_to_input_image( - upsample_img=bg_img_final, draw_box=draw_box, face_upsampler=self.face_upscale_model) - else: - pasted_frame = self.face_helper.restored_faces[0] - if self.face_upscale_model: - pasted_frame = self._run_upscaler(self.face_upscale_model, pasted_frame) - if len(pasted_frame.shape) == 2: pasted_frame = cv2.cvtColor(pasted_frame, cv2.COLOR_GRAY2BGR) - - if pasted_frame.shape[0] != output_video_height or pasted_frame.shape[1] != output_video_width: - pasted_frame = cv2.resize(pasted_frame, (output_video_width, output_video_height), interpolation=cv2.INTER_LANCZOS4) - - vidwriter.write_frame(pasted_frame) - pbar_comfy.update(1) - - vidwriter.close() - return output_video_path - @torch.no_grad() def process_image_sequence(self, image_sequence_tensor: torch.Tensor, diff --git a/tests/test_ci_workflow.py b/tests/test_ci_workflow.py index 3a301fe..fb68436 100644 --- a/tests/test_ci_workflow.py +++ b/tests/test_ci_workflow.py @@ -317,6 +317,63 @@ def test_test_job_installs_the_real_dependencies(workflow): assert '"3.10"' in text, "the 3.10-only installs must be conditional" +#: The distributions of ``requirements.txt`` that the CI may satisfy with a +#: different build - the headless OpenCV wheel has no GUI dependencies. +_CI_INSTALL_ALIASES = { + "opencv-python": ("opencv-python", "opencv-python-headless"), +} + + +def _runtime_requirements() -> list[str]: + """The distributions of ``requirements.txt`` (the true runtime set).""" + text = (REPO_ROOT / "requirements.txt").read_text(encoding="utf-8") + return [ + line.strip() + for line in text.splitlines() + if line.strip() and not line.strip().startswith("#") + ] + + +def _installed_distributions(run_text: str) -> set[str]: + """Every distribution the job installs (the tokens of its pip calls).""" + installed: set[str] = set() + for line in run_text.splitlines(): + parts = line.split() + if "pip" not in parts or "install" not in parts: + continue + for token in parts[parts.index("install") + 1 :]: + token = token.strip().strip('"').strip("'").lower() + if not token or token.startswith("-"): + continue + for separator in ("<", ">", "=", "["): + token = token.split(separator)[0] + installed.add(token) + return installed + + +@pytest.mark.unit +def test_the_ci_installs_every_runtime_requirement(workflow): + """The CI job installs every distribution of ``requirements.txt``. + + ``requirements.txt`` is the true runtime set of the package (P2.6). A + distribution that is missing from the install list kills the unit tier + on a clean runner with a ``ModuleNotFoundError`` - ``requests`` is + imported **eagerly** by ``modules/deps/basicsr/utils/download_util.py`` + (which the package imports in ``modules/utils.py``). + """ + installed = _installed_distributions(_run_text(workflow["jobs"]["test"])) + missing = [] + for requirement in _runtime_requirements(): + name = requirement.lower().replace("_", "-") + aliases = _CI_INSTALL_ALIASES.get(name, (name,)) + if not any(alias in installed for alias in aliases): + missing.append(requirement) + assert not missing, ( + f"the CI job does not install {missing} - requirements.txt is the " + f"true runtime set (P2.6)" + ) + + @pytest.mark.unit def test_test_job_runs_the_local_gate_command(workflow): """The CI command is exactly the local gate command.""" diff --git a/tests/test_structure.py b/tests/test_structure.py index 76e640e..2790d89 100644 --- a/tests/test_structure.py +++ b/tests/test_structure.py @@ -17,7 +17,8 @@ guard what it checks flip owner (c) the root ``__init__.py`` exposes exactly **P4.4** ``comfy_entrypoint`` (no V1 mapping names) (d) no ``sys.path.insert`` / ``sys.path.append`` **P2.5** - outside ``modules/deps/**`` + anywhere in the package (the vendored tree + included - it was exempt only until P2.5) (e) no ``pdb`` / ``torch.profiler`` / ``import pip`` **P2.1** / ``pip.main`` in the vendored runtime closure ===== ================================================ ================== @@ -45,9 +46,6 @@ REPO_ROOT = Path(__file__).resolve().parent.parent #: therefore out of scope - the guard is about the shipped package. PACKAGE_SOURCE_ROOTS = ("__init__.py", "nodes.py", "nodes", "src", "modules") -#: The vendored tree - exempt from guard (d) until P2.5 package-izes it. -VENDORED_PREFIX = ("modules", "deps") - #: The patterns of guard (e). They are the ``pdb``/``torch_profiler``/ #: ``import_pip``/``pip_main`` patterns of the P0.3 scanner, so the guard and #: the scanner never drift apart. @@ -71,7 +69,14 @@ def _relative(path: Path, root: Path) -> str: def _package_files(root: Path) -> list[Path]: - """Every production python file of the package (vendored tree excluded).""" + """Every production python file of the package (the vendored tree included). + + The vendored tree used to be exempt from guard (d): it was a plain folder + on ``sys.path`` until P2.5 package-ized it. Since P2.5 it is a real part + of the package, so the plan's §3 rule ("no ``sys.path`` mutation anywhere + in the package") applies to it as well - the last vendored mutation was + removed in the P2 review fix. + """ files: list[Path] = [] for entry in PACKAGE_SOURCE_ROOTS: target = root / entry @@ -79,13 +84,7 @@ def _package_files(root: Path) -> list[Path]: files.append(target) elif target.is_dir(): files.extend(sorted(target.rglob("*.py"))) - result = [] - for path in files: - parts = path.relative_to(root).parts - if parts[: len(VENDORED_PREFIX)] == VENDORED_PREFIX: - continue - result.append(path) - return result + return files def _closure_files(root: Path) -> list[Path]: @@ -193,7 +192,7 @@ def entrypoint_violations(root: Path) -> list[str]: # guard (d) - no sys.path mutation # --------------------------------------------------------------------------- def sys_path_violations(root: Path) -> list[str]: - """No ``sys.path.insert``/``append`` outside the vendored tree.""" + """No ``sys.path.insert``/``append`` anywhere in the package.""" violations: list[str] = [] for path in _package_files(root): for number, line in enumerate(_read(path).splitlines(), 1): diff --git a/tests/test_vendored_repair.py b/tests/test_vendored_repair.py index 8e6f884..335f92e 100644 --- a/tests/test_vendored_repair.py +++ b/tests/test_vendored_repair.py @@ -11,6 +11,7 @@ Tiers: from __future__ import annotations +import ast import json import os import re @@ -114,37 +115,64 @@ def test_importing_the_vendored_utils_never_calls_pip(): ) -@pytest.mark.unit -def test_the_next_failure_after_p21_is_the_diffusers_import(): - """P2.1 (c): the eager arch scan now fails **exactly** at the diffusers import. +#: The probe of the P2.1 (c) progression. The vendored architecture is +#: imported through the **packaged** layout (P2.5): the top-level ``basicsr`` +#: of ``modules/deps`` does not exist any more, so a probe that put +#: ``modules/deps`` on ``sys.path`` would exercise a *second* copy of the +#: vendored tree instead of the shipped one. +_ARCH_PROBE = """ +import importlib, json, sys +from pathlib import Path - The plan's documented progression: B1 (``from re import T``) is gone, so - B2 (the diffusers import) is the next failure. - """ - code = ( - "import importlib, sys\n" - f"sys.path.insert(0, {str(DEPS_DIR)!r})\n" - "try:\n" - " importlib.import_module('basicsr.archs.keep_arch')\n" - " print('IMPORTED')\n" - "except BaseException as exc:\n" - " import traceback\n" - " print(type(exc).__name__)\n" - " print(str(exc)[:300])\n" - " print('TB_HAS_DIFFUSERS', 'diffusers' in traceback.format_exc())\n" - " print('TB_HAS_RE_T', \"cannot import name 'T' from 're'\" in traceback.format_exc())\n" +root = Path(sys.argv[1]) +sys.path.insert(0, str(root)) +import conftest # registers the package alias and the ComfyUI mocks + +report = {} +try: + module = importlib.import_module( + "ComfyUI_KEEP.modules.deps.basicsr.archs.keep_arch" ) - result = _run_python(code) - assert result.returncode == 0, result.stderr - out = result.stdout - if "IMPORTED" in out: - # P2.2 vendored the diffusers symbols: the arch imports cleanly now - return - # P2.1 state: the failure is exactly the diffusers import (B2) - assert "ImportError" in out, out - assert "cached_download" in out, out - assert "TB_HAS_DIFFUSERS True" in out, out - assert "TB_HAS_RE_T False" in out, out +except BaseException as exc: # noqa: BLE001 - the probe records everything + import traceback + report["error"] = type(exc).__name__ + ": " + str(exc)[:300] + report["traceback"] = traceback.format_exc() +else: + report["module"] = module.__name__ + report["feedforward"] = module.FeedForward.__module__ + report["ada_layer_norm"] = module.AdaLayerNorm.__module__ +report["basicsr_in_sys_modules"] = "basicsr" in sys.modules +print(json.dumps(report)) +""" + + +@pytest.mark.unit +def test_the_next_failure_after_p21_is_resolved_by_the_vendored_diffusers(): + """P2.1 (c): the documented next-failure progression is **complete**. + + The plan's progression: B1 (``from re import T``) is gone, so B2 (the + diffusers import) had to be the next failure. P2.2 vendored the two + diffusers classes, so the architecture imports now - the test has to + *prove* that instead of returning early (a test that returns before its + assertions verifies nothing). + """ + result = _run_python(_ARCH_PROBE, str(REPO_ROOT)) + assert result.returncode == 0, result.stdout + result.stderr + report = json.loads(result.stdout.strip().splitlines()[-1]) + assert "error" not in report, report + # the architecture imports through the package of the node + assert report["module"] == ( + "ComfyUI_KEEP.modules.deps.basicsr.archs.keep_arch" + ), report + # B2 is resolved by the vendored copy, not by a hidden fallback + assert report["feedforward"] == ( + "ComfyUI_KEEP.modules.deps.basicsr.archs._diffusers_compat" + ), report + assert report["ada_layer_norm"] == ( + "ComfyUI_KEEP.modules.deps.basicsr.archs._diffusers_compat" + ), report + # no top-level ``basicsr`` shadows an installed one (P2.5) + assert report["basicsr_in_sys_modules"] is False, report @pytest.mark.unit @@ -486,18 +514,48 @@ def test_importing_the_package_does_not_change_sys_path(): # P2.6 - requirements.txt is the true runtime set # --------------------------------------------------------------------------- -#: The third-party modules the closure imports -> their distribution name. +#: The third-party module names of the closure -> their **distribution** name. +#: Only the names whose distribution differs from the module name are listed; +#: every other module is its own distribution name (``torch`` -> ``torch``, +#: ``numpy`` -> ``numpy``, ...). +#: +#: This map is a **rename** map, not an allowlist: a module that is not in it +#: is *not* dropped, it is mapped to its own name - and therefore shows up as +#: an undeclared dependency (that was the hole of the first version of this +#: guard: it filtered the imports through this map, so every module outside +#: of it was silently ignored). _DISTRIBUTIONS = { - "torch": "torch", - "torchvision": "torchvision", - "numpy": "numpy", - "scipy": "scipy", "cv2": "opencv-python", "PIL": "Pillow", - "einops": "einops", - "tqdm": "tqdm", "yaml": "pyyaml", - "requests": "requests", +} + +#: The modules the **ComfyUI host** provides - they are not dependencies of +#: this package and therefore not part of ``requirements.txt``. +_HOST_MODULES = ("comfy", "comfy_api", "folder_paths") + +#: The vendored packages - imported relatively (P2.5), never a dependency. +_VENDORED_MODULES = ("basicsr", "facelib") + +#: The third-party modules the closure imports **lazily** (inside a +#: function). They are not runtime dependencies of the node, so they are not +#: declared - but every one of them has to be listed here with a reason, so +#: that a new lazy import cannot slip through unnoticed either. +_OPTIONAL_LAZY_IMPORTS = { + "dlib": ( + "facelib FaceRestoreHelper.init_dlib_from_paths - only reached with " + "det_model='dlib'; the node offers retinaface / YOLOv5 only" + ), + "gdown": ( + "facelib misc.download_pretrained_models - never called by the node" + ), + "mc": ( + "basicsr MemcachedBackend - only with FileClient(backend='memcached'); " + "the node reads from the disk backend" + ), + "wandb": ( + "basicsr logger.init_wandb_logger - training only (basicsr/train.py)" + ), } #: The dependency set of the pruned closure (plan P2.6 + the two packages the @@ -525,20 +583,96 @@ def _requirement_entries(): } -def _closure_imports(): - """The third-party module names imported by the runtime closure.""" - import ast +class _ImportCollector(ast.NodeVisitor): + """Collects the imported module names, split by *import time* and *lazy*. - report = _scan_report() - names = set() - for rel in report["closure_files"]: + An import at module level (or in a class body - it runs when the class + is created) is a dependency of the package: it runs on every import. + An import inside a function is **lazy**: it only runs when that function + is called, so it is not a dependency of the package itself. + """ + + def __init__(self) -> None: + self.module_level: set[str] = set() + self.lazy: set[str] = set() + self._function_depth = 0 + + # -- the scope handling ------------------------------------------------ + def _visit_defaults(self, arguments) -> None: + """The default arguments are evaluated at import time.""" + defaults = list(arguments.defaults) + defaults += [value for value in arguments.kw_defaults if value] + for default in defaults: + self.visit(default) + + def _visit_function(self, node) -> None: + # decorators and default arguments are evaluated at import time + for decorator in node.decorator_list: + self.visit(decorator) + self._visit_defaults(node.args) + # the body runs when the function is called + self._function_depth += 1 + for statement in node.body: + self.visit(statement) + self._function_depth -= 1 + + def visit_FunctionDef(self, node) -> None: + self._visit_function(node) + + def visit_AsyncFunctionDef(self, node) -> None: + self._visit_function(node) + + def visit_Lambda(self, node) -> None: + self._visit_defaults(node.args) + self._function_depth += 1 + self.visit(node.body) + self._function_depth -= 1 + + # -- the imports ------------------------------------------------------- + def visit_Import(self, node) -> None: + for alias in node.names: + self._add(alias.name.split(".")[0]) + + def visit_ImportFrom(self, node) -> None: + # a relative import (level > 0) stays inside the vendored tree + if node.level == 0 and node.module: + self._add(node.module.split(".")[0]) + + def _add(self, name: str) -> None: + if self._function_depth: + self.lazy.add(name) + else: + self.module_level.add(name) + + +def _closure_import_collector() -> _ImportCollector: + """The import collector of the whole runtime closure.""" + collector = _ImportCollector() + for rel in _scan_report()["closure_files"]: tree = ast.parse((REPO_ROOT / rel).read_text(encoding="utf-8")) - for node in ast.walk(tree): - if isinstance(node, ast.Import): - names.update(alias.name.split(".")[0] for alias in node.names) - elif isinstance(node, ast.ImportFrom) and node.level == 0 and node.module: - names.add(node.module.split(".")[0]) - return names + collector.visit(tree) + return collector + + +def _closure_imports() -> set[str]: + """Every absolute module name imported by the runtime closure.""" + collector = _closure_import_collector() + return collector.module_level | collector.lazy + + +def _closure_third_party_imports() -> tuple[set[str], set[str]]: + """The third-party imports of the closure: (at import time, lazy only). + + Excluded are the stdlib, the vendored packages (they are imported + relatively, P2.5) and the modules the ComfyUI host provides. + """ + collector = _closure_import_collector() + excluded = ( + set(sys.stdlib_module_names) | set(_VENDORED_MODULES) | set(_HOST_MODULES) + ) + module_level = collector.module_level - excluded + lazy = (collector.lazy - excluded) - module_level + return module_level, lazy @pytest.mark.unit @@ -560,15 +694,45 @@ def test_no_phantom_package_is_imported_in_the_closure(): @pytest.mark.unit def test_every_third_party_import_of_the_closure_is_declared(): - """P2.6: every third-party import of the closure is a declared dependency.""" - imported = _closure_imports() - declared = { - _DISTRIBUTIONS[name] for name in imported if name in _DISTRIBUTIONS - } + """P2.6: every third-party import of the closure is a declared dependency. + + The check is **exhaustive**: every third-party module the closure + imports at import time is mapped to a distribution name (identity, + unless ``_DISTRIBUTIONS`` says otherwise) and that distribution has to + be declared in ``requirements.txt``. A module that is not in + ``_DISTRIBUTIONS`` is therefore *not* dropped - it shows up as an + undeclared dependency. + """ + module_level, _lazy = _closure_third_party_imports() entries = _requirement_entries() + declared = {_DISTRIBUTIONS.get(name, name) for name in module_level} assert declared == entries, ( - f"the closure imports {sorted(declared)}, requirements.txt declares " - f"{sorted(entries)}" + f"the closure imports {sorted(declared)} at import time, " + f"requirements.txt declares {sorted(entries)}; " + f"undeclared: {sorted(declared - entries)}; " + f"declared but unused: {sorted(entries - declared)}" + ) + + +@pytest.mark.unit +def test_every_lazy_third_party_import_is_explicitly_allowed(): + """P2.6: a lazy third-party import is listed with a reason. + + A lazy import (inside a function) is not a dependency of the package, + so it is not declared in ``requirements.txt``. It is still tracked: + an unlisted one fails this test, so a new third-party import cannot + slip through unnoticed - neither at import time nor inside a function. + """ + _module_level, lazy = _closure_third_party_imports() + unlisted = sorted(lazy - set(_OPTIONAL_LAZY_IMPORTS)) + assert unlisted == [], ( + f"the closure imports {unlisted} lazily; either declare them in " + f"requirements.txt or list them in _OPTIONAL_LAZY_IMPORTS with a reason" + ) + stale = sorted(set(_OPTIONAL_LAZY_IMPORTS) - lazy) + assert stale == [], ( + f"_OPTIONAL_LAZY_IMPORTS lists {stale}, which the closure does not " + f"import lazily any more" ) @@ -619,6 +783,28 @@ def test_the_stub_sites_yield_the_real_vendored_symbols(): assert not hasattr(processor, "VideoWriter") +@pytest.mark.unit +def test_the_dead_video_path_references_no_deleted_symbol(): + """P2.1/P2.7: the disabled ``process_video`` references no deleted symbol. + + The vendored ``video_util`` module (with ``VideoReader`` / + ``VideoWriter``) is deleted in P2.1. The unreachable body of + ``process_video`` still called them - it is gone now, so the method is + the raise and nothing else (the AST is checked, so the docstrings that + *mention* the deleted symbols are not offenders). + """ + source = (REPO_ROOT / "modules" / "keep_processor.py").read_text( + encoding="utf-8" + ) + tree = ast.parse(source) + offenders = [ + f"{node.lineno}: {node.id}" + for node in ast.walk(tree) + if isinstance(node, ast.Name) and node.id in ("VideoReader", "VideoWriter") + ] + assert offenders == [], offenders + + @pytest.mark.unit def test_a_broken_vendored_import_fails_loudly(tmp_path): """P2.7 (b): a broken vendored dependency makes the import fail. @@ -657,3 +843,80 @@ def test_a_broken_vendored_import_fails_loudly(tmp_path): control = _run_python(_IMPORT_PACKAGE_SCRIPT, str(REPO_ROOT)) assert control.returncode == 0, control.stdout + control.stderr assert "KEEP_IMPORT_OK" in control.stdout, control.stdout + + +@pytest.mark.unit +def test_no_import_fallback_stub_remains_in_the_vendored_tree(): + """P2.7: the vendored tree has no swallow-and-stub fallback any more. + + A fallback stub is an ``except ImportError`` handler that defines a + function or a class: the import failure is swallowed and the caller + gets a local substitute instead of the vendored symbol. The two sites + that still had one (``facelib/detection`` and ``facelib/parsing``) were + removed in the P2 review fix - a broken vendored dependency has to fail + loudly, not degrade into a (broken) local stub. + """ + offenders = [] + for path in sorted(DEPS_DIR.rglob("*.py")): + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + for node in ast.walk(tree): + if not isinstance(node, ast.Try): + continue + for handler in node.handlers: + caught = handler.type + if isinstance(caught, ast.Tuple): + caught_names = { + element.id + for element in caught.elts + if isinstance(element, ast.Name) + } + elif isinstance(caught, ast.Name): + caught_names = {caught.id} + else: + caught_names = set() + if not caught_names & {"ImportError", "ModuleNotFoundError"}: + continue + for statement in handler.body: + if isinstance( + statement, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef) + ): + offenders.append( + f"{path.relative_to(REPO_ROOT).as_posix()}:" + f"{statement.lineno}: fallback stub " + f"{statement.name}" + ) + assert offenders == [], offenders + + +@pytest.mark.unit +def test_the_facelib_model_helpers_use_the_vendored_download_util(): + """P2.7: the facelib detection / parsing helpers use the vendored symbol.""" + import importlib + + for name in ("detection", "parsing"): + module = importlib.import_module(f"ComfyUI_KEEP.modules.deps.facelib.{name}") + assert module.load_file_from_url.__module__ == ( + "ComfyUI_KEEP.modules.deps.basicsr.utils.download_util" + ), module.load_file_from_url + + +@pytest.mark.unit +def test_the_vendored_tree_never_mutates_sys_path(): + """P2.5: the vendored tree must not mutate ``sys.path``. + + The plan's §3 import rule forbids a ``sys.path`` mutation **anywhere** + in the package. The last one (``basicsr/utils/file_client.py``, the + optional ``sys_path`` of the memcached backend) was removed in the P2 + review fix - the structure guard (d) now covers the vendored tree as + well. + """ + offenders = [] + for path in sorted(DEPS_DIR.rglob("*.py")): + text = path.read_text(encoding="utf-8") + for number, line in enumerate(text.splitlines(), 1): + if re.search(r"sys\.path\s*\.\s*(insert|append|extend)\s*\(", line): + offenders.append( + f"{path.relative_to(REPO_ROOT).as_posix()}:{number}: " + f"{line.strip()}" + ) + assert offenders == [], offenders