[NV] llm-d: re-test max-tpt in-sweep with unique SLURM job-name fix

[ezrasilvera]

What

Isolated re-test of the max-tpt (3P+1D DEP8 conc4096) llm-d-vllm multinode
benchmark with a fix for the in-sweep failure.

Why

max-tpt passes as a standalone e2e (run 28748597199, ~8747 tok/s/GPU) but its
throughput job was killed by an external SIGTERM ~10 min into weight load in
the isolated in-sweep run 28789948711 - before the server came up, so no
result JSON was produced and the launch step failed with "No benchmark result
files found". The engine loaded cleanly with zero errors and the twin eval job
(same server config, separate nodes) passed gsm8k at em_strict 0.959, so this
is not a config or engine problem.

Fix

submit.sh set the SLURM --job-name to $RUNNER_NAME (e.g. gb200-nv_1),
which is stable across runs. The workflow "Slurm cleanup" steps run
scancel --user=$USER --name=<runner.name> at pre-run and post-run, so a
bare runner-name allocation can be cancelled by any concurrent/adjacent job on
the shared cluster+user that shares the runner name. This change appends
GITHUB_RUN_ID and GITHUB_RUN_ATTEMPT to the job name so an exact
--name=<runner.name> scancel from elsewhere no longer matches this
allocation.

The sweep is isolated to the single max-tpt config-key. If max-tpt now passes
in-sweep, the name-scoped scancel was the cause; if it still fails, this rules
it out and points to an environmental/scheduling cause.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 configs/nvidia-master.yaml:9075-9078 — The merge-blocker comment at lines 9075-9078 says 'the image: on both llm-d-vllm keys below (mid-curve + high-tpt) is a personal build ... Republish ... before merge', but this PR adds a THIRD sibling key dsv4-fp4-gb200-llm-d-vllm-max-tpt-megamoe (line ~9160) that also uses ghcr.io/ezrasilvera/llm-d-nokube-vllm:vllm0.23 — and it is the config this PR is actively benchmarking (per the perf-changelog entry). Updating the enumeration to "all three (mid-curve + high-tpt + max-tpt)" avoids a comment-driven cleanup leaving max-tpt on the personal image at merge time.

  Extended reasoning...

  What's stale. The block introduced in the earlier llm-d PR added a merge-blocker note above the mid-curve/high-tpt entries. Verbatim: "PR follow-up (merge-blocking): the image: on both llm-d-vllm keys below (mid-curve + high-tpt) is a personal build. Republish it to the org registry ... and update these image: lines before merge." This PR then adds a third sibling — dsv4-fp4-gb200-llm-d-vllm-max-tpt-megamoe — that reuses the same ghcr.io/ezrasilvera/llm-d-nokube-vllm:vllm0.23 image, so the enumeration "both ... (mid-curve + high-tpt)" is now numerically wrong (three, not two).\n\nWhy it matters. The comment is explicitly a merge-blocking TODO whose intent is republish and swap all sibling image: lines. A reader executing that TODO by following the enumeration literally — the way in-file comments are normally read — could swap the two named keys and leave max-tpt-megamoe pointing at the personal registry. Given that this PR is specifically the max-tpt re-test (per its title and the perf-changelog entry it adds), leaving that one entry on the personal image would be a real regression for the very config this PR ships.\n\nStep-by-step proof.\n1. Reviewer (or the PR author on the last pass) reads nvidia-master.yaml:9075-9078, sees "the image: on both llm-d-vllm keys below (mid-curve + high-tpt) is a personal build".\n2. They locate the two enumerated keys: dsv4-fp4-gb200-llm-d-vllm-mid-curve-megamoe (line 9079) and dsv4-fp4-gb200-llm-d-vllm-high-tpt-megamoe (line ~9115), swap both image: lines to the republished org URL.\n3. They stop, because the comment said "both". The third key dsv4-fp4-gb200-llm-d-vllm-max-tpt-megamoe at line ~9160 remains on ghcr.io/ezrasilvera/llm-d-nokube-vllm:vllm0.23.\n4. Sweep runs against the personal image for the config this PR advertises as validated.\n\nAddressing the refutation. The refutation argues (a) all three image: lines are visible immediately below the comment, (b) grep ezrasilvera catches all three, and (c) perf-changelog announces max-tpt-megamoe. All true, but each depends on the cleanup being done via a search rather than by following the comment's enumeration verbatim. The whole reason this comment exists is that a maintainer will read it before merge — and that reader is being told there are two entries when there are three. Fix cost is one line; the alternative is trusting every future reader to spot the mismatch on their own.\n\nFix. Either (preferred) republish and swap all three image: lines before merge and drop the comment, or update the enumeration to "the three llm-d-vllm keys below (mid-curve + high-tpt + max-tpt)" so a comment-driven cleanup covers max-tpt. Severity is "nit" — this is a comment consistency issue, not a bug in shipped behavior; the underlying personal-image concern is already flagged by the same comment.

• 🟡 perf-changelog.yaml:4560-4566 — The pr-link for the new dsv4-fp4-gb200-llm-d-vllm-max-tpt-megamoe entry (perf-changelog.yaml:4566) points to /pull/2050, but this PR is #2117 and the description block (updated in HEAD commit 10aeffc) describes THIS PR's specific SLURM --job-name fix. If #2050 is being intentionally used as the canonical parent for all llm-d entries, feel free to dismiss; otherwise consider updating to /pull/2117 to match the convention used by sibling entries (#1994, #2010, #2077, etc.).

  Extended reasoning...

  What / how it manifests

  perf-changelog.yaml:4566 sets pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2050 for the dsv4-fp4-gb200-llm-d-vllm-max-tpt-megamoe entry. Per the PR metadata, this is PR #2117 ([NV] llm-d: re-test max-tpt in-sweep with unique SLURM job-name fix).

  Why the description text points at #2117, not #2050

  The description block reads:

  • "Isolated max-tpt (3P+1D DEP8 conc4096) re-test with unique SLURM job-name fix"
  • "max-tpt passes standalone e2e but was killed mid weight-load in sweep (run 28789948711)"
  • "Fix: SLURM --job-name now includes GITHUB_RUN_ID/ATTEMPT so a name-scoped scancel from another job on the shared cluster cannot cancel this allocation"

  Every one of these three bullets describes the SLURM --job-name fix that lives in this PR's HEAD commit (10aeffc [NV] llm-d: unique SLURM job-name to protect max-tpt from name-scoped scancel), which touches benchmarks/multi_node/llm-d/submit.sh to append GITHUB_RUN_ID / GITHUB_RUN_ATTEMPT. The referenced run 28789948711 is the failed sweep noted in PR #2117's description. None of this content lives under PR #2050.

  Why the file convention says #2117

  The other recent entries in perf-changelog.yaml all use their own PR number as pr-link (e.g. #1994 for the minimaxm3-fp4-b300 high-conc add, #2006 for the mtp update, #2007 for the B200 eagle update, #2010 for the DSV4 GB300 dynamo update). The natural reading is that pr-link points at the PR that lands the entry — under that reading, this entry should read /pull/2117.

  Step-by-step

  1. PR #2117 is open with head 10aeffc. git log on this branch shows it and its recent commits.
  2. That head commit modifies the exact perf-changelog block at lines 4560-4566 to describe the SLURM job-name fix.
  3. The submit.sh diff in this same PR is exactly that fix: SLURM_JOB_NAME="${RUNNER_NAME}-${GITHUB_RUN_ID:-manual}-${GITHUB_RUN_ATTEMPT:-0}", replacing the prior bare-$RUNNER_NAME job name — which is what the description bullets attribute to "this PR".
  4. So the description text is unambiguously about #2117, while pr-link says #2050.

  Addressing the counter-argument

  A refuting verifier noted that an earlier branch commit (06f8b3c [NV] llm-d: address PR #2050 review nits (docs + changelog pr-link)) explicitly canonicalized pr-link: XXX -> 2050 for the ORIGINAL llm-d entry (lines 4538-4541, about mid-curve/high-tpt), and that commit e149e9a (which added the max-tpt entry) says "Adds a matching perf-changelog entry under PR #2050". If the author's intent is that #2050 stays the canonical parent for every llm-d changelog entry — even ones landed by descendant PRs — this is by design, not a bug. That is a legitimate reading and is why this is filed as a nit, not a normal issue.

  The countervailing signal is that HEAD commit 10aeffc (in THIS PR, not #2050) rewrote the description text to describe THIS PR's fix specifically, which is inconsistent with "the entry belongs to #2050": if #2050 were truly canonical, the description would describe #2050's content, not #2117's.

  Impact

  Documentation-only. No functional or CI impact. Audit trails from the changelog to PR context would follow the wrong link (readers land on #2050 expecting to see the SLURM job-name fix, and find the original llm-d landing PR instead).

  Fix

  If unintentional: change line 4566 to pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2117.
  If intentional (#2050 as canonical parent): dismiss this comment; consider phrasing the description in past-tense parent-PR terms rather than "this PR" terms to avoid the mismatch.

[claude]

🟡 The pyxis branch of job.slurm builds PYXIS_ENV_LIST (lines 211-218) as an explicit --container-env whitelist but omits MAX_FAILURE_RATE, while the docker branch at line 167 forwards it via -e MAX_FAILURE_RATE=${MAX_FAILURE_RATE:-0.05}. Since launch_gb200-nv.sh sets LLMD_CONTAINER_ENGINE=pyxis unconditionally for the GB200 llm-d-vllm path, the recipe's MAX_FAILURE_RATE=0.5 never reaches the container and benchmark_serving.py:971 falls back to the hardcoded 0.05 default. Fix: append ,MAX_FAILURE_RATE to PYXIS_ENV_LIST (symmetric with the docker -e line).

Extended reasoning...

What / where. The pyxis srun in benchmarks/multi_node/llm-d/job.slurm uses --container-env="$PYXIS_ENV_LIST" to whitelist which env vars propagate into the container. PYXIS_ENV_LIST is built at lines 211-218 and enumerates 30 variables — but MAX_FAILURE_RATE is not one of them. The docker branch at line 167 explicitly forwards it via -e MAX_FAILURE_RATE=${MAX_FAILURE_RATE:-0.05}, and the surrounding PYXIS_ENV_LIST mirrors the docker -e list otherwise. The author's own comment at lines 197-198 ("Pre-export non-SLURM static vars so pyxis --container-env= whitelisting can pick them up") confirms the whitelist design intent.

Step-by-step proof.

1. benchmarks/multi_node/llm-d/submit.sh:88 runs export MAX_FAILURE_RATE=${MAX_FAILURE_RATE:-0.05} — value reaches sbatch env.
2. All three new GB200 llm-d-vllm recipes set MAX_FAILURE_RATE=0.5 in additional-settings (configs/nvidia-master.yaml:9106, :9152, :9196).
3. runners/launch_gb200-nv.sh sets LLMD_CONTAINER_ENGINE=pyxis unconditionally — the docker branch is dead code on this SKU.
4. job.slurm translates the sbatch env through --container-env="$PYXIS_ENV_LIST"; since MAX_FAILURE_RATE is not in the list, it is dropped at the container boundary.
5. Inside the container utils/bench_serving/benchmark_serving.py:970-972 does os.environ.get("MAX_FAILURE_RATE", "0.05") — it silently gets 0.05.
6. At line 975, failure_rate > 0.05 triggers raise SystemExit("FAIL: request failure rate ...") and no result JSON is written.

Impact on this PR. The concrete impact is contingent on the actual failure rate exceeding 5% at conc=4096 with 3P+1D DEP8. The standalone e2e run (28748597199, ~8747 tok/s/GPU) referenced in the PR description already passed on the same pyxis path with the same missing forwarding — strong empirical evidence that either pyxis is propagating the variable via default env inheritance on this cluster, or that the natural failure rate is well under 5% and the 0.05 default is sufficient. Either way, this in-sweep re-test is likely to reflect the same behavior. The current isolated run's observed failure was a SIGTERM during weight load (before any requests were served), which is orthogonal to MAX_FAILURE_RATE.

Why still worth flagging. The docker/pyxis asymmetry is a real contract inconsistency — every other variable the docker branch -es is explicitly enumerated in PYXIS_ENV_LIST. The recipes explicitly opted into a 0.5 tolerance, and if a future transient 5xx spike (well within that tolerance) ever pushes the failure rate above 5%, the run would fail with a symptom (FAIL: request failure rate) that could be confused with the SIGTERM cause this PR is trying to isolate.

Fix. One-line append to PYXIS_ENV_LIST at line 218 or similar:

PYXIS_ENV_LIST+=",MAX_FAILURE_RATE"

Symmetric with the docker -e MAX_FAILURE_RATE=${MAX_FAILURE_RATE:-0.05} line at 167. Also worth adding MAX_FAILURE_RATE to the pre-export block at lines 208-209 for symmetry, though it's already exported by submit.sh.

[github-actions]

see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28918650203
see unofficial run visualizer at https://inferencex.semianalysis.com/evaluation?unofficialRun=28918650203

[ezrasilvera]

Diagnostic complete. The isolated max-tpt re-test with the unique SLURM job-name fix passed (run 28918650203, 8730 tok/s/GPU), confirming the in-sweep failure was a name-scoped scancel, not the config. Folding the fix and restoring the full curve (mid-curve + high-tpt) into llm-d-initial (PR #2050); closing this diagnostic PR.