[None][test] Add E2E test for cancelled disagg gen request with overlap scheduler - #11795
Conversation
📝 WalkthroughWalkthroughIntroduces MPI-based cancellation support for disaggregated worker processes. New Changes
Sequence DiagramsequenceDiagram
participant Orchestrator
participant Worker1
participant Worker2 as Worker2<br/>(support_cancel=True)
participant MPI
Orchestrator->>Worker1: Submit request indices
Orchestrator->>Worker2: Submit request indices
Worker1->>Worker1: Process request (no cancellation)
Worker2->>MPI: Signal MPI_STARTED
Orchestrator->>MPI: Send MPI_CANCEL indices
MPI->>Worker2: Receive cancellation signal
Worker2->>Worker2: Abort specified futures
Worker1->>Orchestrator: Send results (normal path)
Worker2->>Orchestrator: Send results (cancellation path)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/defs/disaggregated/test_disaggregated_single_gpu.py`:
- Line 183: Replace the two occurrences of "raise e" in the test's except blocks
with a bare "raise" so the original exception traceback and failure location are
preserved; locate the except handlers in
tests/integration/defs/disaggregated/test_disaggregated_single_gpu.py that
currently re-raise the caught exception with "raise e" and change them to
"raise" without altering any surrounding logic or variable names.
- Around line 682-683: The test relies on a default for
disable_overlap_scheduler; make it explicit by adding the
disable_overlap_scheduler key to the generation worker config appended to
worker_pytorch_configs (i.e., include disable_overlap_scheduler: False or True
as appropriate in the dict added to worker_pytorch_configs where
cuda_graph_config is set to None) so the cancel-path coverage is stable and the
test intent is clear.
- Around line 766-768: The except block in the test (around the
try/except/finally in test_disaggregated_single_gpu.py) is catching Exception,
printing it, and swallowing it which hides assertion failures; change this so
you either remove the broad try/except and let the test framework surface
failures, or catch only expected specific exceptions and after logging re-raise
the exception (i.e., log the error in the except and then raise) to ensure
assertions and other errors fail the test while keeping the finally cleanup
intact.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration/defs/disaggregated/test_disaggregated_single_gpu.pytests/integration/test_lists/test-db/l0_h100.yml
2f24552 to
094d29c
Compare
|
/bot run --disable-fail-fast |
…ap scheduler Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
094d29c to
685d069
Compare
|
PR_Github #37125 [ run ] triggered by Bot. Commit: |
|
PR_Github #37125 [ run ] completed with state |
…ap scheduler (NVIDIA#11795) Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
…ap scheduler (NVIDIA#11795) Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
…ap scheduler (NVIDIA#11795) Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
Summary by CodeRabbit
New Features
Tests
Description
Add an integration test that exercises the cancel path for disaggregated generation-only requests when the overlap scheduler is enabled. This test catches the bug where a cancelled gen-only request with
py_decoding_iter == 0gets permanently stuck inactive_requestsdue to the overlap scheduler re-adding it without checkingrequest.is_finished.PR Checklist