[https://nvbugs/6510284][fix] Cap gen-only benchmark queue size - #16915
Conversation
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast --stage-list "GB300-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN4-NODE2-GPU8-Post-Merge-1,GB300-40_GPUs-10_Nodes-PyTorch-Disagg-PerfSanity-CTX6-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-1,GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-1,GB300-56_GPUs-14_Nodes-PyTorch-Disagg-PerfSanity-CTX12-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1" |
|
PR_Github #62008 [ run ] triggered by Bot. Commit: |
WalkthroughBoth performance submission scripts now cap GEN-only request queue sizes to GEN executor capacity and use the result in generated environment variables. New pytest coverage validates both implementations against YAML and in-memory configurations. ChangesGEN-only queue capacity
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/scripts/test_perf_submit.py (1)
59-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover generated environment wiring and warnings.
These tests validate the helper only; a regression in either submitter’s
GEN_WORKER_ENV_VARSwiring or capped-request warning would still pass. Add output-level assertions for both generated launch scripts.Test coverage summary: added
test_gen_only_queue_size_does_not_exceed_executor_capacityandtest_gen_only_queue_size_preserves_reachable_concurrency;tests/integration/test_lists/entries are not applicable to this unit-test module; verdict: insufficient until generated environment output is covered. As per path instructions, test-code changes must include a coverage verdict and test-list applicability review.🤖 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 `@tests/unittest/scripts/test_perf_submit.py` around lines 59 - 83, Extend the tests around both submitters’ generated launch scripts to assert the generated output includes the expected GEN_WORKER_ENV_VARS wiring and emits the capped-request warning when applicable. Keep the existing helper tests, add output-level coverage for each submitter, and include the required coverage verdict plus test-list applicability review for this unit-test change.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 `@tests/unittest/scripts/test_perf_submit.py`:
- Around line 59-83: Extend the tests around both submitters’ generated launch
scripts to assert the generated output includes the expected GEN_WORKER_ENV_VARS
wiring and emits the capped-request warning when applicable. Keep the existing
helper tests, add output-level coverage for each submitter, and include the
required coverage verdict plus test-list applicability review for this unit-test
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3008e93f-f52c-4fb1-9113-c1ce33c457ce
📒 Files selected for processing (3)
jenkins/scripts/perf/local/submit.pyjenkins/scripts/perf/submit.pytests/unittest/scripts/test_perf_submit.py
|
PR_Github #62008 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #62048 [ run ] triggered by Bot. Commit: |
|
PR_Github #62048 [ run ] completed with state
|
|
PR_Github #62156 [ skip ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast --stage-list "GB300-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN4-NODE2-GPU8-Post-Merge-2,GB300-40_GPUs-10_Nodes-PyTorch-Disagg-PerfSanity-CTX6-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-2,GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2,GB300-56_GPUs-14_Nodes-PyTorch-Disagg-PerfSanity-CTX12-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-2" |
|
PR_Github #62156 [ skip ] completed with state |
|
PR_Github #62159 [ ] completed with state |
What
TLLM_BENCHMARK_REQ_QUEUES_SIZEto the GEN executor's maximum active-request capacity in both Jenkins CI and local SLURM submitters.max_batch_size * tensor_parallel_sizefor attention-DP GEN workers andmax_batch_sizeotherwise.Why
TLLM_BENCHMARK_REQ_QUEUES_SIZEis used here as a gen-only benchmark fill target. The benchmark fill gate withholds the first decode forward until the executor has fetched and admitted at least that many requests.Fetched requests occupy the executor's active-request capacity:
max_batch_size * tensor_parallel_sizewith attention DP, ormax_batch_sizeotherwise. If the fill target is above that capacity, the executor needs a forward pass to complete requests and free slots, but the gate prohibits that forward until the unreachable target is met. This creates theiter=0livelock.This invariant applies only to the disaggregated gen-only benchmark path when
TLLM_BENCHMARK_REQ_QUEUES_SIZEenables the fill gate. Regular production serving does not enable this gate; its pending queue may safely exceed active capacity and is drained incrementally as requests complete. Thectx_onlyande2ebenchmark modes are also unaffected.For the four NVBUG 6510284 cases, this changes the unreachable targets as follows:
Timeline
concurrencyassignment appeared in [None] [chore] Update wide-ep genonly scripts #6995 and entered the Jenkins multi-node disaggregated path in [TRTLLM-8952][feat] Support Multi-Node Disagg Perf Test in CI #9138.gen_onlydisabled; [None][test] Enable gen_only + ctx_only DeepSeek-V4-Pro perf-sanity on GB300 #16795 enabled those tests and exposed the latent capacity mismatch.Recommended general fix
This PR intentionally keeps the implementation scoped to restoring the four blocked NVBUG 6510284 tests. The submitters derive a reachable fill target from the configuration values available before launch.
As a follow-up, the safety clamp should move into
PyExecutor, after the runtime has derived its authoritative admission ceiling, including attention-DP and other runtime constraints. Launchers would continue to pass the requested benchmark concurrency unchanged, while the executor would use an effective target such as:The executor should log both requested and effective targets when it clamps. This would provide one authoritative implementation for Jenkins, local SLURM, examples, and custom launchers. A fully general multi-GEN solution may also need an explicit per-worker benchmark-round target or end-of-burst signal, because global concurrency does not guarantee equal request distribution across GEN workers.
That broader cleanup is intentionally deferred so this PR can first close the immediate test gap.
Verification
SUCCESSon commitb93ef24(4 passed, 0 failed, 0 skipped); all four selected GB300 stages and Slurm jobs completed successfully.Post-Merge-1shards exercised the four correspondingdisagg_upload-e2ecases. Because these stage definitions use two pytest splits, the affecteddisagg_upload-gen_onlycases are assigned to thePost-Merge-2shards and were not executed by this targeted run.gen_onlycases remains pending the shard-2 results.Dev Engineer Review
TLLM_BENCHMARK_REQ_QUEUES_SIZEin Jenkins and local SLURM GEN-only submitters.max_batch_size * tensor_parallel_sizefor attention-DP workers andmax_batch_sizeotherwise.iter=0livelock without affecting production,ctx_only, ore2epaths.QA Engineer Review
test_gen_only_queue_size_does_not_exceed_executor_capacitytest_gen_only_queue_size_preserves_reachable_concurrencysubmit.pyimplementations and validate configured disaggregated benchmark scenarios.tests/integration/test_lists/(test-db/orqa/).