ci(phoenix): bound SLURM queue wait and combine build+test into one allocation - #1763
Open
sbryngelson wants to merge 4 commits into
Open
ci(phoenix): bound SLURM queue wait and combine build+test into one allocation#1763sbryngelson wants to merge 4 commits into
sbryngelson wants to merge 4 commits into
Conversation
…llocation Phoenix CI jobs were failing as opaque ~8h 'cancelled': jobs sat PENDING for hours on the preemptible 'embers' QOS, burning the 480-min job timeout and holding self-hosted runner slots (which backs up unrelated PRs). A) monitor_slurm_job.sh: bound the queue wait (SLURM_MAX_QUEUE_SECONDS, default 90m). If a job never starts, scancel and fail fast with an explicit 'queue starvation — infrastructure, not code' message and exit 75. A RUNNING job with a merely NFS-delayed output file is exempt. B) Phoenix builds+tests in a single SLURM allocation (build-and-test.sh) so the scheduler queue wait is paid once instead of twice. submit-slurm-job.sh exports job_* so the child scripts inherit them; adds a 'buildtest' time budget (3h30m). Other clusters keep the separate build/test steps.
|
Claude Code Review Head SHA: d95d626 Files changed:
Findings:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Improves Phoenix CI reliability by failing fast on SLURM queue starvation and reducing exposure to queue wait by combining build+test into a single SLURM allocation.
Changes:
- Add a bounded SLURM queue-wait budget with explicit “queue starvation” failure messaging.
- Combine Phoenix build and test into a single SLURM allocation and update log slug collection.
- Export
job_*variables so wrapper scripts can run child phases that inherit job context.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| .github/workflows/test.yml | Routes Phoenix to a combined “Build & Test” SLURM submission and adjusts log printing to include the combined output. |
| .github/workflows/common/build-and-test.sh | New wrapper that runs build then test inside one SLURM allocation. |
| .github/scripts/submit-slurm-job.sh | Adds buildtest job type/time budget and exports job_* vars for child-process inheritance. |
| .github/scripts/monitor_slurm_job.sh | Adds SLURM_MAX_QUEUE_SECONDS queue-wait limit and fails fast with a clear starvation message. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Combined build+test runs both phases in one allocation (one queue | ||
| # wait instead of two). It needs headroom for the build on top of the | ||
| # test budget; kept modest so it still backfills well under 'embers'. | ||
| buildtest_time="03:30:00" |
Comment on lines
+93
to
+97
| case "$job_type" in | ||
| bench) sbatch_time="#SBATCH -t $bench_time" ;; | ||
| buildtest) sbatch_time="#SBATCH -t $buildtest_time" ;; | ||
| *) sbatch_time="#SBATCH -t $test_time" ;; | ||
| esac |
Comment on lines
+116
to
+126
| if [ "$SLURM_MAX_QUEUE_SECONDS" -gt 0 ]; then | ||
| case "$state" in | ||
| RUNNING|COMPLETING) ;; | ||
| *) | ||
| waited=$(( $(date +%s) - queue_start )) | ||
| if [ "$waited" -ge "$SLURM_MAX_QUEUE_SECONDS" ]; then | ||
| abort_queue_starvation "$waited" | ||
| fi | ||
| ;; | ||
| esac | ||
| fi |
Addresses review: buildtest_time was only defined in the phoenix cluster
config, so a build-and-test.sh submission on any other cluster would hit an
unbound-variable crash under 'set -u'. Use ${buildtest_time:-$test_time} so the
combined job type is safe on every cluster (only phoenix uses it today).
…sabling A mistyped override (e.g. '90m') would make the '[ -gt ]' comparison fail its if-condition and silently skip the queue-wait budget — disabling the feature without warning. Validate up front and exit 1 with a clear message.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1763 +/- ##
=======================================
Coverage 61.67% 61.67%
=======================================
Files 84 84
Lines 21619 21619
Branches 3196 3196
=======================================
Hits 13334 13334
Misses 6093 6093
Partials 2192 2192 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
What & why
Phoenix CI has been failing as opaque ~8h
cancelledjobs. Root cause, from the run logs, is SLURM queue starvation, not test failures:PENDINGfor hours on the preemptibleembersQOS (few idle GPU nodes ongpu-l40s), burning the entiretimeout-minutes: 480budget.phoenix-Nrunner slot for the full 8h, so unrelated PRs back up behind it.Across the last 30
test.ymlruns, Phoenix jobs were 61% cancelled / 32% success / 7% failure — but of jobs that actually ran to completion, ~83% passed. The red is overwhelmingly queue/infra, not code.This PR does not try to buy more nodes; it makes the failure mode fast and legible, and halves each job's exposure to the queue.
Changes
A. Bound the queue wait (
monitor_slurm_job.sh)SLURM_MAX_QUEUE_SECONDS(default 90m;0= old behaviour). If a job never leaves the queue,scancelit and fail fast with an explicitQUEUE STARVATION … infrastructure, not a code/test failuremessage (exit 75).RUNNING/COMPLETING) but whose output file is merely NFS-delayed is exempt, so a job already doing work is never killed.cancelled.B. Combine build+test into one Phoenix allocation (
build-and-test.sh,submit-slurm-job.sh,test.yml)build-and-test.shallocation (build → test) so the scheduler queue wait is paid once instead of twice.submit-slurm-job.shexports thejob_*vars so the childbuild.sh/test.shinherit them, and gets abuildtesttime budget (3h30m — test budget plus build headroom, kept modest to preserveembersbackfill).Testing
PENDINGjob aborts with the starvation message and exit 75; aRUNNINGjob is exempt from the budget and proceeds to stream.build-and-test/test/benchso Phoenix's combinedbuild-and-test-*.outis still picked up by Print Logs.bash -non all three scripts; YAML validated.Notes / follow-ups
buildtest_time(3h30m). If combined runs ever approach it, bump there.embers → infernoafter the queue-wait budget elapses, behind a flag, to spend SUs only when the free queue is starved.Acknowledgement