[None][chore] Update benchmark script #7860
Conversation
zerollzeng
requested review from
Tabrizian,
schetlur-nv,
chzblych and
QiJune
zerollzeng
force-pushed
the
update-benchmark-script
branch
from
9609d7c to
b11c881
Compare
|
/bot run |
zerollzeng
force-pushed
the
update-benchmark-script
branch
from
b11c881 to
39976c4
Compare
zerollzeng
changed the title
zerollzeng
changed the title
zerollzeng
force-pushed
the
update-benchmark-script
branch
2 times, most recently
from
d696c96 to
4cbdad9
Compare
|
/bot run |
|
PR_Github #19326 [ run ] triggered by Bot |
|
PR_Github #19326 [ run ] completed with state |
There was a problem hiding this comment.
@zerollzeng Can we make sure that @qiaoxj07 's numbers can be reproduced by using the scripts before the merge? Thanks
zerollzeng
force-pushed
the
update-benchmark-script
branch
from
4cbdad9 to
3f4a87f
Compare
|
/bot run |
|
PR_Github #19660 [ run ] triggered by Bot |
|
/bot kill |
|
PR_Github #19672 [ kill ] triggered by Bot |
|
PR_Github #19660 [ run ] completed with state |
|
PR_Github #19672 [ kill ] completed with state |
📝 WalkthroughWalkthroughRefactors the disaggregated SLURM benchmarking workflow to an argument-driven, configurable system with enhanced error handling, dual benchmarking paths (default and NVIDIA SA), updated worker config generation, and revised submit scripts. Adds a new NVIDIA SA benchmark script and updates wide-EP submission scripts. README is expanded accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant submit.sh
participant SLURM as SLURM Scheduler
participant Job as disaggr_torch.slurm
participant Ctr as Container
participant Cfg as gen_worker_config.py
participant Ctx as Context Workers
participant Gen as Generation Workers
participant Srv as Server
participant BM as Benchmark (default/NV SA)
User->>submit.sh: Configure params and run
submit.sh->>SLURM: sbatch with computed args
SLURM-->>Job: Launch job
Job->>Ctr: Start container
alt build/install TRT-LLM
Job->>Ctr: Build and install (conditional)
end
Job->>Cfg: Generate per-role configs (seq_offset, gpu fracs)
Job->>Ctx: Start context workers
Job->>Gen: Start generation workers
Job->>Srv: Start server
alt use NV SA benchmark
Job->>BM: run_benchmark_nv_sa.sh
else default benchmark
Job->>BM: run_benchmark.sh
end
BM-->>Job: Results and logs
Job->>Job: Cleanup/teardown (on success/error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/disaggregated/slurm/benchmark/gen_worker_config.py (1)
1-1: Add NVIDIA Apache-2.0 header (2025) to comply with repo standards.All source files (.py) must include the NVIDIA Apache-2.0 header.
Apply:
+# Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + import argparse
🧹 Nitpick comments (19)
examples/disaggregated/slurm/benchmark/gen_worker_config.py (1)
25-49: Fix docstring: argument names/descriptions don’t match the function signature.Current docstring references non-existent params (e.g., config_path, model_path, num_ctx_servers).
Suggested replacement:
- """ - Generate configuration YAML file for disaggregated inference. - - Args: - config_path: Path to save the config file - model_path: Path to the model - num_ctx_servers: Number of context servers - ctx_tp_size: Tensor parallel size for context servers - ctx_pp_size: Pipeline parallel size for context servers - ctx_batch_size: Batch size for context servers - ctx_max_num_tokens: Max number of tokens for context servers - ctx_max_seq_len: Max sequence length for context servers - ctx_free_gpu_memory_fraction: Free GPU memory fraction for context servers - ctx_enable_attention_dp: Enable attention DP for context servers - num_gen_servers: Number of generation servers - gen_tp_size: Tensor parallel size for generation servers - gen_pp_size: Pipeline parallel size for generation servers - gen_batch_size: Batch size for generation servers - gen_max_num_tokens: Max number of tokens for generation servers - gen_enable_attention_dp: Enable attention DP for generation servers - gen_gpu_memory_fraction: GPU memory fraction for generation servers - eplb_num_slots: Number of slots for eplb - worker_start_port: Start port for workers - server_port: Server port - """ + """ + Generate ctx/gen worker config YAMLs for disaggregated inference. + + Args: + work_dir: Directory to write ctx_config.yaml and gen_config.yaml. + ctx_tp_size: Tensor parallel size for context workers. + ctx_pp_size: Pipeline parallel size for context workers. + ctx_batch_size: Max batch size for context workers. + ctx_max_num_tokens: Max tokens per request for context workers. + ctx_max_seq_len: Max sequence length for context workers. + ctx_free_gpu_memory_fraction: Fraction of GPU mem for KV cache (ctx). + ctx_enable_attention_dp: Whether to enable attention DP (ctx). + gen_tp_size: Tensor parallel size for generation workers. + gen_pp_size: Pipeline parallel size for generation workers. + gen_batch_size: Max batch size for generation workers. + gen_max_num_tokens: Max tokens per request for generation workers. + gen_max_seq_len: Max sequence length for generation workers. + gen_enable_attention_dp: Whether to enable attention DP (gen). + gen_gpu_memory_fraction: Fraction of GPU mem for KV cache (gen). + eplb_num_slots: MoE load balancer num slots (0 to disable). + mtp_size: MTP next-n predict layers (0 to disable). + cache_transceiver_max_num_tokens: Max tokens in cache transceiver buffer. + """examples/wide_ep/slurm_scripts/submit_e2e.sh (1)
96-101: Avoid SC2155: declare and assign separately to preserve exit status under set -e.Prevents masking command failures when using local with command substitution.
- local gen_nodes=$(calc_nodes "$gen_tp_size" "$gen_num") - local ctx_nodes=$(calc_nodes "$ctx_tp_size" "$ctx_num") - local total_nodes=$((ctx_nodes + gen_nodes)) - local total_tasks=$((total_nodes * gpus_per_node)) + local gen_nodes + gen_nodes=$(calc_nodes "$gen_tp_size" "$gen_num") + local ctx_nodes + ctx_nodes=$(calc_nodes "$ctx_tp_size" "$ctx_num") + local total_nodes + total_nodes=$((ctx_nodes + gen_nodes)) + local total_tasks + total_tasks=$((total_nodes * gpus_per_node))examples/wide_ep/slurm_scripts/submit_gen_only.sh (1)
96-101: Avoid SC2155: declare and assign separately to preserve exit status under set -e.Same pattern as submit_e2e.sh.
- local gen_nodes=$(calc_nodes "$gen_tp_size" "$gen_num") - local ctx_nodes=$(calc_nodes "$ctx_tp_size" "$ctx_num") - local total_nodes=$((ctx_nodes + gen_nodes)) - local total_tasks=$((total_nodes * gpus_per_node)) + local gen_nodes + gen_nodes=$(calc_nodes "$gen_tp_size" "$gen_num") + local ctx_nodes + ctx_nodes=$(calc_nodes "$ctx_tp_size" "$ctx_num") + local total_nodes + total_nodes=$((ctx_nodes + gen_nodes)) + local total_tasks + total_tasks=$((total_nodes * gpus_per_node))examples/disaggregated/slurm/benchmark/start_worker.sh (1)
62-62: Quote $(hostname) to avoid word splitting.Minor robustness improvement.
- trtllm-llmapi-launch ${numa_bind_cmd} trtllm-serve ${model_path} --host $(hostname) --port ${port} --extra_llm_api_options ${config_file} + trtllm-llmapi-launch ${numa_bind_cmd} trtllm-serve ${model_path} --host "$(hostname)" --port ${port} --extra_llm_api_options ${config_file} @@ - --host $(hostname) --port ${port} \ + --host "$(hostname)" --port ${port} \Also applies to: 77-78
examples/disaggregated/slurm/benchmark/submit.sh (2)
95-101: Avoid SC2155: declare and assign separately to preserve exit status under set -e.Same pattern as other submit scripts.
- local gen_nodes=$(calc_nodes "$gen_tp_size" "$gen_num") - local ctx_nodes=$(calc_nodes "$ctx_tp_size" "$ctx_num") - local total_nodes=$((ctx_nodes + gen_nodes)) - local total_tasks=$((total_nodes * gpus_per_node)) + local gen_nodes + gen_nodes=$(calc_nodes "$gen_tp_size" "$gen_num") + local ctx_nodes + ctx_nodes=$(calc_nodes "$ctx_tp_size" "$ctx_num") + local total_nodes + total_nodes=$((ctx_nodes + gen_nodes)) + local total_tasks + total_tasks=$((total_nodes * gpus_per_node))
60-64: Add validation for trtllm_repo path.Consistent with other required path checks.
[[ ! -d "${model_path}" ]] && { echo "Error: model_path not found: ${model_path}" >&2; exit 1; } -[[ ! -d "${work_dir}" ]] && { echo "Error: work_dir '${work_dir}' not found" >&2; exit 1; } +[[ ! -d "${work_dir}" ]] && { echo "Error: work_dir '${work_dir}' not found" >&2; exit 1; } +[[ ! -d "${trtllm_repo}" ]] && { echo "Error: trtllm_repo '${trtllm_repo}' not found" >&2; exit 1; } [[ ! -f "${dataset_file}" ]] && { echo "Error: dataset_file '${dataset_file}' not found" >&2; exit 1; }examples/disaggregated/slurm/benchmark/run_benchmark_nv_sa.sh (4)
51-60: Avoid SC2155: declare and assign separately for start_time/elapsed.Ensures errors in command substitution propagate under set -e.
- local start_time=$(date +%s) + local start_time + start_time=$(date +%s) @@ - local elapsed=$(($(date +%s) - start_time)) + local elapsed + elapsed=$(($(date +%s) - start_time))
67-73: Health check does not validate HTTP 200; it only checks curl exit status.May proceed on non-200 responses.
- while ! curl -s -o /dev/null -w "%{http_code}" "http://${host}:${port}/health"; do - local elapsed=$(($(date +%s) - start_time)) + while :; do + local code + code=$(curl -s -o /dev/null -w "%{http_code}" "http://${host}:${port}/health" || echo 000) + local elapsed + elapsed=$(($(date +%s) - start_time)) + [[ "${code}" == "200" ]] && break [[ $elapsed -ge $TIMEOUT ]] && { echo "Error: Server not healthy after ${TIMEOUT} seconds"; exit 1; } [[ $((elapsed % STATUS_UPDATE_INTERVAL)) -eq 0 ]] && echo "Waiting for server... (${elapsed}s elapsed)" sleep $HEALTH_CHECK_INTERVAL - done + done
143-143: Avoid SC2046 in optional flag injection; prefer explicit conditional.Safer to append the flag conditionally than rely on command substitution.
Example:
args=( --model "${model_name}" --host "${hostname}" --port "${port}" --dataset-name random --num-prompts "${num_prompts}" --max-concurrency "${concurrency}" --ignore-eos --random-input-len "${input_seq_len}" --random-output-len "${output_seq_len}" --random-range-ratio "${ratio}" --save-result --use-chat-template --result-dir "${output_dir}" --result-filename "result.json" --percentile-metrics "ttft,tpot,itl,e2el" ) [ "${streaming}" = "false" ] && args+=( --non-streaming ) python "${BENCH_SCRIPT}" "${args[@]}"
83-89: Unused function do_get_logs; consider removing to reduce noise.It’s defined but never called.
examples/disaggregated/slurm/benchmark/run_benchmark.sh (4)
23-27: Guard SLURM_PROCID under set -u to avoid unbound variable outside SLURM.Safer default.
-if [[ ${SLURM_PROCID} != "0" ]]; then +if [[ ${SLURM_PROCID:-0} != "0" ]]; then
59-71: Health check does not validate HTTP 200; it only checks curl exit status.Mirror the fix as in NV SA script.
-while ! curl -s -o /dev/null -w "%{http_code}" http://${hostname}:${port}/health; do +while :; do + code=$(curl -s -o /dev/null -w "%{http_code}" "http://${hostname}:${port}/health" || echo 000) current_time=$(date +%s) elapsed=$((current_time - start_time)) - if [ $elapsed -ge $timeout ]; then + if [ "$code" = "200" ]; then + break + fi + if [ $elapsed -ge $timeout ]; then echo "Error: Server is not healthy after ${timeout} seconds" exit 1 fi if [ $((elapsed % 30)) -eq 0 ]; then echo "Waiting for server to be healthy... (${elapsed}s elapsed)" fi sleep 10 done
122-122: Avoid SC2046 in optional flag injection.Prefer explicit conditional to append --non-streaming.
- $(if [ "${streaming}" = "false" ]; then echo "--non-streaming"; fi) + $(if [ "${streaming}" = "false" ]; then echo "--non-streaming"; fi)Follow-up: Replace the above with an args array approach as shown in the NV SA comment, or split into two python invocations guarded by an if.
132-135: Use pkill/pgrep instead of parsing ps output; also quote expansions.Simplifies and avoids SC2046 issues.
-kill -9 $(ps aux | grep '[s]tart_server.sh' | awk '{print $2}') >/dev/null 2>&1 || true -kill -9 $(ps aux | grep '[s]tart_worker.sh' | awk '{print $2}') >/dev/null 2>&1 || true -kill -9 $(ps aux | grep '[t]rtllm-serve' | awk '{print $2}') >/dev/null 2>&1 || true +pkill -9 -f 'start_server.sh' >/dev/null 2>&1 || true +pkill -9 -f 'start_worker.sh' >/dev/null 2>&1 || true +pkill -9 -f 'trtllm-serve' >/dev/null 2>&1 || trueexamples/disaggregated/slurm/benchmark/README.md (2)
44-51: Align variable names with scripts (mount_dir vs container_mount).Docs use mount_dir; scripts use container_mount. Unify to avoid confusion.
70-105: disaggr_torch.slurm argument list seems outdated.Scripts pass seq_offset, numa_bind, and benchmark_mode after nsys_on; these aren’t documented here.
Update the list to include:
- seq_offset
- numa_bind
- benchmark_mode
…in the correct order after nsys_on.examples/disaggregated/slurm/benchmark/disaggr_torch.slurm (3)
110-118: Consider adding retry logic for container startupContainer startup can occasionally fail due to transient issues. Consider implementing retry logic with exponential backoff to improve reliability.
# Start container echo "Starting container..." -if ! srun -l --container-image=${container_image} \ - --container-name=${container_name} \ - --container-mounts=${container_mount} \ - --mpi=pmix \ - echo "Container up." &> ${full_logdir}/container_launch.log; then - cleanup_on_failure "Failed to start container. Check ${full_logdir}/container_launch.log" -fi +max_retries=3 +retry_count=0 +while [ $retry_count -lt $max_retries ]; do + if srun -l --container-image=${container_image} \ + --container-name=${container_name} \ + --container-mounts=${container_mount} \ + --mpi=pmix \ + echo "Container up." &> ${full_logdir}/container_launch.log; then + break + fi + retry_count=$((retry_count + 1)) + if [ $retry_count -eq $max_retries ]; then + cleanup_on_failure "Failed to start container after $max_retries attempts. Check ${full_logdir}/container_launch.log" + fi + echo "Container startup failed. Retrying in 5 seconds... (attempt $((retry_count + 1))/$max_retries)" + sleep 5 +done
205-232: Consider adding worker health checks before proceedingAfter starting workers, it would be prudent to verify they're ready before starting the benchmark to avoid race conditions.
Add a health check loop after starting all workers:
done +# Wait for workers to be ready +echo "Waiting for workers to initialize..." +max_wait=60 +elapsed=0 +while [ $elapsed -lt $max_wait ]; do + # Check if log files contain expected initialization messages + gen_ready=true + ctx_ready=true + + for i in $(seq 0 $((num_gen_servers - 1))); do + if ! grep -q "Worker ready" ${full_logdir}/output_gen_${i}.log 2>/dev/null; then + gen_ready=false + break + fi + done + + for i in $(seq 0 $((num_ctx_servers - 1))); do + if ! grep -q "Worker ready" ${full_logdir}/output_ctx_${i}.log 2>/dev/null; then + ctx_ready=false + break + fi + done + + if [ "$gen_ready" = true ] && [ "$ctx_ready" = true ]; then + echo "All workers ready" + break + fi + + sleep 2 + elapsed=$((elapsed + 2)) +done + +if [ $elapsed -ge $max_wait ]; then + echo "Warning: Workers may not be fully initialized after ${max_wait} seconds" +fi + # start the server
269-270: Add graceful shutdown with timeoutThe script ends with an immediate job cancellation. Consider adding a graceful shutdown mechanism.
-# try to kill the server and workers -scancel ${SLURM_JOB_ID} +# Graceful shutdown +echo "Initiating graceful shutdown..." + +# Send SIGTERM to allow graceful shutdown +pkill -TERM -f "start_worker.sh" || true +pkill -TERM -f "start_server.sh" || true + +# Wait briefly for graceful shutdown +sleep 5 + +# Force kill if still running +pkill -KILL -f "start_worker.sh" || true +pkill -KILL -f "start_server.sh" || true + +# Cancel SLURM job +scancel ${SLURM_JOB_ID}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/disaggregated/slurm/benchmark/README.md(4 hunks)examples/disaggregated/slurm/benchmark/disaggr_torch.slurm(3 hunks)examples/disaggregated/slurm/benchmark/gen_worker_config.py(2 hunks)examples/disaggregated/slurm/benchmark/run_benchmark.sh(2 hunks)examples/disaggregated/slurm/benchmark/run_benchmark_nv_sa.sh(1 hunks)examples/disaggregated/slurm/benchmark/start_worker.sh(2 hunks)examples/disaggregated/slurm/benchmark/submit.sh(1 hunks)examples/wide_ep/slurm_scripts/submit_e2e.sh(1 hunks)examples/wide_ep/slurm_scripts/submit_gen_only.sh(1 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:
examples/disaggregated/slurm/benchmark/gen_worker_config.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:
examples/disaggregated/slurm/benchmark/gen_worker_config.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:
examples/disaggregated/slurm/benchmark/gen_worker_config.py
🪛 Shellcheck (0.11.0)
examples/disaggregated/slurm/benchmark/submit.sh
[warning] 95-95: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 96-96: Declare and assign separately to avoid masking return values.
(SC2155)
examples/wide_ep/slurm_scripts/submit_gen_only.sh
[warning] 97-97: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 98-98: Declare and assign separately to avoid masking return values.
(SC2155)
examples/disaggregated/slurm/benchmark/run_benchmark_nv_sa.sh
[warning] 52-52: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 65-65: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 143-143: Quote this to prevent word splitting.
(SC2046)
examples/disaggregated/slurm/benchmark/run_benchmark.sh
[warning] 122-122: Quote this to prevent word splitting.
(SC2046)
[warning] 132-132: Quote this to prevent word splitting.
(SC2046)
[warning] 133-133: Quote this to prevent word splitting.
(SC2046)
[warning] 134-134: Quote this to prevent word splitting.
(SC2046)
examples/wide_ep/slurm_scripts/submit_e2e.sh
[warning] 97-97: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 98-98: Declare and assign separately to avoid masking return values.
(SC2155)
examples/disaggregated/slurm/benchmark/start_worker.sh
[error] 15-15: Braces are required for positionals over 9, e.g. ${10}.
(SC1037)
[warning] 62-62: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (11)
examples/disaggregated/slurm/benchmark/gen_worker_config.py (1)
231-236: Default mismatch for cache_transceiver_max_num_tokens.Function default is 4608; argparse default is 8448. Confirm intended value.
examples/wide_ep/slurm_scripts/submit_e2e.sh (1)
117-119: Validate use of non-standard sbatch option --segment.--segment is not standard in upstream Slurm. Verify cluster supports it or drop it.
examples/wide_ep/slurm_scripts/submit_gen_only.sh (1)
117-119: Validate use of non-standard sbatch option --segment.Confirm site supports it; otherwise remove.
examples/disaggregated/slurm/benchmark/submit.sh (1)
115-117: Validate use of non-standard sbatch option --segment.Confirm it’s supported on the target cluster.
examples/disaggregated/slurm/benchmark/disaggr_torch.slurm (7)
2-2: Good addition of strict error handlingThe addition of
set -euo pipefailis excellent for script robustness - it ensures the script exits on errors, undefined variables, and pipe failures.
4-41: Well-structured argument parsing with clear namingThe expanded parameter list with descriptive variable names improves maintainability. The addition of new parameters like
seq_offset,numa_bind, andbenchmark_modeprovides better configurability.
42-89: Comprehensive logging of configuration improves debuggabilityThe detailed configuration echo statements provide excellent visibility into the script execution parameters, making debugging significantly easier.
103-108: Robust error handling with cleanup functionThe
cleanup_on_failurefunction provides centralized error handling and ensures proper job cancellation on failures, preventing resource leaks.
243-265: Good separation of benchmark modes with proper error handlingThe conditional logic for choosing between NV SA benchmark and default benchmark is well-structured with appropriate error handling for each path.
128-129: Verify CUDA architecture compatibilityThe build command hardcodes --cuda_architectures='100-real' in examples/disaggregated/slurm/benchmark/disaggr_torch.slurm (lines 128–129). build_wheel.py accepts this flag and passes it to CMake (it even documents examples like
90-real;100-real) and will explicitly reject70-real. Confirm your cluster GPUs actually require/support100-real; if not, change to the appropriate semicolon-separated list or use the defaultall/ detect GPUs at runtime.
153-154: seq_offset propagation to worker configs confirmeddisaggr_torch.slurm computes ctx_max_seq_len=$((isl+seq_offset)) and gen_max_seq_len=$((isl+osl+seq_offset)) and passes them to gen_worker_config.py; gen_worker_config.py then writes these values into the ctx/gen 'max_seq_len' fields in the generated YAML, so the seq_offset addition is propagated correctly.
Locations: examples/disaggregated/slurm/benchmark/disaggr_torch.slurm (lines 153–154) → examples/disaggregated/slurm/benchmark/gen_worker_config.py (assigns 'max_seq_len' into ctx/gen config; argparse at ~lines 181–213).
zerollzeng
force-pushed
the
update-benchmark-script
branch
from
b5f9920 to
f5e738a
Compare
Signed-off-by: Zero Zeng <[email protected]> address review comments Signed-off-by: Zero Zeng <[email protected]>
Signed-off-by: Zero Zeng <[email protected]>
Co-authored-by: Kaiyu Xie <[email protected]> Signed-off-by: Zero Zeng <[email protected]>
kaiyux
force-pushed
the
update-benchmark-script
branch
from
f3c452c to
fee7eb3
Compare
|
/bot skip --comment "slurm scripts are not protected by CI pipelines" |
|
PR_Github #19686 [ skip ] triggered by Bot |
|
PR_Github #19686 [ skip ] completed with state |
Summary by CodeRabbit
New Features
Refactor
Documentation
Description
This PR does following things:
USE_NV_SA_BENCHMARK=truein submit.sh. This PR didn't integrate the SA scripts to the code base, instead we fetch the latest script and use it.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 thestage-listparameter 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.mdand the
scripts/test_to_stage_mapping.pyhelper.kill
killKill all running builds associated with pull request.
skip
skip --comment COMMENTSkip 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-pipelineReuse 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.