fix: stop stale Modal apps noninteractively
This commit is contained in:
Ken Simpson
@@ -56,7 +56,7 @@ Each ComfyUI environment receives its own Modal app name on first startup. Modal
|
||||
|
||||
For repository development, `uv sync --extra remote --group test` installs the same pinned SDK. Remote mode uses the stable cloud entrypoint in [`comfyui_modal_sync_cloud.py`](comfyui_modal_sync_cloud.py). On first use, Modal-Sync can auto-deploy the configured Modal app if it does not exist.
|
||||
|
||||
The deployed image uses Python 3.11 plus an exact ComfyUI support and CUDA package set, including the ComfyUI-pinned `comfy-aimdo` and `comfy-kitchen` releases required by current memory-management and quantization imports. Remote prompt executors mirror both legacy scalar and current active/inactive RAM-pressure cache arguments from ComfyUI. The image's local build context is limited to the ComfyUI source packages, top-level Python modules, and runtime configuration needed by the headless worker; model directories, custom nodes, caches, tests, virtual environments, user data, and unknown top-level directories stay out of the image snapshot. Before every process's first remote invocation, Modal-Sync compares the deployed worker's runtime fingerprint with the local source, ComfyUI source, custom-node requirements, and runtime-shaping settings. A missing or mismatched fingerprint is treated as stale and replaced automatically when `COMFY_MODAL_AUTO_DEPLOY=true`.
|
||||
The deployed image uses Python 3.11 plus an exact ComfyUI support and CUDA package set, including the ComfyUI-pinned `comfy-aimdo` and `comfy-kitchen` releases required by current memory-management and quantization imports. Remote prompt executors mirror both legacy scalar and current active/inactive RAM-pressure cache arguments from ComfyUI. The image's local build context is limited to the ComfyUI source packages, top-level Python modules, and runtime configuration needed by the headless worker; model directories, custom nodes, caches, tests, virtual environments, user data, and unknown top-level directories stay out of the image snapshot. Before every process's first remote invocation, Modal-Sync compares the deployed worker's runtime fingerprint with the local source, ComfyUI source, custom-node requirements, and runtime-shaping settings. A missing or mismatched fingerprint is treated as stale and replaced automatically when `COMFY_MODAL_AUTO_DEPLOY=true`; replacement uses Modal's non-interactive SDK stop API when available and an explicitly confirmed CLI fallback, so app shutdown cannot stall on a hidden terminal prompt.
|
||||
|
||||
## Using It In ComfyUI
|
||||
|
||||
|
||||
@@ -308,3 +308,4 @@
|
||||
- A fixed default Modal app name makes independent ComfyUI installations collide inside one Modal workspace and also shares every app-derived Dict namespace. Persist 64 random bits in the effective ComfyUI user directory, publish the fully written identity atomically so concurrent first starts agree, encode it with unpadded URL-safe Base64 for the app suffix, preserve explicit name overrides, and inject the resolved app name into the remote image so workers never generate a second identity.
|
||||
- Current ComfyUI imports `comfy_aimdo` unconditionally from its core memory-management modules, so omitting it from the Modal image prevents the worker from reaching node initialization. Keep both `comfy-aimdo` and `comfy-kitchen` aligned with the exact pins in the supported ComfyUI checkout; adding only the newly missing package can leave a stale kitchen ABI or API behind.
|
||||
- ComfyUI changed `args.cache_ram` from a scalar opt-in threshold to a list of zero, one, or two active/inactive thresholds and made RAM-pressure caching the default. Remote `PromptExecutor` construction must distinguish the legacy scalar and current list shapes, reproduce ComfyUI's system-RAM-derived defaults, and pass `ram_inactive`; comparing the list directly with zero fails only after a healthy worker reaches graph execution.
|
||||
- Modal SDK 1.4 exposes deployed-app stopping through `modal.experimental.stop_app`, not `App.stop`. Falling back to `modal app stop` without `--yes` blocks on an invisible confirmation prompt when ComfyUI has captured the subprocess streams, making the local 120-second subprocess guard look like an app-build timeout. Prefer the SDK API with the active Modal environment, pass `--yes` and `--env` to the CLI fallback, and translate a real CLI timeout into an intentional stop failure.
|
||||
|
||||
+40
-9
@@ -75,6 +75,7 @@ _REMOTE_APP_PROTOCOL_VERSION = REMOTE_APP_PROTOCOL_VERSION
|
||||
_MODAL_AUTO_DEPLOY_LOCK = threading.Lock()
|
||||
_MODAL_AUTO_DEPLOY_STATES: dict[tuple[str, str | None], "_ModalAutoDeployState"] = {}
|
||||
_MODAL_REMOTE_APP_VERSION_OK: set[tuple[str, str | None, str]] = set()
|
||||
_MODAL_APP_STOP_TIMEOUT_SECONDS = 120.0
|
||||
_MODAL_INTERRUPT_DICTS_LOCK = threading.Lock()
|
||||
_MODAL_INTERRUPT_DICTS: dict[tuple[str, str | None], Any] = {}
|
||||
_MAPPED_PROGRESS_NODE_IDS_LOCK = threading.Lock()
|
||||
@@ -4889,6 +4890,24 @@ def _stop_modal_app_via_sdk(app_name: str) -> bool:
|
||||
"""Try to stop a Modal app through the SDK if this SDK version exposes app stopping."""
|
||||
if modal is None:
|
||||
return False
|
||||
try:
|
||||
experimental_namespace = importlib.import_module("modal.experimental")
|
||||
except ModuleNotFoundError as exc:
|
||||
if exc.name not in {"modal", "modal.experimental"}:
|
||||
raise
|
||||
else:
|
||||
stop_app = getattr(experimental_namespace, "stop_app", None)
|
||||
if callable(stop_app):
|
||||
try:
|
||||
_call_modal_method(
|
||||
stop_app,
|
||||
app_name,
|
||||
environment_name=_modal_environment_name(),
|
||||
)
|
||||
except _modal_lookup_error_types():
|
||||
return True
|
||||
return True
|
||||
|
||||
app_namespace = getattr(modal, "App", None)
|
||||
app_lookup = getattr(app_namespace, "lookup", None)
|
||||
if not callable(app_lookup):
|
||||
@@ -4915,18 +4934,30 @@ def _stop_modal_app_via_sdk(app_name: str) -> bool:
|
||||
|
||||
|
||||
def _stop_modal_app_via_cli(app_name: str) -> bool:
|
||||
"""Try to stop a Modal app through the Modal CLI."""
|
||||
"""Try to stop a Modal app non-interactively through the Modal CLI."""
|
||||
modal_cli = shutil.which("modal")
|
||||
if modal_cli is None:
|
||||
return False
|
||||
command = [modal_cli, "app", "stop", app_name]
|
||||
completed = subprocess.run(
|
||||
command,
|
||||
check=False,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=120,
|
||||
)
|
||||
command = [modal_cli, "app", "stop", app_name, "--yes"]
|
||||
environment_name = _modal_environment_name()
|
||||
if environment_name is not None:
|
||||
command.extend(("--env", environment_name))
|
||||
try:
|
||||
completed = subprocess.run(
|
||||
command,
|
||||
check=False,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=_MODAL_APP_STOP_TIMEOUT_SECONDS,
|
||||
)
|
||||
except subprocess.TimeoutExpired:
|
||||
logger.warning(
|
||||
"Modal CLI app stop timed out for app=%s environment=%s after %.1fs.",
|
||||
app_name,
|
||||
environment_name or "<default>",
|
||||
_MODAL_APP_STOP_TIMEOUT_SECONDS,
|
||||
)
|
||||
return False
|
||||
if completed.returncode != 0:
|
||||
logger.warning(
|
||||
"Modal CLI app stop failed for app=%s exit_code=%s stderr=%s",
|
||||
|
||||
@@ -4761,6 +4761,112 @@ def test_remote_modal_replaces_out_of_date_deployed_app(
|
||||
assert deploy_calls == [("comfy-modal-sync", "main")]
|
||||
|
||||
|
||||
def test_remote_modal_stops_app_through_experimental_sdk(
|
||||
remote_modal_app_module: Any,
|
||||
monkeypatch: Any,
|
||||
) -> None:
|
||||
"""Modal 1.4 should use its non-interactive experimental stop API."""
|
||||
stop_calls: list[tuple[str, str | None]] = []
|
||||
|
||||
def fake_stop_app(app_name: str, *, environment_name: str | None = None) -> None:
|
||||
"""Record one SDK stop request."""
|
||||
stop_calls.append((app_name, environment_name))
|
||||
|
||||
fake_experimental_namespace = types.SimpleNamespace(stop_app=fake_stop_app)
|
||||
original_import_module = remote_modal_app_module.importlib.import_module
|
||||
|
||||
def fake_import_module(module_name: str) -> Any:
|
||||
"""Return the Modal experimental namespace and delegate other imports."""
|
||||
if module_name == "modal.experimental":
|
||||
return fake_experimental_namespace
|
||||
return original_import_module(module_name)
|
||||
|
||||
monkeypatch.setattr(
|
||||
remote_modal_app_module,
|
||||
"modal",
|
||||
types.SimpleNamespace(exception=types.SimpleNamespace()),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
remote_modal_app_module.importlib,
|
||||
"import_module",
|
||||
fake_import_module,
|
||||
)
|
||||
monkeypatch.setenv("MODAL_ENVIRONMENT", "main")
|
||||
|
||||
stopped = remote_modal_app_module._stop_modal_app_via_sdk("comfy-modal-sync-instance")
|
||||
|
||||
assert stopped is True
|
||||
assert stop_calls == [("comfy-modal-sync-instance", "main")]
|
||||
|
||||
|
||||
def test_remote_modal_cli_stop_is_noninteractive_and_environment_scoped(
|
||||
remote_modal_app_module: Any,
|
||||
monkeypatch: Any,
|
||||
) -> None:
|
||||
"""The CLI fallback should never block waiting for confirmation."""
|
||||
observed_calls: list[tuple[list[str], dict[str, Any]]] = []
|
||||
|
||||
def fake_run(command: list[str], **kwargs: Any) -> Any:
|
||||
"""Record the CLI command and return success."""
|
||||
observed_calls.append((command, kwargs))
|
||||
return types.SimpleNamespace(returncode=0, stderr="")
|
||||
|
||||
monkeypatch.setattr(remote_modal_app_module.shutil, "which", lambda _: "/venv/bin/modal")
|
||||
monkeypatch.setattr(remote_modal_app_module.subprocess, "run", fake_run)
|
||||
monkeypatch.setenv("MODAL_ENVIRONMENT", "main")
|
||||
|
||||
stopped = remote_modal_app_module._stop_modal_app_via_cli("comfy-modal-sync-instance")
|
||||
|
||||
assert stopped is True
|
||||
assert observed_calls == [
|
||||
(
|
||||
[
|
||||
"/venv/bin/modal",
|
||||
"app",
|
||||
"stop",
|
||||
"comfy-modal-sync-instance",
|
||||
"--yes",
|
||||
"--env",
|
||||
"main",
|
||||
],
|
||||
{
|
||||
"check": False,
|
||||
"capture_output": True,
|
||||
"text": True,
|
||||
"timeout": remote_modal_app_module._MODAL_APP_STOP_TIMEOUT_SECONDS,
|
||||
},
|
||||
)
|
||||
]
|
||||
|
||||
|
||||
def test_remote_modal_cli_stop_timeout_returns_controlled_failure(
|
||||
remote_modal_app_module: Any,
|
||||
monkeypatch: Any,
|
||||
caplog: Any,
|
||||
) -> None:
|
||||
"""A genuine CLI timeout should not leak TimeoutExpired into prompt execution."""
|
||||
command = ["/venv/bin/modal", "app", "stop", "comfy-modal-sync-instance", "--yes"]
|
||||
|
||||
def fake_run(*_: Any, **__: Any) -> Any:
|
||||
"""Simulate an unresponsive Modal CLI."""
|
||||
raise subprocess.TimeoutExpired(
|
||||
command,
|
||||
remote_modal_app_module._MODAL_APP_STOP_TIMEOUT_SECONDS,
|
||||
)
|
||||
|
||||
monkeypatch.setattr(remote_modal_app_module.shutil, "which", lambda _: "/venv/bin/modal")
|
||||
monkeypatch.setattr(remote_modal_app_module.subprocess, "run", fake_run)
|
||||
monkeypatch.delenv("MODAL_ENVIRONMENT", raising=False)
|
||||
|
||||
with caplog.at_level(logging.WARNING):
|
||||
stopped = remote_modal_app_module._stop_modal_app_via_cli(
|
||||
"comfy-modal-sync-instance"
|
||||
)
|
||||
|
||||
assert stopped is False
|
||||
assert "Modal CLI app stop timed out" in caplog.text
|
||||
|
||||
|
||||
def test_remote_modal_rejects_fingerprint_mismatch_when_auto_deploy_is_disabled(
|
||||
remote_modal_app_module: Any,
|
||||
monkeypatch: Any,
|
||||
|
||||
Reference in New Issue
Block a user