[TRTLLMINF-191][infra] Use native pytest capture for S3 logs - #16739
Conversation
|
/bot run --disable-fail-fast |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe 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
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
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR_Github #60995 [ run ] triggered by Bot. Commit: |
|
PR_Github #60995 [ run ] completed with state
|
|
PR_Github #61007 [ run ] triggered by Bot. Commit: |
|
PR_Github #61007 [ run ] completed with state
|
|
PR_Github #61180 [ run ] triggered by Bot. Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (7)
jenkins/L0_Test.groovyjenkins/scripts/slurm_run.shtests/integration/defs/test_unittests.pytests/test_common/s3_output.pytests/test_common/s3_output_hooks.pytests/unittest/test_s3_output.pytests/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
|
PR_Github #61248 [ run ] triggered by Bot. Commit: |
|
PR_Github #61180 [ run ] completed with state |
|
PR_Github #61248 [ run ] completed with state
|
|
PR_Github #61338 [ run ] triggered by Bot. Commit: |
|
PR_Github #61338 [ run ] completed with state
|
|
PR_Github #61449 [ run ] triggered by Bot. Commit: |
|
PR_Github #61449 [ run ] completed with state
|
|
PR_Github #61495 [ run ] triggered by Bot. Commit: |
|
PR_Github #61495 [ run ] completed with state
|
|
PR_Github #61540 [ run ] triggered by Bot. Commit: |
|
PR_Github #62807 [ run ] completed with state
|
|
Reviewed the whole change. Moving off the bespoke session-capture machinery onto native pytest capture is the right call, and the 1.
testFilter[(DETAILED_LOG)] ? "-s" : "",and appends On 2. Output from a test that never finishes is now lost rather than already in the console log. The plugin only observes output at
Worth saying explicitly in the description whether that trade was intended. 3.
Minor: removing the The 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. |
|
Addressed the actionable points in
One clarification on the before/after behavior: S3-enabled
|
|
PR_Github #64295 [ run ] triggered by Bot. Commit: |
|
PR_Github #64295 [ run ] completed with state
|
|
PR_Github #64461 [ run ] triggered by Bot. Commit: |
|
PR_Github #64461 [ run ] completed with state
|
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
The core rewrite is sound and the unit coverage (phase merging, cumulative-section dedup, reruns, deferred backpressure, drain ownership) is convincing. The description accurately states the hard-kill tradeoff, and dropping test_s3_stdout_echo_requires_explicit_opt_in is consistent with the deleted groovy lines.
My concerns are concentrated on the recovery path — the drain exists for abruptly terminated pytest runs, but the wiring leaves it uncovered in the most likely termination scenarios:
- Direct CI: the drain runs after the rerun logic, which can rethrow and skip it (inline comment).
- Sbatch: the only drain is per-rank in
slurm_run.sh, which never runs on a walltime kill or stage-level scancel. The Slurm cleanup (rm -rf ${jobWorkspace}, L0_Test.groovy:742) doesn't know about the sibling spool dir, so orphaned.s3-spool-${jobUID}directories accumulate unboundedly in/home/svc_tensorrt/bloom/scripts/on every cluster. Please add spool-root removal tocleanupCommands(the path is derivable fromjobWorkspace), or drain from the login node before cleanup.
One placement decision worth documenting: the spool root is a sibling of --output-dir, so leftover spool files are also absent from results-*.tar.gz. Combined with the documented hard-kill limitation, a killed test's completed-phase output can now vanish with no artifact fallback at all if the drain also fails. If keeping the spool out of the results tarball is intentional, a sentence in the module docstring would save the next reader the archaeology.
aa8c55d to
09e736b
Compare
|
PR_Github #64994 [ run ] triggered by Bot. Commit: |
|
PR_Github #64994 [ run ] completed with state
|
|
PR_Github #65051 [ run ] triggered by Bot. Commit: |
|
PR_Github #65051 [ run ] completed with state
|
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
All six earlier comments are addressed in 09e736b — I verified each against the current code (try/finally drain covering the rerun/isolated crash paths, sibling spool removal in cleanUpSlurmResources, warn-and-drop for late sections, the PID-reuse comment, the guarded nested drain, and the _write_spool_file removal) and ran tests/unittest/test_s3_output.py locally: 18 passed. Two small residual notes inline; neither blocks merge.
One request for a follow-up (fine to leave out of this PR): please look into timestamping the captured output so the separate stdout / stderr / logging streams can be re-interleaved. This PR removes the timestamped capture mode, which was the only thing that added per-line wall-clock prefixes, and the default log_format in the pytest.ini files has no %(asctime)s — so all three uploaded files are now untimestamped. Details and a concrete example in the inline comment on s3_output.py.
Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>
|
PR_Github #65192 [ run ] triggered by Bot. Commit: |
|
PR_Github #65192 [ run ] completed with state
|
|
PR_Github #65237 [ run ] triggered by Bot. Commit: |
|
PR_Github #65237 [ run ] completed with state
|
|
/bot skip --comment "multi gpu test unnecessary" |
|
PR_Github #65259 [ skip ] triggered by Bot. Commit: |
|
PR_Github #65259 [ skip ] completed with state |
7 participants
Description
The existing S3 plugin maintains its own session-level and per-test FD capture
machinery alongside pytest capture. This duplicates pytest lifecycle handling,
adds per-test capture overhead, and makes report sections and upload state harder
to keep consistent.
This PR switches S3 test logs to pytest's native
--capture=fdoutput andtransforms the resulting report sections:
one S3 URL per stream and attempt.
recoverable spool.
own ordering.
DETAILED_LOGkeeps its existing live-console purpose. When it is enabled, theJenkins command retains
-sand deliberately omits--capture=fdand theper-test S3 upload arguments; native FD capture and live console output cannot
both own the same descriptors.
Abrupt termination limitation
The plugin only receives native captured output after pytest creates a report
for a phase. If pytest is terminated with
os._exit, SIGKILL, or a stage-levelkill while a phase is still running, that phase produces no report and its
in-flight output cannot be transformed or recovered by
--drain-spool.This differs from the implementation currently on
main:-sthere does notsend S3-enabled test output directly to Jenkins because the early
SessionCaptureredirects FD 1/2 into named.s3-spoolfiles. A hard-killedtest is therefore also absent from the live console, but its raw session spool
can remain in
results-*.tar.gz. With native pytest capture, the underlyingtemporary capture file may disappear with the pytest process, so that artifact
fallback is no longer guaranteed. This is an explicit tradeoff of removing the
bespoke capture layer; live hard-kill diagnostics require a separate FD-level
tee design. Pytest's
tee-sysmode is not sufficient because it does not covernative FD writes, MPI workers, or subprocess output.
The previous
enableS3EchoStdoutparameter is also removed. It defaulted todisabled and depended on the legacy capture implementations removed here.
Test Coverage
tests/unittest/test_s3_output.pycovers inline output, stream merging,duplicate reports, reruns, failed-output tails, deferred uploads, and spool
recovery.
tests/integration/defs/test_unittests.pyexercises nested pytest capture andpending-upload draining.
pre-commit run --files jenkins/L0_Test.groovyPR Checklist