[TRTLLM-12648][test] implement disagg cancellation canary thread#15015
Conversation
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
…tests Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
…d tests Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. 📝 WalkthroughWalkthroughThis PR implements a canary thread for the disaggregated cancellation stress harness that sends deterministic greedy OpenAI-compatible completions and records per-request outcomes with optional token-equivalence validation. The work spans harness helpers, configuration, thread implementation, and comprehensive integration tests. ChangesCanary Thread Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/defs/stress_test/disagg_cancel/test_canary.py (1)
22-22: 💤 Low valueOptional:
from __future__ import annotationsis not needed.TensorRT-LLM targets Python 3.10+ (as per
setup.py), so PEP 585 generics and PEP 604 union syntax (str | None) work natively without the__future__import. Removing this line would be consistent with the project's Python version requirements. However, the import is harmless and does not cause issues.Based on learnings, you can use Python 3.10+ features throughout the codebase without adding
from __future__ import annotations.🤖 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_canary.py` at line 22, Remove the unnecessary future import line "from __future__ import annotations" at the top of the test module (tests/integration/defs/stress_test/disagg_cancel/test_canary.py); since the project targets Python 3.10+, PEP 585/604 features are available natively, so delete that import to keep the file consistent with the codebase style.
🤖 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/test_canary.py`:
- Line 22: Remove the unnecessary future import line "from __future__ import
annotations" at the top of the test module
(tests/integration/defs/stress_test/disagg_cancel/test_canary.py); since the
project targets Python 3.10+, PEP 585/604 features are available natively, so
delete that import to keep the file consistent with the codebase style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b3bbf97c-27f0-4226-90e2-0d4f2c8e6538
📒 Files selected for processing (3)
tests/integration/defs/stress_test/disagg_cancel/README.mdtests/integration/defs/stress_test/disagg_cancel/harness.pytests/integration/defs/stress_test/disagg_cancel/test_canary.py
|
PR_Github #52429 [ run ] triggered by Bot. Commit: |
|
PR_Github #52429 [ run ] completed with state |
…DIA#15015) Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
…te for PR NVIDIA#15015 merge PR NVIDIA#15015 (canary_thread) merged on 2026-06-08; 4 of 5 thread bodies now in upstream/main. The two phase0 docs (spec + implementation plan) were originally split as WHAT-to-build vs HOW-to-build, which made sense while implementing. Now that the implementation is mostly done, the impl plan is mostly historical record and its still-useful content (decisions beyond spec, risk mitigations) folds into the spec as an Implementation notes appendix. Changes: * Delete phase0-implementation-plan.md (252 lines). * phase0-stress-test-suite.md status header: skeleton + 4 thread bodies merged (only load_thread + marathon entry pending). * PR chain table: mark PR NVIDIA#14920 (injector) and PR NVIDIA#15015 (canary) as merged with dates. * Replace 'Open questions for the implementing agent' (all TBD at writing, all resolved during implementation) with an 'Implementation notes' section capturing decisions beyond the spec (serial marathon execution, per-thread PR split not per-marathon, kill-only SIGKILL fallback, canary token-equivalence fallback chain, --smoke mode) and the risks the implementation pinned in code. * Rewrite 'Acceptance criteria' from single-PR checklist to cumulative PR-chain checklist with per-PR landed-in attribution. No README updates needed — the parent README's 'Detailed design documents' section already references only the spec, not the impl plan. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Summary by CodeRabbit
New Features
Documentation
Tests
Summary
Fourth thread body in the disaggregated cancellation stress-test suite — the marathon-style (hours-running) harness that gates regressions of the bug class fixed by PR #13713. The skeleton +
log_scanner_threadlanded in PR #14375, themetrics_threadin PR #14807, and theinjector_threadin PR #14920; this PR adds thecanary_thread.This PR ships:
canary_threadbody — loadscanary.prompts_file(resolved relative to the marathon YAML), then sends deterministic greedy completions to the disagg server's/v1/completionsatcanary.rate_per_mincadence and appends a per-request record to_canary_records:{timestamp, elapsed_s, prompt_index, success, token_equivalent, latency_s, error}. Request failures are recorded — not fail-fast — because error spikes during bursts / injections are expected; only the log scanner is fail-fast. The end-of-marathon gates (canary error rate, recovery time) are computed from these records in a later step._send_canary_request—urllibPOST withtemperature=0.0+ fixedseedfor greedy determinism anddetokenize=Falseso the response carries generatedtoken_idsonchoices[0](seeCompletionResponseChoice.token_idsintensorrt_llm/serve/openai_protocol.py). Every transport/parse failure (HTTP error, connection refused, timeout, malformed body) folds into(None, None, error_string)._load_canary_prompts— validates thestress_canary_prompts.jsonschema ({"prompts": [{"prompt", "reference_token_ids", "reference_text?"}, ...]}) and raises a clearValueErroron malformed input._tokens_equivalent— exact token-ID list equality; aNoneon either side (no tokens returned, or no reference recorded) is non-equivalent.bind_server_endpoint(server_url, model_name)— registers the disagg front-end the canary targets, mirroringbind_tracked_workers. Called bysetup()oncesetup_disagg_clusterreturns; settable by tests today.canary_request_timeout_s/canary_interval_s— new ctor parameters onDisaggCancellationStressHarness, matching the existing per-thread tunable pattern (log_scanner_poll_interval_s,metrics_scrape_interval_s,injector_poll_interval_s). Tests pass small values; the marathon derives the interval fromcanary.rate_per_min.The thread sits inside the lifecycle wired up by PR [TRTLLM-12648][test] add disagg cancellation stress-test harness skeleton #14375 —
start()already spawns it,stop()joins it,collect_results()already returnscanary_records. Previous PRs left it as a no-op stub; this PR makes it actually send canaries.PR chain plan
The final PR flips
setup_disagg_clusterto launch workers with explicit per-worker ports +save_log=Trueand callsbind_tracked_workers()/bind_server_endpoint()— at which point the canary thread streams against a live server. Today it correctly no-ops when its inputs are absent: with the cluster launch still stubbed, no server endpoint is bound and the thread logs a warning and exits.Why
urllib, not asyncioThe class docstring previously said the canary thread would run its own asyncio loop. At 5 req/min (one request at a time) asyncio buys nothing and complicates testing, so the canary uses blocking
urlliblike the mergedmetrics_thread— which makes it unit-testable against an in-processhttp.server. The class docstring is updated accordingly; the load thread (next PR) still runs asyncio internally viarun_cancel_stress_test.Tests added
All deterministic, GPU-free, second-scale. Under
tests/integration/defs/stress_test/disagg_cancel/.test_canary.py— 21 unit tests across three layers:prompts/ non-listprompts/ entry missing stringpromptall raiseValueError.None-on-either-side.token_ids; HTTP 503 →http_error; connection refused →url_error; malformed body →parse_error.token_equivalentTrue/False/None (match / mismatch / no-reference / check disabled), error recorded when server down, prompt round-robin, and prompt exit on no endpoint / missing prompts file /failed_event.Run locally (no GPU, no cluster):
PYTHONPATH=tests/integration/defs:tests/integration/defs/disaggregated \ python3 -m pytest -c /dev/null -o addopts= \ --confcutdir=tests/integration/defs/stress_test \ tests/integration/defs/stress_test/disagg_cancel/test_canary.py -vOut of scope (deferred to later PRs in this chain)
load_threadbody + realsetup_disagg_clusterinvocation (cluster launch is still a stub, so no server endpoint is bound in the lifecycle smoke; the canary thread logs a warning and exits)._canary_records(overall error rate < 1%, per-injection-window < 10%, recovery time < 30 s).configs/stress_canary_prompts.json+tools/generate_canary_references.py(Step 6 — one-shot greedy-decode reference generation).canary_request_timeout_s/canary_interval_s(constructor-only for now; the marathon derives cadence fromcanary.rate_per_min).tests/integration/test_lists/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.