[None][fix] Revert CBTS-fanout #15614+#15592 on 2809 HEAD to bisect-confirm 2810 perf-sanity aborts - #15723
Conversation
📝 WalkthroughWalkthroughThe CBTS pipeline removes duration-based split-resize and ChangesCBTS Split-Collapse Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
jenkins/scripts/cbts/blocks.py (1)
618-631: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the keep-filter logic with
write_filtered_test_db.This duplicates the exact filtering/fallback contract used later when writing the narrowed YAML. Since this count drives Groovy split-collapse, future drift could make Jenkins collapse using counts that no longer match the emitted test-db.
♻️ Possible helper extraction
+def _filtered_tests_for_block( + tests: list[str], + prefix_to_waives: dict[str, set[str]], +) -> list[str]: + kept: list[str] = [] + for t in tests: + target = _entry_target(t) + matched_waives: set[str] = set() + for prefix, waives in prefix_to_waives.items(): + if _target_in_filter_subtree(target, prefix): + matched_waives |= waives + if matched_waives and any(_entry_applies_to_waive(t, w) for w in matched_waives): + kept.append(t) + return kept + ... - kept: list[str] = [] - for t in block.tests: - target = _entry_target(t) - matched_waives: set[str] = set() - for prefix, waives in prefix_to_waives.items(): - if _target_in_filter_subtree(target, prefix): - matched_waives |= waives - if not matched_waives: - continue - if any(_entry_applies_to_waive(t, w) for w in matched_waives): - kept.append(t) + kept = _filtered_tests_for_block(block.tests, prefix_to_waives)🤖 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/blocks.py` around lines 618 - 631, The keep-filter logic in this block duplicates the same matching and safety-fallback behavior used by write_filtered_test_db, so refactor both to use a shared helper instead of maintaining two copies. Extract the test-selection and empty-result fallback into a common function used by the count calculation here and by write_filtered_test_db, keeping the behavior for _entry_target, _target_in_filter_subtree, and _entry_applies_to_waive identical in both call sites.
🤖 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_Test.groovy`:
- Around line 1741-1746: The split-collapse logic in _cbtsMaybeCollapseSplits is
happening too late inside the runner, after launchTestJobs has already scheduled
work and runKubernetesPodWithInfraRetry has allocated the pod. Move the CBTS
collapse into the parallelJobs construction/filtering path so splitId/splits are
adjusted before any Kubernetes pod launch, and remove or minimize the in-runner
skip-only handling so collapsed splits never get scheduled in the first place.
---
Nitpick comments:
In `@jenkins/scripts/cbts/blocks.py`:
- Around line 618-631: The keep-filter logic in this block duplicates the same
matching and safety-fallback behavior used by write_filtered_test_db, so
refactor both to use a shared helper instead of maintaining two copies. Extract
the test-selection and empty-result fallback into a common function used by the
count calculation here and by write_filtered_test_db, keeping the behavior for
_entry_target, _target_in_filter_subtree, and _entry_applies_to_waive identical
in both call sites.
🪄 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: 6629de99-e502-40ee-938f-25b192abff74
📒 Files selected for processing (7)
jenkins/L0_MergeRequest.groovyjenkins/L0_Test.groovyjenkins/scripts/cbts/README.mdjenkins/scripts/cbts/blocks.pyjenkins/scripts/cbts/main.pyjenkins/scripts/cbts/tools/dryrun.pyjenkins/scripts/cbts/tools/report_cbts_decision.py
💤 Files with no reviewable changes (1)
- jenkins/scripts/cbts/tools/dryrun.py
| def collapse = _cbtsMaybeCollapseSplits(stageName, splitId, splits) | ||
| if (collapse.skip) { | ||
| return | ||
| } | ||
| splits = collapse.splits | ||
| splitId = collapse.splitId |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Move split-collapse before pod launch.
By Line 5255, launchTestJobs has already handed every split to runKubernetesPodWithInfraRetry, so returning here only skips the test body after the pod is up. For Kubernetes-backed stages, splitId>1 still burns a full pod/GPU allocation, which means this does not actually collapse stage fanout at the scheduling layer. Apply the CBTS collapse when building/filtering parallelJobs instead of inside the runners.
Also applies to: 3829-3834
🤖 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_Test.groovy` around lines 1741 - 1746, The split-collapse logic in
_cbtsMaybeCollapseSplits is happening too late inside the runner, after
launchTestJobs has already scheduled work and runKubernetesPodWithInfraRetry has
allocated the pod. Move the CBTS collapse into the parallelJobs
construction/filtering path so splitId/splits are adjusted before any Kubernetes
pod launch, and remove or minimize the in-runner skip-only handling so collapsed
splits never get scheduled in the first place.
09f7588 to
69311d0
Compare
chenfeiz0326
changed the title
|
/bot run --disable-fail-fast --stage-list "GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE2-GPU8-Post-Merge-2,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-3,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-5,GB200-16_GPUs-4_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE2-GPU8-GEN1-NODE2-GPU8-Post-Merge-1,GB200-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-4,GB200-24_GPUs-6_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE2-GPU8-GEN1-NODE4-GPU16-Post-Merge-1,GB200-24_GPUs-6_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE2-GPU8-GEN1-NODE4-GPU16-Post-Merge-2,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-1,GB200-4_GPUs-PyTorch-PerfSanity-Post-Merge-1,GB200-4_GPUs-PyTorch-PerfSanity-Post-Merge-2,GB200-4_GPUs-PyTorch-PerfSanity-Post-Merge-3,GB200-4_GPUs-PyTorch-PerfSanity-Post-Merge-4,GB200-4_GPUs-PyTorch-PerfSanity-Post-Merge-6,GB200-4_GPUs-PyTorch-PerfSanity-Post-Merge-7,GB200-4_GPUs-PyTorch-PerfSanity-Post-Merge-8,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU2-Post-Merge-1,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-4,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-5,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-7,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU4-Post-Merge-1,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU4-Post-Merge-2,GB200-8_GPUs-2_Nodes-PyTorch-PerfSanity-Node2-GPU8-Post-Merge-1,GB200-8_GPUs-2_Nodes-PyTorch-PerfSanity-Node2-GPU8-Post-Merge-7,GB200-8_GPUs-2_Nodes-PyTorch-PerfSanity-Node2-GPU8-Post-Merge-8,GB200-8_GPUs-2_Nodes-PyTorch-Post-Merge-1" |
|
/bot run --disable-fail-fast --stage-list "GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE2-GPU8-Post-Merge-1,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-2,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-3,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-4,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-5,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-6,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-8,GB200-16_GPUs-4_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE2-GPU8-GEN1-NODE2-GPU8-Post-Merge-1,GB200-16_GPUs-4_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE2-GPU8-GEN1-NODE2-GPU8-Post-Merge-2" |
69311d0 to
a02214a
Compare
Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
|
/bot run --disable-fail-fast --stage-list "GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE2-GPU8-Post-Merge-1,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-2,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-3,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-4,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-5,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-6,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-8,GB200-16_GPUs-4_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE2-GPU8-GEN1-NODE2-GPU8-Post-Merge-1,GB200-16_GPUs-4_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE2-GPU8-GEN1-NODE2-GPU8-Post-Merge-2" |
1 participant
This PR is a bisect-confirmation build only — do not merge.
Context
In post-merge
L0_Test-SBSA-Multi-GPUbuild 2810 (commitc25c23f7), multiple GB200 perf-sanity stages aborted with "Submit Test Result" and "Clean Up Slurm Resource" SKIPPED. Examples:GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-3GB200-8_GPUs-2_Nodes-PyTorch-PerfSanity-Node2-GPU8-Post-Merge-5The abort rate stepped up between post-merge builds 2806 (
a51931ad) and 2809 (86bb1f46). Proximate cause is a 30-secondwithSlurmSshCredentialsprobe to a randomly-drawn OCI HSG login node (login-01was intermittently slow) — when it timed out,uploadResultsSIGTERM'd the stage and thefinallyblock skipped Clean Up. The hypothesis is that the CBTS-fanout change in7cc568c4ee(PR #15592), extended by86bb1f4612(PR #15614), increased the number of parallel narrowed perf-sanity stages, multiplying simultaneous SSH-probe load on the OCI HSG head pool.What this branch does
Branch base:
86bb1f4612(2809 HEAD).Reverts (HEAD-to-old order):
86bb1f4612—[None][infra] take test durations into account to determine cbts splits num (#15614)7cc568c4ee—[None][infra] use default split when CBTS test-db download fails (#15592)After both reverts,
_cbtsMaybeCollapseSplitsreturns to the pre-7cc568c4ee"collapse to 1 shard when narrowed count < 20" behavior.Expected verification result
Trigger the 2810 aborted stage list on this PR. If the CBTS-fanout family is the culprit, the abort rate should drop back to 2806-level (i.e., the
uploadResultsSSH-probe timeouts should largely disappear).Paired with companion PR (cherry-pick
7cc568c4eeonto 2806 HEAD) for the bisect-confirmation in the opposite direction.Summary by CodeRabbit
New Features
Bug Fixes
Documentation