fix: recover from stale cloud module imports
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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'`.
|
||||
|
||||
+14
-3
@@ -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
|
||||
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user