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