[TRTLLM-10253][infra] upload test output to storage

[niukuo]

Summary by CodeRabbit

Release Notes

• New Features

  • Added S3 log upload capability for test results with configurable endpoints, credentials, and upload paths.
  • Introduced pipeline parameter to control whether test results are uploaded.
  • Added new environment variables and command-line options for S3 configuration.

• Dependencies

  • Added boto3 requirement for S3 integration.

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

To see a list of available CI bot commands, please comment /bot help.

[niukuo]

/bot run

[tensorrt-cicd]

PR_Github #43420 [ run ] triggered by Bot. Commit: c3364bc Link to invocation

[coderabbitai]

📝 Walkthrough

Walkthrough

Introduces S3 log upload capability for pytest integration tests by adding new pytest plugin infrastructure, CLI options, and log capture utilities. Updates Jenkins pipeline to conditionally enable test result uploads and pass S3 credentials. Adds boto3 dependency.

Changes

Cohort / File(s)	Summary
Jenkins Pipeline Configuration jenkins/L0_Test.groovy	Added ENABLE_UPLOAD_TEST_RESULTS parameter and S3 credential binding. Modified getPytestBaseCommandLine() to conditionally set junit_logging based on parameter. Updated test command construction to compute S3 upload paths and pass extra arguments for S3 integration when enabled.
Pytest Integration Infrastructure tests/integration/defs/conftest.py	Added pytest CLI options for S3 endpoint configuration (--s3-endpoint, --s3-username, --s3-secret-key, --s3-bucket, --s3-upload-path) with environment variable fallbacks. Implemented conditional plugin registration of UploadLogPlugin when S3 upload path is specified.
S3 Log Upload Implementation tests/integration/defs/s3_output.py	New module implementing EnvDefault action for envvar-aware argument defaults, FDRedirector context manager for capturing file descriptors with timestamped logging, and UploadLogPlugin pytest hook for capturing test output (stdout, stderr, logging) and uploading to S3-compatible endpoints with error handling and report sections.
Dependencies requirements-dev.txt	Added explicit boto3 dependency for S3 operations.

Sequence Diagram(s)

sequenceDiagram
    participant Test as Pytest Test
    participant FDRedir as FDRedirector
    participant LogFile as Log Files
    participant S3 as S3 Endpoint
    participant Report as Test Report

    Test->>FDRedir: pytest_runtest_call (enter context)
    activate FDRedir
    FDRedir->>FDRedir: Redirect FD 1 & 2 to pipes
    FDRedir->>FDRedir: Start reader thread
    Test->>Test: Test execution (print to stdout/stderr)
    FDRedir->>FDRedir: Reader captures output, adds timestamps
    FDRedir->>LogFile: Write timestamped lines to log file
    Test->>Test: Test completes
    FDRedir->>FDRedir: Restore original FDs
    deactivate FDRedir
    
    Test->>UploadLogPlugin: pytest_runtest_logreport (teardown)
    activate UploadLogPlugin
    UploadLogPlugin->>LogFile: Read captured logs (stdout, stderr, logging)
    UploadLogPlugin->>S3: Upload logs with S3 credentials
    activate S3
    S3-->>UploadLogPlugin: Upload success/failure response
    deactivate S3
    UploadLogPlugin->>Report: Append upload outcome section
    deactivate UploadLogPlugin

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The PR description contains only the template with placeholder sections but no actual content explaining the issue, solution, or test coverage.	Fill in the Description and Test Coverage sections with details about what changes were made, why they were made, and what tests validate the functionality.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: uploading test output to storage, which is directly supported by the changeset.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@jenkins/L0_Test.groovy`:
- Around line 1003-1014: The sbatch branch builds extraArgs enabling
"--s3-upload-path" when ENABLE_UPLOAD_TEST_RESULTS is true but never supplies
the S3 secret; replicate the credentials handling used in
runLLMTestlistOnPlatformImpl(): wrap the sbatch submission logic in a
withCredentials block that retrieves the S3_SECRET_KEY (and any related S3
secret ID) and export them into the remote sbatch environment before invoking
the job so tests/integration/defs/s3_output.py can authenticate; update the code
around extraArgs and the sbatch submission to inject/export the S3 credentials
into the remote environment when ENABLE_UPLOAD_TEST_RESULTS is set.
- Line 28: The assignment to ENABLE_UPLOAD_TEST_RESULTS uses the Elvis operator
which treats false as falsy, so explicitly passing false gets overwritten;
change the logic to only default to true when the parameter is absent
(null/undefined) by checking for null (e.g. use
params.containsKey('enableUploadTestResults') or params.enableUploadTestResults
!= null) and assign ENABLE_UPLOAD_TEST_RESULTS = params.enableUploadTestResults
when present, otherwise true, referencing ENABLE_UPLOAD_TEST_RESULTS and
params.enableUploadTestResults in the fix.

In `@tests/integration/defs/conftest.py`:
- Around line 2352-2366: When s3_upload_path and output_dir are provided we must
validate required S3 credentials before constructing or registering
UploadLogPlugin; check values retrieved via config.getoption("--s3-username"),
config.getoption("--s3-secret-key") and config.getoption("--s3-bucket") (and
optionally "--s3-endpoint") and raise a ValueError with a clear message if any
required credential is missing or empty, then only instantiate
UploadLogPlugin(...) and call config.pluginmanager.register(upload_plugin,
"upload_log_plugin") after those validations; ensure the check also enforces
capture_mode == "no" as currently done.

In `@tests/integration/defs/s3_output.py`:
- Around line 1-12: Add the repository's mandatory NVIDIA copyright/SPDX header
at the very top of this new module (above the imports) so it appears before any
code; update the year if required. Locate the file by the module beginning
(imports such as argparse, logging, boto3 and the module-level logger variable
named logger) and insert the standard NVIDIA copyright block/header text used
across the repo (including SPDX identifier) as the first lines of the file.
- Around line 222-237: The except block currently reads and embeds up to 64KB of
file content into report.sections; instead, stop opening/reading the file and
append only failure metadata (error and path/size) matching the success-case
pattern: update the report.sections.append call (referencing section_name,
report.sections, and exception variable e) to include a short message like
"upload failed: {e}\nsize: {filesize} bytes\npath: {filepath}" and remove the
logger/file-read/truncation logic that produces `content` and `trail_content`.
- Around line 142-184: The hook currently defined as pytest_runtest_call only
wraps the test body and misses fixture setup/teardown output; change the hook to
pytest_runtest_protocol(wrapper=True) on the same function so the
FDRedirector/stdout/stderr redirection and catching_logs(handler) wrap the
entire test lifecycle (setup, call, teardown). Update the decorator and
signature to use pytest_runtest_protocol while keeping the same body that builds
test_name via normalize_test_name(item.nodeid), creates output_path, opens
stdout_file/stderr_file/logging.log, configures handler/formatter (using
item.config/pluginmanager and log_date_format/log_format/timestamp_format), and
uses the same with (FDRedirector(1,...), FDRedirector(2,...),
catching_logs(handler)) to yield control for the whole protocol.
- Around line 161-184: The FileHandler created as "handler =
logging.FileHandler(log_file)" is never closed; modify the generator so that
after the "with (FDRedirector(...), catching_logs(handler),): yield" returns
(teardown path) you explicitly call handler.close() (and optionally
handler.flush()) to release the file descriptor; keep the existing use of
catching_logs(handler) to remove the handler from the root logger but ensure
handler.close() is invoked after the yield in the same function (where handler
and catching_logs are used).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1e3cc76d-27d0-4997-b346-c5a74193bad4

📥 Commits

Reviewing files that changed from the base of the PR and between 4825da7 and c3364bc.

📒 Files selected for processing (4)

• jenkins/L0_Test.groovy
• requirements-dev.txt
• tests/integration/defs/conftest.py
• tests/integration/defs/s3_output.py

[tensorrt-cicd]

PR_Github #43420 [ run ] completed with state FAILURE. Commit: c3364bc
/LLM/main/L0_MergeRequest_PR pipeline #33952 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

• Please check the failed tests and fix your PR
• If you cannot view the failures, ask the CI triggerer to share details
• Once fixed, request an NVIDIA team member to trigger CI again

Link to invocation

[dpitman-nvda]

Two questions:

1. What's the thinking behind using (presumably) s8k versus other options like Artifactory?
2. Will the top-level MR scripts eventually be wired in as well? If so, how would we handle things like my failure analyzer agent's output (https://github.com/NVIDIA/TensorRT-LLM/blob/main/jenkins/L0_MergeRequest.groovy#L1360)?

[niukuo]

Two questions:

1. What's the thinking behind using (presumably) s8k versus other options like Artifactory?
2. Will the top-level MR scripts eventually be wired in as well? If so, how would we handle things like my failure analyzer agent's output (https://github.com/NVIDIA/TensorRT-LLM/blob/main/jenkins/L0_MergeRequest.groovy#L1360)?

1. We use s3 client to be compatible with most storage backend. Currently TensorRT uses s8k as log storage so we refer it as the default option. We could change to any storage with minor changes.
2. The url will be kept in the test report. It's expected that agent(s) will follow links in the report (not tested yet).

[tensorrt-cicd]

PR_Github #44666 [ run ] triggered by Bot. Commit: 09a26f6 Link to invocation

[tensorrt-cicd]

PR_Github #44666 [ run ] completed with state FAILURE. Commit: 09a26f6
/LLM/main/L0_MergeRequest_PR pipeline #35039 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

• Please check the failed tests and fix your PR
• If you cannot view the failures, ask the CI triggerer to share details
• Once fixed, request an NVIDIA team member to trigger CI again

Link to invocation

[niukuo]

/bot run

[tensorrt-cicd]

PR_Github #48822 [ run ] triggered by Bot. Commit: f7aa04e Link to invocation

[tensorrt-cicd]

PR_Github #48822 [ run ] completed with state FAILURE. Commit: f7aa04e
/LLM/main/L0_MergeRequest_PR pipeline #38582 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

• Please check the failed tests and fix your PR
• If you cannot view the failures, ask the CI triggerer to share details
• Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

[tensorrt-cicd]

PR_Github #56172 [ run ] triggered by Bot. Commit: 4932dd1 Link to invocation

[tensorrt-cicd]

PR_Github #56172 [ run ] completed with state SUCCESS. Commit: 4932dd1
/LLM/main/L0_MergeRequest_PR pipeline #45035 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

• Please check the failed tests and fix your PR
• If you cannot view the failures, ask the CI triggerer to share details
• Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

[tensorrt-cicd]

PR_Github #56184 [ run ] triggered by Bot. Commit: 4932dd1 Link to invocation

[tensorrt-cicd]

PR_Github #56184 [ run ] completed with state FAILURE. Commit: 4932dd1
/LLM/main/L0_MergeRequest_PR pipeline #45047 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

• Please check the failed tests and fix your PR
• If you cannot view the failures, ask the CI triggerer to share details
• Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

[tensorrt-cicd]

PR_Github #56198 [ run ] triggered by Bot. Commit: 4932dd1 Link to invocation

[tensorrt-cicd]

PR_Github #56198 [ run ] completed with state FAILURE. Commit: 4932dd1
/LLM/main/L0_MergeRequest_PR pipeline #45061 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

• Please check the failed tests and fix your PR
• If you cannot view the failures, ask the CI triggerer to share details
• Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

[tensorrt-cicd]

PR_Github #56235 [ run ] triggered by Bot. Commit: 4932dd1 Link to invocation

[tensorrt-cicd]

PR_Github #56235 [ run ] completed with state FAILURE. Commit: 4932dd1
/LLM/main/L0_MergeRequest_PR pipeline #45096 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

• Please check the failed tests and fix your PR
• If you cannot view the failures, ask the CI triggerer to share details
• Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

[tensorrt-cicd]

PR_Github #56498 [ run ] triggered by Bot. Commit: 4932dd1 Link to invocation

[tensorrt-cicd]

PR_Github #56498 [ run ] completed with state FAILURE. Commit: 4932dd1
/LLM/main/L0_MergeRequest_PR pipeline #45339 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

• Please check the failed tests and fix your PR
• If you cannot view the failures, ask the CI triggerer to share details
• Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

[tensorrt-cicd]

PR_Github #56523 [ run ] triggered by Bot. Commit: 4932dd1 Link to invocation

[tensorrt-cicd]

PR_Github #56523 [ run ] completed with state SUCCESS. Commit: 4932dd1
/LLM/main/L0_MergeRequest_PR pipeline #45361 completed with status: 'SUCCESS'

CI Report

Link to invocation

[yuanjingx87]

approved on oss compliance behalf