Skip to content

[https://nvbugs/5766952][fix] Fix AIPerf issue. - #10666

Merged
Superjomn merged 3 commits into
NVIDIA:mainfrom
dominicshanshan:user/shanshan/nvbug_5766952
Jan 15, 2026
Merged

[https://nvbugs/5766952][fix] Fix AIPerf issue.#10666
Superjomn merged 3 commits into
NVIDIA:mainfrom
dominicshanshan:user/shanshan/nvbug_5766952

Conversation

@dominicshanshan

@dominicshanshan dominicshanshan commented Jan 14, 2026

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • Tests

    • Improved stress test metrics extraction with enhanced fallback mechanisms for model name and concurrency configuration.
    • Added error handling to validate metrics availability and raise exceptions when required data is missing.
  • Refactor

    • Updated artifact directory resolution logic to compute default paths dynamically instead of using fixed relative paths.

✏️ Tip: You can customize this high-level summary in your review settings.

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

  • 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

/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.

Details

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.

Signed-off-by: Wangshanshan <30051912+dominicshanshan@users.noreply.github.com>
Signed-off-by: Wangshanshan <30051912+dominicshanshan@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Jan 14, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Modified a stress test utility file to update function signatures and parameter handling. Changed artifacts directory default from a hardcoded relative path to None with runtime resolution. Updated metrics parsing to use fallback logic for concurrency values and model names. Enhanced JSON file discovery with recursive glob searching. Added error handling for missing or invalid metrics.

Changes

Cohort / File(s) Summary
Stress Test Metrics Extraction
tests/integration/defs/stress_test/stress_test.py
Disabled --verbose flag in aiperf command. Changed extract_stress_test_metrics signature from artifacts_dir="./artifacts" to artifacts_dir=None with dynamic path resolution relative to script location. Updated concurrency extraction to read from input_config.loadgen.concurrency with fallback to input_config.perf_analyzer.stimulus.concurrency. Modified model name extraction with fallback chain: directory → input_config.endpoint.model_namesinput_config.model_names with "unknown" default. Changed JSON file discovery to use glob with recursive=True. Added error handling for missing/invalid metrics.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description only contains the template boilerplate with no actual content. Required sections (Description, Test Coverage) are empty, and the PR Checklist shows no substantive information about what or why the changes were made. Provide a Description explaining the AIPerf issue being fixed and the solution implemented. Include Test Coverage section listing relevant tests that validate the changes.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required format with NVBugs ticket, fix type, and summary. It directly relates to the changeset's focus on fixing AIPerf metrics extraction and related issues.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
tests/integration/defs/stress_test/stress_test.py (5)

809-810: Consider adding a brief comment explaining why verbose is disabled.

The commented-out --verbose flag is acceptable, but a brief inline comment explaining the rationale (e.g., reducing log noise, performance reasons) would improve maintainability for future developers.


1148-1161: Good improvement to make default path relative to script location.

The change from a hardcoded "./artifacts" to a script-relative path is more robust across different execution contexts. Consider normalizing the path for cleaner error messages and logging.

💡 Optional: Normalize the path
     if artifacts_dir is None:
         script_dir = os.path.dirname(os.path.abspath(__file__))
-        artifacts_dir = os.path.join(script_dir, "..", "artifacts")
+        artifacts_dir = os.path.normpath(os.path.join(script_dir, "..", "artifacts"))

1220-1226: Variable naming doesn't follow snake_case convention.

Per coding guidelines, Python local variables should use snake_case. The variable conCurrency should be renamed to con_currency or simply concurrency_value to avoid shadowing the list variable concurrency defined at line 1194.

♻️ Suggested fix
-                conCurrency = results.get("input_config",
-                                          {}).get("loadgen",
-                                                  {}).get("concurrency", 0)
-                if conCurrency == 0:
-                    conCurrency = results.get("input_config", {}).get(
-                        "perf_analyzer", {}).get("stimulus",
-                                                 {}).get("concurrency", 0)
+                concurrency_value = results.get("input_config",
+                                                {}).get("loadgen",
+                                                        {}).get("concurrency", 0)
+                if concurrency_value == 0:
+                    concurrency_value = results.get("input_config", {}).get(
+                        "perf_analyzer", {}).get("stimulus",
+                                                 {}).get("concurrency", 0)

Also update references at lines 1243, 1246 to use concurrency_value.


1233-1240: Variable naming doesn't follow snake_case convention.

Similar to the previous comment, modelName should be renamed to model_name_value (to avoid shadowing the list model_name at line 1196) per coding guidelines.

♻️ Suggested fix
-                    modelName = results.get("input_config", {}).get(
-                        "endpoint", {}).get("model_names", None)
-                    if modelName is None:
-                        modelName = results.get("input_config",
-                                                {}).get("model_names",
-                                                        ["unknown"])
-                    modelName = modelName[0] if isinstance(modelName,
-                                                           list) else modelName
+                    model_name_value = results.get("input_config", {}).get(
+                        "endpoint", {}).get("model_names", None)
+                    if model_name_value is None:
+                        model_name_value = results.get("input_config",
+                                                       {}).get("model_names",
+                                                               ["unknown"])
+                    model_name_value = model_name_value[0] if isinstance(model_name_value,
+                                                                         list) else model_name_value

Also update references at lines 1243, 1247.


1-1: Update copyright year to include 2026.

Per coding guidelines, all source files should contain an NVIDIA copyright header with the year of latest meaningful modification. Since this file is being modified in 2026, update the copyright to 2022-2025 or 2022-2026.

♻️ Suggested fix
-# SPDX-FileCopyrightText: Copyright (c) 2022-2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# SPDX-FileCopyrightText: Copyright (c) 2022-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.

Based on coding guidelines requiring copyright headers with the year of latest meaningful modification.


📜 Recent 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 287f6c2 and da25ee6.

📒 Files selected for processing (1)
  • tests/integration/defs/stress_test/stress_test.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

**/*.py: The code developed for TensorRT-LLM should conform to Python 3.8+
Indent Python code with 4 spaces. Do not use tabs
Always maintain the namespace when importing Python modules, even if only one class or function from a module is used
Python filenames should use snake_case (e.g., some_file.py)
Python classes should use PascalCase (e.g., class SomeClass)
Python functions and methods should use snake_case (e.g., def my_awesome_function():)
Python local variables should use snake_case, with prefix k for variable names that start with a number (e.g., k_99th_percentile)
Python global variables should use upper snake_case with prefix G (e.g., G_MY_GLOBAL)
Python constants should use upper snake_case (e.g., MY_CONSTANT)
Avoid shadowing variables declared in an outer scope in Python
Initialize all externally visible members of a Python class in the constructor
For Python interfaces that may be used outside a file, prefer docstrings over comments
Use comments in Python for code within a function, or interfaces that are local to a file
Use Google-style docstrings for Python classes and functions, which can be parsed by Sphinx
Python attributes and variables can be documented inline with the format """<type>: Description"""
Avoid using reflection in Python when functionality can be easily achieved without reflection
When using try-except blocks in Python, limit the except clause to the smallest set of errors possible
When using try-except blocks in Python to handle multiple possible variable types (duck-typing), keep the body of the try as small as possible and use the else block for the main logic

Files:

  • tests/integration/defs/stress_test/stress_test.py
**/*.{cpp,cc,cxx,h,hpp,hxx,cu,cuh,py}

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

All TensorRT-LLM source files (.cpp, .h, .cu, .py, and other source files) should contain an NVIDIA copyright header with the year of latest meaningful modification

Files:

  • tests/integration/defs/stress_test/stress_test.py
⏰ 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

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

@dominicshanshan

Copy link
Copy Markdown
Collaborator Author

Error introduced from this PR

Signed-off-by: Wangshanshan <30051912+dominicshanshan@users.noreply.github.com>
@dominicshanshan

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #31954 [ run ] triggered by Bot. Commit: 6d71ce3

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #31954 [ run ] completed with state SUCCESS. Commit: 6d71ce3
/LLM/main/L0_MergeRequest_PR pipeline #24752 completed with status: 'SUCCESS'

@Superjomn Superjomn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Superjomn
Superjomn merged commit 0f2d61b into NVIDIA:main Jan 15, 2026
5 checks passed
greg-kwasniewski1 pushed a commit to nv-auto-deploy/TensorRT-LLM that referenced this pull request Jan 15, 2026
Signed-off-by: Wangshanshan <30051912+dominicshanshan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants