Conversation
… sites PR NVIDIA#830 threaded exclude_nodes through the cached node-allocation path (get_cached_nodes_spec), but six other call sites still call get_nodes_by_spec without it: nemo_run, nemo_launcher, triton_inference and ai_dynamo's own cached-spec override, the shared _enable_vboost_cmd, and both call sites in SingleSbatchRunner (extract_sbatch_nodes_spec, get_single_tr_block). A user's exclude_nodes setting is silently ignored on any of those paths. Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughSlurm node selection now applies configured exclusions in batch runners and workload command strategies. Tests verify filtered allocations, generated commands, and forwarded exclusion arguments. ChangesSlurm node exclusion propagation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Configured Slurm node exclusions are applied across the affected allocation and command-generation paths. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
1 participant
Fixes #832
Wired
exclude_nodesthrough the six call sites you listed, same pattern as #830: parsetest_run.exclude_nodesand pass it asexclude_nodes=...intoget_nodes_by_spec.single_sbatch_runner.py'sextract_sbatch_nodes_specunionsexclude_nodesacross all test runs in the batch before calling it once.One correction to the issue:
deepep/slurm_command_gen_strategy.pyno longer needs a change. It routes throughget_cached_nodes_spec()now (both call sites), which already forwardsexclude_nodessince #830, so it's not one of the remaining gaps.Added a regression test per call site asserting the mock forwards
exclude_nodes(or, where the node names are literal, that the excluded node is actually missing from the result). The new test at each site is red without the fix. Full suite, coverage, ruff, pyright and pre-commit are clean; thetest_base_installer/test_test_definitionsfailures are the same root-in-container filesystem permission ones main already has.