[TRTLLMINF-191][infra] Use native pytest capture for S3 logs

[niukuo]

Dev Engineer Review

• Migrated S3 log handling from legacy stdout/session + --s3-echo-stdout / ENABLE_S3_ECHO_STDOUT wiring to native pytest --capture=fd, with report-section transformation in tests/test_common/s3_output.py.
• Standardized S3-enabled pytest extra args when ENABLE_UPLOAD_TEST_RESULTS is enabled (both SLURM sbatch and non-SLURM execution paths):
  • --capture=fd
  • --s3-upload-path=${uploadPath}/${stageName}
  • --s3-upload-mode=deferred
  • Removed prior -s usage and legacy/conditional S3 argument wiring (including optional --s3-echo-stdout and prior capture/session/timestamp behavior).
• Reworked deferred upload implementation (tests/test_common/s3_output.py):
  • Spool-backed capture transformation with inline-vs-upload thresholding.
  • sync vs deferred upload modes with bounded concurrency (ThreadPoolExecutor).
  • Deferred mode deletes spool sources only after successful upload; upload failures preserve relevant spool/report details.
  • Added drain_pending_uploads(output_path, secret_key) to recover orphaned .s3-spool artifacts from crashed pytest processes (per-owner coordination with fcntl.flock + hostname/pid liveness checks).
• Simplified pytest hook registration (tests/test_common/s3_output_hooks.py):
  • Removed early conftest capture/claimed-state machinery.
  • Registers the upload transformer only on pytest workers (skips xdist controller) via a single pytest_configure hook.
• CI/lifecycle integration:
  • jenkins/scripts/slurm_run.sh: drains per-rank spool after eval $pytestCommand completes (drain failures tolerated with || true so diagnostics/exit-code handling still runs).
  • jenkins/L0_Test.groovy: removed ENABLE_S3_ECHO_STDOUT initialization; standardized pytest args as above; drains deferred output after non-SLURM pytest run via python3 .../s3_output.py --drain-spool "${WORKSPACE}/${stageName}" when upload results are enabled.
  • tests/integration/defs/test_unittests.py: when --s3-upload-path is set, enforces --capture=fd, adjusts the inner pytest command list to remove legacy -s, and drains pending uploads in a finally block using s3_output_module.drain_pending_uploads(...).
• API consistency checks:
  • UploadLogPlugin constructor signature updated to remove older echo/session capture parameters.
  • register_plugin(config) signature updated and now enforces --s3-upload-path together with native --capture=fd.

Potential review focus:

• Correctness and robustness of spool ownership/drain selection (flock scope + pid/hostname liveness) under crashes and pid reuse.
• Deferred upload ordering/cleanup guarantees and behavior on partial failures/timeouts.
• Ensuring every S3-enabled command-line path always enforces --capture=fd and that plugin registration call sites match the new register_plugin(config) contract.

QA Engineer Review

Touched test code (under tests/):

tests/unittest/test_s3_output.py (rewritten/expanded behavioral coverage)

Test functions present:

• test_small_stdout_remains_inline
• test_stdout_at_threshold_is_replaced_with_url
• test_inline_threshold_applies_to_combined_stream
• test_logging_section_is_uploaded_even_when_small
• test_sync_upload_transforms_native_sections
• test_duplicate_capture_sections_share_one_object
• test_cumulative_capture_sections_are_uploaded_once
• test_same_nodeid_rerun_gets_distinct_test_path
• test_failed_report_keeps_only_recent_bounded_output
• test_deferred_upload_starts_before_session_finish
• test_deferred_upload_reuses_cumulative_section
• test_rerun_keeps_one_url_per_attempt
• test_parent_drain_retries_upload_left_by_failed_process
• test_parent_drain_uses_configured_upload_workers
• test_parent_drain_skips_live_pytest_process
• test_register_plugin_requires_native_fd_capture
• test_register_plugin_uses_report_transformer
• test_s3_hook_skips_xdist_controller

tests/unittest/tools/test_test_to_stage_mapping.py

• Removed the legacy validation test for S3 stdout echo opt-in (test_s3_stdout_echo_requires_explicit_opt_in); current file contains:
  • test_data_availability
  • test_bidirectional_mapping_consistency (skipped)
  • test_search_functionality
  • test_cli_functionality
  • test_backend_filtering_consistency

tests/integration/defs/test_unittests.py

• Modified test_unittests_v2

Coverage in tests/integration/test_lists/ / test-db/ / qa/:

• Not determined from the provided context for the functions above. Verdict: needs follow-up.

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)

• If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in the PR title.

• 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 --disable-fail-fast

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Changes

The S3 pytest integration now transforms captured report sections, inlines or spools output, supports synchronous and deferred uploads, and drains pending spool files. Jenkins and integration runners enable FD capture and perform cleanup. Unit tests cover transformation, recovery, reruns, registration, and xdist behavior.

S3 deferred output workflow

Layer / File(s)	Summary
Report transformation and upload plugin tests/test_common/s3_output.py	UploadLogPlugin transforms pytest report sections, inlines small output, spools larger output, uploads synchronously or asynchronously, and exposes revised registration and CLI options.
Pending spool recovery tests/test_common/s3_output.py	drain_pending_uploads discovers orphaned spool files, validates ownership, uploads them concurrently, and removes completed spool state.
Pytest and Jenkins execution wiring tests/test_common/s3_output_hooks.py, tests/integration/defs/test_unittests.py, jenkins/L0_Test.groovy, jenkins/scripts/slurm_run.sh	Pytest workers register the plugin, commands use --capture=fd with deferred S3 uploads, and runners drain pending uploads after execution.
Upload behavior validation tests/unittest/test_s3_output.py	Tests validate report transformation, upload modes, rerun handling, spool recovery, capture requirements, and xdist registration.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Jenkins
  participant Pytest
  participant UploadLogPlugin
  participant Spool
  participant S3
  Jenkins->>Pytest: enable capture=fd and deferred S3 upload
  Pytest->>UploadLogPlugin: process pytest report sections
  UploadLogPlugin->>Spool: write oversized captured output
  UploadLogPlugin->>S3: upload or schedule deferred upload
  Jenkins->>Spool: drain pending uploads after pytest
  Spool->>S3: upload orphaned files

Loading

Possibly related PRs

• NVIDIA/TensorRT-LLM#16935: Modifies the overlapping S3 stdout echo, session spool, and Jenkins integration paths.

Suggested reviewers: qijune, schetlur-nv, zhanruisunch

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is only the template and lacks the required issue summary, solution details, and test coverage.	Fill in the Description and Test Coverage sections with a brief problem statement, the implemented change, and the relevant tests run.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the main change and follows the repository's ticket/type summary format.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[tensorrt-cicd]

PR_Github #60995 [ run ] triggered by Bot. Commit: 19e1af6 Link to invocation

[tensorrt-cicd]

PR_Github #60995 [ run ] completed with state FAILURE. Commit: 19e1af6
/LLM/main/L0_MergeRequest_PR pipeline #49254 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 #61007 [ run ] triggered by Bot. Commit: e9e5dd1 Link to invocation

[tensorrt-cicd]

PR_Github #61007 [ run ] completed with state FAILURE. Commit: e9e5dd1
/LLM/main/L0_MergeRequest_PR pipeline #49265 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 #61180 [ run ] triggered by Bot. Commit: ba8cca9 Link to invocation

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_common/s3_output.py`:
- Around line 268-274: Update the hashlib.md5 call in normalize_test_name to
pass usedforsecurity=False, preserving the existing nodeid-derived suffix and
filename normalization behavior.

🪄 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: Enterprise

Run ID: 67cdefca-2930-4fea-beb8-6027f9e62363

📥 Commits

Reviewing files that changed from the base of the PR and between e9e5dd1 and ba8cca9.

📒 Files selected for processing (7)

• jenkins/L0_Test.groovy
• jenkins/scripts/slurm_run.sh
• tests/integration/defs/test_unittests.py
• tests/test_common/s3_output.py
• tests/test_common/s3_output_hooks.py
• tests/unittest/test_s3_output.py
• tests/unittest/tools/test_test_to_stage_mapping.py

🚧 Files skipped from review as they are similar to previous changes (3)

• tests/integration/defs/test_unittests.py
• jenkins/L0_Test.groovy
• tests/test_common/s3_output_hooks.py

[tensorrt-cicd]

PR_Github #61248 [ run ] triggered by Bot. Commit: ba8cca9 Link to invocation

[tensorrt-cicd]

PR_Github #61180 [ run ] completed with state ABORTED. Commit: ba8cca9

Link to invocation

[tensorrt-cicd]

PR_Github #61248 [ run ] completed with state FAILURE. Commit: ba8cca9
/LLM/main/L0_MergeRequest_PR pipeline #49486 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 #61338 [ run ] triggered by Bot. Commit: e764c3c Link to invocation

[tensorrt-cicd]

PR_Github #61338 [ run ] completed with state FAILURE. Commit: e764c3c
/LLM/main/L0_MergeRequest_PR pipeline #49565 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 #61449 [ run ] triggered by Bot. Commit: e764c3c Link to invocation

[tensorrt-cicd]

PR_Github #61449 [ run ] completed with state SUCCESS. Commit: e764c3c
/LLM/main/L0_MergeRequest_PR pipeline #49671 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 #61495 [ run ] triggered by Bot. Commit: e764c3c Link to invocation

[tensorrt-cicd]

PR_Github #61495 [ run ] completed with state SUCCESS. Commit: e764c3c
/LLM/main/L0_MergeRequest_PR pipeline #49714 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 #61540 [ run ] triggered by Bot. Commit: e764c3c Link to invocation

[tensorrt-cicd]

PR_Github #61540 [ run ] completed with state SUCCESS. Commit: e764c3c
/LLM/main/L0_MergeRequest_PR pipeline #49751 completed with status: 'SUCCESS'

CI Report

Link to invocation

[tensorrt-cicd]

PR_Github #62381 [ run ] triggered by Bot. Commit: 6106fa2 Link to invocation

[tensorrt-cicd]

PR_Github #62381 [ run ] completed with state FAILURE. Commit: 6106fa2
/LLM/main/L0_MergeRequest_PR pipeline #50545 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 #62658 [ run ] triggered by Bot. Commit: 6106fa2 Link to invocation

[tensorrt-cicd]

PR_Github #62658 [ run ] completed with state SUCCESS. Commit: 6106fa2
/LLM/main/L0_MergeRequest_PR pipeline #50797 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 #62710 [ run ] triggered by Bot. Commit: 6106fa2 Link to invocation

[tensorrt-cicd]

PR_Github #62710 [ run ] completed with state SUCCESS. Commit: 6106fa2
/LLM/main/L0_MergeRequest_PR pipeline #50846 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 #62807 [ run ] triggered by Bot. Commit: 6106fa2 Link to invocation

[tensorrt-cicd]

PR_Github #62807 [ run ] completed with state FAILURE. Commit: 6106fa2
/LLM/main/L0_MergeRequest_PR pipeline #50933 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

[BowenFu]

Reviewed the whole change. Moving off the bespoke session-capture machinery onto native pytest capture is the right call, and the -815 in s3_output.py plus the -90 in s3_output_hooks.py are real simplification. Three things on the default path, one of them a knob that stops working.

1. --capture=fd silently disables the existing DETAILED_LOG filter.

getPytestBaseCommandLine puts the detailed-log flag near the front of testCmdLine:

testFilter[(DETAILED_LOG)] ? "-s" : "",

and appends extraArgs last (L0_Test.groovy:1392-1394), which is where --capture=fd now arrives. -s is store_const(const="no", dest="capture") and --capture writes the same dest, so last-one-wins and --capture=fd overrides it. Because ENABLE_UPLOAD_TEST_RESULTS defaults to true (:49), that is the default configuration for every stage.

On main there is no conflict: the S3 branch also added -s, so a DETAILED_LOG run passed -s twice and worked. After this PR, someone selecting DETAILED_LOG to debug a stage gets captured output instead and no indication why. If the intent is that --capture=fd is mandatory whenever --s3-upload-path is set — which register_plugin now enforces with raise ValueError("--s3-upload-path requires pytest --capture=fd") — then DETAILED_LOG needs to either suppress the S3 args or fail loudly, rather than being quietly overridden.

2. Output from a test that never finishes is now lost rather than already in the console log.

The plugin only observes output at pytest_runtest_logreport (s3_output.py:639), reading report.sections (:566) — i.e. after a test phase completes. Under -s on main, test stdout/stderr goes straight to the process stdout and is in the Jenkins console log as it is produced, so when a hanging test gets SIGKILLed by the stage timeout, everything it printed up to that point survives. Under --capture=fd it sits in pytest's fd buffer until the phase ends; a kill takes the buffer with it.

--drain-spool does not cover this. drain_pending_uploads walks .s3-spool-* for existing upload-config.json directories (:129-135), and spool files are only written from _append_capture on the report path — so it recovers completed-but-unuploaded tests, not the in-flight one. That is the case that matters most here: this repo has dedicated timeout machinery (results-timeout.xml, generateTimeoutTestResultXml, and the recent work to name the in-flight test in the timeout report) because hangs are a recurring problem, and this makes the hanging test the one whose output you can no longer see.

Worth saying explicitly in the description whether that trade was intended. --capture=tee-sys would keep both, at the cost of duplicating output into the console.

3. -o console_output_style=progress-even-when-capture-no now contradicts its own configuration.

:1390-1392 still injects it under the same ENABLE_UPLOAD_TEST_RESULTS guard that now sets --capture=fd. That option exists to fix progress rendering when capture is off; paired with --capture=fd it is a leftover from the -s era. Harmless, but it is the kind of stale pairing that makes the next reader think capture is still disabled.

Minor: removing the enableS3EchoStdout build parameter drops an operator-facing toggle with no replacement. It defaulted to false, so no default behaviour changes and I am not treating it as a regression — but if anyone was flipping it in the Jenkins UI to debug, that path is gone and the tests that asserted the wiring existed go with it (test_test_to_stage_mapping.py, -30).

The test_unittests.py hunk is fine — s3_output_module and inner_output_dir are both assigned under the same if s3_upload_path: guard that gates the finally, so the closure can't hit an unbound name. Bounded ThreadPoolExecutor and delete-only-after-successful-upload in deferred mode both look right.

Not blocking on 3 or the minor. 1 is a working knob that stops working on the default path; 2 is a diagnosability trade that should at least be a stated decision.