fix: synchronize self-benchmark capacity across DP ranks - #12021
Conversation
|
👋 Hi liyuanzhe1991! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
This comment has been minimized.
This comment has been minimized.
WalkthroughChangesBenchmark capacity coordination
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/src/dynamo/vllm/tests/test_vllm_instrumented_scheduler.py (1)
552-699: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
@pytest.mark.timeout(...)to the threaded synchronizer tests.These three tests spawn a follower thread and rely on ZMQ socket polling/waits between ranks. If coordination ever wedges,
follower.join(timeout=2)returns while the socket-blocked thread lingers; a bounded per-test timeout keeps CI from hanging on regressions. As per path instructions: "Add@pytest.mark.timeout(...)for tests that may exceed 30s or contain polling/sleeps/waits."🤖 Prompt for 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. In `@components/src/dynamo/vllm/tests/test_vllm_instrumented_scheduler.py` around lines 552 - 699, Add an appropriate `@pytest.mark.timeout`(...) decorator to each threaded synchronizer test: test_benchmark_synchronizer_negotiates_minimum_capacity_and_grid, test_benchmark_synchronizer_rejects_capacity_invariant_mismatch, and test_benchmark_synchronizer_rejects_grid_mismatch_before_warmup. Keep the existing thread joins and test behavior unchanged.Source: Path instructions
🤖 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.
Nitpick comments:
In `@components/src/dynamo/vllm/tests/test_vllm_instrumented_scheduler.py`:
- Around line 552-699: Add an appropriate `@pytest.mark.timeout`(...) decorator to
each threaded synchronizer test:
test_benchmark_synchronizer_negotiates_minimum_capacity_and_grid,
test_benchmark_synchronizer_rejects_capacity_invariant_mismatch, and
test_benchmark_synchronizer_rejects_grid_mismatch_before_warmup. Keep the
existing thread joins and test behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 75d54ebb-7b81-43c1-b341-42f7548b71af
📒 Files selected for processing (2)
components/src/dynamo/vllm/instrumented_scheduler.pycomponents/src/dynamo/vllm/tests/test_vllm_instrumented_scheduler.py
tedzhouhk
left a comment
There was a problem hiding this comment.
Three blocking findings from the agentic review are noted inline.
e7d5768 to
79717ca
Compare
|
/ok to test 79717ca |
|
/ok to test 077a2c5 |
077a2c5 to
1f16d7e
Compare
|
/ok to test 1f16d7e |
Signed-off-by: YZLi <yuanli@nvidia.com>
- Hash the unfiltered cudagraph capture configuration in the grid invariants digest and re-filter decode capture sizes with the negotiated max_num_running_reqs, so ranks that differ only in request limits negotiate the minimum instead of failing the invariant check before negotiation starts. - Use the negotiated max_num_scheduled_tokens when materializing explicit prefill points so cudagraph sample_reasons (engine_limit vs geometric_tail) and per-point digests match across ranks. - Drop the unused capacity locals in _bench_generate_decode_grid (ruff F841). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: YZLi <yuanli@nvidia.com>
_bench_build_grid now negotiates capacity and re-filters the decode capture list on entry, so every stub that reaches it needs the preflight installed and a _bench_decode_capture_sizes attribute. The scheduled-token-limit test moves its constraint into the negotiated envelope, which is where _bench_capacity_limit reads it from now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: YZLi <yuanli@nvidia.com>
mypy: asdict() rejects _BenchmarkCapacityEnvelope | None; bind the attribute to a local via walrus so the None-check narrows it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: YZLi <yuanli@nvidia.com>
…ization Steady-coordinate normalization (ai-dynamo#12358) merges each batch's sub-2B decode presets into a single point, shrinking the synthetic negotiation grid from 1368 to 1266 points and moving batch=1's feasibility boundary from index 19 to 18. Signed-off-by: YZLi <yuanli@nvidia.com>
1f16d7e to
bc00233
Compare
|
/ok to test bc00233 |
tedzhouhk
left a comment
There was a problem hiding this comment.
Verified the three previously raised P1 concerns against the current head: decode capture invariants are negotiated correctly, explicit prefill metadata uses the common limit, and the unused locals are removed. The added regression coverage matches the fixes.
dyn-3691-extract-shared-target-pid-cuda-customstorage-operation-layer * 'main' of https://github.com/ai-dynamo/dynamo: (65 commits) fix(frontend): emit SGLang stream role once (#12741) docs(fern): promote v1.3.1 to current release (#12752) fix(docs): remove duplicate unscoped community-rail CSS rules (#12615) feat(operator): migrate CRD storage to v1beta1 (#11904) fix: synchronize self-benchmark capacity across DP ranks (#12021) chore(deps): bump dynamo-tokenizers to 1.8.0 (#12707) fix(frontend): preserve split UTF-8 characters (#12688) docs: align Kubernetes build selector with CLI (#12729) fix(frontend): preserve completion backend error status (#12706) fix(operator): replace snapshot pods after GMS restart (#11286) refactor(media): rename installer module, drop --packages per review fix(media): harden installer against three pre-redesign review findings fix(media): verify installs in a fresh interpreter; teach --pip-args= form test(serve): install test-time decoders at the validated bounds feat(media): explicit installer for additional media decoders docs(spica): correct kv_load_ratio support guidance (#12714) feat(operator): add experimental grove.forceScalingGroup for single-node components (#11772) fix(vllm): declare entry-stage engine_input_source in GLM-Image NIXL config (#12709) chore: bump trtllm to v1.3.0rc23 (#12532) perf: remove trtllm postprocessing workers from the args as post processing workers are not effective in dynamo (#12592) ... Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Overview:
Self-benchmarking with attention-DP > 1 intermittently deadlocks at startup: each DP rank probes its local KV-cache capacity independently, and any run-to-run variation (block-pool rounding, allocator state) makes ranks generate different benchmark grids. The lockstep synchronization protocol then waits forever on points that only a subset of ranks generated. In our GLM-5.2-NVFP4 / 4×B200 campaigns, decode benchmarks with dp=4 failed 3 out of 4 attempts before this fix.
Details:
_bench_make_local_capacity: max_model_len, max_num_running_reqs, max_num_scheduled_tokens, usable KV blocks) and exchanges it through the existing benchmark synchronizer before grid generation._bench_capacity_limit()/_bench_grid_usable_blocks(), which return the minimum across ranks — every rank derives byte-identical grids by construction._bench_decode_feasible_max_batch_size()now computes from the synchronized envelope, so both call sites (the native decode grid and explicit benchmark points) agree across ranks.Validated on 4-rank and 8-rank attention-DP topologies (GLM-5.2-NVFP4, B200, vLLM 0.25.1): previously flaky decode cells pass deterministically; full prefill+decode campaigns (5 466 and 6 508 grid points) completed with zero lockstep timeouts.
Where should the reviewer start?
components/src/dynamo/vllm/instrumented_scheduler.py:_BenchmarkCapacityEnvelope,_bench_make_local_capacity,_bench_capacity_limit,_bench_grid_usable_blocks_BenchmarkSynchronizerhandshake_bench_decode_feasible_max_batch_size(now envelope-based)components/src/dynamo/vllm/tests/test_vllm_instrumented_scheduler.py— capacity-divergence regression testsRelated Issues
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit
New Features
Bug Fixes
Tests