[https://nvbugs/6541356][fix] align cache transceiver harness setup - #17121
[https://nvbugs/6541356][fix] align cache transceiver harness setup#17121chuangz0 wants to merge 7 commits into
Conversation
chuangz0
changed the title
458ac02 to
ce1f8d9
Compare
|
/bot run --stage-list "GB300-56_GPUs-14_Nodes-PyTorch-Disagg-PerfSanity-CTX12-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge*" |
|
PR_Github #63016 [ run ] triggered by Bot. Commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change centralizes fabric-memory setup for Python transceivers, applies it before KV-cache allocation, propagates shell-quoted ChangesFabric-memory setup and precheck integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PerfSubmit
participant PrecheckConfig
participant run_precheck
participant FabricMemoryHelper
participant KVCacheManager
PerfSubmit->>PrecheckConfig: pass shell-quoted LLM_MODELS_ROOT
PrecheckConfig->>run_precheck: generate context and generation commands
run_precheck->>FabricMemoryHelper: configure resolved transceiver and cache manager
FabricMemoryHelper->>KVCacheManager: set fabric-memory default before construction
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 (2)
tensorrt_llm/_torch/pyexecutor/kv_cache_transceiver.py (1)
38-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public helper parameters.
Add a Google-style
Args:section forcache_transceiver_configandkv_cache_manager_cls. This helper is imported outside its defining module.Proposed documentation update
"""Default the C++ V1 KV pool to fabric memory for the Python transceiver. This must run before any KV pool allocation because the C++ environment getter caches the value on first read. Explicit user settings are always respected. + + Args: + cache_transceiver_config: Configuration that selects the transceiver. + kv_cache_manager_cls: KV-cache manager class to evaluate. """🤖 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/pyexecutor/kv_cache_transceiver.py` around lines 38 - 46, Update the docstring for maybe_enable_fabric_memory_for_python_transceiver with a Google-style Args: section documenting cache_transceiver_config and kv_cache_manager_cls, including their roles and expected types. Preserve the existing behavior and explanatory text.Source: Coding guidelines
tests/unittest/others/test_kv_cache_transceiver.py (1)
47-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the new test to
tests/integration/test_lists/test-db/l0_sanity_check.yml.The test covers four parameterized cases, but only other tests from
test_kv_cache_transceiver.pyare listed. Coverage verdict: insufficient.🤖 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_kv_cache_transceiver.py` around lines 47 - 67, Add test_maybe_enable_fabric_memory_for_python_transceiver from test_kv_cache_transceiver.py to tests/integration/test_lists/test-db/l0_sanity_check.yml so all four parameterized cases are included in the integration test list.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 `@tests/scripts/perf-sanity/cache_transceiver_precheck/precheck_config.py`:
- Line 167: The shlex.quote() call on line 167 produces single-quote escaping
that breaks when the quoted value is embedded in the double-quoted export
statements on lines 177-180. Replace the quoting strategy for model_root_env to
survive embedding in double-quoted strings, either by passing LLM_MODELS_ROOT as
a runtime environment variable without shell string interpolation or by quoting
the entire export value instead of the path alone. Add a regression test that
validates the fix with a path containing an apostrophe.
---
Nitpick comments:
In `@tensorrt_llm/_torch/pyexecutor/kv_cache_transceiver.py`:
- Around line 38-46: Update the docstring for
maybe_enable_fabric_memory_for_python_transceiver with a Google-style Args:
section documenting cache_transceiver_config and kv_cache_manager_cls, including
their roles and expected types. Preserve the existing behavior and explanatory
text.
In `@tests/unittest/others/test_kv_cache_transceiver.py`:
- Around line 47-67: Add test_maybe_enable_fabric_memory_for_python_transceiver
from test_kv_cache_transceiver.py to
tests/integration/test_lists/test-db/l0_sanity_check.yml so all four
parameterized cases are included in the integration test 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: 49468c21-5a6c-412d-820a-cb887e0184ef
📒 Files selected for processing (7)
examples/disaggregated/slurm/cache_transceiver_test/run_cache_transceiver_test.pytensorrt_llm/_torch/pyexecutor/_util.pytensorrt_llm/_torch/pyexecutor/kv_cache_transceiver.pytests/scripts/perf-sanity/cache_transceiver_precheck/precheck_config.pytests/scripts/perf-sanity/cache_transceiver_precheck/run_precheck.pytests/unittest/others/test_cache_transceiver_precheck_config.pytests/unittest/others/test_kv_cache_transceiver.py
|
PR_Github #63016 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #63342 [ run ] triggered by Bot. Commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/unittest/others/test_cache_transceiver_precheck_config.py`:
- Around line 368-376: Update the test setup before the precheck_prefix_lines
call to also remove the TRTLLM_DISAGG_CT_PRECHECK environment variable with
monkeypatch, while preserving the existing LLM_MODELS_ROOT cleanup and test
arguments.
In `@tests/unittest/scripts/test_perf_submit.py`:
- Around line 103-123: Add a test near test_ci_missing_model_root_is_detectable
that passes an unterminated-quote pytestCommand prefix to
ci_submit_module._get_pytest_command_env_var and asserts it raises ValueError.
Keep the existing valid-command and missing-variable coverage unchanged.
🪄 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: 0e901ae8-3a8a-490a-a39c-a64512c2bbcc
📒 Files selected for processing (8)
jenkins/scripts/perf/cluster_env.pyjenkins/scripts/perf/local/submit.pyjenkins/scripts/perf/submit.pytests/scripts/perf-sanity/cache_transceiver_precheck/precheck_config.pytests/unittest/others/test_cache_transceiver_precheck_config.pytests/unittest/others/test_cache_transceiver_precheck_run.pytests/unittest/scripts/test_cluster_env.pytests/unittest/scripts/test_perf_submit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/scripts/perf-sanity/cache_transceiver_precheck/precheck_config.py
|
/bot run --disable-fail-fast |
|
PR_Github #64281 [ run ] triggered by Bot. Commit: |
|
PR_Github #64281 [ run ] completed with state
|
602898a to
ee02767
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #64366 [ run ] triggered by Bot. Commit: |
|
PR_Github #64366 [ run ] completed with state |
3b1e0c3 to
a25e116
Compare
Concern: default flip of the cache-transceiver precheck impacts both L0 dev CI and QA weekly perf testsThanks for the work here — the Why this is risky:
Suggested path (either one):
Net: the feature itself works, but flipping the default on an unvalidated hard gate — with no opt-out wired in on either dev or QA — can broadly red-line disagg cases across both CI and the QA weekly perf test. Please do more validation before enabling by default. |
|
/bot run --disable-reuse-test --disable-fail-fast --stage-list "Disagg-PerfSanity" |
|
PR_Github #64553 [ run ] triggered by Bot. Commit: |
|
@fredricz-20070104 Please pick a GB200 and a GB300 disagg perf sanity case, and run on lyris to validate the functionality of pre-check. |
|
Is the oci-aga UCX change intentional? KV transfer there falls back to TCP on the real worker commands, not just the precheck. Besides, |
Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
- Only fail submit on a missing LLM_MODELS_ROOT when the cache-transceiver precheck is enabled for the config; degrade to a warning otherwise. - Re-split the inbound pytestCommand value only when it is wrapped in an outer quote pair, so a bare token with escaped whitespace survives. - Log that the fabric-memory default applies to the whole transceiver-test run, including C++ cases. - Pin PYTEST_COMMON_VARS word-splitting and the auto transceiver-runtime behavior in tests. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
…nity case Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
adec7d1 to
cc6c8bf
Compare
|
/bot run --disable-reuse-test --disable-fail-fast --stage-list "Disagg-PerfSanity" |
|
/bot run --disable-reuse-test --disable-fail-fast --stage-list "Disagg-PerfSanity" |
|
PR_Github #64582 [ run ] triggered by Bot. Commit: |
|
PR_Github #64553 [ run ] completed with state |
|
PR_Github #64582 [ run ] completed with state
|
10 participants
Description
Fixes the cache-transceiver precheck, a shared component of the disaggregated perf-sanity tests (NVBug 6541356 is one manifestation). Everything in this PR serves making that component work correctly across clusters:
LLM_MODELS_ROOTandPYTEST_COMMON_VARSto the cache-transceiver precheck command so automatic KV-cache-manager selection can load the model configuration instead of silently falling back to V1.examples/disaggregated/slurm/cache_transceiver_testbefore KV-pool allocation. ExplicitTRTLLM_KVCACHE_POOL_USE_FABRIC_MEMORYvalues are preserved; C++ transceiver and KV cache manager V2 behavior is unchanged.oci-agaUCX environment: the previously pinned RDMA/VF config only worked in the standalone transceiver test; in the real serve benchmark MPI (pml_ucx) and the KV transceiver fail to create UCX workers because the rail VFs carry IPv6-only addresses TCP cannot bind, soUCX_TLS=cuda_ipc,cuda_copy,sm,self,tcp+UCX_TCP_AF_PRIO=inetis required. Intra-node transfers still usecuda_ipc; the TCP cost is inter-node only, accepted until the VF fabric issue is resolved cluster-side.LLM_MODELS_ROOTonly when the precheck is enabled for the config (sharedprecheck_enabled()helper); only re-split the inboundpytestCommandvalue when it is wrapped in an outer quote pair; log that the fabric-memory default applies to the whole transceiver-test run including C++ cases.Root cause
The generated precheck command did not forward the model-root/common environment. Consequently, auto manager resolution could not read the model configuration and selected cache manager V1. In addition, the standalone precheck and SLURM transceiver harness instantiate KV-cache managers directly, bypassing the serving path where #16832 enables fabric memory for the Python transceiver with V1.
Unwaives
NVBug 6561566 is a bulk post-merge bucket (12 cases across b200/gb200/gb300) that will take several PRs to burn down. This PR unwaives exactly the two
DGX_B200-16_GPUs-2_Nodescases that its CI run validated as passing:disagg_upload-gen_only-b200_deepseek-r1-fp4_8k1k_con1_ctx1_dep4_gen1_tep8_eplb0_mtp3_ccb-NIXLdisagg_upload-gen_only-b200_deepseek-r1-fp4_8k1k_con256_ctx1_dep4_gen1_dep8_eplb0_mtp1_ccb-NIXLThe gb200/gb300 siblings stay waived until their fixes are verified the same way.
Related
/bot runon the perf-sanity stages before flipping the default back on.Validation
pre-commit run --files <changed files>pytest -q tests/unittest/others/test_cache_transceiver_precheck_config.py tests/unittest/others/test_cache_transceiver_precheck_run.py tests/unittest/scripts/test_perf_submit.py tests/unittest/disaggregated/test_cache_transceiver_harness_report.py -k 'not TestInternalApiContract'(all passed)PYTEST_COMMON_VARSword-splitting)Dev Engineer Review
KVCacheManagerV2behavior.LLM_MODELS_ROOTandPYTEST_COMMON_VARSwith shell-safe quoting.oci-agaUCX configuration consistently.QA Engineer Review
Test code changed in:
tests/unittest/others/test_cache_transceiver_precheck_config.pytests/unittest/others/test_kv_cache_transceiver.pytests/unittest/others/test_cache_transceiver_precheck_run.pyKVCacheManagerV2selection for DeepSeek V4.tests/unittest/scripts/test_cluster_env.pyoci-aga-cs-001UCX environment.tests/unittest/scripts/test_perf_submit.pyLLM_MODELS_ROOTfrom quoted and unquoted commands, including special characters and missing values.The fabric-memory test is listed in
tests/integration/test_lists/test-db/l0_sanity_check.yml. The other test functions are not listed intests/integration/test_lists/test-db or QA files.Targeted validation passed.
Verdict: sufficient