fix(validator): fail fast on degraded GPU node; dedicated model-cache timeout#1866
Conversation
… timeout Two independent performance-validator fixes. nccl-all-reduce-bw (NVIDIA#1858): determineGPUConfig sized whole-node workers from the first target node's GPU count, so a node short of its peers — a degraded GPU or driver fault, e.g. an H100 node enumerating 7 of 8 GPUs — left one worker Pending and the launcher eventually failed with an opaque "pod failed". Add uniformGPUCountPerNode: it logs a per-node GPU census on every call and fails fast, naming the short node(s) and their count, when a target node is below its peers. A uniformly low count is not flagged (a coherent, if unusual, topology). The diagnostic names both the transient device-plugin-rollout cause and a genuine hardware fault. Mirrors the existing uniformFabricResourceCount fail-closed pattern. inference-perf (NVIDIA#1859): the one-time model-cache populate Job reused the 10m DynamoGraphDeployment workload-ready budget, so a cold image pull plus a first-ever multi-GB Hugging Face download shared one deadline and flaked on slow/throttled nights. Give it a dedicated ModelCachePopulateTimeout (13m, env-overridable via AICR_INFERENCE_PERF_MODEL_CACHE_POPULATE_TIMEOUT), sized to keep the inference sequential worst case under CheckExecutionTimeout. The optional HF-token secret (already wired into the populate Job) removes the anonymous-download throttling; the timeout error now surfaces both remedies in-band. The new knob is validated up front in validatePerfTuningEnvs alongside its siblings, and its decoupling from the workload-ready budget is test-guarded. Fixes NVIDIA#1858 Fixes NVIDIA#1859 Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
njhensley
added
the
theme/validation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe inference validator now uses a dedicated, configurable timeout for the one-time model-cache populate Job, separate from workload readiness, with documentation, validation, and tests. The NCCL bandwidth validator inventories allocatable GPUs across all target nodes, logs the census, and fails fast with diagnostics when counts are non-uniform. Tests cover timeout resolution, invalid configuration, uniform GPU counts, degraded nodes, and edge cases. Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@validators/performance/model_cache_test.go`:
- Around line 213-243: Convert TestPopulateJobTimeout in
validators/performance/model_cache_test.go (lines 213-243) into a table-driven
test with per-case environment setup covering the default, workload-ready
isolation, and dedicated override scenarios. Also update the validation cases in
validators/performance/inference_perf_test.go (lines 1171-1181) to fold the
malformed populate-timeout case into the existing table-driven list; no other
behavior changes are needed.
In `@validators/performance/nccl_all_reduce_bw_constraint.go`:
- Around line 728-733: Make the GPU census cancellable: in
validators/performance/nccl_all_reduce_bw_constraint.go lines 728-733, pass
ctx.Ctx from the validator into uniformGPUCountPerNode; in lines 764-803, accept
the context, check ctx.Done() during each node-scan loop, and return a correctly
coded wrapped cancellation error; in
validators/performance/nccl_gpu_census_test.go lines 94-112, add a
canceled-context test and check context cancellation while iterating the table
cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 80a49104-244e-4bf2-8ac8-b80f7131db9f
📒 Files selected for processing (9)
docs/contributor/validator.mdpkg/defaults/timeouts.gopkg/defaults/timeouts_test.govalidators/performance/inference_perf_constraint.govalidators/performance/inference_perf_test.govalidators/performance/model_cache.govalidators/performance/model_cache_test.govalidators/performance/nccl_all_reduce_bw_constraint.govalidators/performance/nccl_gpu_census_test.go
Address CodeRabbit review on NVIDIA#1866: convert the three populate-timeout scenarios (default, workload-ready isolation, dedicated override) into a table-driven test per the repo's test conventions, and restore the TestEnsureModelCache_DisabledNoop doc comment to its function. Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
The core GPU-census and timeout-decoupling changes look sound. One non-blocking error-propagation issue remains: the newly added timeout remediation is currently discarded. Nothing here blocks merge.
| // triaging a red run won't be reading the env-var reference): the | ||
| // download is likely throttled anonymously — provide the HF-token secret | ||
| // — or the model is large enough to need a bigger budget. | ||
| return errors.PropagateOrWrap(werr, errors.ErrCodeInternal, |
There was a problem hiding this comment.
[P2] Preserve the timeout remediation when propagating coded errors
WaitForJobCompletion returns a structured ErrCodeTimeout, and PropagateOrWrap returns any structured error unchanged. This formatted remediation is therefore discarded on the timeout path it is meant to improve. Return a timeout-coded error that carries this message while propagating other coded failures unchanged, and add a test asserting the timeout text.
There was a problem hiding this comment.
Good catch — fixed in f189a68. Extracted wrapPopulateJobError: on the timeout path (stderrors.Is(werr, ErrCodeTimeout)) it now returns a fresh ErrCodeTimeout that carries the remediation and wraps the original error, so the message survives on exactly the path it targets; other coded failures (e.g. ErrCodeUnavailable when the watch closes) still propagate unchanged. Added TestWrapPopulateJobError asserting the timeout text is present and that non-timeout errors are propagated without it.
…path Address @mchmarny review on NVIDIA#1866: WaitForJobCompletion returns a structured ErrCodeTimeout and errors.PropagateOrWrap keeps coded errors unchanged, so the actionable remediation (provide HF-token / raise the budget) was discarded on the very timeout path it targets. Extract wrapPopulateJobError: on a timeout it returns a fresh ErrCodeTimeout carrying the remedy and wrapping the original; other coded failures propagate unchanged. Add TestWrapPopulateJobError. Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@validators/performance/model_cache_test.go`:
- Around line 266-289: The two independent error scenarios in the test should be
consolidated into a table-driven test. Replace the separate subtests around
wrapPopulateJobError with a []struct containing each input error and expected
assertions, then iterate with t.Run while preserving the timeout
remediation/code checks and the unavailable propagation/no-remediation checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e6b921c9-0d62-443d-926a-8a819bb9f7e1
📒 Files selected for processing (2)
validators/performance/model_cache.govalidators/performance/model_cache_test.go
Consolidate the two independent error-outcome subtests (timeout carries remediation and stays timeout-coded; non-timeout coded error propagates unchanged) into a single []struct loop, matching the mandatory table-driven pattern used by the sibling parseModelCacheSize tests. Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@validators/performance/model_cache_test.go`:
- Around line 280-300: The non-timeout case does not verify that
wrapPopulateJobError returns the original error instance. Update the test case
and assertion around wrapPopulateJobError to retain the input error and assert
identity for the non-timeout coded error, while preserving the existing code,
text, and remediation checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 44d7206c-e531-473a-a829-99df9b009e38
📒 Files selected for processing (1)
validators/performance/model_cache_test.go
Add an identity assertion (got == input) to the non-timeout case of TestWrapPopulateJobError. The code+substring checks alone would still pass if wrapPopulateJobError re-wrapped a coded error in a fresh coded error; this pins the contract that PropagateOrWrap returns the exact instance unchanged. Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
2 participants
Summary
Two independent performance-validator fixes:
nccl-all-reduce-bwnow fails fast with a per-node GPU census when a target node is short of its peers (#1858), and theinference-perfmodel-cache populate Job gets its own timeout budget instead of sharing the 10m workload-ready deadline (#1859).Motivation / Context
determineGPUConfigsized whole-node NCCL workers from the first target node's GPU count. A node short of its peers — a degraded GPU / driver fault, e.g. an H100 node enumerating 7 of 8 GPUs — left one workerPendingand the launcher eventually failed with an opaque "pod failed", costing minutes and yielding no actionable signal.InferenceWorkloadReadyTimeout. A cold vLLM/cache image pull (~3.5m) plus a first-ever multi-GB anonymous Hugging Face download shared that single deadline and flaked at exactly 10m on slow/throttled nights.Fixes: #1858, #1859
Related: N/A
Type of Change
Component(s) Affected
pkg/validator)validators/performance(NCCL + inference-perf checks),pkg/defaults(timeout),docs/contributorImplementation Notes
uniformGPUCountPerNode(mirrors the existinguniformFabricResourceCountfail-closed pattern): logs a per-node GPU census on every call, and fails fast naming the short node(s) and their count vs the peer max. A uniformly low count is not flagged (a coherent, if unusual, topology; the census still surfaces it). The diagnostic names both the transient device-plugin-rollout cause ("re-run once uniform") and a genuine hardware fault (nvidia-smi -L/dmesgNVRM/Xid).defaults.ModelCachePopulateTimeout(13m), env-overridable viaAICR_INFERENCE_PERF_MODEL_CACHE_POPULATE_TIMEOUT, resolved through a namedpopulateJobTimeout()seam so the decoupling from the workload-ready budget stays test-guarded. Sized to keep the inference sequential worst case (5+13+10+5+5+15 = 53m) underCheckExecutionTimeout(55m). The optional HF-token secret is already wired into the populate Job; the timeout error now surfaces both remedies in-band, and the new knob is validated up front invalidatePerfTuningEnvsalongside its siblings.Testing
go test ./validators/performance/... ./pkg/defaults/... -race golangci-lint run -c .golangci.yaml ./validators/performance/... ./pkg/defaults/...-race;golangci-lint0 issues;gofmtclean.uniformGPUCountPerNodeandpopulateJobTimeoutat 100% coverage. Tests cover: uniform pass, single node, one-short (named), degraded-node-first (guards the "expected = max" invariant against a first-node regression), multiple-short (all named), uniformly-low (not flagged), empty, all-zero; the populate-timeout decoupling from workload-ready; and the up-front validation of the new knob.Risk Assessment
Rollout notes: Behavior change — a run against a genuinely non-uniform GPU cohort now hard-fails (previously it hung or ran a non-representative degraded benchmark). Operators who had widened cache-populate via
AICR_INFERENCE_PERF_WORKLOAD_READY_TIMEOUTmust switch to the new..._MODEL_CACHE_POPULATE_TIMEOUTknob (documented as a migration note). No breaking API/flag changes.Checklist
-race, affected packages)golangci-linton affected packages)docs/contributor/validator.md)git commit -S)