tests/scripts/perf-sanity/benchmark_config.yaml (1)

466-468: Remove trailing whitespace to keep YAML lint-clean

Line 468 has a trailing space after # - [4096, 2].
Many CI linters treat this as an error and will fail the pipeline.

-    #  - [4096, 2] 
+    #  - [4096, 2]

tests/scripts/perf-sanity/run_benchmark_serve.sh (1)

218-229: Use a trap to ensure the server is always terminated

If the benchmark loop exits early (e.g., Ctrl-C or an error despite set -e), the background trtllm-serve may stay alive.
Registering a trap keeps nodes clean and avoids port conflicts on the next run.

+# Ensure background server is cleaned up on exit
+cleanup() {
+  [[ -n "$server_pid" ]] && kill -9 "$server_pid" 2>/dev/null || true
+}
+trap cleanup EXIT

tests/scripts/perf-sanity/benchmark-job.sh (1)

1-1: Prefer env-based shebang for portability

/usr/bin/bash is not guaranteed on all systems; /usr/bin/env bash is the common portable form.

-#! /usr/bin/bash
+#!/usr/bin/env bash

tests/scripts/perf-sanity/benchmark-bench-prerelease.sh (2)

1-1: Use a portable shebang

Replace hard-coded path with env lookup.

-#! /usr/bin/bash
+#!/usr/bin/env bash

---

13-14: script_dir is calculated but never used

Remove it or make use of it to avoid reader confusion and silence SC2034.

tests/scripts/perf-sanity/benchmark-serve.sh (1)

1-1: Prefer /usr/bin/env for portability.

Use env to locate bash across environments.

-#! /usr/bin/bash
+#!/usr/bin/env bash

tests/scripts/perf-sanity/benchmark-serve-prerelease.sh (3)

1-1: Prefer /usr/bin/env for portability.

-#! /usr/bin/bash
+#!/usr/bin/env bash

---

46-47: Quote subshells in report for safety (SC2046).

-    echo "Report path" $(realpath ${results})
-    echo "START" $start_time "-" "END" ${end_time} $(hostname)
+    echo "Report path $(realpath "${results}")"
+    echo "START ${start_time} - END ${end_time} $(hostname)"

---

14-19: Hard-coded DEFAULT_COMMIT will become stale; prefer dynamic default or explicit arg.

Consider defaulting to the current repo HEAD when available, else "unknown". Keeps tracking accurate without frequent edits.

-DEFAULT_COMMIT="4d040b50b77a737dd5a87d5888babbc364eca557"
+DEFAULT_COMMIT="$(git rev-parse --short HEAD 2>/dev/null || echo unknown)"

tests/scripts/perf-sanity/README.md (2)

195-195: Add language tag to fenced code block (MD040).

Improves rendering and lint compliance.

-```
+```text

---

236-243: Align Python version with repo guidelines (3.8+).

Project guideline specifies Python 3.8+; update dependency note accordingly.

-- Python 3.7+
+- Python 3.8+

tests/scripts/perf-sanity/parse_benchmark_results.py (2)

289-289: Portability: os.uname() isn’t available on all platforms.

Use platform.node() to avoid AttributeError outside POSIX.

-    print(f"Parsed at: {end_time} on {os.uname().nodename}")
+    import platform
+    print(f"Parsed at: {end_time} on {platform.node()}")

---

3-6: Docstring nits (punctuation/format).

Minor docstring cleanup for D205/D415/D200; optional for tests, but improves readability.

-"""
-Script to parse benchmark metrics from a specified folder and generate Excel table
-Usage: python parse_benchmark_results.py <folder_name>
-"""
+"""
+Parse benchmark metrics from a specified folder and generate a report.
+
+Usage: python parse_benchmark_results.py <folder_name>
+"""
@@
-    Extract configuration from log file content using "Completed benchmark with Configuration:" pattern
+    Extract configuration from log content using the "Completed benchmark with Configuration:" pattern.
@@
-    Extract basic configuration from filename as fallback
-    Expected format: serve.{model_name}.tp{tp}.ep{ep}.isl{isl}.osl{osl}.concurrency{concurrency}.log
+    Extract basic configuration from filename as fallback.
+    Expected format:
+      serve.{model_name}.tp{tp}.ep{ep}.isl{isl}.osl{osl}.concurrency{concurrency}.log
@@
-    Return default configuration values when log content parsing fails
+    Return default configuration values when log content parsing fails.

Also applies to: 18-21, 93-98, 128-141

tests/scripts/perf-sanity/run_benchmark_serve.py (2)

2-9: Wrap long header docstring to satisfy linters and readability.

-"""
-Script to run benchmarks from YAML configuration file
-Usage: python run_benchmark_serve.py --output_folder <output_folder> --commit <commit> --config_file <config_file> [--skip <skip_pattern>] [--select <select_pattern>]
-Skip pattern format: "2,4-1" means skip test case 2 and test case 4's 1st concurrency
-Select pattern format: "1,3,5" means only run test cases 1, 3, and 5
-If select_pattern is empty, all test cases are selected
-If skip_pattern is empty, no test cases are skipped
-"""
+"""
+Run benchmarks from a YAML configuration file.
+
+Usage:
+  python run_benchmark_serve.py --output_folder <output_folder> --commit <commit> \
+    --config_file <config_file> [--skip <skip_pattern>] [--select <select_pattern>]
+
+Notes:
+  - Skip pattern: "2,4-1" skips test case 2 and test case 4's 1st concurrency.
+  - Select pattern: "1,3,5" runs only test cases 1, 3, and 5.
+  - If select_pattern is empty, all test cases are selected.
+  - If skip_pattern is empty, no test cases are skipped.
+"""

---

185-201: Benchmark log capture: ensure both stdout/stderr preserved in order (optional).

Subprocess capture merges stdout/stderr only when stderr is redirected; current approach writes them sequentially which can reorder timestamps. Optional: redirect stderr to stdout in the command to keep ordering in a single stream, or run without capture and tee to file.

-            result = subprocess.run(benchmark_cmd, capture_output=True, text=True, check=True)
-            
-            # Write output to log file
-            with open(log_filename, 'w') as f:
-                f.write(result.stdout)
-                f.write(result.stderr)
+            result = subprocess.run(
+                benchmark_cmd,
+                stdout=subprocess.PIPE,
+                stderr=subprocess.STDOUT,
+                text=True,
+                check=True,
+            )
+            # Write output to log file (preserves ordering)
+            with open(log_filename, 'w') as f:
+                f.write(result.stdout)

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: galagam
PR: NVIDIA/TensorRT-LLM#6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.

Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.

Learnt from: yibinl-nvidia
PR: NVIDIA/TensorRT-LLM#6506
File: examples/models/core/mixtral/requirements.txt:3-3
Timestamp: 2025-08-01T15:14:45.673Z
Learning: In TensorRT-LLM, examples directory can have different dependency versions than the root requirements.txt file. Version conflicts between root and examples dependencies are acceptable because examples are designed to be standalone and self-contained.

Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.

Learnt from: galagam
PR: NVIDIA/TensorRT-LLM#6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.

Learnt from: yibinl-nvidia
PR: NVIDIA/TensorRT-LLM#6506
File: examples/models/core/mixtral/requirements.txt:3-3
Timestamp: 2025-08-01T15:14:45.673Z
Learning: In TensorRT-LLM, examples directory can have different dependency versions than the root requirements.txt file. Version conflicts between root and examples dependencies are acceptable because examples are designed to be standalone and self-contained.

tests/scripts/perf-sanity/benchmark-serve-prerelease.sh (1)

32-35: Quote $(pwd) instead of back-ticks.

Same remark as previous review: replace

-v `pwd`:`pwd` \
-w `pwd` \

with

-        -v `pwd`:`pwd` \
-        -w `pwd`  \
+        -v "$(pwd)":"$(pwd)" \
+        -w "$(pwd)" \

Prevents word-splitting (SC2046).

tests/scripts/perf-sanity/parse_benchmark_results.py (1)

206-208: GPU count falls back to TP – still incorrect.

gpus should default to a sane value (e.g., 1 or the default config) instead of tp. This was raised earlier but not fixed.

tests/scripts/perf-sanity/run_benchmark_serve.py (1)

260-299: Handle server_process safely and terminate gracefully.

server_process may be undefined if Popen fails, and the cleanup always sends kill -9. Initialise to None, use terminate()/kill() guards, then wait. See prior review for diff.

tests/scripts/perf-sanity/benchmark_config.yaml (1)

458-468: Add missing newline & remove trailing spaces.

Line 468 has trailing whitespace and the file lacks a terminating newline, tripping YAML-lint (new-line-at-end-of-file).
Minor, but fixing avoids false CI noise.

tests/scripts/perf-sanity/README.md (1)

175-183: Specify a language on fenced block.

Add “bash” (or “text”) after the triple-backticks to satisfy MD040 and improve syntax highlighting.

tests/scripts/perf-sanity/benchmark-serve-prerelease.sh (1)

46-47: Quote variable expansion in echo/realpath.

Wrap ${results} in quotes to avoid path issues with spaces:

-    echo "Report path" $(realpath ${results})
+    echo "Report path" "$(realpath "${results}")"

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: galagam
PR: NVIDIA/TensorRT-LLM#6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.

Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.

Learnt from: yibinl-nvidia
PR: NVIDIA/TensorRT-LLM#6506
File: examples/models/core/mixtral/requirements.txt:3-3
Timestamp: 2025-08-01T15:14:45.673Z
Learning: In TensorRT-LLM, examples directory can have different dependency versions than the root requirements.txt file. Version conflicts between root and examples dependencies are acceptable because examples are designed to be standalone and self-contained.

Learnt from: galagam
PR: NVIDIA/TensorRT-LLM#6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.

Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.

Learnt from: yibinl-nvidia
PR: NVIDIA/TensorRT-LLM#6506
File: examples/models/core/mixtral/requirements.txt:3-3
Timestamp: 2025-08-01T15:14:45.673Z
Learning: In TensorRT-LLM, examples directory can have different dependency versions than the root requirements.txt file. Version conflicts between root and examples dependencies are acceptable because examples are designed to be standalone and self-contained.

cpp/tensorrt_llm/kernels/quantization.cu (5)

165-168: Mirror the padding-aware grid; same guard/constant concerns apply

Same feedback as above for the non-FP8 path: great to pad m when SWIZZLED and clamp by occupancy. Please use the shared constexpr and confirm kernel-side row bounds checks.

Apply within this hunk:

-        // The number of blocks for m. The m dimension will be padded to 128 for swizzled layout.
-        int numBlocksForM = layout == QuantizationSFLayout::SWIZZLED ? PadUpFn(m, 128) : m;
-        dim3 grid(std::min(numBlocksForM, multiProcessorCount * numBlocksPerSM));
+        // The number of blocks for m. The m dimension will be padded to kSWIZZLE_ROWS_PER_TILE for swizzled layout.
+        int const numBlocksForM =
+            (layout == QuantizationSFLayout::SWIZZLED) ? PadUpFn(m, kSWIZZLE_ROWS_PER_TILE) : m;
+        dim3 grid(std::min(numBlocksForM, multiProcessorCount * numBlocksPerSM));

---

203-206: MXFP8 path: consistent padding; prefer named constexpr and verify kernel bounds

Consistency here is good. Please switch 128 to a named constexpr and confirm quantize kernels safely ignore padded rows (blockIdx.x >= m).

Apply within this hunk:

-    // The number of blocks for m. The m dimension will be padded to 128 for swizzled layout.
-    int numBlocksForM = layout == QuantizationSFLayout::SWIZZLED ? PadUpFn(m, 128) : m;
-    dim3 grid(std::min(numBlocksForM, multiProcessorCount * numBlocksPerSM));
+    // The number of blocks for m. The m dimension will be padded to kSWIZZLE_ROWS_PER_TILE for swizzled layout.
+    int const numBlocksForM =
+        (layout == QuantizationSFLayout::SWIZZLED) ? PadUpFn(m, kSWIZZLE_ROWS_PER_TILE) : m;
+    dim3 grid(std::min(numBlocksForM, multiProcessorCount * numBlocksPerSM));

---

141-149: De-duplicate grid computation logic across the three call sites

The same grid sizing logic is repeated three times. Suggest extracting a small helper to reduce duplication and keep the occupancy arithmetic in one place.

Example helper (add in an internal namespace in this TU):

namespace {
inline dim3 makeGridForM(int m, QuantizationSFLayout layout, int blockX, int multiProcessorCount) {
    int const numBlocksPerSM = std::max(1, 2048 / std::max(1, blockX)); // blockX cannot be 0
    int const mBlocks = (layout == QuantizationSFLayout::SWIZZLED) ? PadUpFn(m, kSWIZZLE_ROWS_PER_TILE) : m;
    return dim3(std::min(mBlocks, multiProcessorCount * numBlocksPerSM));
}
} // anonymous namespace

Then replace:

dim3 grid = makeGridForM(m, layout, block.x, multiProcessorCount);

Note: If numBlocksPerSM differs per path in the future, pass the divisor as a parameter or create variants.

Also applies to: 160-168, 198-206

---

1-15: Update copyright year

Per the coding guidelines, source files should carry the current year. Please update to include 2025.

- * Copyright (c) 2019-2023, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (c) 2019-2025, NVIDIA CORPORATION.  All rights reserved.

---

146-149: Ensure OOB safety and replace the magic 128 with a named constant

Before landing this change, please:

• Centralize the swizzle-tile size and remove the literal 128. For example, near the top of cpp/tensorrt_llm/kernels/quantization.cu (after your namespace opens) add:

// Swizzled layout uses 128-row tiles; keep this centralized to avoid magic numbers.
constexpr int kSWIZZLE_ROWS_PER_TILE = 128;

• In the block that computes numBlocksForM (around lines 146–149), update to:

-        // The number of blocks for m. The m dimension will be padded to 128 for swizzled layout.
-        int numBlocksForM = layout == QuantizationSFLayout::SWIZZLED ? PadUpFn(m, 128) : m;
+        // The number of blocks for m. The m dimension will be padded to kSWIZZLE_ROWS_PER_TILE for swizzled layout.
+        int const numBlocksForM =
+            (layout == QuantizationSFLayout::SWIZZLED)
+                ? PadUpFn(m, kSWIZZLE_ROWS_PER_TILE)
+                : m;
         dim3 grid(std::min(numBlocksForM, multiProcessorCount * numBlocksPerSM));

• Verify that each quantize_with_block_size<…> kernel (in the corresponding .cuh file) guards against extra padded rows when blockIdx.x ≥ m—either via a strided loop (for (rowIdx = blockIdx.x; rowIdx < paddedRows; rowIdx += gridDim.x)) or an early return. If it doesn’t, add an explicit if (blockIdx.x >= m) return; (or similar) at kernel entry.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.

Learnt from: achartier
PR: NVIDIA/TensorRT-LLM#6763
File: tests/integration/defs/triton_server/conftest.py:16-22
Timestamp: 2025-08-11T20:09:24.378Z
Learning: In the TensorRT-LLM test infrastructure, the team prefers simple, direct solutions (like hard-coding directory traversal counts) over more complex but robust approaches when dealing with stable directory structures. They accept the maintenance cost of updating tests if the layout changes.

Learnt from: galagam
PR: NVIDIA/TensorRT-LLM#6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.

tests/scripts/perf-sanity/run_benchmark_serve.py (3)

600-606: Extract log filename generation into a helper method.

The log filename generation logic is duplicated between server and benchmark logs. Consider extracting it into a helper method for better maintainability.

Add a helper method to generate log filenames:

def generate_log_filename(self, test_case: Dict[str, Any], prefix: str, suffix: str = "") -> str:
    """Generate consistent log filename based on test case configuration."""
    filename = (
        f"{prefix}.{test_case['model']}.tp{test_case['tp']}.ep{test_case['ep']}."
        f"attn{test_case['attn_backend']}.moe{test_case['moe_backend']}."
        f"gpu{test_case['free_gpu_mem_fraction']}.batch{test_case['max_batch_size']}."
        f"isl{test_case['isl']}.osl{test_case['osl']}."
        f"tokens{test_case['max_num_tokens']}.moetokens{test_case['moe_max_num_tokens']}"
    )
    if suffix:
        filename = f"{filename}.{suffix}"
    return f"{filename}.log"

Then update the usage:

-        server_log_filename = (
-            f"trtllm-serve.{model_label}.tp{test_case['tp']}.ep{test_case['ep']}."
-            f"attn{test_case['attn_backend']}.moe{test_case['moe_backend']}."
-            f"gpu{test_case['free_gpu_mem_fraction']}.batch{test_case['max_batch_size']}."
-            f"isl{test_case['isl']}.osl{test_case['osl']}."
-            f"tokens{test_case['max_num_tokens']}.moetokens{test_case['moe_max_num_tokens']}.log"
-        )
+        server_log_filename = self.generate_log_filename(test_case, "trtllm-serve")

---

46-58: Use an external configuration for model paths.

The hard-coded model paths should be moved to an external configuration file for better maintainability and to avoid exposing internal directory structures in the code.

Consider moving the model paths to a separate YAML configuration file (e.g., model_paths.yaml):

model_paths:
  70B-FP4: "/home/scratch.trt_llm_data/llm-models/llama-3.3-models/Llama-3.3-70B-Instruct-FP4"
  70B-FP8: "/home/scratch.trt_llm_data/llm-models/llama-3.3-models/Llama-3.3-70B-Instruct-FP8"
  Scout-FP4: "/home/scratch.trt_llm_data/llm-models/llama4-models/Llama-4-Scout-17B-16E-Instruct-FP4"
  Scout-FP8: "/home/scratch.trt_llm_data/llm-models/llama4-models/Llama-4-Scout-17B-16E-Instruct-FP8"
  R1-FP8: "/home/scratch.trt_llm_data/llm-models/DeepSeek-R1/DeepSeek-R1/"
  R1-FP4: "/home/scratch.trt_llm_data/llm-models/DeepSeek-R1/DeepSeek-R1-0528-FP4"

Then update the initialization to load from the configuration:

-        # Model path mapping
-        self.model_paths = {
-            "70B-FP4":
-            "/home/scratch.trt_llm_data/llm-models/llama-3.3-models/Llama-3.3-70B-Instruct-FP4",
-            "70B-FP8":
-            "/home/scratch.trt_llm_data/llm-models/llama-3.3-models/Llama-3.3-70B-Instruct-FP8",
-            "Scout-FP4":
-            "/home/scratch.trt_llm_data/llm-models/llama4-models/Llama-4-Scout-17B-16E-Instruct-FP4",
-            "Scout-FP8":
-            "/home/scratch.trt_llm_data/llm-models/llama4-models/Llama-4-Scout-17B-16E-Instruct-FP8",
-            "R1-FP8":
-            "/home/scratch.trt_llm_data/llm-models/DeepSeek-R1/DeepSeek-R1/",
-            "R1-FP4":
-            "/home/scratch.trt_llm_data/llm-models/DeepSeek-R1/DeepSeek-R1-0528-FP4"
-        }
+        # Load model path mapping from configuration
+        model_paths_file = self.config_file.parent / "model_paths.yaml"
+        if model_paths_file.exists():
+            with open(model_paths_file, 'r') as f:
+                model_config = yaml.safe_load(f)
+                self.model_paths = model_config.get('model_paths', {})
+        else:
+            self.model_paths = {}
+            print(f"Warning: Model paths configuration file not found: {model_paths_file}")

---

653-665: Fix UnboundLocalError and improve server cleanup.

The server_process variable may be undefined if subprocess.Popen fails, causing an UnboundLocalError in the finally block. Also, using kill -9 immediately is too aggressive.

Initialize server_process before the try block and use a more graceful shutdown approach:

+        server_process = None
         try:
             with open(server_log_filename, 'w') as log_file:
                 log_file.write(f"extra-llm-api-config.yml:\n")
                 log_file.write(config_content)
                 log_file.write("\n")

             with open(server_log_filename, 'a') as log_file:
                 server_process = subprocess.Popen(serve_cmd,
                                                   stdout=log_file,
                                                   stderr=subprocess.STDOUT)
 
             # ... rest of the try block ...
 
         finally:
-            # Cleanup: Kill server process using shell commands like in the original bash script
             print(f"Stopping server for {model_label}")
             try:
-                # Use shell commands for more reliable process killing
-                subprocess.run(f"kill -9 {server_process.pid}",
-                               shell=True,
-                               check=False)
-                subprocess.run(f"wait {server_process.pid} 2>/dev/null || true",
-                               shell=True,
-                               check=False)
+                if server_process and server_process.poll() is None:
+                    # Try graceful termination first
+                    server_process.terminate()
+                    try:
+                        server_process.wait(timeout=10)
+                    except subprocess.TimeoutExpired:
+                        print("Server didn't exit on SIGTERM; sending SIGKILL.")
+                        server_process.kill()
+                        server_process.wait(timeout=5)
             except Exception as e:
                 print(f"Warning: Error during server cleanup: {e}")

tests/scripts/perf-sanity/run_benchmark_serve.py (2)

16-20: Add copyright header to the file.

All source files should include the NVIDIA copyright header as per the coding guidelines.

Add the following copyright header at the beginning of the file:

# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
# 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.

---

384-384: Consider more specific error patterns.

The error checking is comprehensive, but consider checking for more specific patterns to avoid false positives.

-                    if "RuntimeError" in content or "runtime error" in content or "illegal memory access" in content or "terminate called" in content:
+                    error_patterns = [
+                        "RuntimeError:",
+                        "runtime error:",
+                        "CUDA error: an illegal memory access",
+                        "terminate called after throwing",
+                        "Segmentation fault",
+                        "Aborted (core dumped)"
+                    ]
+                    if any(pattern in content for pattern in error_patterns):
                         return True

tests/scripts/perf-sanity/benchmark-serve.sh (2)

1-1: Add copyright header to the file.

All source files should include the NVIDIA copyright header as per the coding guidelines.

Add the following copyright header after the shebang:

# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
# 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.

---

51-51: Consider making --pull configurable.

The --pull always flag forces pulling the image every time, which may not be desirable in all environments (e.g., offline or bandwidth-limited scenarios).

Make the pull behavior configurable:

+DOCKER_PULL_POLICY=${DOCKER_PULL_POLICY:-always}
+
 run_benchmark_and_parse() {
     # Run benchmark and parse results in a single Docker container
     docker run --rm --ipc=host --ulimit memlock=-1 --ulimit stack=67108864 \
         --gpus all \
         -v /home/scratch.trt_llm_data:/home/scratch.trt_llm_data:ro \
         -v $output_dir:$output_dir:rw \
         -v $bench_dir:$bench_dir:ro \
         -w "$(pwd)"  \
-        --pull always \
+        --pull ${DOCKER_PULL_POLICY} \
         ${IMAGE} \

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: achartier
PR: NVIDIA/TensorRT-LLM#6763
File: tests/integration/defs/triton_server/conftest.py:16-22
Timestamp: 2025-08-11T20:09:24.378Z
Learning: In the TensorRT-LLM test infrastructure, the team prefers simple, direct solutions (like hard-coding directory traversal counts) over more complex but robust approaches when dealing with stable directory structures. They accept the maintenance cost of updating tests if the layout changes.

Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.