[None][test] Replace disaggregated DWDP accuracy tests with aggregated coverage - #17546
Open
tianyuz-nv wants to merge 1 commit into
Open
[None][test] Replace disaggregated DWDP accuracy tests with aggregated coverage#17546tianyuz-nv wants to merge 1 commit into
tianyuz-nv wants to merge 1 commit into
Conversation
tianyuz-nv
force-pushed
the
feat/dwdp-agg-accuracy
branch
from
August 12, 2026 08:35
d9fdec3 to
3d36a19
Compare
…d coverage DWDP accuracy was gated by three disaggregated-serving tests that are currently waived on GB200 and B200, so the feature has no effective CI coverage. Those tests exercise DWDP through the disaggregated KV cache transceiver, which makes them sensitive to per-cluster UCX transport configuration rather than to DWDP itself. Add an aggregated equivalent instead. A single instance running attention DP satisfies the invariant DWDP relies on -- every rank is a complete model replica owning one expert slice -- because Mapping.dp_size == tp_size there and attention is replicated rather than tensor-sharded. Relax the DWDP gate in create_py_executor accordingly: tp_size > 1 is now accepted when attention DP is enabled, and real tensor parallelism is still rejected with an explicit error. Mapping already forces moe_tp = moe_ep = 1 whenever dwdp_size > 1, so expert weights stay unsharded and ConfigurableMoE selects no MoE communication strategy on this path. The new tests run at dwdp_size=4 rather than the 2 the disaggregated tests used: aggregated serving has no generation server, so the whole allocation goes to DWDP peers. Three remote peers per rank also make contention_opt meaningful, since it interleaves prefetch slices across peers -- with a single remote peer that path was degenerate. Retire the disaggregated tests from the CI lists and drop their now-dead waives, but keep the file in tree as a manual reproduction of the disaggregated DWDP path, with a note on the UCX_TLS setting to check first. Signed-off-by: tianyuz-nv <tianyuz@nvidia.com>
tianyuz-nv
force-pushed
the
feat/dwdp-agg-accuracy
branch
from
August 12, 2026 09:07
3d36a19 to
42c5352
Compare
tianyuz-nv
marked this pull request as ready for review
August 12, 2026 09:12
tianyuz-nv
requested review from
StanleySun639,
asfiyab-nvidia,
brnguyen2,
cascade812,
chienchunhung,
crazydemo,
fredricz-20070104,
ruodil,
schetlur-nv,
tburt-nv and
yuanjingx87
August 12, 2026 09:12
Collaborator
Author
|
/bot run --disable-fail-fast |
Contributor
WalkthroughDWDP initialization now supports aggregated attention-DP layouts and validates incompatible configurations. New DeepSeek-V3-Lite aggregated accuracy tests cover three expert-partitioning modes. Test lists replace disaggregated entries, and manual disaggregated-test guidance is documented. ChangesDWDP aggregated serving
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TestDwdpAggDeepSeekV3Lite
participant AttentionDP_LLM
participant PyExecutor_DWDP
participant GSM8K
TestDwdpAggDeepSeekV3Lite->>AttentionDP_LLM: configure four-worker aggregated DWDP
AttentionDP_LLM->>PyExecutor_DWDP: initialize DWDP layout
PyExecutor_DWDP-->>AttentionDP_LLM: validate configuration
AttentionDP_LLM->>GSM8K: evaluate prompts
GSM8K-->>TestDwdpAggDeepSeekV3Lite: return accuracy results
Possibly related PRs
Suggested labels: 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/pyexecutor/py_executor_creator.py`:
- Around line 562-567: Extend the validation condition guarding the DWDP
complete-replica invariant to also reject mapping.pp_size > 1, while preserving
the existing tp_size and enable_attention_dp checks and error path. Update the
ValueError message to clearly identify the invalid pipeline-parallel
configuration and retain the valid configuration guidance.
In `@tests/integration/defs/accuracy/test_dwdp_aggregated.py`:
- Line 66: Update the test_dwdp_agg_accuracy method signature by annotating
num_experts_per_worker and num_prefetch_experts as int, contention_opt as bool,
and the return type as None.
In `@tests/integration/defs/accuracy/test_dwdp_disaggregated_serving.py`:
- Around line 13-18: Correct the manual-run guidance to reflect that
disaggregated workers use the value returned by get_ucx_tls(), assigned through
run_env["UCX_TLS"] in the test setup. Either document that effective policy and
its configuration source, or add and validate an override parameter that
controls the context and generation workers before describing manual UCX_TLS
overrides.
- Around line 6-11: Update the manual execution documentation in the test file’s
introductory NOTE to state that running it directly with pytest requires GPU
access, model weights, and LLM_MODELS_ROOT set to a valid model root (or an
available fallback directory), and include these prerequisites in the documented
command.
🪄 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: d6be1cd9-f2a6-4fcb-abe7-2653319ac949
📒 Files selected for processing (6)
tensorrt_llm/_torch/pyexecutor/py_executor_creator.pytests/integration/defs/accuracy/test_dwdp_aggregated.pytests/integration/defs/accuracy/test_dwdp_disaggregated_serving.pytests/integration/test_lists/qa/llm_function_core.txttests/integration/test_lists/test-db/l0_gb200_multi_gpus.ymltests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
Comment on lines
+562
to
+567
| if mapping.tp_size > 1 and not mapping.enable_attention_dp: | ||
| raise ValueError( | ||
| "DWDP requires each rank to be a complete model replica: use " | ||
| "tp_size=1 (disaggregated context worker) or " | ||
| "enable_attention_dp=True (aggregated serving), but got " | ||
| f"tp_size={mapping.tp_size} with enable_attention_dp=False.") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject pipeline parallelism for DWDP.
These checks accept pp_size > 1 when tp_size == 1. Pipeline parallelism makes each rank a model shard, which violates the complete-replica invariant documented on Lines 540-555. Reject mapping.pp_size > 1 with the same validation path.
Proposed fix
- if mapping.tp_size > 1 and not mapping.enable_attention_dp:
+ if (mapping.pp_size > 1
+ or (mapping.tp_size > 1
+ and not mapping.enable_attention_dp)):
raise ValueError(
"DWDP requires each rank to be a complete model replica: use "
- "tp_size=1 (disaggregated context worker) or "
+ "tp_size=1 and pp_size=1 (disaggregated context worker) or "
"enable_attention_dp=True (aggregated serving), but got "
- f"tp_size={mapping.tp_size} with enable_attention_dp=False.")
+ f"tp_size={mapping.tp_size}, pp_size={mapping.pp_size}, "
+ f"and enable_attention_dp={mapping.enable_attention_dp}.")📝 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.
Suggested change
| if mapping.tp_size > 1 and not mapping.enable_attention_dp: | |
| raise ValueError( | |
| "DWDP requires each rank to be a complete model replica: use " | |
| "tp_size=1 (disaggregated context worker) or " | |
| "enable_attention_dp=True (aggregated serving), but got " | |
| f"tp_size={mapping.tp_size} with enable_attention_dp=False.") | |
| if (mapping.pp_size > 1 | |
| or (mapping.tp_size > 1 | |
| and not mapping.enable_attention_dp)): | |
| raise ValueError( | |
| "DWDP requires each rank to be a complete model replica: use " | |
| "tp_size=1 and pp_size=1 (disaggregated context worker) or " | |
| "enable_attention_dp=True (aggregated serving), but got " | |
| f"tp_size={mapping.tp_size}, pp_size={mapping.pp_size}, " | |
| f"and enable_attention_dp={mapping.enable_attention_dp}.") |
🤖 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/py_executor_creator.py` around lines 562 -
567, Extend the validation condition guarding the DWDP complete-replica
invariant to also reject mapping.pp_size > 1, while preserving the existing
tp_size and enable_attention_dp checks and error path. Update the ValueError
message to clearly identify the invalid pipeline-parallel configuration and
retain the valid configuration guidance.
| ], | ||
| ids=["mode_a_uniform", "mode_b_overlap", "mode_a_uniform_contention_opt"], | ||
| ) | ||
| def test_dwdp_agg_accuracy(self, num_experts_per_worker, num_prefetch_experts, contention_opt): |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add type annotations to the test method.
Annotate the three parameters as int, int, and bool. Add -> None to the method signature.
As per coding guidelines, “Annotate every function, use None for procedures.”
🤖 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/integration/defs/accuracy/test_dwdp_aggregated.py` at line 66, Update
the test_dwdp_agg_accuracy method signature by annotating num_experts_per_worker
and num_prefetch_experts as int, contention_opt as bool, and the return type as
None.
Source: Coding guidelines
Comment on lines
+6
to
+11
| NOTE: these tests are intentionally not registered in any CI test list. DWDP | ||
| accuracy is gated in CI by test_dwdp_aggregated.py, which exercises the same | ||
| expert-sharing paths without the disaggregated KV cache transceiver and is | ||
| therefore not exposed to per-cluster transport configuration. This file is kept | ||
| as a manual reproduction of the disaggregated DWDP path; run it directly with | ||
| pytest. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the required model-root setup.
pytest alone is not sufficient for this integration test. tests/integration/defs/conftest.py:99-113 requires LLM_MODELS_ROOT or an available fallback model directory. The test also requires GPU access and model weights. Document these requirements in the manual command.
As per path instructions, integration tests require GPU access and model weights, and LLM_MODELS_ROOT must be set before execution.
Proposed documentation update
- pytest.
+ LLM_MODELS_ROOT=/path/to/llm-models pytest tests/integration/defs/accuracy/test_dwdp_disaggregated_serving.py📝 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.
Suggested change
| NOTE: these tests are intentionally not registered in any CI test list. DWDP | |
| accuracy is gated in CI by test_dwdp_aggregated.py, which exercises the same | |
| expert-sharing paths without the disaggregated KV cache transceiver and is | |
| therefore not exposed to per-cluster transport configuration. This file is kept | |
| as a manual reproduction of the disaggregated DWDP path; run it directly with | |
| pytest. | |
| NOTE: these tests are intentionally not registered in any CI test list. DWDP | |
| accuracy is gated in CI by test_dwdp_aggregated.py, which exercises the same | |
| expert-sharing paths without the disaggregated KV cache transceiver and is | |
| therefore not exposed to per-cluster transport configuration. This file is kept | |
| as a manual reproduction of the disaggregated DWDP path; run it directly with | |
| LLM_MODELS_ROOT=/path/to/llm-models pytest tests/integration/defs/accuracy/test_dwdp_disaggregated_serving.py |
🤖 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/integration/defs/accuracy/test_dwdp_disaggregated_serving.py` around
lines 6 - 11, Update the manual execution documentation in the test file’s
introductory NOTE to state that running it directly with pytest requires GPU
access, model weights, and LLM_MODELS_ROOT set to a valid model root (or an
available fallback directory), and include these prerequisites in the documented
command.
Source: Path instructions
Comment on lines
+13
to
+18
| When running it manually, check the launcher's UCX settings first: SLURM | ||
| enroot/pyxis injects ``UCX_TLS=tcp`` from the host MPI stack on some clusters, | ||
| which pins the KV cache transceiver to a transport that can fail there and hang | ||
| the run in ``check_gen_transfer_status``. Clear or pin ``UCX_TLS`` for the | ||
| cluster before running -- see jenkins/scripts/slurm_env_setup.sh and | ||
| examples/disaggregated/slurm/benchmark/start_worker_dwdp.sh. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the UCX override guidance.
tests/integration/defs/disaggregated/test_disaggregated.py:1000-1095 unconditionally sets run_env["UCX_TLS"] = get_ucx_tls() and propagates that value to the context and generation workers. Clearing or pinning inherited launcher UCX_TLS does not control those workers. Document the effective get_ucx_tls() policy, or add a validated override parameter before documenting manual overrides.
🤖 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/integration/defs/accuracy/test_dwdp_disaggregated_serving.py` around
lines 13 - 18, Correct the manual-run guidance to reflect that disaggregated
workers use the value returned by get_ucx_tls(), assigned through
run_env["UCX_TLS"] in the test setup. Either document that effective policy and
its configuration source, or add and validate an override parameter that
controls the context and generation workers before describing manual UCX_TLS
overrides.
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #65553 [ run ] triggered by Bot. Commit: |
Shixiaowei02
approved these changes
Aug 12, 2026
Collaborator
|
LGTM. Please address the bot comments above. Thanks! |
Collaborator
|
PR_Github #65553 [ run ] completed with state
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DWDP accuracy is currently guarded only through disaggregated serving. That couples
the feature's CI signal to a much larger system: when disaggregated serving breaks
for reasons unrelated to DWDP — cluster transport configuration, KV cache
transceiver issues, and so on — the DWDP accuracy tests break with it. The result is
that all three disaggregated DWDP accuracy tests are currently waived on GB200 and
B200, so DWDP has no effective end-to-end accuracy coverage at all.
This PR decouples the two. DWDP accuracy is guarded by aggregated serving instead,
which exercises the same expert-sharing paths without depending on disaggregation.
What changed
tests/integration/defs/accuracy/test_dwdp_aggregated.py(3 cases, GSM8K onDeepSeek-V3-Lite,
dwdp_size=4) and register it in the CI lists.create_py_executorso a single aggregated instance withattention DP is accepted. Aggregated attention DP satisfies the invariant DWDP
relies on — every rank is a complete model replica owning one expert slice —
because
Mapping.dp_size == tp_sizethere and attention is replicated rather thantensor-sharded. Real tensor parallelism is still rejected, now with an explicit
error instead of a bare assert. The change sits entirely inside the existing
if llm_args.dwdp_config is not None:branch, so non-DWDP paths are untouched.waives. The file itself is kept in tree as a manual reproduction of the
disaggregated path.
The disaggregated failures themselves are not fixed here; the related tracking bugs
(nvbugs 6276923 and 6525009) are being updated separately.
Test coverage
DeepSeek-V3-Lite has 72 routed experts and rank
rstores[r * num_prefetch_experts, r * num_prefetch_experts + num_experts_per_worker).mode_a_uniformmode_b_overlapmode_a_uniform_contention_optdwdp_size=4rather than the 2 the disaggregated tests used: aggregated serving hasno generation server, so the whole 4-GPU allocation goes to DWDP peers. Three remote
peers per rank is also what makes
contention_optmeaningful, since it interleavesprefetch slices across peers. Case count is unchanged (3 → 3).
Verification
Ran on GB200 (4 GPU) before opening this PR:
63.710 reference.
(65.011 vs 64.740 reference), so disaggregated serving is unaffected.
Test Coverage
accuracy/test_dwdp_aggregated.py::TestDwdpAggDeepSeekV3Lite::test_dwdp_agg_accuracy[mode_a_uniform]accuracy/test_dwdp_aggregated.py::TestDwdpAggDeepSeekV3Lite::test_dwdp_agg_accuracy[mode_b_overlap]accuracy/test_dwdp_aggregated.py::TestDwdpAggDeepSeekV3Lite::test_dwdp_agg_accuracy[mode_a_uniform_contention_opt]PR Checklist
/bot run)