[TRTLLMINF-191][infra] Use native pytest capture for S3 logs - #16739
[TRTLLMINF-191][infra] Use native pytest capture for S3 logs#16739niukuo wants to merge 4 commits into
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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 #61540 [ run ] completed with state |
Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>
Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>
Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>
Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>
|
PR_Github #62381 [ run ] triggered by Bot. Commit: |
|
PR_Github #62381 [ run ] completed with state
|
|
PR_Github #62658 [ run ] triggered by Bot. Commit: |
|
PR_Github #62658 [ run ] completed with state
|
|
PR_Github #62710 [ run ] triggered by Bot. Commit: |
|
PR_Github #62710 [ run ] completed with state
|
5 participants
Dev Engineer Review
--s3-echo-stdout/ENABLE_S3_ECHO_STDOUTwiring to native pytest--capture=fd, with report-section transformation intests/test_common/s3_output.py.ENABLE_UPLOAD_TEST_RESULTSis enabled (both SLURM sbatch and non-SLURM execution paths):--capture=fd--s3-upload-path=${uploadPath}/${stageName}--s3-upload-mode=deferred-susage and legacy/conditional S3 argument wiring (including optional--s3-echo-stdoutand prior capture/session/timestamp behavior).tests/test_common/s3_output.py):syncvsdeferredupload modes with bounded concurrency (ThreadPoolExecutor).drain_pending_uploads(output_path, secret_key)to recover orphaned.s3-spoolartifacts from crashed pytest processes (per-owner coordination withfcntl.flock+ hostname/pid liveness checks).tests/test_common/s3_output_hooks.py):pytest_configurehook.jenkins/scripts/slurm_run.sh: drains per-rank spool aftereval $pytestCommandcompletes (drain failures tolerated with|| trueso diagnostics/exit-code handling still runs).jenkins/L0_Test.groovy: removedENABLE_S3_ECHO_STDOUTinitialization; standardized pytest args as above; drains deferred output after non-SLURM pytest run viapython3 .../s3_output.py --drain-spool "${WORKSPACE}/${stageName}"when upload results are enabled.tests/integration/defs/test_unittests.py: when--s3-upload-pathis set, enforces--capture=fd, adjusts the inner pytest command list to remove legacy-s, and drains pending uploads in afinallyblock usings3_output_module.drain_pending_uploads(...).UploadLogPluginconstructor signature updated to remove older echo/session capture parameters.register_plugin(config)signature updated and now enforces--s3-upload-pathtogether with native--capture=fd.Potential review focus:
--capture=fdand that plugin registration call sites match the newregister_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_inlinetest_stdout_at_threshold_is_replaced_with_urltest_inline_threshold_applies_to_combined_streamtest_logging_section_is_uploaded_even_when_smalltest_sync_upload_transforms_native_sectionstest_duplicate_capture_sections_share_one_objecttest_cumulative_capture_sections_are_uploaded_oncetest_same_nodeid_rerun_gets_distinct_test_pathtest_failed_report_keeps_only_recent_bounded_outputtest_deferred_upload_starts_before_session_finishtest_deferred_upload_reuses_cumulative_sectiontest_rerun_keeps_one_url_per_attempttest_parent_drain_retries_upload_left_by_failed_processtest_parent_drain_uses_configured_upload_workerstest_parent_drain_skips_live_pytest_processtest_register_plugin_requires_native_fd_capturetest_register_plugin_uses_report_transformertest_s3_hook_skips_xdist_controllertests/unittest/tools/test_test_to_stage_mapping.pytest_s3_stdout_echo_requires_explicit_opt_in); current file contains:test_data_availabilitytest_bidirectional_mapping_consistency(skipped)test_search_functionalitytest_cli_functionalitytest_backend_filtering_consistencytests/integration/defs/test_unittests.pytest_unittests_v2Coverage in
tests/integration/test_lists//test-db//qa/: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-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin 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.