Conversation
cquil11
changed the title
cquil11
added
the
non-canary-full-sweep-enabled
|
View unofficial run (performance): https://inferencex.semianalysis.com/inference?unofficialRun=36621227295 View unofficial run (accuracy): https://inferencex.semianalysis.com/evaluation?unofficialRun=36621227295 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I checked whether labeling an arbitrary PR with srt-streaming-test could let stream (lines 64-66) bypass build's branch/fork restriction (lines 23-26) — it can't, since GitHub Actions implicitly ANDs a job's custom if with success(), so stream (which needs: build) is itself skipped whenever build is skipped.
Extended reasoning...
This PR adds a new CI smoke-test workflow, bash script, and SRT recipe that streams logs/process metrics from a 90-second no-model job on a self-hosted B300 GPU runner, touching status-endpoint/token secrets and label-gated access to privileged compute. Two confirmed correctness bugs were found in run.sh (sacct completion check missing -X, and the scancel trap being registered only after job-id parsing that can fail) and are posted as inline comments. I separately verified the label-gating bypass concern raised during investigation is not exploitable, because GitHub Actions' implicit success() requirement on a job's needs causes stream to skip whenever build is skipped for a disallowed branch or fork.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| while [[ -n "$(squeue --noheader --jobs "$job_id" --format '%i')" ]]; do | ||
| sleep 5 | ||
| done | ||
| state=$(sacct --noheader --jobs "$job_id" --format State --parsable2 | head -1) |
There was a problem hiding this comment.
🔴 This smoke test can report a false pass/fail for the B300 job because its Slurm completion check omits -X and any retry, unlike the repo's own helper. sacct --noheader --jobs "$job_id" --format State --parsable2 | head -1 returns rows for every job step (batch/extern), not just the allocation, then blindly takes the first line; it also makes one attempt with no allowance for accounting lag. inferencex-e2e/runners/slurm_utils.sh's verify_slurm_job_status explicitly uses sacct -X -n -P with the comment "inspect only the allocation, never successful service steps" and retries 10 times with an scontrol fallback for exactly this lag. …
Why this was flagged
…Fix: use sacct -X to select the allocation row and retry/fallback like verify_slurm_job_status, so a lagging or reordered sacct row doesn't make [[ "$state" == COMPLETED ]] fail the CI job spuriously or mask a failed allocation behind a successful step.
Trigger: right after squeue (run.sh:57) stops listing the job, sacct's accounting DB can briefly lag or list a service step (extern/batch) before/instead of the main allocation row, since no -X filter is applied at run.sh:60. The script does head -1 and asserts state == COMPLETED at run.sh:61 with no retry. On the base pattern (inferencex-e2e/runners/slurm_utils.sh verify_slurm_job_status), the same race is handled by sacct -X -n -P plus up to 10 retries and an scontrol fallback specifically because 'accounting can lag briefly' and steps must be excluded. Here, a transient empty/step-row result makes the CI job fail even though the real Slurm job completed, or could mask a failed allocation if a step's row reports COMPLETED.
Verification: normal. run.sh:60-61 performs the Slurm completion check as a single query with no retry, run with no delay immediately after the squeue loop empties (run.sh:57-59): state=$(sacct --noheader --jobs "$job_id" --format State --parsable2 | head -1) then [[ "$state" == COMPLETED ]] under set -eo pipefail. Slurm accounting (slurmdbd) commonly lags behind squeue removal, so at the instant…
| uv run --no-sync srtctl apply -f "$script_dir/recipe.yaml" -o "$SRT_TEST_OUTPUT" --json > "$SRT_TEST_OUTPUT/submission.json" | ||
| job_id=$(uv run --no-sync python - "$SRT_TEST_OUTPUT/submission.json" <<'PY' | ||
| import json | ||
| import sys | ||
| from pathlib import Path | ||
| records = [json.loads(line) for line in Path(sys.argv[1]).read_text().splitlines() if line.startswith('{')] | ||
| print(records[-1]['slurm_job_id']) | ||
| PY | ||
| ) | ||
| [[ "$job_id" =~ ^[0-9]+$ ]] | ||
| printf '%s\n' "$job_id" > "$SRT_TEST_OUTPUT/job-id.txt" | ||
| echo "SRT streaming job: b300-dsxe:$job_id" | ||
| trap 'scancel "$job_id" 2>/dev/null || true' EXIT |
There was a problem hiding this comment.
🟡 (optional) If srtctl apply (run.sh:42) submits the Slurm job but the JSON on line 43-49 fails to parse (missing/extra slurm_job_id, empty submission.json, etc.), job_id=$(...) fails under set -eo pipefail and the script exits at line 51 before the trap 'scancel "$job_id"...' EXIT on line 54 is ever registered. The already-submitted 8-GPU B300 job is left running with no cleanup and no job-id.txt recorded for manual cancellation. Fix: register the scancel trap (parameterized so it's a no-op until job_id is known) immediately after srtctl apply succeeds, before attempting to parse job_id, so any later failure still cancels the submitted job.
Why this was flagged
Trigger: srtctl apply at run.sh:42 succeeds and Slurm accepts the job, but the python block at run.sh:43-49 raises (e.g. IndexError on empty records, or KeyError if slurm_job_id is absent/misnamed in the JSON). Under set -eo pipefail the job_id=$(...) assignment failure at line 43 aborts the script at line 51, before the trap ... EXIT is set at line 54. No trap is registered, so no scancel runs on exit, and $SRT_TEST_OUTPUT/job-id.txt (line 52) was never written, leaving no record of the job to cancel manually. The base has no such job at all (test didn't exist); this diff adds a path where a failed CI run leaks a running 8-GPU b300-dsxe Slurm allocation until it hits the recipe's own 00:10:00 time limit.
Verification: nit. The trap-ordering gap is real. run.sh line 42 submits the Slurm job (srtctl apply ... > submission.json), but the EXIT cleanup trap is only registered at line 54 (trap 'scancel "$job_id" 2>/dev/null || true' EXIT). Between them, lines 43-52 have no cleanup registered. Under set -eo pipefail (line 2), the command substitution job_id=$(uv run ... python ... <<'PY' ...) at lines…
|
Heads-up: #3576 (merged) replaced the bash launchers with a Python launcher, so this PR will conflict when you merge |
This branch has not been deployed
2 participants
Run the existing B300 GLM-5.2 FP8 SGLang AgentX sweep with SRT Slurm #539 applied.
The upstream patch is applied to the current SRT submodule; nothing is merged upstream. The earlier services-only smoke has been removed.