Add opt-in Modal app cleanup after sandbox completion#4993
Add opt-in Modal app cleanup after sandbox completion#4993raza-khan0108 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in cleanup mechanism for the Modal step operator to stop per-step Modal apps after a sandbox finishes, reducing “deployed / Tasks: 0” dashboard clutter without changing default behavior.
Changes:
- Introduces
stop_app_after_run: bool = FalseinModalStepOperatorSettings. - Adds a
sandbox_utils.stop_modal_app(...)helper that invokesmodal app stopvia subprocess (with env-based auth forwarding). - Hooks cleanup into
ModalStepOperator.get_status()(on terminal state) andcancel().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/zenml/integrations/modal/step_operators/modal_step_operator.py |
Triggers opt-in stop of the step-scoped Modal app on completion/cancelation. |
src/zenml/integrations/modal/sandbox_utils.py |
Adds CLI-based helper to stop a deployed Modal app by name. |
src/zenml/integrations/modal/flavors/modal_step_operator_flavor.py |
Adds the stop_app_after_run user-facing settings field and description. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if status.is_finished: | ||
| settings = cast( | ||
| ModalStepOperatorSettings, self.get_settings(step_run) | ||
| ) | ||
| if settings.stop_app_after_run: | ||
| self._stop_step_app(step_run, settings) | ||
| return status |
| stop_app_after_run: bool = Field( | ||
| False, | ||
| description="Stop the per-step Modal app after the sandbox reaches a terminal " | ||
| "state. When True, ZenML calls `modal app stop` on the app that was " | ||
| "created for this step 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.", | ||
| ) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f986c5c88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
| import subprocess | ||
|
|
||
| cmd = ["modal", "app", "stop", "--yes", app_name] |
There was a problem hiding this comment.
Handle Modal 1.4.0/1.4.1 before passing --yes
This hard-codes --yes, but the Modal integration still declares support for modal>=1.4,<2 in src/zenml/integrations/modal/__init__.py; Modal's official changelog says the confirmation prompt and --yes skip flag were added in 1.4.2 (https://modal.com/docs/sdk/py/changelog#142-2026-04-16). When users are on the supported 1.4.0 or 1.4.1 releases, the cleanup command will reject the unknown option and stop_app_after_run=True will only log a warning while leaving the app deployed, so either the requirement needs to be raised or the flag needs to be conditional.
Useful? React with 👍 / 👎.
|
@raza-khan0108 A few things please:
|
|
#4990 is merged now, so as soon as you rebase on develop you can use it. |
5f986c5 to
15319f5
Compare
After a step sandbox finishes, the Modal app entry remains in the dashboard as 'deployed, Tasks: 0'. This is purely visual clutter— idle apps incur no compute charges—but accumulates quickly for users running many ZenML pipelines or steps on Modal. Add a new stop_app_after_run: bool = False setting to ModalSettingsMixin (shared by both the step operator and orchestrator). When True, ZenML overrides cleanup_step_submission() to call modal app stop --yes 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. Changes: - modal_base_flavor.py: add stop_app_after_run to ModalSettingsMixin - sandbox_utils.py: add stop_modal_app() helper that delegates to the Modal CLI and forwards token credentials from ZenML config - modal_step_operator.py: override cleanup_step_submission() (the hook added in zenml-io#4990) to call stop_modal_app() when the setting is enabled; failures are logged as warnings and never re-raised Fixes zenml-io#4889
15319f5 to
ec83b0d
Compare
|
Hi @schustmi — addressed all the feedback:
The PR description has been updated to reflect the new implementation. Let me know if anything else needs changing! |
| 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) |
There was a problem hiding this comment.
Is it not possible to do this via the modal python sdk?
There was a problem hiding this comment.
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 App.stop() equivalent). The supported path today is modal app stop, so this PR uses the CLI via subprocess.run().
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 MODAL_TOKEN_ID / MODAL_TOKEN_SECRET from the configured ZenML integration so it doesn’t depend on ambient auth state.
If Modal adds a public SDK method for this, I’m happy to switch this implementation to that.
| RuntimeError: If the ``modal app stop`` command exits with a | ||
| non-zero status code. | ||
| """ | ||
| import subprocess |
| 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. |
There was a problem hiding this comment.
| 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. |
| 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. |
There was a problem hiding this comment.
| 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. |
| if not settings.stop_app_after_run: | ||
| return | ||
|
|
||
| app_name = f"zenml-{step_run.id}-{step_run.name}"[:64] |
There was a problem hiding this comment.
This should be extracted in some helper function if it's used in multiple places.
| 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( |
There was a problem hiding this comment.
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_app or something similar makes more sense, as for example the step operator stops it after a step finishes, not a run.
| 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.", |
There was a problem hiding this comment.
| 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.", | |
| description="Stop the Modal app after the sandbox reaches a terminal " | |
| "state.", |
Describe changes
After a step sandbox finishes, the Modal app entry remains in the Modal
dashboard as
deployed, Tasks: 0. This is purely visual clutter — idleapps incur no compute charges — but accumulates quickly for users running
many ZenML pipelines or steps on Modal (each step run creates one app
named
zenml-<step-run-id>-<step-name>).This PR adds a new
stop_app_after_run: bool = Falsesetting toModalSettingsMixin(shared by both the step operator and orchestratorflavors). When enabled, ZenML overrides
cleanup_step_submission()— thehook introduced in #4990 — to call
modal app stop --yes <app-name>viasubprocess after the sandbox reaches a terminal state. The app entry moves
from deployed → stopped in the Modal dashboard. Stopped apps are
still visible in the "recently stopped" section with their full log history
intact, so debugging is unaffected.
Why subprocess instead of the Python SDK?
modal.Apphas no public.stop()method. The Modal CLI is the onlysupported programmatic path for stopping a deployed app. We call it via
subprocess.run()and injectMODAL_TOKEN_ID/MODAL_TOKEN_SECRETfromZenML's configured credentials so the call works regardless of ambient auth
state.
Changes
modal_base_flavor.py: addstop_app_after_run: bool = FalsetoModalSettingsMixinsandbox_utils.py: addstop_modal_app()helper that runs the ModalCLI and forwards token credentials
modal_step_operator.py: overridecleanup_step_submission()(fromAdd isolated step cleanup hooks #4990) — early-returns when the setting is
False, otherwise callsstop_modal_app()with warning-only error handling so cleanup can nevermask the step result
Usage