Five review findings of the P3 phase, each fixed at its root:
1. (medium) The P3.7 metadata test imported `tomllib` at module level: the
**3.10 CI job** (the declared `requires-python >=3.10` minimum) died
during collection, so the registry-metadata assertions never ran on 3.10.
The parser import is now guarded (`tomllib` -> `tomli`, the package the
3.10 job installs); without any parser the module *skips* instead of
dying. `pyproject.toml` gained the test extra
`tomli>=2.0; python_version < '3.11'`.
Proof: a 3.10 simulation (the stdlib `tomllib` refused for the test
module, `tomli` on sys.path) -> 6 passed; the control without `tomli`
-> 6 skipped (no collection error).
2. (medium) The P3.5 "primary-root-first multi-root" resolution was not
wired into the production load path: `resolve_model_file` (the only
`get_full_path_or_raise` caller) was used by tests only, so a weight in
a secondary model root was downloaded into the primary root again.
`src/keep_paths.py` now has `find_model_file()` (the multi-root lookup
through `folder_paths.get_full_path_or_raise`) and `resolve_model_root()`
(the root the weights really live in); `load_file_from_url_comfy()` - the
production path of `src/keep_model_loader.py:107` - uses the lookup, and
the facelib root follows the weights (the facelib helper builds its
paths from a single root). New tests prove that the production path
uses a weight of an extra model root and that the primary root wins.
The `models_dir` grep guard is no longer a blanket exemption: only the
registration in `src/keep_paths.py` may use the attribute.
3. (low) `[tool.comfy].Icon` was an empty string and the test only checked
that the key exists. The preview asset **is** committed
(`git cat-file -s HEAD:assets/keep-preview.png` -> 93279 bytes); only the
working copy was missing (an uncommitted local deletion, restored from
HEAD - no asset change is in this commit). The key now points at
`assets/keep-preview.png` and the test asserts that the value is not
empty and names a file that is really in the repository.
4. (low) `tests/test_package_entrypoint.py` hard-coded the V1 state and
would have gone red (not xfail) when P4.4 flips the root `__init__.py`
to `comfy_entrypoint`. The assertions now follow the entry point that is
really there: exactly **one** mapping style (V1 XOR V3). The mocked
smoke test of `tests/test_package_import.py` got the same treatment; the
headless variant is P4.4's job (its test list names it).
5. (low) The plan's §3 rule ("`comfy.*` imports inside `src/` are lazy
(function-level) so the pack imports without a ComfyUI server") was
violated and unguarded. The `comfy` imports of `src/keep_model_loader.py`
and `src/keep_processor.py` are function-level now; the vendored facelib
helper (which imports `comfy` itself) is imported lazily through a module
`__getattr__` (PEP 562), so `loader.FaceRestoreHelper` still resolves to
the vendored class. Structure **guard (f)** (`tests/test_structure.py`)
forbids a module-level `comfy`/`comfy_api` import in `src/**` (with the
self-test the guards file demands), and the new
`tests/test_lazy_imports.py` proves at runtime that the engines import
while every `comfy` import is blocked.
Gate after the commit:
python.exe -m pytest -q tests -m "unit or mock_integration"
-> 153 passed, 1 skipped, 5 deselected, 3 xfailed
COMFYUI_ROOT=C:/_Dev/ComfyUI_dev/ComfyUI-source/ComfyUI python.exe
-m pytest -q tests -m "unit or mock_integration or headless"
-> 158 passed, 1 skipped, 3 xfailed
Details: docs/2026-10-23-p3-review-findings.md
149 lines
5.1 KiB
Python
149 lines
5.1 KiB
Python
"""P3.4 - the package entry point and the logger.
|
|
|
|
The root ``__init__.py`` is the whole public surface of the pack while it
|
|
registers as **V1** (P4.4 switches it to the V3 ``comfy_entrypoint``):
|
|
|
|
* it sets up the logger (now ``src/keep_logger.py``),
|
|
* it exports the node mappings.
|
|
|
|
The tests assert the entry point is *boring* (one mapping style, no side
|
|
effects beyond the logger) and that the P1.2 logger constraint is **lifted**:
|
|
the engines import without the package entry point being executed.
|
|
|
|
The assertions do **not** hard-code the phase: they hold for the V1 entry
|
|
point of P3 and for the V3 entry point of P4.4 (the strict "V3 only" check
|
|
is structure guard (c), which P4.4 flips).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import importlib
|
|
import subprocess
|
|
import sys
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
REPO_ROOT = Path(__file__).resolve().parent.parent
|
|
INIT_FILE = REPO_ROOT / "__init__.py"
|
|
|
|
PACK_NODE_IDS = [
|
|
"KEEP_FaceUpscaleImage",
|
|
"KEEP_ModelLoader",
|
|
"KEEP_ProcessImageSequence",
|
|
]
|
|
|
|
#: The probe: the package is registered under an alias **without** executing
|
|
#: the root ``__init__.py`` and **without** the placeholder logger of the test
|
|
#: harness. The engines must import all the same - that is the lifted
|
|
#: constraint.
|
|
_NO_INIT_PROBE = """
|
|
import importlib.util, sys
|
|
|
|
root = sys.argv[1]
|
|
sys.path.insert(0, root)
|
|
import conftest # the ComfyUI mocks
|
|
|
|
alias = "ComfyUI_KEEP_NO_INIT"
|
|
spec = importlib.util.spec_from_file_location(
|
|
alias, root + "/__init__.py", submodule_search_locations=[root]
|
|
)
|
|
module = importlib.util.module_from_spec(spec)
|
|
sys.modules[alias] = module
|
|
# the alias has **no** logger attribute - the engines must not need it
|
|
|
|
importlib.import_module(alias + ".src.keep_model_loader")
|
|
importlib.import_module(alias + ".src.keep_processor")
|
|
importlib.import_module(alias + ".src.keep_model_configs")
|
|
importlib.import_module(alias + ".nodes")
|
|
print("KEEP_CONSTRAINT_LIFTED True")
|
|
"""
|
|
|
|
|
|
@pytest.mark.mock_integration
|
|
def test_the_package_imports_under_the_mocks():
|
|
"""The entry point executes and exports the node mappings.
|
|
|
|
The pack registers as **V1** until P4.4 and as **V3**
|
|
(``comfy_entrypoint``) afterwards - the test follows the entry point
|
|
that is really there.
|
|
"""
|
|
conftest = importlib.import_module("conftest")
|
|
package = conftest.execute_package_init()
|
|
if hasattr(package, "comfy_entrypoint"):
|
|
# V3 (P4.4): the entry point is the only registration surface
|
|
assert callable(package.comfy_entrypoint)
|
|
return
|
|
assert sorted(package.NODE_CLASS_MAPPINGS) == PACK_NODE_IDS
|
|
assert package.NODE_DISPLAY_NAME_MAPPINGS["KEEP_FaceUpscaleImage"] == (
|
|
"KEEP Single Image"
|
|
)
|
|
|
|
|
|
@pytest.mark.mock_integration
|
|
def test_the_logger_has_exactly_one_handler():
|
|
"""The logger of the package owns exactly one handler."""
|
|
conftest = importlib.import_module("conftest")
|
|
package = conftest.execute_package_init()
|
|
logger_module = importlib.import_module("ComfyUI_KEEP.src.keep_logger")
|
|
assert package.logger is logger_module.logger
|
|
assert len(package.logger.handlers) == 1, package.logger.handlers
|
|
assert package.logger.level == 20 # INFO
|
|
|
|
|
|
@pytest.mark.mock_integration
|
|
def test_the_entry_point_defines_exactly_one_mapping_style():
|
|
"""Either the V1 mappings or the V3 entry point - never both.
|
|
|
|
The test does **not** hard-code the phase: P4.4 flips the root
|
|
``__init__.py`` to the V3 ``comfy_entrypoint`` and removes the V1
|
|
exports, and this test has to stay green on both sides of that flip.
|
|
The strict "V3 only" check is structure guard (c) - P4.4 flips it.
|
|
"""
|
|
import ast
|
|
|
|
tree = ast.parse(INIT_FILE.read_text(encoding="utf-8"), filename=str(INIT_FILE))
|
|
defines_entrypoint = any(
|
|
isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef))
|
|
and node.name == "comfy_entrypoint"
|
|
for node in tree.body
|
|
)
|
|
imported = {
|
|
alias.name
|
|
for node in tree.body
|
|
if isinstance(node, ast.ImportFrom)
|
|
for alias in node.names
|
|
}
|
|
assigned = {
|
|
target.id
|
|
for node in tree.body
|
|
if isinstance(node, ast.Assign)
|
|
for target in node.targets
|
|
if isinstance(target, ast.Name)
|
|
}
|
|
v1_names = {"NODE_CLASS_MAPPINGS", "NODE_DISPLAY_NAME_MAPPINGS"}
|
|
defines_v1 = bool((imported | assigned) & v1_names)
|
|
|
|
assert defines_v1 != defines_entrypoint, (
|
|
"the entry point must define exactly one mapping style - the V1 "
|
|
f"mappings (defined={defines_v1}) or the V3 entry point "
|
|
f"(defined={defines_entrypoint}), never both and never neither"
|
|
)
|
|
|
|
|
|
@pytest.mark.unit
|
|
def test_the_engines_import_without_the_package_entry_point():
|
|
"""P3.4: the ``from .. import logger`` constraint is lifted."""
|
|
result = subprocess.run(
|
|
[sys.executable, "-c", _NO_INIT_PROBE, str(REPO_ROOT)],
|
|
cwd=str(REPO_ROOT),
|
|
capture_output=True,
|
|
text=True,
|
|
timeout=600,
|
|
)
|
|
assert result.returncode == 0, (
|
|
f"the engines need the package entry point\nstdout:\n{result.stdout}\n"
|
|
f"stderr:\n{result.stderr}"
|
|
)
|
|
assert "KEEP_CONSTRAINT_LIFTED True" in result.stdout, result.stdout
|