Split the modal CI budget into acquisition and test phases - #8404
Merged
Merged
Conversation
The modal-torch-latest GPU job is killed at its 75-minute cap before the test suite finishes. On master this now happens in roughly half the runs. The suite is not what grew. Phase timings taken from the job logs show the regression is confined to Modal sandbox provisioning, which went from about 0.1 min in late August to 18-22 min from 2026-08-31 onward, while pytest itself stayed in its usual 38-49 min band for a near-identical test count: run provisioning env setup pytest tests 08-22 12:53 0.1m 6.0m 37.5m 1150 passed 08-29 00:40 0.2m 6.2m 39.3m 1194 passed 09-01 11:47 17.8m 6.6m 48.4m 1241 passed 09-02 04:00 22.0m 4.8m 38.5m 1241 passed Raise the outer GitHub job budget to 90 minutes. Raise the inner Modal sandbox lifetime to 70 minutes as well: its clock starts when the container starts, so it has to cover env setup plus pytest, which has already been observed at 55 min, leaving only 5 minutes of headroom against the old 3600s value. This only buys back the margin the provisioning regression consumed. The regression itself still needs investigation on the Modal side. Signed-off-by: Ma, Guokai <guokai.ma@intel.com>
A single wall-clock budget cannot tell "we never got a GPU" apart from "the tests ran long". Both currently surface as the same job timeout, and a run that waits 22 minutes for capacity spends that time out of the budget the tests still need. Sandbox.create() returns before the container exists, so the wait for the l40s:2 reservation surfaces on the first exec. Bound that wait on its own: - acquisition: 30 min (SANDBOX_ACQUIRE_TIMEOUT_SECONDS). Past that the run aborts with SandboxStartTimeout, which says no test ran, instead of holding a runner for the rest of the budget. - tests: 70 min, unchanged. The Sandbox lifetime clock starts when the container starts, so this budget is always fully available once a GPU is reserved, no matter how long acquisition took. - the job timeout becomes a backstop at 105 min, covering 30 + 70 plus runner setup and cleanup. The startup duration is now printed with flush=True. Sandbox output is otherwise block-buffered and lost when the job is killed, which is why the timed-out runs show a silent gap rather than any progress. Observed acquisition times were 12s, 0.9, 1.3, 7.9, 10.8, 17.8 and 22.0 min, so 30 min leaves headroom over the worst case while still failing fast. Signed-off-by: Ma, Guokai <guokai.ma@intel.com>
Signed-off-by: Guokai Ma <guokai.ma@intel.com> # Conflicts: # .github/workflows/modal-torch-latest.yml # ci/test_torch_latest.py # ci/torch_latest.py
delock
marked this pull request as ready for review
September 6, 2026 11:40
delock
marked this pull request as draft
September 6, 2026 11:41
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58b5c96dd7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| name: modal-torch-latest / DeepSpeedAI CI | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 90 | ||
| timeout-minutes: 105 |
There was a problem hiding this comment.
This is a non-merge commit, but its message has no Signed-off-by trailer, violating the repository's mandatory commit requirement; recreate this commit with --signoff before acceptance.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
delock
marked this pull request as ready for review
September 7, 2026 00:55
This was referenced Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #8403 — review that one first; this PR's base retargets to
masterautomatically once it merges.Problem
A single wall-clock budget cannot tell "we never got a GPU" apart from "the tests ran long". Both surface as the same job timeout today, and a run that spends 22 minutes waiting for capacity takes that time out of the budget the tests still need.
Measured on completed master runs, the wait between
Sandbox.create()and the first sandbox output:Sandbox.create()returns before the container exists, so the wait for thel40s:2reservation surfaces on the firstexec.Change
Bound the two phases separately:
SANDBOX_ACQUIRE_TIMEOUT_SECONDS). Past that the run aborts withSandboxStartTimeout, whose message states that no test ran, instead of holding a runner for the rest of the budget.30 min leaves headroom over the worst observed acquisition (22.0 min) while still failing fast when capacity never arrives.
Observability
The startup duration is printed with
flush=True. Sandbox output is otherwise block-buffered by Python and lost when the job is killed, which is why the timed-out runs show a silent gap rather than any progress. This makes the acquisition wait directly visible in the log instead of something you have to reconstruct from timestamps.Tests
ci/test_torch_latest.pygains coverage for the abort path:test_await_sandbox_start_gives_up_when_the_container_never_runs— catches a controller that blocks forever on a reservation that is never satisfied.test_controller_aborts_without_running_tests_when_sandbox_never_starts— catches a controller that spends the whole budget waiting, runs commands against a Sandbox that never started, or leaks the Sandbox when startup times out.test_await_sandbox_start_reports_startup_durationVerified by mutation: with the
is_alive()check removed, the suite hangs instead of failing, which is the exact defect these tests guard against.The acquisition budget is also pinned in
test_sandbox_kwargs_are_fixed_and_secret_free, so changing a resource limit stays visible in review like the other fixed Sandbox parameters.Not addressed here
Why acquisition grew from ~0.1 min to 18-22 min. Ruled out from the repo side (image name, preset table, pinned
modal==1.2.6, sandbox kwargs all unchanged); confirming whether it is GPU queueing or an image pull needs the Modal dashboard for appdeepspeedai-torch-latest-ci. This PR makes that case fail fast and legibly rather than fixing it.