Conversation
esmeetu
marked this pull request as ready for review
July 18, 2026 02:31
esmeetu
requested review from
alec-flowers,
csahithi,
ishandhanani and
nlevin-ui
as code owners
July 18, 2026 02:31
esmeetu
marked this pull request as draft
July 21, 2026 04:45
ishandhanani
added a commit
that referenced
this pull request
Sep 3, 2026
…2 stalls Both clone paths (real SLURM/sbatch and --bash dev-mode) did a plain git clone/fetch with no defenses. Hit in practice on NVIDIA/InferenceMAX PR #271: cloning ai-dynamo/dynamo for the thunderagent-router auxiliary service failed with exit 128 on a B200 compute node, the same failure mode Ankur Singh root-caused and fixed (-c http.version=HTTP/1.1) for this cluster's login-node srt-slurm clone -- compute nodes hit it too. Apply the same guard here: -c http.version=HTTP/1.1, GIT_TERMINAL_PROMPT=0, and a 120s timeout, on both the clone and the fetch.
ishandhanani
added a commit
that referenced
this pull request
Sep 3, 2026
The clone/fetch already had the guard, but checkout was missed: with --filter=blob:none (a partial clone), `git checkout FETCH_HEAD` does its own network round-trip to lazily fetch missing blobs from the promisor remote, hitting the same intermittent failure on a different git command. Confirmed via a live NVIDIA/InferenceMAX PR #271 run: clone+fetch succeeded fully (100% files, FETCH_HEAD resolved), then checkout failed with "could not fetch <blob-sha> from promisor remote" -- exit 128, same signature, different round-trip.
2 tasks
ishandhanani
added a commit
that referenced
this pull request
Sep 3, 2026
…S lag Clone/checkout, build, and launch are three separate srun invocations. Confirmed via a live NVIDIA/InferenceMAX PR #271 run: the clone's process exits 0 with the source dir genuinely written and verified present moments later, but the immediately-following build step's `cd` still hit "No such file or directory" -- a client-side NFS attribute-cache race, not a real missing directory. Poll up to 10s before cd'ing instead of failing the whole service on a stale cache entry.
ishandhanani
added a commit
that referenced
this pull request
Sep 3, 2026
…TTP failures (#376) * Add cluster-wide git_http_version config to work around intermittent git smart-HTTP failures Some clusters see intermittent git smart-HTTP failures negotiating HTTP/2 against github.com -- stalls, or truncated responses git misreports as "could not read Username" auth-prompt failures -- on certain network paths. Observed on NVIDIA/InferenceMAX PR #271 on both a login host and its compute nodes, across three separate git operations (clone, fetch, and checkout of a partial clone, which does its own lazy blob fetch). Add srtctl.core.config.git_clone_command_prefix(), reading a new git_http_version srtslurm.yaml setting (e.g. "HTTP/1.1"), and wire it into every git clone/fetch srtctl performs on the real sbatch path: the dynamo-from-source install scripts (core/schema.py, the highest-impact site -- used by any recipe with dynamo.install: true) and the srtlog post-processing clone (cli/mixins/postprocess_stage.py). --bash dev-mode (render/direct_stages/) can't read srtslurm.yaml (deliberately stdlib-only, no srtctl.core imports) -- applies the same http.version=HTTP/1.1 guard unconditionally instead, rather than leaving it unfixed. Default (git_http_version unset) is unchanged: plain `git`, no behavior change for clusters that don't need this. * Document git_http_version in srtslurm.yaml.example
ishandhanani
added a commit
that referenced
this pull request
Sep 3, 2026
…eclared srtslurm.yaml is schema-validated (ClusterConfig), and an unknown top-level key fails that validation and silently drops every cluster default for the whole config, not just the unrecognized key -- confirmed live on NVIDIA/InferenceMAX PR #271: "Failed to load or validate srtslurm.yaml: {'git_http_version': ['Unknown field.']}", which also broke model_paths resolution as collateral damage (FileNotFoundError on a literal "deepseek-v4-pro" path instead of the aliased /scratch/... path). get_srtslurm_setting() reads from the raw dict and never required the key to be declared on ClusterConfig, so this shipped without failing locally. Same lesson TestHostSetup.test_cluster_schema_accepts_the_key already captured for an earlier field -- this needed the same test.
2 tasks
ishandhanani
added a commit
that referenced
this pull request
Sep 3, 2026
…eclared) (#377) #376 added git_clone_command_prefix() reading git_http_version via get_srtslurm_setting(), but never declared the key on ClusterConfig. srtslurm.yaml is schema-validated, and an unknown top-level key fails that validation and silently drops every cluster default for the whole config, not just the unrecognized key -- confirmed live on NVIDIA/InferenceMAX PR #271: "Failed to load or validate srtslurm.yaml: {'git_http_version': ['Unknown field.']}", which also broke model_paths resolution as collateral damage (FileNotFoundError on a literal "deepseek-v4-pro" path instead of the aliased /scratch/... path). Same lesson TestHostSetup.test_cluster_schema_accepts_the_key already captured for an earlier field -- #376 needed the same test and didn't have it.
ishandhanani
added a commit
that referenced
this pull request
Sep 3, 2026
…ost path Root cause of "auxiliary_services[...] build_command failed" / cd: No such file or directory, 100% reproducible on NVIDIA/InferenceMAX PR #271 -- not the NFS visibility race the earlier commit (poll before cd) assumed. runtime.container_mounts binds log_dir at /logs inside every container (RuntimeContext.from_config); the clone/checkout step runs on the bare host (container_image=None) and can use the real host path, but build/launch run inside the service's container, where that host path doesn't exist at all -- only /logs/... does. Every previous run's "clone succeeded, then cd failed" was this, not a timing issue. Add _container_path() to translate work_dir (host-side) to its in-container path before cd'ing in build/launch. Keep the poll loop as a harmless second line of defense, but it was never the actual fix.
ishandhanani
added a commit
that referenced
this pull request
Sep 3, 2026
… 600s Confirmed live on InferenceMAX PR #271 (job 10859, thunderagent-router source clone against ai-dynamo/dynamo): clone succeeded but the working-tree checkout was killed by the 120s timeout at 82% (4583/5588 files), exit 124. Not a network stall like the HTTP/2 corruption case -- writing thousands of small files to the /logs bind mount is just slow. 600s gives real headroom.
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.
Summary
prefill_dp_launch_mode,decode_dp_launch_mode, andaggregated_dp_launch_modeoverrides for the vLLM backend.Dependency
This is a stacked follow-up to #90. PR #90 now contains only the global opt-in
dp_launch_mode: per_nodeimplementation; this PR's intended review diff is the top commit,feat(vllm): support per-role DP launch modes.Until #90 merges, GitHub will also show its prerequisite commit in this PR. After #90 lands, this branch will be rebased onto
mainso only the mixed-DP changes remain.Motivation
Some vLLM roles need one process per node so node-local DP ranks share a CUDA namespace, while another role may need independent per-GPU Dynamo workers. The override is process-topology flexibility; it does not claim either layout has an inherent request-ingest performance advantage.
Unset role overrides inherit the global
dp_launch_mode.Validation
make check867 passed, 2 skipped, 6 deselected.