[https://nvbugs/6108994][fix] add kv_transfer_timeout_ms to avoid timeout#15152
Conversation
|
/bot run |
📝 WalkthroughWalkthroughFour disaggregated benchmark YAML configuration files receive identical updates: ChangesKV Transfer Timeout Configuration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
tests/scripts/perf-sanity/disaggregated/gb300_deepseek-v32-fp4_32k4k_con256_ctx1_dep8_gen1_dep8_eplb0_mtp0_ccb-NIXL.yaml (1)
70-73: 💤 Low valueConsider adding a comment explaining the large timeout value.
The 600-second timeout is a 10x increase from the default 60 seconds. Given the long input sequences (32k tokens), large output (4k tokens), and high concurrency (256), this is reasonable, but an inline comment would help future maintainers understand why such a large value is necessary for this benchmark scenario.
📝 Suggested comment
cache_transceiver_config: max_tokens_in_buffer: 120000 backend: NIXL + # 600s timeout (10x default) needed for 32k4k long-context disaggregated transfers at high concurrency kv_transfer_timeout_ms: 600000Apply the same comment pattern to the ctx section at line 101.
🤖 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/scripts/perf-sanity/disaggregated/gb300_deepseek-v32-fp4_32k4k_con256_ctx1_dep8_gen1_dep8_eplb0_mtp0_ccb-NIXL.yaml` around lines 70 - 73, Add an inline comment next to the cache_transceiver_config kv_transfer_timeout_ms: 600000 setting explaining the rationale for the large 600s timeout (long 32k token inputs, 4k token outputs, high concurrency 256 and heavy transfer latencies in this benchmark) so future maintainers understand it's intentional; also add the same explanatory comment pattern to the ctx section's timeout/transfer-related field (the ctx block referenced in the diff) to keep both places consistent.
🤖 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
`@tests/scripts/perf/disaggregated/gb300_deepseek-v32-fp4_32k4k_con256_ctx1_dep8_gen1_dep8_eplb0_mtp0_ccb-NIXL.yaml`:
- Around line 70-73: The disaggregated configs are inconsistent: only some
include cache_transceiver_config.kv_transfer_timeout_ms while others (DeepSeek
long-context variants using NIXL/UCX) omit it, which may trigger nvbugs/6108994;
update every disaggregated DeepSeek long-context NIXL/UCX YAML that defines
cache_transceiver_config to add kv_transfer_timeout_ms: 600000 (i.e., ensure the
cache_transceiver_config.kv_transfer_timeout_ms key is present and set to 600000
across those variants), or alternatively add a short comment/doc explaining why
a higher timeout is not needed for specific files if you deliberately omit it.
---
Nitpick comments:
In
`@tests/scripts/perf-sanity/disaggregated/gb300_deepseek-v32-fp4_32k4k_con256_ctx1_dep8_gen1_dep8_eplb0_mtp0_ccb-NIXL.yaml`:
- Around line 70-73: Add an inline comment next to the cache_transceiver_config
kv_transfer_timeout_ms: 600000 setting explaining the rationale for the large
600s timeout (long 32k token inputs, 4k token outputs, high concurrency 256 and
heavy transfer latencies in this benchmark) so future maintainers understand
it's intentional; also add the same explanatory comment pattern to the ctx
section's timeout/transfer-related field (the ctx block referenced in the diff)
to keep both places consistent.
🪄 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: CHILL
Plan: Enterprise
Run ID: 83089d61-58f9-43c4-8fd6-07e114892a4f
📒 Files selected for processing (4)
tests/scripts/perf-sanity/disaggregated/gb300_deepseek-v32-fp4_32k4k_con256_ctx1_dep8_gen1_dep8_eplb0_mtp0_ccb-NIXL.yamltests/scripts/perf/disaggregated/gb300_deepseek-r1-fp4_128k8k_con256_ctx1_pp4_gen1_dep8_eplb0_mtp1_ccb-NIXL.yamltests/scripts/perf/disaggregated/gb300_deepseek-r1-fp4_128k8k_con256_ctx1_pp4_gen1_dep8_eplb0_mtp1_ccb-UCX.yamltests/scripts/perf/disaggregated/gb300_deepseek-v32-fp4_32k4k_con256_ctx1_dep8_gen1_dep8_eplb0_mtp0_ccb-NIXL.yaml
|
PR_Github #53024 [ run ] triggered by Bot. Commit: |
|
PR_Github #53024 [ run ] completed with state
|
|
/bot run |
|
PR_Github #53078 [ run ] triggered by Bot. Commit: |
|
PR_Github #53078 [ run ] completed with state
|
|
/bot run |
|
PR_Github #53128 [ run ] triggered by Bot. Commit: |
|
PR_Github #53128 [ run ] completed with state
|
|
/bot run |
|
PR_Github #53160 [ run ] triggered by Bot. Commit: |
|
PR_Github #53160 [ run ] completed with state
|
|
/bot run |
|
PR_Github #53210 [ run ] triggered by Bot. Commit: |
|
PR_Github #53210 [ run ] completed with state
|
|
/bot run --disable-fail-fast --stage-list "GB200-24_GPUs-6_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE2-GPU8-GEN1-NODE4-GPU16-Post-Merge-1,GB200-24_GPUs-6_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE2-GPU8-GEN1-NODE4-GPU16-Post-Merge-2,GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-4" |
|
PR_Github #53278 [ run ] triggered by Bot. Commit: |
|
PR_Github #53278 [ run ] completed with state
|
…eout Signed-off-by: Bo Deng <deemod@nvidia.com>
|
/bot help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
|
/bot skip --comment "just unwaive tests, CI https://prod.blsm.nvidia.com/sw-tensorrt-top-1/job/LLM/job/main/job/L0_MergeRequest_PR/42468/" |
|
PR_Github #53414 [ skip ] triggered by Bot. Commit: |
|
PR_Github #53414 [ skip ] completed with state |
3 participants
Summary by CodeRabbit
Release Notes
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.