From f97fa2b3300bb0f225c73ff3dca0fbf672c6025d Mon Sep 17 00:00:00 2001 From: Ken Simpson Date: Sun, 19 Apr 2026 10:30:49 -0700 Subject: [PATCH] fix: recover from stale cloud module imports --- README.md | 1 + docs/LEARNINGS.md | 1 + remote/modal_app.py | 17 +++++-- tests/test_modal_executor_node.py | 81 +++++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 1d8989f..126da98 100644 --- a/README.md +++ b/README.md @@ -78,6 +78,7 @@ Remote mode requires: - `COMFY_MODAL_EXECUTION_MODE` set to something other than `local` The normal remote path is deployed-app lookup by name. On first remote use, the extension can auto-deploy the stable cloud entrypoint from [`comfyui_modal_sync_cloud.py`](comfyui_modal_sync_cloud.py) if the configured app does not exist yet. +If a previous attempt imported that cloud entrypoint only partially before failing, the loader now discards the stale cached module and retries the import cleanly before auto-deploy. ```bash export COMFY_MODAL_EXECUTION_MODE=remote diff --git a/docs/LEARNINGS.md b/docs/LEARNINGS.md index 46fd828..368154e 100644 --- a/docs/LEARNINGS.md +++ b/docs/LEARNINGS.md @@ -36,6 +36,7 @@ - Modal objects defined in-process are not directly callable from an arbitrary long-running app like ComfyUI unless they are inside a hydrated `app.run()` session or looked up from a deployed app with `modal.Cls.from_name(...)`. In practice the correct default for this project is deployed lookup only, because ephemeral `app.run()` fallback rebuilds too much state between workflow executions and undermines the scale-to-zero cost model. - A deployed-only strategy is still correct, but the custom node should bootstrap that deployment itself on first use. The pragmatic flow is: try `Cls.from_name(...)`, and if the app is missing, programmatically call `app.deploy(...)` from the stable cloud entry module, then retry the lookup. - ComfyUI loads custom node packs from directories under synthetic module names derived from filesystem paths, and for directory-based packs that module key is not guaranteed to be a valid Python import path. Modal container startup requires a real importable module name for `@app.cls` services, so the cloud entrypoint has to live in a separate, stable module such as `comfyui_modal_sync_cloud.py` rather than on the custom-node package module itself. +- When the local side imports that stable cloud entrypoint for first-run auto-deploy, a failed import can poison `sys.modules` with a half-initialized module. The loader has to evict cached modules that do not expose a real deployable `app`, and it has to remove failed imports from `sys.modules`, or later auto-deploy attempts will keep reusing the broken stub. - Shipping the ComfyUI source tree into the Modal image is not enough by itself. The container also needs ComfyUI's import-time Python dependencies such as `psutil`, `aiohttp`, `einops`, `torchsde`, `torchvision`, and related runtime packages, or the remote worker fails before it can execute any graph nodes. - Core ComfyUI imports extend beyond the obvious tensor stack. In practice the Modal image also needs startup/runtime packages such as `alembic`, `pydantic-settings`, `spandrel`, and `kornia`, because built-in nodes and config-parsing paths import them before a workflow gets very far. - Quantized ComfyUI models add another hard dependency: if `comfy-kitchen` is missing in the Modal image, `comfy.quant_ops.get_layout_class(...)` falls back to `None` and FP8/FP4 model loading eventually fails with `'NoneType' object has no attribute 'Params'`. diff --git a/remote/modal_app.py b/remote/modal_app.py index 45e286b..de964c8 100644 --- a/remote/modal_app.py +++ b/remote/modal_app.py @@ -477,8 +477,15 @@ def _modal_lookup_error_types() -> tuple[type[BaseException], ...]: def _load_modal_cloud_module() -> Any: """Load the stable Modal cloud entry module under a valid Python name.""" - if _MODAL_CLOUD_MODULE_NAME in sys.modules: - return sys.modules[_MODAL_CLOUD_MODULE_NAME] + existing_module = sys.modules.get(_MODAL_CLOUD_MODULE_NAME) + if existing_module is not None and getattr(existing_module, "app", None) is not None: + return existing_module + if existing_module is not None: + logger.warning( + "Discarding partially initialized Modal cloud module %s before reload.", + _MODAL_CLOUD_MODULE_NAME, + ) + sys.modules.pop(_MODAL_CLOUD_MODULE_NAME, None) cloud_module_path = Path(__file__).resolve().parents[1] / f"{_MODAL_CLOUD_MODULE_NAME}.py" module_spec = importlib.util.spec_from_file_location( @@ -492,7 +499,11 @@ def _load_modal_cloud_module() -> Any: cloud_module = importlib.util.module_from_spec(module_spec) sys.modules[_MODAL_CLOUD_MODULE_NAME] = cloud_module - module_spec.loader.exec_module(cloud_module) + try: + module_spec.loader.exec_module(cloud_module) + except BaseException: + sys.modules.pop(_MODAL_CLOUD_MODULE_NAME, None) + raise return cloud_module diff --git a/tests/test_modal_executor_node.py b/tests/test_modal_executor_node.py index cd70f86..ca63d1b 100644 --- a/tests/test_modal_executor_node.py +++ b/tests/test_modal_executor_node.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import importlib.util import sys import threading import time @@ -948,6 +949,86 @@ def test_remote_modal_auto_deploys_missing_app_by_default( assert deploy_calls == [("comfy-modal-sync", None)] +def test_load_modal_cloud_module_reloads_stale_partial_module( + remote_modal_app_module: Any, + monkeypatch: Any, +) -> None: + """Stale partially imported cloud modules should be discarded and reloaded.""" + original_module = sys.modules.get(remote_modal_app_module._MODAL_CLOUD_MODULE_NAME) + stale_module = types.SimpleNamespace(app=None) + sys.modules[remote_modal_app_module._MODAL_CLOUD_MODULE_NAME] = stale_module + + loaded_module = types.SimpleNamespace(app="fresh-app") + + class FakeLoader: + """Populate the fresh replacement module during exec.""" + + def create_module(self, spec: Any) -> None: + """Use the default module creation path.""" + del spec + return None + + def exec_module(self, module: Any) -> None: + """Install the expected deployable app onto the reloaded module.""" + module.app = loaded_module.app + + monkeypatch.setattr( + remote_modal_app_module.importlib.util, + "spec_from_file_location", + lambda *args, **kwargs: importlib.util.spec_from_loader( + remote_modal_app_module._MODAL_CLOUD_MODULE_NAME, + FakeLoader(), + ), + ) + try: + reloaded_module = remote_modal_app_module._load_modal_cloud_module() + finally: + sys.modules.pop(remote_modal_app_module._MODAL_CLOUD_MODULE_NAME, None) + if original_module is not None: + sys.modules[remote_modal_app_module._MODAL_CLOUD_MODULE_NAME] = original_module + + assert reloaded_module is not stale_module + assert getattr(reloaded_module, "app", None) == "fresh-app" + + +def test_load_modal_cloud_module_clears_failed_import_from_sys_modules( + remote_modal_app_module: Any, + monkeypatch: Any, +) -> None: + """Failed cloud-module imports should not leave a poisoned cache entry behind.""" + original_module = sys.modules.get(remote_modal_app_module._MODAL_CLOUD_MODULE_NAME) + + class FakeLoader: + """Raise during module execution to simulate a partial import failure.""" + + def create_module(self, spec: Any) -> None: + """Use the default module creation path.""" + del spec + return None + + def exec_module(self, module: Any) -> None: + """Fail while the module is being initialized.""" + module.app = None + raise RuntimeError("boom") + + monkeypatch.setattr( + remote_modal_app_module.importlib.util, + "spec_from_file_location", + lambda *args, **kwargs: importlib.util.spec_from_loader( + remote_modal_app_module._MODAL_CLOUD_MODULE_NAME, + FakeLoader(), + ), + ) + try: + with pytest.raises(RuntimeError, match="boom"): + remote_modal_app_module._load_modal_cloud_module() + assert remote_modal_app_module._MODAL_CLOUD_MODULE_NAME not in sys.modules + finally: + sys.modules.pop(remote_modal_app_module._MODAL_CLOUD_MODULE_NAME, None) + if original_module is not None: + sys.modules[remote_modal_app_module._MODAL_CLOUD_MODULE_NAME] = original_module + + def test_remote_modal_consumes_streamed_progress_and_result( remote_modal_app_module: Any, monkeypatch: Any,