[None][feat] Dev scaffolding bench load_generator #8559

dcaox · 2025-10-22T01:44:36Z

Summary by CodeRabbit

New Features
- Introduced load generation strategy framework for flexible benchmark load patterns.
- Added multiple strategy types: Synchronous, Concurrent, Throughput, Constant Rate, and Poisson Rate.
- Enhanced benchmarking output to include per-request timing metrics.
Tests
- Updated test suite to validate load generation strategies and timing calculations.

Description

Test Coverage

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)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
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

/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...

Provide a user friendly way for developers to interact with a Jenkins server.

Run /bot [-h|--help] to print this help message.

See details below for each supported subcommand.

run [--reuse-test (optional)pipeline-id --disable-fail-fast --skip-test --stage-list "A10-PyTorch-1, xxx" --gpu-type "A30, H100_PCIe" --test-backend "pytorch, cpp" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" --detailed-log --debug(experimental)]

Launch build/test pipelines. All previously running jobs will be killed.

--reuse-test (optional)pipeline-id (OPTIONAL) : Allow the new pipeline to reuse build artifacts and skip successful test stages from a specified pipeline or the last pipeline if no pipeline-id is indicated. If the Git commit ID has changed, this option will be always ignored. The DEFAULT behavior of the bot is to reuse build artifacts and successful test results from the last pipeline.

--disable-reuse-test (OPTIONAL) : Explicitly prevent the pipeline from reusing build artifacts and skipping successful test stages from a previous pipeline. Ensure that all builds and tests are run regardless of previous successes.

--disable-fail-fast (OPTIONAL) : Disable fail fast on build/tests/infra failures.

--skip-test (OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.

--stage-list "A10-PyTorch-1, xxx" (OPTIONAL) : Only run the specified test stages. Examples: "A10-PyTorch-1, xxx". Note: Does NOT update GitHub check status.

--gpu-type "A30, H100_PCIe" (OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.

--test-backend "pytorch, cpp" (OPTIONAL) : Skip test stages which don't match the specified backends. Only support [pytorch, cpp, tensorrt, triton]. Examples: "pytorch, cpp" (does not run test stages with tensorrt or triton backend). Note: Does NOT update GitHub pipeline status.

--only-multi-gpu-test (OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.

--disable-multi-gpu-test (OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.

--add-multi-gpu-test (OPTIONAL) : Force run the multi-GPU tests in addition to running L0 pre-merge pipeline.

--post-merge (OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.

--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" (OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx".

--detailed-log (OPTIONAL) : Enable flushing out all logs to the Jenkins console. This will significantly increase the log volume and may slow down the job.

--debug (OPTIONAL) : Experimental feature. Enable access to the CI container for debugging purpose. Note: Specify exactly one stage in the stage-list parameter to access the appropriate container environment. Note: Does NOT update GitHub check status.

For guidance on mapping tests to stage names, see docs/source/reference/ci-overview.md
and the scripts/test_to_stage_mapping.py helper.

kill

kill

Kill all running builds associated with pull request.

skip

skip --comment COMMENT

Skip testing for latest commit on pull request. --comment "Reason for skipping build/test" is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.

reuse-pipeline

reuse-pipeline

Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.

coderabbitai · 2025-10-22T01:51:44Z

📝 Walkthrough

Walkthrough

The changes introduce a load generation strategy framework for TensorRT-LLM benchmarking, replacing simple concurrency controls with multiple configurable strategies (synchronous, concurrent, throughput-based, constant-rate, and Poisson-rate). The benchmark function now accepts optional strategies and returns per-request start times alongside execution times.

Changes

Cohort / File(s)	Summary
New load generation strategy framework `tensorrt_llm/scaffolding/load_generation_strategy.py`	New module defining abstract `LoadGenerationStrategy` base class with six concrete implementations: `SynchronousStrategy`, `ConcurrentStrategy`, `ThroughputStrategy`, `ConstantRateStrategy`, `PoissonRateStrategy`, and `_UnlimitedSemaphore`. Strategies provide semaphore management and request timing generation for different load patterns.
Benchmark refactoring `tensorrt_llm/scaffolding/benchmark.py`	Updated `process_request` to accept typed `scaffolding_llm` parameter and new `target_time` parameter for per-request scheduling. Modified `run_scaffolding_llm` to accept `LoadGenerationStrategy` instead of raw `concurrency`. Updated `async_scaffolding_benchmark` to accept optional `strategy` and `concurrency` parameters, with fallback logic to construct `ConcurrentStrategy` from concurrency value. Return type expanded to include `requests_start_time` list. Added `LoadGenerationStrategy` import.
Test updates `tests/unittest/scaffolding/test_bench.py`	Updated `DummyWorker` to return `GenerationResult` objects instead of strings. Modified test assertions to access `results[0].cur_output.output_str`. Added helper functions: `setup_scaffolding`, `create_requests`, `calculate_intervals`, `verify_strategy_timing`. Extended test coverage to validate timing behavior for multiple strategy types (Synchronous, Concurrent, Throughput, ConstantRate, PoissonRate). Added imports: `math`, `numpy`, additional strategy types.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Benchmark as async_scaffolding_benchmark
    participant Strategy as LoadGenerationStrategy
    participant Runner as run_scaffolding_llm
    participant Processor as process_request
    participant Queue as output_queue

    Client->>Benchmark: requests, strategy (or concurrency)
    Benchmark->>Benchmark: initialize strategy if needed
    Benchmark->>Runner: launch with strategy
    
    loop for each request
        Runner->>Strategy: request_times() generator
        Strategy-->>Runner: target_time
        Runner->>Strategy: get_semaphore()
        Strategy-->>Runner: semaphore
        
        Runner->>Processor: create task (target_time, semaphore)
        Processor->>Processor: wait until target_time
        Processor->>Processor: execute request
        Processor->>Queue: enqueue (result, request_start_time, request_execution_time)
    end
    
    Benchmark->>Benchmark: collect requests_start_time, requests_execution_time
    Benchmark-->>Client: (results, requests_start_time, requests_execution_time, total_time)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

The changes introduce a new strategy abstraction framework with multiple implementations, require understanding the refactored benchmark control flow, and necessitate validation of timing semantics across different strategy types. While the individual pieces follow clear patterns, the interconnected changes across three files demand careful review of signature changes, return type modifications, and timing correctness in the test cases.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The PR description is incomplete and largely unfilled. The template requires a "Description" section explaining the issue and solution, and a "Test Coverage" section listing relevant tests that safeguard the changes. However, both of these critical sections are empty in the provided description—they contain only the placeholder comments from the template. The PR Checklist shows only one generic item checked (the final checkbox), while all substantive items (PR clarity, coding guidelines adherence, test coverage, dependency scanning, CODEOWNERS updates, and documentation) remain unchecked. The majority of the description content is boilerplate GitHub Bot help documentation rather than project-specific information about the changes.
Docstring Coverage	⚠️ Warning	Docstring coverage is 41.38% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The PR title "[None][feat] Dev scaffolding bench load_generator" follows the required format and relates to the core changes in the pull request. The title references the scaffolding benchmark load generator functionality that is being introduced, which aligns with the main objectives of implementing a new LoadGenerationStrategy framework and updating the benchmark module to use load generation strategies. While the abbreviation "Dev" is somewhat informal, the title is sufficiently specific and clear enough that a teammate reviewing commit history would understand that the PR introduces load generation capabilities for scaffolding benchmarks.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 6

🧹 Nitpick comments (4)

tests/unittest/scaffolding/test_bench.py (3)
64-64: Prefix unused unpacked variables with underscore.

The variables requests_start_time and total_time are unpacked but never used in this test. Prefix them with underscores to indicate they are intentionally unused.

Apply this diff:
-    results, requests_start_time, requests_execution_time, total_time = asyncio.run(
+    results, _requests_start_time, requests_execution_time, _total_time = asyncio.run(
         async_scaffolding_benchmark(scaffolding_llm, task_collection_types,
                                     requests, concurrency))
156-156: Prefix unused unpacked variables with underscore.

The variables results, requests_execution_time, and total_time are unpacked but only requests_start_time is used for timing verification. Prefix unused variables with underscores.

Apply this diff:
-        results, requests_start_time, requests_execution_time, total_time = asyncio.run(
+        _results, requests_start_time, _requests_execution_time, _total_time = asyncio.run(
             async_scaffolding_benchmark(scaffolding_llm,
                                         task_collection_types,
                                         requests,
                                         strategy=strategy))
165-165: Remove unnecessary f-string prefix.

The string has no placeholders, so the f prefix is unnecessary.

Apply this diff:
-    print(f"before shutdown")
+    print("before shutdown")
tensorrt_llm/scaffolding/load_generation_strategy.py (1)
295-302: Consider sorting __all__ alphabetically.

Static analysis suggests alphabetical sorting for consistency. However, the current order (base class first, then strategies) is logical and acceptable.

If you prefer alphabetical sorting:
 __all__ = [
+    "ConcurrentStrategy",
+    "ConstantRateStrategy",
     "LoadGenerationStrategy",
+    "PoissonRateStrategy",
     "SynchronousStrategy",
-    "ConcurrentStrategy",
     "ThroughputStrategy",
-    "ConstantRateStrategy",
-    "PoissonRateStrategy",
 ]

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50d4e5b and 64f6193.

📒 Files selected for processing (3)

tensorrt_llm/scaffolding/benchmark.py (5 hunks)
tensorrt_llm/scaffolding/load_generation_strategy.py (1 hunks)
tests/unittest/scaffolding/test_bench.py (3 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{h,hpp,hh,hxx,cpp,cxx,cc,cu,cuh,py}

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

Use only spaces, no tabs; indent with 4 spaces.

Files:

tests/unittest/scaffolding/test_bench.py
tensorrt_llm/scaffolding/load_generation_strategy.py
tensorrt_llm/scaffolding/benchmark.py

**/*.py

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

**/*.py: Python code must target Python 3.8+.
Indent Python code with 4 spaces; do not use tabs.
Maintain module namespace when importing; prefer 'from package.subpackage import foo' then 'foo.SomeClass()' instead of importing the class directly.
Python filenames should be snake_case (e.g., some_file.py).
Python classes use PascalCase names.
Functions and methods use snake_case names.
Local variables use snake_case; prefix 'k' for variables that start with a number (e.g., k_99th_percentile).
Global variables use upper SNAKE_CASE prefixed with 'G' (e.g., G_MY_GLOBAL).
Constants use upper SNAKE_CASE (e.g., MY_CONSTANT).
Avoid shadowing variables from an outer scope.
Initialize all externally visible members of a class in the constructor.
Prefer docstrings for interfaces that may be used outside a file; comments for in-function or file-local interfaces.
Use Google-style docstrings for classes and functions (Sphinx-parsable).
Document attributes and variables inline so they render under the class/function docstring.
Avoid reflection when a simpler, explicit approach suffices (e.g., avoid dict(**locals()) patterns).
In try/except, catch the most specific exceptions possible.
For duck-typing try/except, keep the try body minimal and use else for the main logic.

Files:

tests/unittest/scaffolding/test_bench.py
tensorrt_llm/scaffolding/load_generation_strategy.py
tensorrt_llm/scaffolding/benchmark.py

**/*.{cpp,cxx,cc,h,hpp,hh,hxx,cu,cuh,py}

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

Prepend the NVIDIA Apache-2.0 copyright header with current year to the top of all source files (e.g., .cpp, .h, .cu, .py).

Files:

tests/unittest/scaffolding/test_bench.py
tensorrt_llm/scaffolding/load_generation_strategy.py
tensorrt_llm/scaffolding/benchmark.py

🧬 Code graph analysis (3)

tests/unittest/scaffolding/test_bench.py (7)

tensorrt_llm/executor/request.py (1)

GenerationRequest (84-133)

tensorrt_llm/scaffolding/task.py (7)

result (71-72)

result (75-76)

GenerationTask (30-117)

Task (12-20)

TaskStatus (23-26)

output_str (87-88)

output_str (91-93)

tensorrt_llm/executor/result.py (2)

GenerationResult (656-830)

prompt_token_ids (708-709)

tensorrt_llm/scaffolding/benchmark.py (2)

ScaffoldingBenchRequest (15-16)

async_scaffolding_benchmark (78-126)

tensorrt_llm/scaffolding/scaffolding_llm.py (2)

ScaffoldingLlm (22-237)

shutdown (215-237)

tensorrt_llm/scaffolding/worker.py (1)

Worker (18-40)

tensorrt_llm/scaffolding/load_generation_strategy.py (6)

ConcurrentStrategy (116-147)

ConstantRateStrategy (178-229)

LoadGenerationStrategy (28-86)

PoissonRateStrategy (232-291)

SynchronousStrategy (89-113)

ThroughputStrategy (150-175)

tensorrt_llm/scaffolding/load_generation_strategy.py (1)

tensorrt_llm/llmapi/llm_args.py (1)

Field (70-97)

tensorrt_llm/scaffolding/benchmark.py (3)

tensorrt_llm/scaffolding/load_generation_strategy.py (6)

LoadGenerationStrategy (28-86)

get_semaphore (44-55)

get_semaphore (105-108)

get_semaphore (136-139)

request_times (57-70)

ConcurrentStrategy (116-147)

tensorrt_llm/scaffolding/scaffolding_llm.py (1)

ScaffoldingLlm (22-237)

tensorrt_llm/scaffolding/result.py (1)

ScaffoldingResult (7-80)

🪛 Ruff (0.14.1)

tests/unittest/scaffolding/test_bench.py

64-64: Unpacked variable requests_start_time is never used