[None][test] Replace disaggregated DWDP accuracy tests with aggregated coverage - #17546
Conversation
d9fdec3 to
3d36a19
Compare
3d36a19 to
42c5352
Compare
|
/bot run --disable-fail-fast |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 11 remain after this review. WalkthroughDWDP initialization now validates supported aggregated attention-DP layouts. MoE chunk sizing supports non-communicating DWDP execution. DeepSeek-V3-Lite aggregated accuracy tests cover three expert layouts and are registered for QA and GB200 runs. ChangesDWDP aggregated serving
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR replaces disaggregated DWDP accuracy coverage with aggregated coverage and updates the related execution and chunking paths; no actionable merge-blocking risk remains based on the supplied evidence. Possibly related PRs
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: 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
|
/bot run --disable-fail-fast |
|
PR_Github #65553 [ run ] triggered by Bot. Commit: |
3e97ba9 to
6f89bf9
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #66425 [ run ] triggered by Bot. Commit: |
|
PR_Github #66425 [ run ] completed with state
|
6f89bf9 to
c4e5e1a
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. |
|
/bot run --disable-fail-fast |
|
PR_Github #66669 [ run ] triggered by Bot. Commit: |
|
PR_Github #66669 [ run ] completed with state
|
…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>
Reject pipeline and context parallelism for DWDP. The previous tp_size == 1 assert never covered them, yet a pipeline or context parallel rank is a shard of a replica rather than a replica, and dwdp_rank = global_mpi_rank() % dwdp_size would pair ranks holding different layers as DWDP peers -- wrong expert weights instead of a startup error. Test the tensor-parallel invariant directly as dp_size == tp_size instead of using enable_attention_dp as a proxy for it. The two are equivalent while Mapping.dp_size derives from that flag, but a future partial attention DP would let the flag admit a tensor-sharded rank. Assert in the aggregated test that the LLM still carries dwdp_config. create_py_executor either honours the config or raises, so this distinguishes a DWDP run from one where the config was dropped and MoE silently fell back to the normal parallel path -- which is also correct and would score the same. Keep the disaggregated tests in the QA list under their existing waive rather than dropping them entirely. The file imports helpers from test_disaggregated_serving.py and would rot unnoticed if nothing collected it, and keeping the waive makes dropping it the natural signal that the disaggregated path is healthy again. Only the pre-merge list entry is removed, so pre-merge no longer depends on disaggregated serving. Also note the world-size relationship the test relies on, and annotate the test parameters. Signed-off-by: tianyuz-nv <tianyuz@nvidia.com>
`ConfigurableMoE.calculate_num_chunks` only recognised two shapes: DP with a
comm strategy (rows = num_dp_ranks * max_tokens_per_rank after dispatch) and
non-DP, which asserts the caller passed a single-element `all_rank_num_tokens`.
DWDP is a third shape. It prefetches expert weights to every rank instead of
dispatching tokens to experts, so `_create_comm_strategy_auto` returns None and
the non-DP branch is taken. Disaggregated context workers pass its assert only
because they run tp_size=1, so the list has one entry anyway. Aggregated serving
with attention DP passes one entry per DP rank, the assert fires, and the
executor worker dies during attention warmup:
non-DP path expects a single-element list, got 4
Size the chunks from `max(all_rank_num_tokens)` when DWDP is on: without a
dispatch a rank only ever processes its own tokens, never more.
The branch is keyed off `enable_dwdp` rather than `comm is None` so that no
non-DWDP configuration changes the branch it takes. `enable_dwdp` is assigned
once in `__init__`, before both the comm strategy and the scheduler are built,
and implies `comm is None`, so the new branch is always reached when DWDP is on
and never reachable otherwise. Single-element lists are unaffected either way,
since `max([n]) == n`.
Signed-off-by: tianyuz-nv <tianyuz@nvidia.com>
c4e5e1a to
e8cfdcd
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #66928 [ run ] triggered by Bot. Commit: |
|
PR_Github #66928 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #67033 [ run ] triggered by Bot. Commit: |
|
PR_Github #67033 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #67247 [ run ] triggered by Bot. Commit: |
|
PR_Github #67247 [ run ] completed with state |
|
Hi @brnguyen2 thanks for your feedback. I’ve addressed your comments. Could you please take another look and approve the PR if everything looks good? |
11 participants
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.
ConfigurableMoE.calculate_num_chunksfor DWDP. DWDP prefetches expert weightsinstead of dispatching tokens, so it has no comm strategy and falls into the non-DP
branch, which asserts a single-element
all_rank_num_tokens. Disaggregated contextworkers slip past that assert only because they run
tp_size=1; aggregatedattention DP passes one entry per DP rank, so the assert fires and the executor
worker dies during attention warmup. Chunks are now sized from
max(all_rank_num_tokens)when DWDP is on: without a dispatch a rank onlyprocesses its own tokens, never more. The branch is keyed off
enable_dwdpratherthan
comm is None, so no non-DWDP configuration changes the branch it takes.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.
That run predates the
calculate_num_chunksfix, which is why it did not catch theassert: the workspace it used still had the pre-#15397 chunking code, where the
comm is Nonebranch summedall_rank_num_tokensinstead of rejecting amulti-element list. The fix is exercised by the same three tests in pre-merge CI.
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
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.
Dev Engineer Review
dwdp_size=4.contention_optexpert layouts.create_py_executorvalidation for supported DWDP combinations.L0_MergeRequest_PRpipelines.QA Engineer Review
TestDwdpAggDeepSeekV3Lite.test_dwdp_agg_accuracy.tests/integration/test_lists/test-db/l0_gb200_multi_gpus.yml.tests/integration/test_lists/qa/llm_function_core.txt.