[TRTLLM-12648][test] implement disagg cancellation load thread#15124
Conversation
1679c49 to
de5c27d
Compare
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
de5c27d to
d3906ca
Compare
|
/bot run --disable-fail-fast |
📝 WalkthroughWalkthroughThis PR implements a functional load thread for the disaggregated cancellation stress harness, converting a no-op stub into a duration-bounded loop that executes cancellation stress iterations in configurable steady or burst load patterns, records detailed per-iteration results, and handles error scenarios with fail-fast behavior. ChangesLoad Thread Feature
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 (2)
tests/integration/defs/stress_test/disagg_cancel/harness.py (1)
1227-1230: 💤 Low valueConsider catching more specific exceptions from the load generator.
The broad
Exceptioncatch here is flagged by static analysis (BLE001). While recording and failing fast on any runner error is reasonable, catchingExceptionmay mask unexpected issues likeKeyboardInterruptbeing caught (though it's aBaseException). If the underlyingrun_cancel_stress_testraises known exception types (e.g.,ConnectionError,TimeoutError,RuntimeError), consider narrowing the catch clause.That said, since this is test infrastructure and the error is both logged and triggers fail-fast, the current behavior is defensible for robustness.
🤖 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 1227 - 1230, Replace the broad "except Exception as exc:" in the load thread block with a narrow catch for the known runner errors (e.g., except (ConnectionError, TimeoutError, RuntimeError) as exc:) so you only record and call self.mark_failed for expected failures from run_cancel_stress_test; additionally add a separate except BaseException: raise to avoid swallowing system-level interrupts (KeyboardInterrupt/SystemExit). Ensure you still set record["error"] and call self.mark_failed(f"load_thread runner failed: {record['error']}") in the narrowed exception handler.Source: Coding guidelines
tests/integration/defs/stress_test/disagg_cancel/README.md (1)
81-100: 💤 Low valueOptional: Consider clarifying step numbering.
The grouping of "Steps 1-2" for log_scanner + metrics, followed by separate Steps 3, 4, 5, is understandable but slightly inconsistent. Consider either:
- Numbering all five components separately (Steps 1 through 5), or
- Using "Step 1" for the grouped command since it's executed as a single pytest invocation
The current form is functional and the commands are correct, so this is purely a readability suggestion.
🤖 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 81 - 100, The step numbering is inconsistent: "Steps 1-2 — log scanner + metrics" then separate Steps 3–5; update the README to make numbering clear by either splitting the grouped command into two numbered steps (Step 1: tests/integration/defs/stress_test/disagg_cancel/test_log_scanner.py and Step 2: tests/integration/defs/stress_test/disagg_cancel/test_metrics_thread.py) or relabel the grouped pytest invocation as a single Step (e.g., "Step 1 — log scanner + metrics (run together)") and then increment following steps accordingly (refer to the commands invoking test_log_scanner.py, test_metrics_thread.py, test_injector.py, test_canary.py, and test_load_thread.py when editing).
🤖 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 1227-1230: Replace the broad "except Exception as exc:" in the
load thread block with a narrow catch for the known runner errors (e.g., except
(ConnectionError, TimeoutError, RuntimeError) as exc:) so you only record and
call self.mark_failed for expected failures from run_cancel_stress_test;
additionally add a separate except BaseException: raise to avoid swallowing
system-level interrupts (KeyboardInterrupt/SystemExit). Ensure you still set
record["error"] and call self.mark_failed(f"load_thread runner failed:
{record['error']}") in the narrowed exception handler.
In `@tests/integration/defs/stress_test/disagg_cancel/README.md`:
- Around line 81-100: The step numbering is inconsistent: "Steps 1-2 — log
scanner + metrics" then separate Steps 3–5; update the README to make numbering
clear by either splitting the grouped command into two numbered steps (Step 1:
tests/integration/defs/stress_test/disagg_cancel/test_log_scanner.py and Step 2:
tests/integration/defs/stress_test/disagg_cancel/test_metrics_thread.py) or
relabel the grouped pytest invocation as a single Step (e.g., "Step 1 — log
scanner + metrics (run together)") and then increment following steps
accordingly (refer to the commands invoking test_log_scanner.py,
test_metrics_thread.py, test_injector.py, test_canary.py, and
test_load_thread.py when editing).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f8012131-bef8-4d15-849e-7ed2dffe484c
📒 Files selected for processing (5)
tests/integration/defs/stress_test/disagg_cancel/README.mdtests/integration/defs/stress_test/disagg_cancel/configs/marathon_cpp_v1_deepseek.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_load_thread.py
|
PR_Github #52874 [ run ] triggered by Bot. Commit: |
|
PR_Github #52874 [ run ] completed with state |
Summary
Fifth thread body in the disaggregated cancellation stress-test suite - the marathon-style harness that gates regressions of the disaggregated KV-transfer cancellation / poison failure class. The skeleton + log scanner landed in the first harness change, followed by the metrics thread, injector thread, and canary thread; this change adds the load thread.
This PR ships:
load_threadbody - replaces the no-op stub with a duration-bounded loop. The thread exits onstop_event/failed_event, runs untilstress_config.duration_minelapses, and setsstop_eventat normal end-of-marathon so canary, metrics, injector, and log-scanner threads wind down.stress_config. Steady state usesbase_concurrencyandinput_length; burst windows usebursts.interval_min,bursts.duration_s,bursts.concurrency, andbursts.input_length. Bursts begin after the first full interval so the marathon starts with a steady-state baseline.run_cancel_stress_testwrapper - calls the existing disaggregated cancellation load generator one burst at a time via_run_cancel_stress_iteration(...). The import is lazy so unit tests can monkeypatch the method without importing the heavyweight disaggregated integration module._load_recordsentry per load-generator call with{timestamp, elapsed_s, mode, num_bursts, requests_per_burst, prompt_len_range, cancel_after_range, success, error, duration_s}and exposes those records fromcollect_results()asload_records._parse_token_range,_parse_cancel_after_range, and_load_iteration_shapeto validate load-shape knobs and fail fast on malformed load config, including malformed burst timing.setup()-stub path fast: if no server endpoint is bound,load_threadlogs a warning, setsstop_event, and exits, so the marathon pytest remains a lifecycle smoke until real cluster setup lands.PR chain plan
log_scanner_threadmetrics_threadinjector_threadinjection_eventscountcanary_threadload_threadsetup_disagg_clusterinvocation withsave_log=True+ endpoint bindingThis change still does not enable the marathon in CI. Registration in
tests/integration/test_lists/qa/llm_function_stress.txtremains deferred.Why one-burst wrapper calls
run_cancel_stress_testalready owns the asyncio request-spam/cancel behavior used by existing disaggregated cancellation tests. Rather than reimplementing the HTTP client,load_threadcalls it withnum_bursts=1in a loop and lets the harness decide which load shape is active for each iteration.This keeps Step 5 small and reviewable:
client_cancel_rateoroutput_length, can land separately without restructuring the harness.Tests added
All deterministic, GPU-free, second-scale. Under
tests/integration/defs/stress_test/disagg_cancel/.test_load_thread.py- 10 unit tests across three layers:stop_event, steady iterations record successfulload_records, and externally setstop_eventstops after the current runner call.test_disagg_cancel_stress.py- lifecycle smoke now asserts theload_recordsresult bucket.Run locally (no GPU, no cluster):
Full component suite:
Validation
12 passed.89 passed.git diff --check.ruff-formatreformattedharness.py. The local commit skipped onlywaive list checkandvalidate-test-listsbecause the hook runner's Python failed while parsing the repo script annotationstr | Noneinscripts/check_test_list.py.Note: the full component suite was run outside the filesystem sandbox because existing canary/metrics tests bind local loopback HTTP servers.
Out of scope (deferred to later changes in this chain)
setup_disagg_clusterinvocation and worker/server endpoint binding.setup()is still a stub, so the lifecycle smoke does not launch a real cluster yet.run_cancel_stress_testto honorclient_cancel_rateandoutput_length; Step 5 intentionally reuses the existing cancellation load generator as-is._load_records,_canary_records,_kv_utilization_samples, and_injection_events.configs/stress_canary_prompts.jsonplustools/generate_canary_references.py.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.Summary by CodeRabbit
Tests
Documentation