build_orchestrator: merge model-card slurm/distributed config in the main build path#152
Draft
i-kosarev wants to merge 1 commit into
Conversation
i-kosarev
requested review from
Cemberk,
Rohan138,
coketaste,
gargrahul and
leconcio
as code owners
July 8, 2026 16:30
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent runtime GPU auto-detection from failing on SLURM login nodes (where no local GPU is present) by reusing GPU vendor information already recorded in the build manifest, allowing runs to proceed far enough to be dispatched to GPU compute nodes. It also reduces the risk of node-info commands hanging by bounding package-manager queries.
Changes:
- Pre-load
gpu_vendor(and related GPU docker env vars) from the build manifest intoadditional_contextbefore runtime context initialization. - Add a 10-second timeout to host package-manager queries in
_show_node_info().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+532
to
+553
| manifest_context = manifest.get("context", {}) | ||
| if not self.additional_context: | ||
| self.additional_context = {} | ||
| if "gpu_vendor" in manifest_context and "gpu_vendor" not in self.additional_context: | ||
| vendor = manifest_context["gpu_vendor"] | ||
| self.additional_context["gpu_vendor"] = vendor | ||
| # Pre-populate GPU docker_env_vars so init_gpu_context() skips the per-field | ||
| # detection calls (get_system_ngpus etc.) that fail on GPU-less login nodes. | ||
| # Use -1 / empty placeholders; the actual values are set inside the container. | ||
| if "docker_env_vars" not in self.additional_context: | ||
| self.additional_context["docker_env_vars"] = {} | ||
| denv = self.additional_context["docker_env_vars"] | ||
| if "MAD_GPU_VENDOR" not in denv: | ||
| denv["MAD_GPU_VENDOR"] = vendor | ||
| if "MAD_SYSTEM_NGPUS" not in denv: | ||
| denv["MAD_SYSTEM_NGPUS"] = -1 | ||
| if "MAD_SYSTEM_GPU_ARCHITECTURE" not in denv: | ||
| denv["MAD_SYSTEM_GPU_ARCHITECTURE"] = manifest_context.get("gpu_arch", "") | ||
| if "MAD_SYSTEM_HIP_VERSION" not in denv: | ||
| denv["MAD_SYSTEM_HIP_VERSION"] = "" | ||
| if "MAD_SYSTEM_GPU_PRODUCT_NAME" not in denv: | ||
| denv["MAD_SYSTEM_GPU_PRODUCT_NAME"] = "" |
Comment on lines
795
to
+802
| if "HOST_UBUNTU" in host_os: | ||
| print(self.console.sh("apt show rocm-libs -a", canFail=True)) | ||
| print(self.console.sh("timeout 10 apt show rocm-libs -a", canFail=True)) | ||
| elif "HOST_CENTOS" in host_os: | ||
| print(self.console.sh("yum info rocm-libs", canFail=True)) | ||
| print(self.console.sh("timeout 10 yum info rocm-libs", canFail=True)) | ||
| elif "HOST_SLES" in host_os: | ||
| print(self.console.sh("zypper info rocm-libs", canFail=True)) | ||
| print(self.console.sh("timeout 10 zypper info rocm-libs", canFail=True)) | ||
| elif "HOST_AZURE" in host_os: | ||
| print(self.console.sh("tdnf info rocm-libs", canFail=True)) | ||
| print(self.console.sh("timeout 10 tdnf info rocm-libs", canFail=True)) |
Comment on lines
+527
to
+531
| # Pre-load gpu_vendor from manifest context into additional_context so that | ||
| # _init_runtime_context() can skip local GPU detection on nodes (e.g. SLURM | ||
| # login nodes) where the GPU is on remote compute nodes, not the local host. | ||
| # The build phase records the target GPU vendor; re-use it here instead of | ||
| # trying to detect a GPU that does not exist on the login node. |
…main build path too _execute_with_prebuilt_image() already merged the first discovered model's "slurm"/"distributed" fields into manifest deployment_config, letting the run phase auto-detect target=slurm from the model card alone. The normal build-from-Dockerfile path (the one actually used when building images from source, e.g. any CI run that isn't --use-image) never did this merge -- it only called _save_deployment_config(), which populates deployment_config purely from --additional-context. A model card with a real "slurm" config but no matching --additional-context override was silently ignored: deployment_config.slurm stayed unset, _infer_deployment_target() fell back to "local", and on GPU-less nodes (e.g. a SLURM login node used only to submit jobs) this caused "GPU detection failed: Unable to determine gpu vendor" instead of dispatching via sbatch. Extract the merge logic into _merge_model_config_into_deployment() and call it from both build paths right after _save_deployment_config().
i-kosarev
force-pushed
the
fix/slurm-sglang-disagg-co-located-proxy
branch
from
July 22, 2026 13:47
32142bf to
183b679
Compare
i-kosarev
changed the title
i-kosarev
marked this pull request as draft
July 22, 2026 15:42
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.
Problem
_execute_with_prebuilt_image()already merged the first discovered model'sslurm/distributedfields into manifestdeployment_config, letting the run phase auto-detecttarget=slurmfrom the model card alone. The normal build-from-Dockerfile path (used whenever images are actually built from source, not--use-image) never did this merge -- it only called_save_deployment_config(), which populatesdeployment_configpurely from--additional-context.A model card with a real
"slurm": {...}config but no matching--additional-contextoverride was silently ignored:deployment_config.slurmstayed unset,_infer_deployment_target()fell back to"local", and on GPU-less nodes (e.g. a SLURM login node used only to submit jobs) this causedGPU detection failed: Unable to determine gpu vendorinstead of dispatching viasbatch.Fix
Extract the merge logic into
_merge_model_config_into_deployment()and call it from both build paths right after_save_deployment_config().(Supersedes the earlier version of this PR, which worked around the symptom by pre-loading
gpu_vendorbefore local GPU detection. That workaround is no longer needed oncedeployment_config.slurmis populated correctly -- target inference routes toSlurmDeploymentbefore local GPU detection ever runs.)