[https://nvbugs/6480621][test] Revert to 60-second KV transfer timeout for GB300 DeepSeek V4 Pro disaggregated perf-sanity - #17137
Conversation
|
/bot run --disable-fail-fast --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2" |
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2" |
|
PR_Github #63105 [ run ] triggered by Bot. Commit: |
|
PR_Github #63106 [ run ] triggered by Bot. Commit: |
|
PR_Github #63105 [ run ] completed with state |
chienchunhung
changed the title
|
PR_Github #63106 [ run ] completed with state |
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-1" |
|
PR_Github #63144 [ run ] triggered by Bot. Commit: |
|
PR_Github #63144 [ run ] completed with state
|
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-1" |
|
PR_Github #63150 [ run ] triggered by Bot. Commit: |
|
PR_Github #63150 [ run ] completed with state
|
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-1" |
|
PR_Github #63165 [ run ] triggered by Bot. Commit: |
|
PR_Github #63165 [ run ] completed with state
|
38ca970 to
3e6c120
Compare
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-1" |
|
PR_Github #63555 [ run ] triggered by Bot. Commit: |
|
PR_Github #63555 [ run ] completed with state
|
3e6c120 to
b7bd98a
Compare
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2" |
|
PR_Github #63564 [ run ] triggered by Bot. Commit: |
|
PR_Github #63564 [ run ] completed with state |
6e9981a to
538239c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
538239c to
d0a7c6e
Compare
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2" |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
chienchunhung
added
the
api-compatible
ChatGPT Codex Connector
|
PR_Github #65119 [ run ] triggered by Bot. Commit: |
![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
♻️ Duplicate comments (1)
tests/unittest/scripts/test_perf_submit.py (1)
302-306: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe malformed-payload branch is still uncovered.
Line 303 leaves the outer double quote unterminated.
shlex.splitfails on the whole line, so the error iscannot parse exported pytestCommandand the payload parser never runs. The new test at Line 332 has the same shape.To reach the payload branch, close the outer quote and leave the extracted payload unbalanced.
This repeats the finding from the previous review round. That thread was marked as fixed upstream, but this head still lacks the case.
💚 Proposed additional test for the payload branch
def test_extract_pytest_command_env_rejects_malformed_export(ci_submit_module: ModuleType): lines = ['export pytestCommand="LLM_ROOT=/src LLM_MODELS_ROOT=/models pytest'] with pytest.raises(ValueError, match="cannot parse exported pytestCommand"): ci_submit_module.extract_pytest_command_env(lines, "LLM_MODELS_ROOT") + + +def test_extract_pytest_command_env_rejects_malformed_payload(ci_submit_module: ModuleType): + # The outer export parses; the extracted payload has an unbalanced quote. + lines = ["export pytestCommand='LLM_ROOT=/src \"unterminated'"] + + with pytest.raises(ValueError, match="cannot parse pytestCommand payload"): + ci_submit_module.extract_pytest_command_env(lines, "LLM_MODELS_ROOT")🤖 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 302 - 306, Update test_extract_pytest_command_env_rejects_malformed_export and the corresponding test near line 332 so the outer export quote is closed while the extracted pytestCommand payload remains unbalanced. This ensures shlex.split succeeds and exercises the payload parser’s malformed-input ValueError branch rather than the whole-line parsing error.
🧹 Nitpick comments (5)
jenkins/scripts/perf/submit.py (1)
656-683: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing one pytestCommand parser.
_pytest_command_tokensat lines 92-109 already locates theexport pytestCommand=line, strips the outer quotes, and shell-splits the payload.extract_pytest_command_envrepeats that work with a different mechanism. One shared helper that returns the payload tokens would keep both call sites consistent if the Groovy emission format changes.🤖 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 `@jenkins/scripts/perf/submit.py` around lines 656 - 683, Refactor extract_pytest_command_env to reuse the existing _pytest_command_tokens helper for locating, unquoting, and splitting pytestCommand instead of duplicating that parsing logic. Preserve its leading environment-variable scan, name matching, and _PytestCommandEnvMissing behavior, while retaining appropriate errors from the shared parser.tests/unittest/others/test_cache_transceiver_precheck_config.py (1)
512-520: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExecute only the model-root export line.
Line 512 joins every generated prefix line into the script that bash runs. The assertion needs only the
export LLM_MODELS_ROOT=line. Ifprecheck_prefix_lineslater emits a line that invokes a command, this test would run it inside CI. Filter to the export line to keep the quoting assertion and remove that surface.♻️ Proposed narrowing
- script = "\n".join(lines) + '\nprintf "%s" "$LLM_MODELS_ROOT"\n' + export_line = next(line for line in lines if line.startswith("export LLM_MODELS_ROOT=")) + script = export_line + '\nprintf "%s" "$LLM_MODELS_ROOT"\n'🤖 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/others/test_cache_transceiver_precheck_config.py` around lines 512 - 520, Update the test’s script construction before subprocess.run to retain only the generated export LLM_MODELS_ROOT= line from precheck_prefix_lines, excluding all other prefix commands; preserve the existing bash execution and stdout assertion for validating its quoting.tests/unittest/disaggregated/test_transceiver_bounded_polling.py (2)
94-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing
_FakeTaskand_FakeClockwith the harness test.
tests/unittest/disaggregated/test_cache_transceiver_harness.pydefines awaitand amonotonichelper with the same bodies. Both modules are in the same directory, so aconftest.pyfixture or a small shared helper module would keep the two doubles from drifting asTaskStatushandling changes.🤖 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/disaggregated/test_transceiver_bounded_polling.py` around lines 94 - 125, Share the duplicated _FakeTask and _FakeClock test doubles between test_transceiver_bounded_polling.py and test_cache_transceiver_harness.py by moving them into a shared conftest.py fixture or helper module. Update both tests to import or use the shared implementations, preserving their existing wait, monotonic, timeout-tracking, and TaskStatus behavior.
151-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the session double from the real constructor.
_make_tx_sessionusesobject.__new__(TxSession)and then sets a hand-picked attribute subset. It omits_base_args,request_id,transfer_start_time, andtransfer_end_time, which the real__init__sets.disagg_request_idreadsself._base_args, so a futurewait_completeorclosechange that logs the request id would fail these tests withAttributeErrorinstead of a readable assertion.test_tx_session_first_send_anchors_deadline_onceat lines 663-696 already shows the real constructor works with mocks; reusing that path here would remove the drift risk.🤖 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/disaggregated/test_transceiver_bounded_polling.py` around lines 151 - 174, Update _make_tx_session to construct TxSession through its real __init__ path, using the same mock-based setup demonstrated by test_tx_session_first_send_anchors_deadline_once, instead of object.__new__ with manually assigned fields. Preserve the helper’s configurable kv_tasks, need_aux, aux_task, timeout_s, and deadline_monotonic_s behavior while ensuring constructor-initialized fields such as _base_args, request_id, transfer_start_time, and transfer_end_time are present.tests/unittest/disaggregated/test_cache_transceiver_harness.py (1)
445-651: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a ctx successful-release test
tests/unittest/disaggregated/test_cache_transceiver_harness.pyis registered intests/integration/test_lists/test-db/l0_h100.yml. Add a ctx success case that covers completion validation, the release handshake, release order, and exactly onefree_sequencecall.Coverage summary: The added tests cover generation ownership, timeout, dispatch failure, handshake validation, release safety, and abort handling. No tests were removed. All tests in this file are included through the CI test-list entry. Verdict: insufficient until the ctx success path is covered.
🤖 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/disaggregated/test_cache_transceiver_harness.py` around lines 445 - 651, Add a successful context-release test to TestRequestTransferOwnershipFlow that exercises completed-request validation and the release handshake, asserting the expected event ordering through free and exactly one free_sequence call with the context ownership arguments. Use the existing _run_ctx_request helper and established handshake/final-response conventions, while preserving the current failure and timeout coverage.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.
Inline comments:
In
`@examples/disaggregated/slurm/cache_transceiver_test/run_cache_transceiver_test.py`:
- Around line 793-797: Secure the ZMQ control channel in
run_cache_transceiver_test.py before any peer payload reaches the pickle.loads
calls in the leader message-handling paths. Reuse the sibling precheck’s
per-session key, 0600 address-file exchange, and pack_msg/unpack_msg framing so
both bind/connect setup and every send/receive path authenticate and validate
messages, eliminating unauthenticated pickle deserialization.
---
Duplicate comments:
In `@tests/unittest/scripts/test_perf_submit.py`:
- Around line 302-306: Update
test_extract_pytest_command_env_rejects_malformed_export and the corresponding
test near line 332 so the outer export quote is closed while the extracted
pytestCommand payload remains unbalanced. This ensures shlex.split succeeds and
exercises the payload parser’s malformed-input ValueError branch rather than the
whole-line parsing error.
---
Nitpick comments:
In `@jenkins/scripts/perf/submit.py`:
- Around line 656-683: Refactor extract_pytest_command_env to reuse the existing
_pytest_command_tokens helper for locating, unquoting, and splitting
pytestCommand instead of duplicating that parsing logic. Preserve its leading
environment-variable scan, name matching, and _PytestCommandEnvMissing behavior,
while retaining appropriate errors from the shared parser.
In `@tests/unittest/disaggregated/test_cache_transceiver_harness.py`:
- Around line 445-651: Add a successful context-release test to
TestRequestTransferOwnershipFlow that exercises completed-request validation and
the release handshake, asserting the expected event ordering through free and
exactly one free_sequence call with the context ownership arguments. Use the
existing _run_ctx_request helper and established handshake/final-response
conventions, while preserving the current failure and timeout coverage.
In `@tests/unittest/disaggregated/test_transceiver_bounded_polling.py`:
- Around line 94-125: Share the duplicated _FakeTask and _FakeClock test doubles
between test_transceiver_bounded_polling.py and
test_cache_transceiver_harness.py by moving them into a shared conftest.py
fixture or helper module. Update both tests to import or use the shared
implementations, preserving their existing wait, monotonic, timeout-tracking,
and TaskStatus behavior.
- Around line 151-174: Update _make_tx_session to construct TxSession through
its real __init__ path, using the same mock-based setup demonstrated by
test_tx_session_first_send_anchors_deadline_once, instead of object.__new__ with
manually assigned fields. Preserve the helper’s configurable kv_tasks, need_aux,
aux_task, timeout_s, and deadline_monotonic_s behavior while ensuring
constructor-initialized fields such as _base_args, request_id,
transfer_start_time, and transfer_end_time are present.
In `@tests/unittest/others/test_cache_transceiver_precheck_config.py`:
- Around line 512-520: Update the test’s script construction before
subprocess.run to retain only the generated export LLM_MODELS_ROOT= line from
precheck_prefix_lines, excluding all other prefix commands; preserve the
existing bash execution and stdout assertion for validating its quoting.
🪄 Autofix
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: d4271b36-13db-4efa-b3f8-acebffb26dac
📒 Files selected for processing (18)
examples/disaggregated/slurm/cache_transceiver_test/README.mdexamples/disaggregated/slurm/cache_transceiver_test/config.yamlexamples/disaggregated/slurm/cache_transceiver_test/run_cache_transceiver_test.pyjenkins/scripts/perf/local/submit.pyjenkins/scripts/perf/submit.pytensorrt_llm/_torch/disaggregation/native/transfer.pytensorrt_llm/_torch/disaggregation/transceiver.pytensorrt_llm/llmapi/llm_args.pytests/scripts/perf-sanity/cache_transceiver_precheck/README.mdtests/scripts/perf-sanity/cache_transceiver_precheck/precheck_config.pytests/scripts/perf-sanity/cache_transceiver_precheck/run_precheck.pytests/scripts/perf-sanity/disaggregated/gb300_deepseek-v4-pro-fp4_8k1k_con180_ctx3_dep4_gen1_dep32_eplb384_mtp3_ccb-NIXL.yamltests/unittest/disaggregated/test_cache_transceiver_harness.pytests/unittest/disaggregated/test_cache_transceiver_precheck_e2e.pytests/unittest/disaggregated/test_transceiver_bounded_polling.pytests/unittest/others/test_cache_transceiver_precheck_config.pytests/unittest/others/test_cache_transceiver_precheck_run.pytests/unittest/scripts/test_perf_submit.py
🚧 Files skipped from review as they are similar to previous changes (9)
- examples/disaggregated/slurm/cache_transceiver_test/config.yaml
- tensorrt_llm/_torch/disaggregation/transceiver.py
- tests/scripts/perf-sanity/cache_transceiver_precheck/precheck_config.py
- tests/scripts/perf-sanity/cache_transceiver_precheck/README.md
- tensorrt_llm/_torch/disaggregation/native/transfer.py
- examples/disaggregated/slurm/cache_transceiver_test/README.md
- tests/scripts/perf-sanity/disaggregated/gb300_deepseek-v4-pro-fp4_8k1k_con180_ctx3_dep4_gen1_dep32_eplb384_mtp3_ccb-NIXL.yaml
- jenkins/scripts/perf/local/submit.py
- tests/unittest/disaggregated/test_cache_transceiver_precheck_e2e.py
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
All five threads from the previous round are genuinely fixed, not just resolved:
- Silent sender poll loop —
TxSession.wait_completenow enforces an absolute deadline anchored at the firstsend(), with a finite 60 s fallback whenkv_transfer_timeout_msis null, and expiry logs the rid and deadline source ([transceiver.py:753](https://github.com/NVIDIA/TensorRT-LLM/pull/17137/files#diff-3cab8f284e64d42a9c7004535a761cabcdc7323d787ddbecdbd467b7e4f935b9R753)) while retaining the session and pages. - Dead TIMEOUT branch — reachable again (option 2 as discussed), and the
timed_outplumbing is gone from_ctx_consensus_outcome. blocking=True→Noneaux window — closed: once every KV task is terminal, a missing required aux task becomes an invariant ERROR ([transfer.py:1461](https://github.com/NVIDIA/TensorRT-LLM/pull/17137/files#diff-337dabe32d190384907e8c1330d67ed73ff63f41fbf0c1a347ff9f2ad18d4590R1461)), andrespond_and_send_asyncdoes install aux synchronously via_finalize_sendbefore publishing to_send_reqs, so the invariant holds.[:num_prompt_blocks]slice —_request_block_viewsnow raises on under-allocation with rid/layer/required/available before trimming surplus speculative pages.LLM_MODELS_ROOThard abort —_resolve_llm_models_rootfalls back to the submitter environment and fails closed only when the precheck is actually enabled.
Test coverage for the new deadline semantics is thorough — anchor-once, boundary-precedence races, shared task/aux deadline, and the None-fallback path all have dedicated cases in test_transceiver_bounded_polling.py.
One non-blocking asymmetry noted inline: the Rx side didn't get the None-timeout fallback the Tx side did ([transceiver.py:130](https://github.com/NVIDIA/TensorRT-LLM/pull/17137/files#diff-3cab8f284e64d42a9c7004535a761cabcdc7323d787ddbecdbd467b7e4f935b9R130)).
Merge order as described: land #17223 first, after which this PR reduces to the one YAML delta already validated by the targeted GB300 stage.
|
PR_Github #65119 [ run ] completed with state
|
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2" |
d0a7c6e to
51841d8
Compare
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2" |
|
PR_Github #65363 [ run ] triggered by Bot. Commit: |
|
PR_Github #65363 [ run ] completed with state
|
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2" |
|
PR_Github #65410 [ run ] triggered by Bot. Commit: |
|
PR_Github #65410 [ run ] completed with state
|
51841d8 to
3fa575a
Compare
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
3fa575a to
de31862
Compare
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2" |
|
PR_Github #66404 [ run ] triggered by Bot. Commit: |
|
PR_Github #66404 [ run ] completed with state |
7 participants
Summary
Depends on #17223. This PR's unique diff is one GB300 DeepSeek V4 Pro perf-sanity YAML:
cache_transceiver_precheckfor this target;kv_transfer_timeout_msfrom 600000 ms to 60000 ms.Stack and merge order
cb03f673ee8ebb0f0b2c134d35d1d596d3b6057251841d8f36d1cec10b16dfd1da4ba0880f3f0a1amain; parent-only checks can use bot skip.Validation
Exact command:
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2"Exact test:
perf/test_perf_sanity.py::test_e2e[disagg_upload-gen_only-gb300_deepseek-v4-pro-fp4_8k1k_con180_ctx3_dep4_gen1_dep32_eplb384_mtp3_ccb-NIXL]The restacked YAML parses, the precheck is enabled, and both timeout values are 60000 ms. Fresh targeted CI: trigger comment → PR_Github #65363 → L0 #53128 at the exact current head, with test reuse disabled. Historical L0 #51530 passed all six precheck combinations and 180/180 requests, but predates the final parent revision.
This 3-CTX, concurrency-180 proxy does not replace the original 8-CTX, concurrency-1760 stress validation.
Related PRs