[None][test] Add KV transfer lifecycle acceptance regressions - #19663
chienchunhung wants to merge 4 commits into
Conversation
8e811e3 to
0216388
Compare
Cover cancellation and timeout retention with CPU regression controls and a seven-case native GPU/NIXL suite across separate CTX and GEN MPI worlds. Include allocation-reuse checks, bounded software containment, B200 test mapping, and an explicit qualification coverage ledger. Signed-off-by: Chien-Chun Hung <[email protected]>
Signed-off-by: Chien-Chun Hung <[email protected]>
Signed-off-by: Chien-Chun Hung <[email protected]>
Signed-off-by: Chien-Chun Hung <[email protected]>
416a386 to
a16bc40
Compare
|
/bot run --disable-fail-fast |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/TensorRT-LLM/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThis change adds acceptance documentation and CPU tests for receive cancellation and resource reuse. It adds GPU/NIXL lifecycle qualification and process-exit regression tests, then includes the GPU test in the four-GPU B200 pre-merge list. ChangesTransfer lifecycle qualification
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~60 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds only tests and documentation for KV transfer lifecycle behavior, plus a four-GPU B200 CI registration. It does not change production code. No concrete merge-blocking issue was found; the remaining step is a passing full CI run. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 42 functions across 3 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR_Github #76115 [ run ] triggered by Bot. Commit: |
|
PR_Github #76115 [ run ] completed with state
|
|
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements. |
|
/bot run --disable-fail-fast |
|
PR_Github #76145 [ run ] triggered by Bot. Commit: |
|
PR_Github #76145 [ run ] completed with state
|
| ) | ||
| if role == "ctx" and rank == 0: | ||
| _record(directory, "endpoint", endpoint=transceiver._context_info_endpoint) | ||
| _wait(lambda: (directory / "endpoint.json").exists(), "CTX endpoint") |
There was a problem hiding this comment.
Workers time out after 30 seconds, while the supervisor allows 120 seconds for startup. Use a shared startup deadline so healthy initialization delays cannot cause ready workers to exit prematurely.
|
|
||
| def lookup(pid: int) -> psutil.Process: | ||
| """Return the selected original rank, or its observed disappearance.""" | ||
| if pid == 101 or scenario == "absent" or (scenario == "exits_later" and now[0] > 0): |
There was a problem hiding this comment.
Rank 1 is always simulated as absent, so these cases still pass if the observer checks only rank 0. Add mirrored cases where rank 1 remains alive or becomes unobservable.
This branch has not been deployed
5 participants
Summary
Add test-only lifecycle qualification on top of merged #19674: KV/AUX memory must remain owned until physical access settles, even after logical cancellation or timeout. Rebased onto main
f7518a6427; 5 files, +1,679 lines, no production changes.Major changes
Verification
a16bc402a7: self-review clean; full CI requested, runtime results pending. Added coverage: 20 pass-required cases (7 GPU + 13 CPU) and 2 strict XFAILs. Multi-GPU execution requiresci: full pre-merge approved; a green run that skips those stages is insufficient.668e49d2eb, ComputeLab 4661843, four B200s: GPU 6/7 passed; the destination-fatal process-exit assertion required the identity-aware observer correction now included. The broader CPU selection reported 127 passed, 2 XFAILs; both explicit--runxfailnegative controls failed at the intended premature-reuse assertion. These results do not validate the current revision.Qualification boundary
The harness uses production cancellation/cleanup methods on a model-free executor shell, not full serving. Completion masking begins after native NIXL DONE: it tests software retention/containment, not outstanding-DMA fencing. Dense FP4 MLA serving, fully initialized executor cleanup, platform fencing, and safe replacement remain separate qualification requirements. Expected failures document gaps, not acceptance.
Dev Engineer Review
DONE. It does not prove safety while DMA remains active, GPU/RDMA fencing, fully initialized executor cleanup, or safe replacement.QA Engineer Review
tests/integration/test_lists/test-db/l0_dgx_b200.yml. No manual-QA list entry is reported.Per-File QA Perspective
tests/integration/test_lists/test-db/l0_dgx_b200.yml— Addsunittest/disaggregated/test_transfer_lifecycle_gpu.pyto the four-GPU B200 pre-merge list. Verify the CI selector runs this test on the intended hardware.tests/unittest/disaggregated/lifecycle_acceptance.md— Documents the acceptance profile, coverage boundaries, execution commands, and remaining qualification gaps. Verify the documented commands and status against current CI evidence.tests/unittest/disaggregated/test_transfer_lifecycle_acceptance.py— Adds CPU-only cancellation and timeout regressions, including retention until KV and AUX accessors settle, plus strict expected failures for default-off behavior. The ledger says existing CPU CI selects this directory; this file is not listed in the B200 GPU CI entry.tests/unittest/disaggregated/test_transfer_lifecycle_gpu.py— Adds seven real GPU/NIXL cases using separate MPI CTX/GEN worlds and allocator pressure. It is listed in the four-GPU B200 pre-merge CI; verify current hardware execution and retain the stated boundary that masked completion does not model active DMA.tests/unittest/disaggregated/test_transfer_lifecycle_process_exit.py— Adds parameterized checks for bounded rank-exit observation and process identity cases before cleanup. The ledger reports CPU selection of the disaggregated unittest directory; this file is not listed in the B200 GPU CI entry.