-
Notifications
You must be signed in to change notification settings - Fork 642
Add opt-in Modal app cleanup after sandbox completion #4993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -68,6 +68,15 @@ class ModalSettingsMixin(BaseSettings): | |||||||||||||||||
| f"Examples: 3600 (1 hour), 7200 (2 hours), {DEFAULT_TIMEOUT_SECONDS} (24 hours maximum). " | ||||||||||||||||||
| "Execution will be terminated if it exceeds this timeout", | ||||||||||||||||||
| ) | ||||||||||||||||||
| stop_app_after_run: bool = Field( | ||||||||||||||||||
| False, | ||||||||||||||||||
| description="Stop the Modal app after the sandbox reaches a terminal " | ||||||||||||||||||
| "state. When True, ZenML calls `modal app stop` on the app created " | ||||||||||||||||||
| "for this run, removing it from the Modal dashboard's deployed-apps " | ||||||||||||||||||
| "list. Stopped apps are still visible in the 'recently stopped' " | ||||||||||||||||||
| "section and their logs remain accessible. Defaults to False to " | ||||||||||||||||||
| "preserve the previous behavior.", | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| class ModalCredentialsMixin(BaseSettings): | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |||||||||||||||||
| """Shared Modal Sandbox helpers.""" | ||||||||||||||||||
|
|
||||||||||||||||||
| import math | ||||||||||||||||||
| import os | ||||||||||||||||||
| import time | ||||||||||||||||||
| from typing import Any, Dict, List, Optional, Protocol, Tuple | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -398,3 +399,56 @@ def terminate_sandbox( | |||||||||||||||||
| """Terminate a Modal sandbox by ID.""" | ||||||||||||||||||
| sandbox = get_sandbox_by_id(sandbox_id, modal_client=modal_client) | ||||||||||||||||||
| sandbox.terminate() | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def stop_modal_app( | ||||||||||||||||||
| app_name: str, | ||||||||||||||||||
| *, | ||||||||||||||||||
| modal_environment: Optional[str], | ||||||||||||||||||
| token_id: Optional[str] = None, | ||||||||||||||||||
| token_secret: Optional[str] = None, | ||||||||||||||||||
| ) -> None: | ||||||||||||||||||
| """Stop a deployed Modal app by name using the Modal CLI. | ||||||||||||||||||
|
|
||||||||||||||||||
| Moves the app from 'deployed' to 'stopped' in the Modal dashboard, | ||||||||||||||||||
| eliminating the visual clutter of idle ``Tasks: 0`` app entries. | ||||||||||||||||||
| Stopped apps remain visible in the 'recently stopped' section with | ||||||||||||||||||
| their logs intact, so debugging is not affected. | ||||||||||||||||||
|
|
||||||||||||||||||
| This should only be called after the associated Modal sandbox has | ||||||||||||||||||
| reached a terminal state, because stopping the app does not wait for | ||||||||||||||||||
| running sandboxes to finish. | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| Args: | ||||||||||||||||||
| app_name: The Modal app name to stop (e.g. ``zenml-<id>-<step>``). | ||||||||||||||||||
| modal_environment: Modal environment name passed as ``--env``. | ||||||||||||||||||
| ``None`` uses the CLI's default environment. | ||||||||||||||||||
| token_id: Modal token ID to forward as ``MODAL_TOKEN_ID``. When | ||||||||||||||||||
| ``None``, the ambient CLI credential is used. | ||||||||||||||||||
| token_secret: Modal token secret to forward as | ||||||||||||||||||
| ``MODAL_TOKEN_SECRET``. When ``None``, the ambient CLI | ||||||||||||||||||
| credential is used. | ||||||||||||||||||
|
|
||||||||||||||||||
| Raises: | ||||||||||||||||||
| RuntimeError: If the ``modal app stop`` command exits with a | ||||||||||||||||||
| non-zero status code. | ||||||||||||||||||
| """ | ||||||||||||||||||
| import subprocess | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Import at the top |
||||||||||||||||||
|
|
||||||||||||||||||
| cmd = ["modal", "app", "stop", "--yes", app_name] | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This hard-codes Useful? React with 👍 / 👎. |
||||||||||||||||||
| if modal_environment: | ||||||||||||||||||
| cmd += ["--env", modal_environment] | ||||||||||||||||||
|
|
||||||||||||||||||
| env = os.environ.copy() | ||||||||||||||||||
| token_pair = resolve_modal_token_pair( | ||||||||||||||||||
| token_id=token_id, token_secret=token_secret | ||||||||||||||||||
| ) | ||||||||||||||||||
| if token_pair: | ||||||||||||||||||
| env[MODAL_TOKEN_ID_ENV_KEY] = token_pair[0] | ||||||||||||||||||
| env[MODAL_TOKEN_SECRET_ENV_KEY] = token_pair[1] | ||||||||||||||||||
|
|
||||||||||||||||||
| result = subprocess.run(cmd, capture_output=True, text=True, env=env) | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it not possible to do this via the modal python sdk?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked into using the Modal Python SDK first, but there doesn’t appear to be a public API to stop an already deployed app programmatically (e.g., no I also made sure the call is non-blocking for pipeline outcome: cleanup is best-effort (warning-only on failure), and credentials are passed explicitly via If Modal adds a public SDK method for this, I’m happy to switch this implementation to that. |
||||||||||||||||||
| if result.returncode != 0: | ||||||||||||||||||
| raise RuntimeError( | ||||||||||||||||||
| f"Failed to stop Modal app '{app_name}': {result.stderr.strip()}" | ||||||||||||||||||
| ) | ||||||||||||||||||
|
schustmi marked this conversation as resolved.
Outdated
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -267,3 +267,45 @@ def cancel(self, step_run: "StepRunResponse") -> None: | |||||||||||||
| sandbox_id = str(step_run.run_metadata[STEP_SANDBOX_ID_METADATA_KEY]) | ||||||||||||||
| modal_client = self._get_modal_client() | ||||||||||||||
| sandbox_utils.terminate_sandbox(sandbox_id, modal_client=modal_client) | ||||||||||||||
|
|
||||||||||||||
| def cleanup_step_submission(self, step_run: "StepRunResponse") -> None: | ||||||||||||||
| """Stop the Modal app after a sandbox run completes. | ||||||||||||||
|
|
||||||||||||||
| When ``stop_app_after_run`` is enabled in the step settings, this | ||||||||||||||
| calls ``modal app stop`` on the per-step app, moving its entry from | ||||||||||||||
| 'deployed' to 'stopped' in the Modal dashboard. Stopped apps remain | ||||||||||||||
| visible in the 'recently stopped' section with their full log history | ||||||||||||||
| intact, so debugging is unaffected. Failures are logged as warnings | ||||||||||||||
| and never re-raised so that cleanup cannot mask the step result. | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| Args: | ||||||||||||||
| step_run: The finished step run. | ||||||||||||||
| """ | ||||||||||||||
| settings = cast(ModalStepOperatorSettings, self.get_settings(step_run)) | ||||||||||||||
| if not settings.stop_app_after_run: | ||||||||||||||
| return | ||||||||||||||
|
|
||||||||||||||
| app_name = f"zenml-{step_run.id}-{step_run.name}"[:64] | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be extracted in some helper function if it's used in multiple places. |
||||||||||||||
| modal_environment = sandbox_utils.normalize_optional_config_value( | ||||||||||||||
| settings.modal_environment | ||||||||||||||
| ) | ||||||||||||||
|
schustmi marked this conversation as resolved.
Outdated
|
||||||||||||||
| try: | ||||||||||||||
| sandbox_utils.stop_modal_app( | ||||||||||||||
| app_name, | ||||||||||||||
| modal_environment=modal_environment, | ||||||||||||||
| token_id=self.config.token_id, | ||||||||||||||
| token_secret=self.config.token_secret, | ||||||||||||||
| ) | ||||||||||||||
| logger.debug( | ||||||||||||||
| "Stopped Modal app '%s' for step run '%s'.", | ||||||||||||||
| app_name, | ||||||||||||||
| step_run.id, | ||||||||||||||
| ) | ||||||||||||||
| except Exception: | ||||||||||||||
| logger.warning( | ||||||||||||||
| "Failed to stop Modal app '%s' for step run '%s'. " | ||||||||||||||
| "The app may still appear in the Modal dashboard.", | ||||||||||||||
| app_name, | ||||||||||||||
| step_run.id, | ||||||||||||||
| exc_info=True, | ||||||||||||||
| ) | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is added on the base settings class, but only respected when using the step operator. I think we should move it to the step operator settings, or implement it for all flavors inheriting from this (sandbox, orchestrator, step operator).
Also, I think
stop_appor something similar makes more sense, as for example the step operator stops it after a step finishes, not a run.