diff --git a/README.md b/README.md index 8584413..617919b 100644 --- a/README.md +++ b/README.md @@ -169,7 +169,7 @@ Remote deployment behavior: Remote runtime behavior: -- The deployed class reads its GPU type from `COMFY_MODAL_GPU`. +- The deployed class reads its GPU type from `COMFY_MODAL_GPU`. If a Modal app with the configured `COMFY_MODAL_APP_NAME` already exists while the cloud entrypoint is being built, Modal-Sync now fails fast and asks you to delete the existing app first instead of redeploying over hardware that may have been created with a different GPU type. - CPU memory snapshots are enabled by default. - GPU memory snapshots remain opt-in. - When GPU snapshots are enabled, Modal-Sync now derives a stable snapshot profile from root literal loader nodes and runs those loaders in `@modal.enter(snap=True)`, so later cold restarts for the same model set can restore post-loader GPU state instead of reloading those weights from scratch. diff --git a/comfyui_modal_sync_cloud.py b/comfyui_modal_sync_cloud.py index 62641e7..a89df64 100644 --- a/comfyui_modal_sync_cloud.py +++ b/comfyui_modal_sync_cloud.py @@ -14,6 +14,8 @@ import logging import math import os import queue +import shutil +import subprocess import sys import tempfile import threading @@ -107,6 +109,143 @@ class RemoteSubgraphExecutionError(RuntimeError): """Raised when remote subgraph execution fails.""" +class ExistingModalAppError(RuntimeError): + """Raised when deploying would overwrite an existing Modal app.""" + + +def _modal_exception_types( + modal_module: Any, + names: Sequence[str], +) -> tuple[type[BaseException], ...]: + """Return available Modal exception types from a Modal SDK-like module.""" + exception_namespace = getattr(modal_module, "exception", None) + if exception_namespace is None: + return tuple() + + error_types: list[type[BaseException]] = [] + for error_name in names: + error_type = getattr(exception_namespace, error_name, None) + if isinstance(error_type, type) and issubclass(error_type, BaseException): + error_types.append(error_type) + return tuple(error_types) + + +def _modal_lookup_create_flag_name(app_lookup: Callable[..., Any]) -> str | None: + """Return the App.lookup keyword that disables creation when the SDK exposes one.""" + try: + lookup_signature = inspect.signature(app_lookup) + except (TypeError, ValueError): + return None + for flag_name in ("create_if_missing", "create"): + if flag_name in lookup_signature.parameters: + return flag_name + return None + + +def _modal_app_exists_via_sdk(modal_module: Any, app_name: str) -> bool | None: + """Return whether a Modal app exists using a non-creating SDK lookup when available.""" + app_namespace = getattr(modal_module, "App", None) + app_lookup = getattr(app_namespace, "lookup", None) + if not callable(app_lookup): + return None + + create_flag_name = _modal_lookup_create_flag_name(app_lookup) + if create_flag_name is None: + return None + + not_found_errors = _modal_exception_types(modal_module, ("NotFoundError",)) + try: + app_lookup(app_name, **{create_flag_name: False}) + except not_found_errors: + return False + return True + + +def _app_name_from_modal_app_list_entry(entry: Mapping[str, Any]) -> str | None: + """Extract an app name from one `modal app list --json` entry.""" + for key in ("name", "app_name", "Name"): + value = entry.get(key) + if isinstance(value, str) and value.strip(): + return value.strip() + return None + + +def _modal_app_exists_via_cli(app_name: str) -> bool | None: + """Return whether a Modal app exists by parsing `modal app list --json` when available.""" + modal_cli = shutil.which("modal") + if modal_cli is None: + return None + + command = [modal_cli, "app", "list", "--json"] + try: + completed = subprocess.run( + command, + check=False, + capture_output=True, + text=True, + timeout=30, + ) + except (OSError, subprocess.SubprocessError) as exc: + logger.warning("Unable to list Modal apps through the Modal CLI: %s", exc) + return None + if completed.returncode != 0: + logger.warning( + "Modal CLI app list failed with exit code %s: %s", + completed.returncode, + completed.stderr.strip(), + ) + return None + + try: + listed_apps = json.loads(completed.stdout) + except json.JSONDecodeError as exc: + logger.warning("Modal CLI app list returned invalid JSON: %s", exc) + return None + if not isinstance(listed_apps, list): + return None + + return any( + _app_name_from_modal_app_list_entry(entry) == app_name + for entry in listed_apps + if isinstance(entry, Mapping) + ) + + +def _running_inside_modal_container(modal_module: Any) -> bool: + """Return whether this import is happening inside a Modal worker container.""" + if os.getenv("MODAL_TASK_ID"): + return True + is_local = getattr(modal_module, "is_local", None) + if callable(is_local): + try: + return not bool(is_local()) + except RuntimeError: + return False + return False + + +def _existing_modal_app_error_message(app_name: str) -> str: + """Return the user-facing error for refusing to overwrite a Modal app.""" + return ( + f"Modal app {app_name!r} already exists. Delete the existing app before launching " + "ComfyUI-Modal with this configuration, especially after changing COMFY_MODAL_GPU, " + f"for example: modal app stop {app_name!r}." + ) + + +def _guard_against_existing_modal_app(settings: Any, modal_module: Any) -> None: + """Fail local Modal app construction when the configured app already exists.""" + if _running_inside_modal_container(modal_module): + return + + app_name = str(settings.app_name) + app_exists = _modal_app_exists_via_sdk(modal_module, app_name) + if app_exists is None: + app_exists = _modal_app_exists_via_cli(app_name) + if app_exists: + raise ExistingModalAppError(_existing_modal_app_error_message(app_name)) + + def _payload_remote_session_handle(payload: dict[str, Any]) -> RemoteSessionHandle | None: """Return the decoded prompt-scoped remote session handle for one payload.""" remote_session = payload.get("remote_session") @@ -4409,6 +4548,7 @@ def _execute_loader_prewarm_plans( if modal is not None: # pragma: no branch - remote entrypoint configuration. settings = get_settings() + _guard_against_existing_modal_app(settings, modal) app = modal.App(settings.app_name) vol = modal.Volume.from_name(settings.volume_name, create_if_missing=True) interrupt_flags = modal.Dict.from_name( diff --git a/docs/LEARNINGS.md b/docs/LEARNINGS.md index 8bf41e3..aa1677f 100644 --- a/docs/LEARNINGS.md +++ b/docs/LEARNINGS.md @@ -219,3 +219,4 @@ - Remote node UI needs two separate non-executing signals to stay legible under Modal fan-out: "ready but not in the active component" and "reused from the persisted node cache." The former should be derived locally from component membership and `active_node_id`, while the latter needs an explicit backend event because cached nodes often never emit ordinary execution progress at all. - Once mapped fan-out is allowed to return before every spare lane finishes seeding, late failures from those detached lanes must not flow back through the main prompt future. They are no longer part of the critical path, so the scheduler has to ignore them after all outputs are already complete. - Combined first-run and second-run performance logs are now clear enough to prioritize work. In this workflow, uploads and volume reload were already cheap, while the recurring non-sampling bottleneck was static-phase model loading (`UNETLoader` + `CLIPLoader` at roughly `24-25s` total per run). The second run also showed that local proxy caching can skip the static proxy dispatch while still leaving a large replay cost inside the mapped phase, which makes two no-brainer optimizations obvious: reuse heavy loader outputs on the same worker, and avoid replaying the whole static producer branch just to reconstruct session-backed bridge values. +- Modal deployment updates are too permissive for GPU-type changes. Because a named deployed app can be updated in place while its resource expectations and synced storage assumptions may have come from an older `COMFY_MODAL_GPU`, the cloud entrypoint should fail local app construction when that app name already exists and require the operator to stop/delete the old app before building the new one. diff --git a/tests/test_modal_executor_node.py b/tests/test_modal_executor_node.py index 48f7f68..d7ddadc 100644 --- a/tests/test_modal_executor_node.py +++ b/tests/test_modal_executor_node.py @@ -7,6 +7,7 @@ import copy from concurrent.futures import Future import importlib.util import json +import subprocess import sys import threading import time @@ -2350,6 +2351,134 @@ def test_modal_cloud_pins_cu128_pytorch_stack( assert modal_cloud_module._PYTORCH_CUDA_INDEX_URL == "https://download.pytorch.org/whl/cu128" +def test_modal_cloud_existing_app_guard_uses_non_creating_sdk_lookup( + modal_cloud_module: Any, +) -> None: + """Deploy-time app construction should fail if the configured Modal app already exists.""" + + class FakeNotFoundError(Exception): + """Stand-in for Modal app lookup misses.""" + + lookup_calls: list[tuple[str, bool]] = [] + + class FakeApp: + """Minimal Modal App namespace with non-creating lookup support.""" + + @staticmethod + def lookup(app_name: str, create_if_missing: bool = True) -> object: + """Record lookup arguments and return an existing app.""" + lookup_calls.append((app_name, create_if_missing)) + return object() + + fake_modal = types.SimpleNamespace( + App=FakeApp, + exception=types.SimpleNamespace(NotFoundError=FakeNotFoundError), + is_local=lambda: True, + ) + settings = types.SimpleNamespace(app_name="comfy-modal-sync") + + with pytest.raises(modal_cloud_module.ExistingModalAppError) as exc_info: + modal_cloud_module._guard_against_existing_modal_app(settings, fake_modal) + + assert lookup_calls == [("comfy-modal-sync", False)] + assert "Delete the existing app" in str(exc_info.value) + assert "COMFY_MODAL_GPU" in str(exc_info.value) + + +def test_modal_cloud_existing_app_guard_allows_missing_sdk_lookup( + modal_cloud_module: Any, +) -> None: + """A missing Modal app should not block first-run app construction.""" + + class FakeNotFoundError(Exception): + """Stand-in for Modal app lookup misses.""" + + class FakeApp: + """Minimal Modal App namespace that reports a missing app.""" + + @staticmethod + def lookup(app_name: str, create_if_missing: bool = True) -> object: + """Raise the SDK's not-found error without creating the app.""" + del app_name, create_if_missing + raise FakeNotFoundError("app not found") + + fake_modal = types.SimpleNamespace( + App=FakeApp, + exception=types.SimpleNamespace(NotFoundError=FakeNotFoundError), + is_local=lambda: True, + ) + + modal_cloud_module._guard_against_existing_modal_app( + types.SimpleNamespace(app_name="comfy-modal-sync"), + fake_modal, + ) + + +def test_modal_cloud_existing_app_guard_falls_back_to_cli_json( + modal_cloud_module: Any, + monkeypatch: Any, +) -> None: + """SDKs without a non-creating lookup should fall back to `modal app list --json`.""" + + class FakeApp: + """Modal App namespace whose lookup signature cannot safely check existence.""" + + @staticmethod + def lookup(app_name: str) -> object: + """This must not be called because it may create the app.""" + raise AssertionError(f"unsafe lookup called for {app_name}") + + completed = subprocess.CompletedProcess( + args=["modal", "app", "list", "--json"], + returncode=0, + stdout=json.dumps([{"name": "other"}, {"name": "comfy-modal-sync"}]), + stderr="", + ) + observed_commands: list[list[str]] = [] + + def fake_run(command: list[str], **kwargs: Any) -> subprocess.CompletedProcess[str]: + """Return a deterministic Modal CLI app list response.""" + del kwargs + observed_commands.append(command) + return completed + + monkeypatch.setattr(modal_cloud_module.shutil, "which", lambda name: f"/usr/bin/{name}") + monkeypatch.setattr(modal_cloud_module.subprocess, "run", fake_run) + fake_modal = types.SimpleNamespace(App=FakeApp, exception=types.SimpleNamespace(), is_local=lambda: True) + + with pytest.raises(modal_cloud_module.ExistingModalAppError): + modal_cloud_module._guard_against_existing_modal_app( + types.SimpleNamespace(app_name="comfy-modal-sync"), + fake_modal, + ) + + assert observed_commands == [["/usr/bin/modal", "app", "list", "--json"]] + + +def test_modal_cloud_existing_app_guard_skips_remote_container_import( + modal_cloud_module: Any, + monkeypatch: Any, +) -> None: + """The guard should not run inside Modal worker containers that import the same module.""" + + class FakeApp: + """Modal App namespace that would fail if the guard tried a lookup.""" + + @staticmethod + def lookup(app_name: str, create_if_missing: bool = True) -> object: + """Raise if remote-container imports accidentally perform local deploy checks.""" + del app_name, create_if_missing + raise AssertionError("remote container import should not query Modal apps") + + monkeypatch.setenv("MODAL_TASK_ID", "ta-123") + fake_modal = types.SimpleNamespace(App=FakeApp, exception=types.SimpleNamespace(), is_local=lambda: False) + + modal_cloud_module._guard_against_existing_modal_app( + types.SimpleNamespace(app_name="comfy-modal-sync"), + fake_modal, + ) + + def test_modal_cloud_builds_snapshot_enabled_cls_options( modal_cloud_module: Any, ) -> None: