[TRTLLM-12648][test] add disagg cancellation stress-test harness skeleton - #14375
Merged
chienchunhung merged 4 commits intoMay 28, 2026
Merged
Conversation
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
chienchunhung
changed the title
…validate sanity Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #49515 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #49515 [ run ] completed with state |
chienchunhung
marked this pull request as ready for review
May 21, 2026 01:48
chienchunhung
enabled auto-merge (squash)
May 21, 2026 01:50
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new disaggregated KV-transfer cancellation stress-test suite. It adds documentation and module setup, configuration parsing with YAML schema validation, two concrete marathon test configurations (C++ V1 and Python V2 backends), a harness class orchestrating five worker threads with event-based coordination and log-scanner fail-fast detection, integration tests exercising the harness, and comprehensive unit/integration tests for the log-scanner component. ChangesDisaggregated Cancellation Stress-Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (5)
tests/integration/defs/stress_test/disagg_cancel/test_log_scanner.py (2)
455-455: ⚡ Quick winReplace the
type: ignorewith a narrow non-None guard.This suppresses type checking where a direct assertion can make the invariant explicit and preserve checker value.
As per coding guidelines, “avoid using `# type: ignore` annotations unless absolutely necessary.”Proposed change
def test_log_source_poll_returns_false_when_file_absent(tmp_path: Path): spec = _make_spec("ctx", 0, tmp_path / "missing.log") - src = _LogSource(spec=spec, path=Path(spec.log_path)) # type: ignore[arg-type] + assert spec.log_path is not None + src = _LogSource(spec=spec, path=Path(spec.log_path))🤖 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/stress_test/disagg_cancel/test_log_scanner.py` at line 455, The construction of _LogSource uses a broad "# type: ignore[arg-type]" to silence a possibly None log_path; instead add an explicit non-None guard for spec.log_path (e.g., assert or raise) before building Path and creating _LogSource so the type checker can see the invariant; specifically, check spec.log_path is not None (or raise a ValueError) just above the _LogSource(...) call so you can call Path(spec.log_path) without the type: ignore.
86-87: ⚡ Quick winAdd an explicit return type to the fixture.
harness_with_two_workersis missing a return type annotation; adding it keeps typing expectations consistent and clearer for fixture consumers.As per coding guidelines, “always annotate Python functions with return types (use `None` if no return); make return type explicit.”Proposed change
`@pytest.fixture` -def harness_with_two_workers(tmp_path: Path): +def harness_with_two_workers( + tmp_path: Path, +) -> tuple[DisaggCancellationStressHarness, Path, Path]:🤖 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/stress_test/disagg_cancel/test_log_scanner.py` around lines 86 - 87, Add an explicit return type annotation to the pytest fixture function harness_with_two_workers: annotate the function with the concrete type it yields (or a generator type) instead of leaving it untyped — e.g. change the signature to include -> Generator[HarnessType, None, None] or -> HarnessType (or the actual fixture class/type used in tests) so the fixture's return/yield type is explicit and consistent with typing guidelines.tests/integration/defs/stress_test/disagg_cancel/test_disagg_cancel_stress.py (1)
59-60: Register this new integration test in CI/QA test lists before relying on it.This adds a new integration test under
tests/integration/defs/; please add a follow-up entry in the authoritative CI test-db list and the matching QA schedule list (likelytests/integration/test_lists/qa/llm_function_core.txtfor single-node functional coverage), otherwise regressions here may not be exercised regularly.As per coding guidelines, “If the change adds or materially alters an integration test under tests/integration/defs/, call out whether an entry is needed under tests/integration/test_lists/qa/.”
🤖 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/stress_test/disagg_cancel/test_disagg_cancel_stress.py` around lines 59 - 60, This new integration test function test_disagg_cancellation_marathon (parametrized by _MARATHON_CONFIGS) must be registered with our CI/QA test lists: add an entry for this test to the authoritative CI test-db and also add it to the single-node functional QA schedule list (the QA list that covers LLM function core tests) so it runs regularly; ensure the test identifier matches test_disagg_cancellation_marathon and include any required tags/labels used by the test-runner so it is picked up by CI/QA automation.tests/integration/defs/stress_test/disagg_cancel/README.md (1)
32-42: 💤 Low valueSpecify language for fenced code block.
The directory tree at lines 32-42 should specify a language identifier (e.g.,
textor leave empty) to satisfy markdown linters.📝 Proposed fix
-``` +```text tests/integration/defs/stress_test/disagg_cancel/🤖 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/stress_test/disagg_cancel/README.md` around lines 32 - 42, The fenced code block showing the directory tree in README.md lacks a language tag required by markdown linters; update the block (the triple-backtick block that contains the tree under tests/integration/defs/stress_test/disagg_cancel/) to include a language identifier such as text (e.g., replace ``` with ```text) so the tree block is properly annotated.tests/integration/defs/stress_test/disagg_cancel/harness.py (1)
92-94: ⚡ Quick winConsider adding field-specific error context for type coercion failures.
If a YAML field contains invalid data (e.g.,
duration_min: "not-a-number"), the current code will raise a genericValueErrorfromint()without indicating which field caused the failure. Wrapping the coercion in a try-except that re-raises with field context would improve the debugging experience.♻️ Proposed enhancement
for name, coerce in _STRESS_CONFIG_COERCERS.items(): if name in sc: - kwargs[name] = coerce(sc[name]) + try: + kwargs[name] = coerce(sc[name]) + except (ValueError, TypeError) as exc: + raise ValueError( + f"Invalid value for stress_config.{name}: {sc[name]!r} " + f"(expected {coerce.__name__})" + ) from exc🤖 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/stress_test/disagg_cancel/harness.py` around lines 92 - 94, Wrap each call to the coercer for entries in _STRESS_CONFIG_COERCERS in a try/except to catch conversion errors and re-raise a new ValueError that includes the field name (the loop variable name), the offending raw value (sc[name]) and the original exception message; ensure you reference the same variables (name, coerce, sc, kwargs) so the new error gives clear field-specific context while preserving or chaining the original exception.
🤖 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.
Nitpick comments:
In `@tests/integration/defs/stress_test/disagg_cancel/harness.py`:
- Around line 92-94: Wrap each call to the coercer for entries in
_STRESS_CONFIG_COERCERS in a try/except to catch conversion errors and re-raise
a new ValueError that includes the field name (the loop variable name), the
offending raw value (sc[name]) and the original exception message; ensure you
reference the same variables (name, coerce, sc, kwargs) so the new error gives
clear field-specific context while preserving or chaining the original
exception.
In `@tests/integration/defs/stress_test/disagg_cancel/README.md`:
- Around line 32-42: The fenced code block showing the directory tree in
README.md lacks a language tag required by markdown linters; update the block
(the triple-backtick block that contains the tree under
tests/integration/defs/stress_test/disagg_cancel/) to include a language
identifier such as text (e.g., replace ``` with ```text) so the tree block is
properly annotated.
In
`@tests/integration/defs/stress_test/disagg_cancel/test_disagg_cancel_stress.py`:
- Around line 59-60: This new integration test function
test_disagg_cancellation_marathon (parametrized by _MARATHON_CONFIGS) must be
registered with our CI/QA test lists: add an entry for this test to the
authoritative CI test-db and also add it to the single-node functional QA
schedule list (the QA list that covers LLM function core tests) so it runs
regularly; ensure the test identifier matches test_disagg_cancellation_marathon
and include any required tags/labels used by the test-runner so it is picked up
by CI/QA automation.
In `@tests/integration/defs/stress_test/disagg_cancel/test_log_scanner.py`:
- Line 455: The construction of _LogSource uses a broad "# type:
ignore[arg-type]" to silence a possibly None log_path; instead add an explicit
non-None guard for spec.log_path (e.g., assert or raise) before building Path
and creating _LogSource so the type checker can see the invariant; specifically,
check spec.log_path is not None (or raise a ValueError) just above the
_LogSource(...) call so you can call Path(spec.log_path) without the type:
ignore.
- Around line 86-87: Add an explicit return type annotation to the pytest
fixture function harness_with_two_workers: annotate the function with the
concrete type it yields (or a generator type) instead of leaving it untyped —
e.g. change the signature to include -> Generator[HarnessType, None, None] or ->
HarnessType (or the actual fixture class/type used in tests) so the fixture's
return/yield type is explicit and consistent with typing guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 114aaa5e-fb57-4401-b5d5-58405ae1b6b9
📒 Files selected for processing (8)
tests/integration/defs/stress_test/disagg_cancel/README.mdtests/integration/defs/stress_test/disagg_cancel/__init__.pytests/integration/defs/stress_test/disagg_cancel/configs/README.mdtests/integration/defs/stress_test/disagg_cancel/configs/marathon_cpp_v1_deepseek.yamltests/integration/defs/stress_test/disagg_cancel/configs/marathon_python_v2_qwen.yamltests/integration/defs/stress_test/disagg_cancel/harness.pytests/integration/defs/stress_test/disagg_cancel/test_disagg_cancel_stress.pytests/integration/defs/stress_test/disagg_cancel/test_log_scanner.py
pcastonguay
reviewed
May 21, 2026
…tress harness Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #49791 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #49791 [ run ] completed with state |
xinhe-nv
reviewed
May 22, 2026
StanleySun639
requested changes
May 28, 2026
StanleySun639
approved these changes
May 28, 2026
bmarimuthu-nv
pushed a commit
to nv-auto-deploy/TensorRT-LLM
that referenced
this pull request
May 28, 2026
…eton (NVIDIA#14375) Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
This was referenced Jun 5, 2026
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
5 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 by CodeRabbit
Tests
Documentation
Summary
Skeleton + first thread body of a marathon-style (hours-running) disaggregated cancellation stress-test suite. Gates regressions of the bug class fixed by PR #13713: requests getting cancelled due to timeout or failures under high traffic.
This PR ships:
setup → start → wait_until_done → stop → collect_results), 5-thread architecture, fail-fast plumbing,StressConfigYAML parser, the C++ transceiver marathon (marathon_cpp_v1_deepseek.yaml: V1 KV cache + C++ transceiver + NIXL + DeepSeek-V3-Lite/bf16), and a symmetric Python transceiver marathon (marathon_python_v2_qwen.yaml: V2 KV cache + Python transceiver + NIXL + Qwen2.5-7B-Instruct) as a ready-to-wire template.log_scannerthread body — tails each worker's stdout/stderr against configurable hard-zero regex patterns (Broken promise,Segfault,Poisoned .* cache transfer buffer, …). On a hit, trips fail-fast with a structured reason that surfaces incollect_results(). Handles block-buffered C++ writes via a carry buffer; survives missing / late-created log files.The harness is a thin coordinating layer over the existing disagg-test infrastructure under
tests/integration/defs/disaggregated/: cluster launch will delegate tosetup_disagg_cluster, cancellation load generation will wraprun_cancel_stress_test, and per-worker subprocess management reusesProcessWrapper(itslog_pathfield is whatWorkerLaunchSpec.log_pathreceives). Marathon YAMLs extend the existingdisagg_config_cancel_stress_test*.yamlschema with a newstress_config:top-level block; thehostname / model / backend / context_servers / generation_serverskeys pass through tosetup_disagg_clusterunchanged.The marathon entry point (
test_disagg_cancellation_marathon) currently exercises only lifecycle plumbing + log-scan integration; content-level assertions accumulate as subsequent thread bodies land. Not registered in any test list yet — registration lands together with the load thread.PR chain plan
Staged delivery — each follow-up adds one thread body against the harness contract locked in by this PR. Reviewable in isolation, each layer earning a new marathon assertion in the same
test_disagg_cancellation_marathonentry point.log_scannermetrics_threadinjector_threadinjection_eventscountcanary_threadload_thread+ realsetup()withsave_log=Trueduration_min; marathon entry point becomes a real marathonPR 5 also flips
setup_disagg_clustertosave_log=Truewith explicit per-worker ports — at which point thelog_scannershipped here starts catching real worker logs (today it correctly no-ops with a warning whenlog_pathisNone).Tests added
All deterministic, GPU-free, millisecond-scale. Under
tests/integration/defs/stress_test/disagg_cancel/.test_disagg_cancel_stress.py— 1 lifecycle smoke (test_disagg_cancellation_marathon, parametrized on the C++ transceiver marathon) + 1test_all_marathon_yamls_parse_and_validatethat walks the configs directory and validates everymarathon_*.yamlagainst theStressConfigschema (covers both backends, including configs not yet parametrized).test_log_scanner.py— 17 unit tests covering pattern matching, block-buffered partial-line carry, multi-worker tailing,stop_event/failed_event/ file-created-late lifecycle paths, misconfig (empty / invalid patterns, missinglog_path, no specs), and_LogSourceinternal guarantees.Out of scope (deferred to later PRs in this chain)
setup_disagg_clusterinvocation (cluster launch is still a stub).metrics,injector,canary,load.stress_canary_prompts.json.qa/llm_function_stress.txtfor nightly / weekly CI.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.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.