[None][infra] Stop CBTS coverage writes from failing the stage results tar - #17304
Conversation
|
/bot run --stage-list "DGX_H100-PyTorch-1, DGX_H100-PyTorch-2, DGX_H100-PyTorch-3, DGX_H100-PyTorch-4, DGX_H100-PyTorch-5, DGX_H100-PyTorch-6" |
|
PR_Github #64009 [ 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: 4
🤖 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 `@jenkins/L0_MergeRequest.groovy`:
- Around line 349-350: Remove the temporary CBTS coverage enablement from the
merge-request pipeline by deleting the `testFilter[(CBTS_COVERAGE)] =
ENABLE_CBTS_COVERAGE` assignment and its debug-only comment. Preserve the
existing post-merge coverage gate behavior elsewhere in the pipeline.
In `@jenkins/L0_Test.groovy`:
- Around line 377-379: Update the archive-retention check in the command
constructing cbtsArchive to validate its members with tar -tzf, retaining the
archive only when it contains a .cbtscov.*.sqlite entry; otherwise remove it.
Replace the current size-only [ -s '${cbtsArchive}' ] check while preserving the
existing archive creation flow.
In `@jenkins/scripts/cbts/coverage_utils/README.md`:
- Line 45: Add the standard NVIDIA copyright header to the modified README,
using 2026 as the latest copyright year. Preserve the existing documentation
content and formatting after the header.
In `@jenkins/scripts/cbts/coverage_utils/sitecustomize.py`:
- Around line 114-119: Implement a shared inter-process writer-drain protocol
around _frozen() and _tracker.save() in
jenkins/scripts/cbts/coverage_utils/sitecustomize.py:114-119 so new saves are
suppressed and writers already admitted are tracked and drained before
collection. Document the suppression and active-writer drain in
jenkins/scripts/cbts/coverage_utils/README.md:45. Update
jenkins/L0_Test.groovy:223-236 to use the shared drain protocol instead of the
single empty-glob check and ignored sentinel failures, and update
jenkins/L0_Test.groovy:372-379 to use the same drain for SLURM collection
instead of the fixed two-second sleep.
🪄 Autofix
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: c2d1a52d-ba9a-43c4-9b81-012d0d7ca296
📒 Files selected for processing (4)
jenkins/L0_MergeRequest.groovyjenkins/L0_Test.groovyjenkins/scripts/cbts/coverage_utils/README.mdjenkins/scripts/cbts/coverage_utils/sitecustomize.py
|
PR_Github #64009 [ run ] completed with state |
b861948 to
fc6800c
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughCBTS coverage now uses a stop-file sentinel. Jenkins passes the sentinel path to test processes, freezes coverage before packaging, waits for output stability, and removes stale or empty artifacts. ChangesCBTS coverage lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant JenkinsPipeline
participant PytestProcess
participant CoverageUtility
participant ResultArchive
JenkinsPipeline->>PytestProcess: provide CBTS_STOP_FILE
PytestProcess->>CoverageUtility: periodic coverage save
JenkinsPipeline->>JenkinsPipeline: create stop sentinel
CoverageUtility->>CoverageUtility: stop coverage writes
JenkinsPipeline->>ResultArchive: package stabilized coverage output
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
jenkins/scripts/cbts/coverage_utils/sitecustomize.py (1)
114-119:⚠️ Potential issue | 🟠 MajorCreating
CBTS_STOP_FILEsuppresses saves that have not yet passed_frozen(), but it does not drain writers that already passed the check. The collectors can therefore archive a.cbtscovfile while it is still being written.
- jenkins/scripts/cbts/coverage_utils/sitecustomize.py#L114-L119: Add the shared inter-process lock or writer-registration protocol.
- jenkins/scripts/cbts/coverage_utils/sitecustomize.py#L141-L142: Run
_save_active()under that protocol.- jenkins/scripts/cbts/coverage_utils/sitecustomize.py#L151-L152: Run
_final_save()under that protocol.- jenkins/scripts/cbts/coverage_utils/sitecustomize.py#L234-L236: Register and drain periodic writers before stopping them.
- jenkins/scripts/cbts/coverage_utils/README.md#L45-L45: Document the active-writer drain.
- jenkins/L0_Test.groovy#L223-L236: Require successful sentinel creation and a completed local drain.
- jenkins/L0_Test.groovy#L2912-L2914: Abort or skip the result archive when the freeze does not complete.
- jenkins/L0_Test.groovy#L372-L379: Replace the fixed remote delay with the same writer-drain protocol.
🤖 Prompt for 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. In `@jenkins/scripts/cbts/coverage_utils/sitecustomize.py` around lines 114 - 119, Add a shared inter-process lock or writer-registration/drain protocol around _frozen in sitecustomize.py#L114-L119; execute _save_active and _final_save under it at lines 141-142 and 151-152, and register and drain periodic writers before stopping them at lines 234-236. Document the active-writer drain in jenkins/scripts/cbts/coverage_utils/README.md#L45. In jenkins/L0_Test.groovy#L223-L236 require successful sentinel creation and local drain completion, abort or skip archiving at lines 2912-2914 when freezing fails, and replace the fixed remote delay at lines 372-379 with the same writer-drain protocol.jenkins/scripts/cbts/coverage_utils/README.md (1)
45-45:⚠️ Potential issue | 🟡 MinorAdd the NVIDIA copyright header.
This modified README has no NVIDIA copyright header. Add the repository-standard header and use 2026 as the latest meaningful modification year.
As per coding guidelines,
**/*requires the NVIDIA copyright header on modified files and the latest meaningful modification year.🤖 Prompt for 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. In `@jenkins/scripts/cbts/coverage_utils/README.md` at line 45, Add the repository-standard NVIDIA copyright header at the beginning of README.md, using 2026 as the latest meaningful modification year. Preserve the existing README content and formatting after the header.Source: Coding guidelines
jenkins/L0_MergeRequest.groovy (1)
349-350:⚠️ Potential issue | 🟠 MajorRemove the temporary merge-request coverage override before merge.
ENABLE_CBTS_COVERAGEistrue, and this assignment does not checkenv.JOB_NAME. It propagatescbts_coverage=trueto merge-request jobs, which starts periodic SQLite writers and archive work in pre-merge stages. Restore the official post-merge gate before merging.The PR objective identifies this assignment as temporary debug enablement intended to be reverted before merge.
🤖 Prompt for 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. In `@jenkins/L0_MergeRequest.groovy` around lines 349 - 350, Remove the temporary testFilter[(CBTS_COVERAGE)] = ENABLE_CBTS_COVERAGE override and its debug comment from the merge-request pipeline configuration. Restore the official post-merge JOB_NAME-gated coverage behavior, leaving CBTS_COVERAGE enabled only for the intended post-merge jobs.
🤖 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.
Duplicate comments:
In `@jenkins/L0_MergeRequest.groovy`:
- Around line 349-350: Remove the temporary testFilter[(CBTS_COVERAGE)] =
ENABLE_CBTS_COVERAGE override and its debug comment from the merge-request
pipeline configuration. Restore the official post-merge JOB_NAME-gated coverage
behavior, leaving CBTS_COVERAGE enabled only for the intended post-merge jobs.
In `@jenkins/scripts/cbts/coverage_utils/README.md`:
- Line 45: Add the repository-standard NVIDIA copyright header at the beginning
of README.md, using 2026 as the latest meaningful modification year. Preserve
the existing README content and formatting after the header.
In `@jenkins/scripts/cbts/coverage_utils/sitecustomize.py`:
- Around line 114-119: Add a shared inter-process lock or
writer-registration/drain protocol around _frozen in sitecustomize.py#L114-L119;
execute _save_active and _final_save under it at lines 141-142 and 151-152, and
register and drain periodic writers before stopping them at lines 234-236.
Document the active-writer drain in
jenkins/scripts/cbts/coverage_utils/README.md#L45. In
jenkins/L0_Test.groovy#L223-L236 require successful sentinel creation and local
drain completion, abort or skip archiving at lines 2912-2914 when freezing
fails, and replace the fixed remote delay at lines 372-379 with the same
writer-drain protocol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bb5023c1-0be0-468c-b0b4-1377703fa8d7
📒 Files selected for processing (4)
jenkins/L0_MergeRequest.groovyjenkins/L0_Test.groovyjenkins/scripts/cbts/coverage_utils/README.mdjenkins/scripts/cbts/coverage_utils/sitecustomize.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Every instrumented process saves its .cbtscov sqlite every 5 seconds into the stage output directory, which is also what "tar -czvf results-<stage>" archives. A process that outlives pytest keeps creating files there while the tar runs, so GNU tar reports "file changed as we read it" and exits 1, failing the whole stage. Add a CBTS_STOP_FILE sentinel that sitecustomize.py checks before every periodic and final save; result collection creates it before archiving and waits for in-flight writes to land, so the directory is stable by the time the tar starts. Saves are full snapshots, so skipping the post-freeze save loses no touches. The sentinel is cleared at stage start so a reused workspace cannot suppress a later build's coverage. On the SLURM side the per-stage coverage archive is now kept when tar warns, instead of deleting a complete archive. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
fc6800c to
fe417be
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --stage-list "DGX_H100-PyTorch-1, DGX_H100-PyTorch-2, DGX_H100-PyTorch-3, DGX_H100-PyTorch-4, DGX_H100-PyTorch-5, DGX_H100-PyTorch-6" |
|
PR_Github #64052 [ run ] triggered by Bot. Commit: |
|
PR_Github #64052 [ run ] completed with state |
fe417be to
df443f8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run |
|
PR_Github #64176 [ run ] triggered by Bot. Commit: |
|
PR_Github #64176 [ run ] completed with state
|
|
/bot skip --comment "the failure is not related to this pr" |
|
PR_Github #64247 [ skip ] triggered by Bot. Commit: |
|
PR_Github #64247 [ skip ] completed with state |
3 participants
Summary
CBTS_STOP_FILEhandling to stop coverage writes before result collection.freezeCbtsCoverage(String stageDir)to stabilize coverage output before packaging.sitecustomize.pyto stop periodic and final coverage writes when the sentinel exists.Dev Engineer Review
QA Engineer Review
No test changes.
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.