[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
|
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
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
|
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
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 |
WalkthroughThe changes forward ChangesDisaggregated precheck transfer
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SubmitScript
participant PrecheckConfig
participant PRECHECK
participant TxSession
participant KVPages
SubmitScript->>PrecheckConfig: pass LLM_MODELS_ROOT
PrecheckConfig->>PRECHECK: export model root and configure manager
PRECHECK->>TxSession: submit and poll transfer wave
TxSession-->>PRECHECK: completed, failed, cancelled, or pending status
PRECHECK->>KVPages: release pages only after successful terminal status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 3
🧹 Nitpick comments (5)
tests/scripts/perf-sanity/cache_transceiver_precheck/precheck_config.py (1)
132-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate and document the shared launch-script interface.
precheck_prefix_linesis called by both submit modules. Add precise parameter and return annotations. Add Google-styleArgsandReturnssections that documentllm_models_rootand the generated export lines.As per coding guidelines, “Annotate every function” and “Prefer docstrings for external interfaces, use Google-style docstrings, document public function arguments.”
🤖 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/cache_transceiver_precheck/precheck_config.py` around lines 132 - 139, Update precheck_prefix_lines with precise type annotations for every parameter and its return value, and add a Google-style docstring documenting all arguments—especially llm_models_root—and that the function returns generated export lines. Keep the shared launch-script interface behavior unchanged.Source: Coding guidelines
tests/unittest/others/test_cache_transceiver_precheck_run.py (1)
380-397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the two event lists.
_ctx_finish_runnerbinds its own list tofree_events, and the test then overrides_free_allto append to the localevents. The assertionfree_events == []therefore checks the discarded list. The test is correct, but the names invert the reader's expectation. Take the runner list as_or drop the override and assert on the runner list only.♻️ Proposed simplification
-def test_ctx_finish_wave_frees_only_after_block_all_returns_every_request(monkeypatch): - events = [] - - def check_status(at_least_request_num): - events.append(("block_all", at_least_request_num)) - return [101, 102], [] - - runner, free_events = _ctx_finish_runner(monkeypatch, check_status) +def test_ctx_finish_wave_frees_only_after_block_all_returns_every_request(monkeypatch): + calls = [] + + def check_status(at_least_request_num): + calls.append(("block_all", at_least_request_num)) + return [101, 102], [] + + runner, events = _ctx_finish_runner(monkeypatch, check_status) reqs = { } - runner._free_all = lambda owned: events.append(("free", sorted(owned))) runner.ctx_finish_wave(reqs) - assert events == [("block_all", None), ("free", [0, 1])] - assert free_events == [] + assert calls == [("block_all", None)] + assert events == [("free", [0, 1])]🤖 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_run.py` around lines 380 - 397, Clarify the event-list usage in test_ctx_finish_wave_frees_only_after_block_all_returns_every_request by discarding the unused free_events value from _ctx_finish_runner and asserting the _free_all callback’s local events directly, or otherwise assert only the runner-owned list without checking the discarded list.tests/unittest/disaggregated/test_transceiver_bounded_polling.py (1)
421-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test does not reach the in-loop sibling check.
wait_completecallshas_failed()before the KV loop, so the pre-existingTaskStatus.ERRORonfailed_taskreturnsWaitResult.FAILEDimmediately. The assertionspending_task.wait_calls == []confirm that. The test therefore duplicatestest_tx_session_blocking_wait_treats_task_failure_as_terminaland leaves the sibling recheck at lines 1368-1371 oftensorrt_llm/_torch/disaggregation/native/transfer.pyuncovered.To cover that path, make the sibling fail during the first wait slice.
💚 Proposed test change to exercise the sibling recheck
def test_tx_session_blocking_wait_detects_failed_sibling_behind_pending_task() -> None: pending_task = _FakeTask(TaskStatus.TRANSFERRING, wait_result=False) - failed_task = _FakeTask(TaskStatus.ERROR) + failed_task = _FakeTask(TaskStatus.TRANSFERRING, wait_result=False) session = _make_tx_session([pending_task, failed_task]) + wait = pending_task.wait + + def fail_sibling_during_wait(timeout: Optional[float] = None) -> bool: + result = wait(timeout) + failed_task.status = TaskStatus.ERROR + return result + + pending_task.wait = fail_sibling_during_wait assert session.wait_complete(blocking=True) == WaitResult.FAILED - assert pending_task.wait_calls == [] + assert pending_task.wait_calls == [0.25] assert failed_task.wait_calls == []🤖 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 421 - 428, Update test_tx_session_blocking_wait_detects_failed_sibling_behind_pending_task so failed_task starts in a non-error state and transitions to TaskStatus.ERROR during pending_task’s first wait slice, allowing wait_complete(blocking=True) to reach and validate the in-loop sibling failure recheck. Preserve the assertions that the result is WaitResult.FAILED and both tasks’ wait-call behavior remains correct.tests/scripts/perf-sanity/disaggregated/gb300_deepseek-v4-pro-fp4_8k1k_con180_ctx3_dep4_gen1_dep32_eplb384_mtp3_ccb-NIXL.yaml (1)
70-70: 🩺 Stability & Availability | 🔵 TrivialNote the reduced receive deadline for the high-concurrency workload.
kv_transfer_timeout_msfeedsrx_timeout_sinKvCacheTransceiverV2.__init__(tensorrt_llm/_torch/disaggregation/transceiver.pyline 108). The 10x reduction to 60 s tightens the receive deadline. The PR description states that the targeted run does not establish resolution of the concurrency-1760, 8-CTX-worker case. Under that load a cold NIXL link can exceed 60 s and the request then fails instead of completing late. Track a follow-up run at the original concurrency before this config is used as the perf-sanity baseline.Also applies to: 101-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-v4-pro-fp4_8k1k_con180_ctx3_dep4_gen1_dep32_eplb384_mtp3_ccb-NIXL.yaml` at line 70, Restore kv_transfer_timeout_ms to its previous value for this high-concurrency perf-sanity configuration, rather than using the reduced 60000 ms receive deadline. Apply the same correction to the additionally referenced occurrence and retain the original timeout until the concurrency-1760, 8-CTX-worker follow-up run validates a shorter deadline.tensorrt_llm/_torch/disaggregation/native/transfer.py (1)
1374-1390: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRemove the unreachable
WaitResult.TIMEOUThandling. Nowait_completeimplementation returnsWaitResult.TIMEOUT; remove the branch andtimed_outplumbing from_ctx_consensus_outcome.🤖 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 `@tensorrt_llm/_torch/disaggregation/native/transfer.py` around lines 1374 - 1390, The _ctx_consensus_outcome flow still contains obsolete timeout handling. Remove the unreachable WaitResult.TIMEOUT branch and all timed_out plumbing from _ctx_consensus_outcome, while preserving the existing FAILED and COMPLETED outcomes and auxiliary-task waiting behavior.
🤖 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 `@tensorrt_llm/_torch/disaggregation/native/transfer.py`:
- Around line 1355-1373: Update the wait-slice handling in TxSession’s
blockAll/wait_complete flow so nonpositive or None _timeout_s values still use a
small positive polling interval instead of becoming an unbounded
task.wait(timeout=None). Preserve the existing wait loop and has_failed()
recheck, ensuring cancellation of TRANSFERRING tasks can reach the terminal
failure result.
In `@tests/scripts/perf-sanity/cache_transceiver_precheck/README.md`:
- Around line 80-86: Update every documented command in the README that assigns
LLM_MODELS_ROOT so the model-root placeholder is quoted, including both the
dry-run and SLURM examples; preserve the existing command structure and
arguments.
In `@tests/unittest/scripts/test_perf_submit.py`:
- Around line 136-140: Update
test_extract_pytest_command_env_rejects_malformed_export to use a valid, closed
outer pytestCommand export containing an unclosed payload quote, then assert
ValueError matches "cannot parse pytestCommand payload". Add this test to the
applicable CI and QA test lists.
---
Nitpick comments:
In `@tensorrt_llm/_torch/disaggregation/native/transfer.py`:
- Around line 1374-1390: The _ctx_consensus_outcome flow still contains obsolete
timeout handling. Remove the unreachable WaitResult.TIMEOUT branch and all
timed_out plumbing from _ctx_consensus_outcome, while preserving the existing
FAILED and COMPLETED outcomes and auxiliary-task waiting behavior.
In `@tests/scripts/perf-sanity/cache_transceiver_precheck/precheck_config.py`:
- Around line 132-139: Update precheck_prefix_lines with precise type
annotations for every parameter and its return value, and add a Google-style
docstring documenting all arguments—especially llm_models_root—and that the
function returns generated export lines. Keep the shared launch-script interface
behavior unchanged.
In
`@tests/scripts/perf-sanity/disaggregated/gb300_deepseek-v4-pro-fp4_8k1k_con180_ctx3_dep4_gen1_dep32_eplb384_mtp3_ccb-NIXL.yaml`:
- Line 70: Restore kv_transfer_timeout_ms to its previous value for this
high-concurrency perf-sanity configuration, rather than using the reduced 60000
ms receive deadline. Apply the same correction to the additionally referenced
occurrence and retain the original timeout until the concurrency-1760,
8-CTX-worker follow-up run validates a shorter deadline.
In `@tests/unittest/disaggregated/test_transceiver_bounded_polling.py`:
- Around line 421-428: Update
test_tx_session_blocking_wait_detects_failed_sibling_behind_pending_task so
failed_task starts in a non-error state and transitions to TaskStatus.ERROR
during pending_task’s first wait slice, allowing wait_complete(blocking=True) to
reach and validate the in-loop sibling failure recheck. Preserve the assertions
that the result is WaitResult.FAILED and both tasks’ wait-call behavior remains
correct.
In `@tests/unittest/others/test_cache_transceiver_precheck_run.py`:
- Around line 380-397: Clarify the event-list usage in
test_ctx_finish_wave_frees_only_after_block_all_returns_every_request by
discarding the unused free_events value from _ctx_finish_runner and asserting
the _free_all callback’s local events directly, or otherwise assert only the
runner-owned list without checking the discarded list.
🪄 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: 0c62c6c3-d848-439d-947f-9a0a1e3e5e7d
📒 Files selected for processing (13)
jenkins/scripts/perf/local/submit.pyjenkins/scripts/perf/submit.pytensorrt_llm/_torch/disaggregation/native/transfer.pytensorrt_llm/_torch/disaggregation/transceiver.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_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
| # ``_timeout_s`` bounds one scheduler wait slice; it is not a transfer | ||
| # deadline. A successful blockAll must not return merely because one | ||
| # slice expired: NIXL may still be reading the request's KV pages. | ||
| wait_slice_s = self._timeout_s | ||
| if wait_slice_s is not None and wait_slice_s <= 0: | ||
| wait_slice_s = None | ||
| # A None timeout intentionally preserves Event.wait()'s unbounded | ||
| # standalone behavior; configured transceivers supply a positive slice. | ||
| for task in self.kv_tasks: | ||
| if not task.wait(timeout=self._timeout_s): | ||
| return WaitResult.TIMEOUT | ||
| while not task.wait(timeout=wait_slice_s): | ||
| # cancel() leaves a TRANSFERRING task's event unset until the | ||
| # physical writer finishes. Preserve the bounded-slice control | ||
| # point so blockAll can still report terminal cancellation. | ||
| # Check every task: a sibling slice can fail while this one is | ||
| # still pending without setting the session terminal status. | ||
| if self.has_failed(): | ||
| return WaitResult.FAILED | ||
| if task.status == TaskStatus.ERROR: | ||
| return WaitResult.FAILED |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A nonpositive _timeout_s removes the cancellation control point.
Lines 1359-1360 convert a nonpositive slice to None, and task.wait(timeout=None) blocks without limit. cancel() does not set the event of a TRANSFERRING task, so has_failed() never runs and wait_complete(blocking=True) cannot return. The comment states that configured transceivers supply a positive slice, but TxSession also accepts timeout_s=None from its constructor default.
Consider clamping the slice to a small positive floor so the failure recheck always executes.
🛡️ Proposed fix to keep a bounded recheck interval
- wait_slice_s = self._timeout_s
- if wait_slice_s is not None and wait_slice_s <= 0:
- wait_slice_s = None
- # A None timeout intentionally preserves Event.wait()'s unbounded
- # standalone behavior; configured transceivers supply a positive slice.
+ # Keep a bounded slice so the terminal-status recheck below always runs:
+ # cancel() leaves a TRANSFERRING task's event unset, and an unbounded
+ # Event.wait() would never reach the has_failed() control point.
+ wait_slice_s = self._timeout_s
+ if wait_slice_s is None or wait_slice_s <= 0:
+ wait_slice_s = _DEFAULT_WAIT_SLICE_S🤖 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 `@tensorrt_llm/_torch/disaggregation/native/transfer.py` around lines 1355 -
1373, Update the wait-slice handling in TxSession’s blockAll/wait_complete flow
so nonpositive or None _timeout_s values still use a small positive polling
interval instead of becoming an unbounded task.wait(timeout=None). Preserve the
existing wait loop and has_failed() recheck, ensuring cancellation of
TRANSFERRING tasks can reach the terminal failure result.
| LLM_MODELS_ROOT=<models> python3 run_precheck.py --role gen --server-idx 0 --dry-run \ | ||
| --config ../disaggregated/<test>.yaml --work-dir /tmp/ct --llm-src <repo> | ||
|
|
||
| # On a SLURM allocation: one srun per instance, e.g. ctx dep4 + gen dep8: | ||
| srun -N1 --ntasks=4 --mpi=pmix python3 run_precheck.py --role ctx --server-idx 0 \ | ||
| --config <yaml> --work-dir <shared-dir> --llm-src <repo> & | ||
| srun -N2 --ntasks=8 --mpi=pmix python3 run_precheck.py --role gen --server-idx 0 \ | ||
| LLM_MODELS_ROOT=<models> srun -N1 --ntasks=4 --mpi=pmix python3 run_precheck.py \ | ||
| --role ctx --server-idx 0 --config <yaml> --work-dir <shared-dir> --llm-src <repo> & | ||
| LLM_MODELS_ROOT=<models> srun -N2 --ntasks=8 --mpi=pmix python3 run_precheck.py --role gen --server-idx 0 \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote the documented model-root assignment.
The examples fail when the model-root path contains spaces. Quote the placeholder in each command.
Proposed documentation fix
-LLM_MODELS_ROOT=<models> python3 run_precheck.py
+LLM_MODELS_ROOT='<models>' python3 run_precheck.py
-LLM_MODELS_ROOT=<models> srun -N1
+LLM_MODELS_ROOT='<models>' srun -N1
-LLM_MODELS_ROOT=<models> srun -N2
+LLM_MODELS_ROOT='<models>' srun -N2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LLM_MODELS_ROOT=<models> python3 run_precheck.py --role gen --server-idx 0 --dry-run \ | |
| --config ../disaggregated/<test>.yaml --work-dir /tmp/ct --llm-src <repo> | |
| # On a SLURM allocation: one srun per instance, e.g. ctx dep4 + gen dep8: | |
| srun -N1 --ntasks=4 --mpi=pmix python3 run_precheck.py --role ctx --server-idx 0 \ | |
| --config <yaml> --work-dir <shared-dir> --llm-src <repo> & | |
| srun -N2 --ntasks=8 --mpi=pmix python3 run_precheck.py --role gen --server-idx 0 \ | |
| LLM_MODELS_ROOT=<models> srun -N1 --ntasks=4 --mpi=pmix python3 run_precheck.py \ | |
| --role ctx --server-idx 0 --config <yaml> --work-dir <shared-dir> --llm-src <repo> & | |
| LLM_MODELS_ROOT=<models> srun -N2 --ntasks=8 --mpi=pmix python3 run_precheck.py --role gen --server-idx 0 \ | |
| LLM_MODELS_ROOT='<models>' python3 run_precheck.py --role gen --server-idx 0 --dry-run \ | |
| --config ../disaggregated/<test>.yaml --work-dir /tmp/ct --llm-src <repo> | |
| # On a SLURM allocation: one srun per instance, e.g. ctx dep4 + gen dep8: | |
| LLM_MODELS_ROOT='<models>' srun -N1 --ntasks=4 --mpi=pmix python3 run_precheck.py \ | |
| --role ctx --server-idx 0 --config <yaml> --work-dir <shared-dir> --llm-src <repo> & | |
| LLM_MODELS_ROOT='<models>' srun -N2 --ntasks=8 --mpi=pmix python3 run_precheck.py --role gen --server-idx 0 |
🤖 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/cache_transceiver_precheck/README.md` around lines
80 - 86, Update every documented command in the README that assigns
LLM_MODELS_ROOT so the model-root placeholder is quoted, including both the
dry-run and SLURM examples; preserve the existing command structure and
arguments.
| 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") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -F 'tests/unittest/scripts/test_perf_submit.py' \
tests/integration/test_lists/test-db tests/integration/test_lists/qa || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- changed test file ---'
git diff -- tests/unittest/scripts/test_perf_submit.py
printf '%s\n' '--- extractor definition and callers ---'
rg -n -C 8 'def extract_pytest_command_env|extract_pytest_command_env\(' .
printf '%s\n' '--- test-list references ---'
rg -n -F 'test_perf_submit.py' tests/integration/test_lists/test-db tests/integration/test_lists/qa || true
rg -n -F 'test_extract_pytest_command_env' tests tests/integration/test_lists || true
printf '%s\n' '--- relevant test file ---'
sed -n '1,220p' tests/unittest/scripts/test_perf_submit.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 11549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- extractor implementation ---'
sed -n '454,490p' jenkins/scripts/perf/submit.py
printf '%s\n' '--- available test-list files ---'
find tests/integration/test_lists/test-db tests/integration/test_lists/qa \
-maxdepth 2 -type f -print | sort | head -200
printf '%s\n' '--- all references to the test file ---'
rg -n -F 'test_perf_submit.py' tests/integration/test_lists tests || true
printf '%s\n' '--- shlex behavior for both malformed inputs ---'
python3 - <<'PY'
import shlex
cases = {
"malformed outer export": 'export pytestCommand="LLM_ROOT=/src LLM_MODELS_ROOT=/models pytest',
"valid outer export, malformed payload": 'export pytestCommand=\'LLM_ROOT=/src "unterminated\'',
}
for name, line in cases.items():
print(name)
try:
outer_tokens = shlex.split(line)
print("outer_tokens:", outer_tokens)
command = next(token.split("=", 1)[1] for token in outer_tokens if token.startswith("pytestCommand="))
print("payload:", command)
print("payload_tokens:", shlex.split(command))
except ValueError as exc:
print("ValueError:", exc)
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 6806
Add coverage for the malformed pytestCommand payload path.
test_extract_pytest_command_env_rejects_malformed_export has an unclosed outer quote. It raises cannot parse exported pytestCommand before the payload parser runs. Add a valid outer export with an unclosed payload quote and assert cannot parse pytestCommand payload.
Test coverage summary: Insufficient. Changed tests: test_extract_pytest_command_env, test_extract_pytest_command_env_rejects_missing_leading_assignment, and test_extract_pytest_command_env_rejects_malformed_export. None is listed in the CI or QA test lists.
🤖 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 136 - 140, Update
test_extract_pytest_command_env_rejects_malformed_export to use a valid, closed
outer pytestCommand export containing an unclosed payload quote, then assert
ValueError matches "cannot parse pytestCommand payload". Add this test to the
applicable CI and QA test lists.
Source: Path instructions
| ) | ||
| try: | ||
| defaults = model_cls.get_model_defaults(None) or {} | ||
| except Exception as e: # noqa: BLE001 - model hooks are third-party extension points |
There was a problem hiding this comment.
Could we avoid making cache-manager auto-resolution failure fatal in this precheck? The primary purpose of this precheck is to validate cache-transceiver connectivity and basic data movement before launching the actual benchmark. If the manager version cannot be resolved, falling back to V1 with a prominent warning still provides useful network-path validation and allows the benchmark to run.
4 participants
Summary
kv_transfer_timeout_msfrom 600000 ms to 60000 ms for both GEN and CTX in the targeted GB300 DeepSeek V4 Pro disaggregated perf-sanity configuration.Stack and merge order
dc597f98ded698a4f840a7c410dc6a0a90cb1aacb7bd98a1049ad02eb335f86cb2cb64c057bb4495mainbefore merging.Because the GitHub base is still
main, the Files tab currently includes the parent fix as well as this PR's two-line YAML change. Reviewers should review #17223 first and treat the timeout commit as the child-only change.Motivation and diagnosis
NVBug 6480621 reported KV-transfer request failures after the 60-second timeout under a high-concurrency GB300 DeepSeek V4 Pro disaggregated E2E workload.
Earlier CI attempts at 60 seconds failed in the synthetic
cache_transceiver_precheckwith byte mismatches before the real benchmark started:Those failures were not 60-second request-deadline expirations. The exact target configuration also produced the same precheck byte-corruption symptom with a 600-second timeout in Main #2875,
Post-Merge-2.The common problem was the Python sender's one-second future wait slice being treated as block-all completion. The precheck could release and reuse source KV pages while a transfer was still nonterminal. #17223 fixes that ownership contract, propagates the real model/runtime into the precheck, and excludes the intentionally untransferred MTP reserve page from exact-boundary verification.
Validation
Local:
kv_transfer_timeout_ms: 60000;Diagnostic run on the preceding stack revision:
Fresh targeted CI with test reuse disabled: PASSED
b7bd98a1049ad02eb335f86cb2cb64c057bb4495SUCCESS: one target test passed, with zero failures or skips.GB300-44_GPUs-11_Nodes-PyTorch-Disagg-PerfSanity-CTX3-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2model_dir=/scratch.trt_llm_data/llm-models/DeepSeek-V4-Pro,kv_cache_manager=V2, andtransceiver_runtime=PYTHONon all roles.gen_0passed all six combinations: three context peers × request lengths 1,024 and 7,408. Mismatch, transfer-error, and initialization-error counts were zero.Target 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]Interpretation and remaining scope
The fresh run validates the corrected precheck and the concurrency-180, 3-CTX-server gen-only CI proxy at the original 60-second request deadline.
It does not prove that the original NVBug workload is resolved:
Before closing NVBug 6480621, the reporter should rerun the original or an equivalently stressful E2E workload at 60 seconds, preferably more than once.
Review and merge readiness
This PR is ready for stacked/dependent review after or alongside #17223.
It is not yet merge-ready:
BLOCKED;Before merging:
mainand confirm that its remaining diff is only the two timeout values;The timeout change may be merged as a scoped CI-policy/test change once those conditions are satisfied, without claiming NVBug 6480621 closed.
Dev Engineer Review
kv_transfer_timeout_msfrom600000to60000for GEN and CTX in the targeted GB300 DeepSeek V4 Pro configuration.LLM_MODELS_ROOT.TxSession.wait_completeto use_timeout_sas a retry interval. Blocking waits now continue until completion or failure.QA Engineer Review
Test-code changes include:
extract_pytest_command_envtests for quoted values, spaces,=characters, missing assignments, and malformed exports.test_tx_session_wait_complete_defaults_to_blocking.test-db/orqa/test-list entries were added or modified.