tests/integration/defs/test_e2e.py (1)

411-411: Remove the hard-coded sensitive test data.

Line 411 contains a hard-coded message with mathematical expressions and prompts that appear to be test data. This should be externalized to a configuration file or test data repository for better maintainability and security.

Replace the hard-coded message with a reference to external test data:

-    message = r"<｜begin▁of▁sentence｜><｜User｜>The operation $\otimes$ is defined for all nonzero numbers by $a \otimes b = \frac{a^{2}}{b}$. Determine $[(1 \otimes 2) \otimes 3] - [1 \otimes (2 \otimes 3)]$. Let's think step by step and output the final answer within \boxed{}.<｜Assistant｜>"
+    # Load test message from external configuration
+    message = load_test_message("qwen_math_operation_test")

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.

tests/integration/defs/test_e2e.py (3)

633-633: Fix function docstring formatting and wording.

Use a proper docstring with closing punctuation; also “benchmark” is one word.

-    "run bench mark in MIG mode, check if the throughput is increasing by concurrency"
+    """Run benchmark in MIG mode; check that throughput increases with concurrency."""

---

573-601: Parse numeric throughput/latency values; current string parsing will break downstream.

parse_benchmark_output currently stores raw strings (likely with units), but the MIG test converts them to float, which will raise ValueError when units are present. Parse and store numeric floats.

     def parse_benchmark_output(self, output):
         """Parse the benchmark output to extract key metrics."""
         result = {
             'concurrency': self.concurrency,
             'num_requests': self.num_requests,
-            'throughput': 0,
-            'latency': 0
+            'throughput': 0.0,
+            'latency': 0.0
         }
 
         lines = output.split('\n')
         for line in lines:
             line = line.strip()
             if 'total token throughput' in line.lower(
             ) and 'tokens/sec' in line.lower():
                 try:
-                    throughput = line.split(":")[1].strip()
-                    result['throughput'] = throughput
-                except (IndexError, ValueError) as e:
-                    print(f"Failed to parse throughput from line: {line}. Error: {e}")
+                    throughput_str = line.split(":", 1)[1].strip()
+                    m = re.search(r'([-+]?\d*\.?\d+)', throughput_str)
+                    if m:
+                        result['throughput'] = float(m.group(1))
+                except (IndexError, ValueError) as e:
+                    print(f"Failed to parse throughput from line: {line}. Error: {e}")
                     pass
             elif 'total latency' in line.lower() and 'ms' in line.lower():
                 try:
-                    latency = line.split(":")[1].strip()
-                    result['latency'] = latency
-                except (IndexError, ValueError) as e:
-                    print(f"Failed to parse latency from line: {line}. Error: {e}")
+                    latency_str = line.split(":", 1)[1].strip()
+                    m = re.search(r'([-+]?\d*\.?\d+)', latency_str)
+                    if m:
+                        result['latency'] = float(m.group(1))
+                except (IndexError, ValueError) as e:
+                    print(f"Failed to parse latency from line: {line}. Error: {e}")
                     pass
 
         return result

---

678-679: Relax growth factor and wrap long assert message (E501).

1. The 1.3x requirement is aggressive and may be flaky; use 1.2x (or make it a constant).
2. The assert line exceeds 120 chars; wrap with parentheses.

-            prev_throughput = float(results[concurrency_list[idx - 1]].get('throughput', 0))
-            assert throughput > prev_throughput * 1.3, f"Throughput is not increasing for concurrency {concurrency_list[idx]}"
+            prev_throughput = float(
+                results[concurrency_list[idx - 1]].get('throughput', 0)
+            )
+            growth_factor = 1.2
+            assert throughput > prev_throughput * growth_factor, (
+                f"Expected >= {growth_factor}x throughput increase from "
+                f"{concurrency_list[idx-1]} to {concurrency_list[idx]}: "
+                f"{prev_throughput} -> {throughput}"
+            )

tests/integration/defs/test_e2e.py (1)

563-567: Use explicit None checks for CLI options to avoid dropping 0-values.

Truthiness checks will skip valid zero-values (e.g., concurrency=0). Prefer explicit is not None.

-        if self.concurrency:
-            benchmark_cmd += f" --concurrency {self.concurrency}"
-        if self.num_requests:
-            benchmark_cmd += f" --num_requests {self.num_requests}"
+        if self.concurrency is not None:
+            benchmark_cmd += f" --concurrency {self.concurrency}"
+        if self.num_requests is not None:
+            benchmark_cmd += f" --num_requests {self.num_requests}"

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.

tests/integration/defs/test_e2e.py (3)

635-635: Fix docstring punctuation and wording.

End the docstring with a period and use “benchmark” as one word. D415 flagged the missing punctuation.

Apply this diff:

-    "run bench mark in MIG mode, check if the throughput is increasing by concurrency"
+    """Run benchmark in MIG mode, check if the throughput increases with concurrency."""

---

681-685: Relax growth factor and wrap long assertion message (E501).

1.3x may be too aggressive and can flake depending on scheduling/noise; 1.2x is more conservative. Also wrap the assertion string to satisfy the 120-column limit.

Apply this diff:

-        if idx > 0:
-            prev_throughput = float(results[concurrency_list[idx - 1]].get(
-                'throughput', 0))
-            assert throughput > prev_throughput * 1.3, f"Throughput is not increasing for concurrency {concurrency_list[idx]}"
+        if idx > 0:
+            prev_throughput = float(
+                results[concurrency_list[idx - 1]].get('throughput', 0)
+            )
+            assert throughput > prev_throughput * 1.2, (
+                f"Expected throughput increase from {concurrency_list[idx - 1]} "
+                f"to {concurrency_list[idx]}: {prev_throughput} -> {throughput}"
+            )

---

575-603: Parse numeric throughput/latency; return floats instead of raw strings.

The parser currently stores raw substrings (e.g., “123 tokens/sec”), which will cause float(...) conversion errors downstream. Extract the numeric portion and return floats. Also split only once on “:” to avoid surprises.

Apply this diff:

-    def parse_benchmark_output(self, output):
-        """Parse the benchmark output to extract key metrics."""
-        result = {
-            'concurrency': self.concurrency,
-            'num_requests': self.num_requests,
-            'throughput': 0,
-            'latency': 0
-        }
-
-        lines = output.split('\n')
-        for line in lines:
-            line = line.strip()
-            if 'total token throughput' in line.lower(
-            ) and 'tokens/sec' in line.lower():
-                try:
-                    throughput = line.split(":")[1].strip()
-                    result['throughput'] = throughput
-                except (IndexError, ValueError) as e:
-                    print(
-                        f"Failed to parse throughput from line: {line}. Error: {e}"
-                    )
-            elif 'total latency' in line.lower() and 'ms' in line.lower():
-                try:
-                    latency = line.split(":")[1].strip()
-                    result['latency'] = latency
-                except (IndexError, ValueError) as e:
-                    print(
-                        f"Failed to parse latency from line: {line}. Error: {e}"
-                    )
-
-        return result
+    def parse_benchmark_output(self, output: str):
+        """Parse benchmark stdout to extract numeric metrics.
+
+        Returns:
+            dict: {
+                'concurrency': Optional[int],
+                'num_requests': int,
+                'throughput': float,  # tokens/sec
+                'latency': float,     # ms
+            }
+        """
+        result = {
+            'concurrency': self.concurrency,
+            'num_requests': self.num_requests,
+            'throughput': 0.0,
+            'latency': 0.0,
+        }
+
+        lines = output.split('\n')
+        for line in lines:
+            s = line.strip()
+            low = s.lower()
+            if 'total token throughput' in low and 'tokens/sec' in low:
+                try:
+                    payload = s.split(":", 1)[1].strip()
+                    m = re.search(r'([-+]?\d*\.?\d+)', payload)
+                    if m:
+                        result['throughput'] = float(m.group(1))
+                except (IndexError, ValueError) as e:
+                    print(f"Failed to parse throughput from line: {s}. Error: {e}")
+            elif 'total latency' in low and 'ms' in low:
+                try:
+                    payload = s.split(":", 1)[1].strip()
+                    m = re.search(r'([-+]?\d*\.?\d+)', payload)
+                    if m:
+                        result['latency'] = float(m.group(1))
+                except (IndexError, ValueError) as e:
+                    print(f"Failed to parse latency from line: {s}. Error: {e}")
+
+        return result

Note: This also preserves your targeted exception handling introduced here. Nice improvement over the previous bare except.

tests/integration/defs/test_e2e.py (2)

452-455: New BenchRunner parameters look good; please document them.

Adding concurrency and num_requests is a clean extension and aligns with the new MIG-flow test usage. Consider adding a short Google-style docstring for __init__ to document these params and their semantics (e.g., whether 0 is meaningful) for future readers.

---

563-567: Use explicit None-checks to avoid silently skipping zero values.

Truthiness checks will skip 0, which could be a valid explicit input in some scenarios. Prefer explicit None-checks.

Apply this diff:

-        if self.concurrency:
-            benchmark_cmd += f" --concurrency {self.concurrency}"
-        if self.num_requests:
-            benchmark_cmd += f" --num_requests {self.num_requests}"
+        if self.concurrency is not None:
+            benchmark_cmd += f" --concurrency {self.concurrency}"
+        if self.num_requests is not None:
+            benchmark_cmd += f" --num_requests {self.num_requests}"

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

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

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.