Skip to content

[None][infra] use default split when CBTS test-db download fails - #15592

Merged
crazydemo merged 1 commit into
NVIDIA:mainfrom
crazydemo:fix-xbts
Jun 25, 2026
Merged

[None][infra] use default split when CBTS test-db download fails#15592
crazydemo merged 1 commit into
NVIDIA:mainfrom
crazydemo:fix-xbts

Conversation

@crazydemo

@crazydemo crazydemo commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • New Features
    • Improved test sharding behavior for smaller filtered test sets, helping reduce unnecessary split groups and avoid wasted scheduling.
    • Added smarter fallback handling so jobs keep the expected shard layout when narrowed test data isn’t available.

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.

@crazydemo
crazydemo requested a review from a team June 24, 2026 12:14
@crazydemo
crazydemo requested a review from a team as a code owner June 24, 2026 12:14
@crazydemo
crazydemo marked this pull request as draft June 24, 2026 12:14
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR refactors CBTS test-sharding split-collapse into two phases: a caller-side early-skip (_cbtsShouldSkipSplitGroup) before SLURM allocation, and an executor-side collapse (_cbtsEffectiveSplits) conditioned on whether renderTestDB confirms the narrowed CBTS test-db was actually loaded. renderTestDB now returns a {path, usedCbtsOverride} map, and a new CBTS_SPLIT_COLLAPSE_THRESHOLD field (default 20) controls the threshold. README documentation is updated to match.

Changes

CBTS Two-Phase Split-Collapse Refactor

Layer / File(s) Summary
renderTestDB return type and CBTS override tracking
jenkins/L0_Test.groovy
renderTestDB now tracks usedCbtsOverride to record whether the narrowed CBTS test-db directory was valid and loaded, adds a CBTS_SPLIT_COLLAPSE_THRESHOLD @Field configurable via env (default 20), and returns {path: testList, usedCbtsOverride: boolean} instead of a raw path string. CI-budget estimate labels are also branched on usedCbtsOverride.
New CBTS Layer 2.5 helpers replacing _cbtsMaybeCollapseSplits
jenkins/L0_Test.groovy
Removes _cbtsMaybeCollapseSplits and introduces three replacements: _cbtsNarrowedBelowThreshold checks the narrowed count against threshold; _cbtsShouldSkipSplitGroup provides caller-side early-return for splitId > 1 groups before allocation; _cbtsEffectiveSplits collapses to splits=1, splitId=1 on the executor only when usedCbtsOverride is true, preserving default splits otherwise to avoid collapse-then-fallback timeouts. runLLMTestlistOnSlurm is updated to call _cbtsShouldSkipSplitGroup.
Call-site integration in SLURM and platform runners
jenkins/L0_Test.groovy
runLLMTestlistWithSbatch, runLLMTestlistOnPlatformImpl, and runLLMTestlistOnPlatform unpack the new renderTestDB map result and pass renderResult.usedCbtsOverride into _cbtsEffectiveSplits, replacing all former _cbtsMaybeCollapseSplits call sites.
README two-phase collapse and fallback documentation
jenkins/scripts/cbts/README.md
Rewrites the "Split-collapse heuristic (Layer 2.5)" section to describe both phases, updates the summary table to reflect the renderTestDB-gated rule, and expands the "Fallback paths" entry to specify that a failed cbts_test_db tarball reverts the surviving stage to default splits (running only ~1/N of the source list).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • niukuo
  • yuanjingx87
  • dpitman-nvda
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR body is still just the template with placeholders and no actual description or test coverage details. Fill in the Description and Test Coverage sections with the issue, solution, and relevant tests; keep checklist items as applicable.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the main change and clearly describes the CBTS fallback behavior fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
jenkins/L0_Test.groovy (1)

2834-2837: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Do not swallow pipeline interruptions during CBTS artifact download.

The generic catch (Exception e) also catches abort/timeout interruption paths used elsewhere in this file as hard stops. Re-throw InterruptedException before falling back to source.

Proposed fix
-            } catch (Exception e) {
+            } catch (InterruptedException e) {
+                throw e
+            } catch (Exception e) {
                 echo "CBTS Layer 3: artifact download failed " +
                      "(${e.class.simpleName}: ${e.message}); falling back to source test-db"
             }
🤖 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 2834 - 2837, The CBTS artifact download
fallback in the surrounding try/catch is swallowing pipeline interruption
signals because the generic catch on the artifact download path also captures
hard-stop cases; update the exception handling in the CBTS Layer 3 download
block to detect InterruptedException and re-throw it immediately before any
fallback logic, while keeping the existing source test-db fallback only for
non-interruption failures. Use the existing artifact download and fallback code
in L0_Test.groovy as the target, and preserve the current logging for ordinary
exceptions.
🤖 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 2828-2832: The current download guard in the Jenkins flow only
checks the override directory, so a stale cbts_test_db/ can prevent re-download
even when the expected context YAML is missing. Update the logic around the
dirExists check and the subsequent artifact extraction in this block to verify
the presence of ${testContext}.yml inside the extracted context first; if it is
missing, remove the partial directory and re-download/re-extract the artifact
instead of falling back to source.
- Around line 1842-1844: The CBTS split collapse threshold is being initialized
too early from env, but it should stay at the default value at declaration time.
Keep CBTS_SPLIT_COLLAPSE_THRESHOLD as a plain `@Field` default of 20 in
L0_Test.groovy, and move the env.CBTS_SPLIT_COLLAPSE_THRESHOLD override logic
into the Setup Environment step so the value is read after startup.

---

Outside diff comments:
In `@jenkins/L0_Test.groovy`:
- Around line 2834-2837: The CBTS artifact download fallback in the surrounding
try/catch is swallowing pipeline interruption signals because the generic catch
on the artifact download path also captures hard-stop cases; update the
exception handling in the CBTS Layer 3 download block to detect
InterruptedException and re-throw it immediately before any fallback logic,
while keeping the existing source test-db fallback only for non-interruption
failures. Use the existing artifact download and fallback code in L0_Test.groovy
as the target, and preserve the current logging for ordinary exceptions.
🪄 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: 4c35d53d-b836-4af6-ad5e-776220f792a6

📥 Commits

Reviewing files that changed from the base of the PR and between 71613f9 and 4c1c73a.

📒 Files selected for processing (2)
  • jenkins/L0_Test.groovy
  • jenkins/scripts/cbts/README.md

Comment thread jenkins/L0_Test.groovy Outdated
Comment thread jenkins/L0_Test.groovy
@crazydemo crazydemo closed this Jun 24, 2026
@crazydemo crazydemo reopened this Jun 24, 2026
@crazydemo
crazydemo force-pushed the fix-xbts branch 4 times, most recently from dd8c4cd to bb07475 Compare June 24, 2026 13:14
…t count

The cbts_test_db tarball download in renderTestDB was the only wget without
retry (a one-shot `wget -q`), so a transient Artifactory blip killed it while
every other download on the same node survived by retrying; the stage then
silently fell back to the full source test-db.

- renderTestDB: wrap the download in trtllm_utils.llmExecStepWithRetry and use
  -nv (not -q) so it retries and a failure is visible in the log.
- _cbtsMaybeCollapseSplits: replace the rigid "collapse to 1 shard when narrowed
  count < 20" with K = min(narrowed count, default splits). A narrowed stage now
  runs at most one test per shard, capped at its default N, so slow / TIMEOUT(60)
  cases can no longer pile into a single shard and overrun the SLURM walltime.
  Also guard a null CBTS result and a missing tarball (keep default splits so the
  source-test-db fallback shards normally instead of piling into one shard).

Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
@crazydemo
crazydemo marked this pull request as ready for review June 24, 2026 13:18
@crazydemo

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #55507 [ run ] triggered by Bot. Commit: a10f3be Link to invocation

@crazydemo
crazydemo enabled auto-merge (squash) June 24, 2026 14:30
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #55507 [ run ] completed with state FAILURE. Commit: a10f3be
/LLM/main/L0_MergeRequest_PR pipeline #44431 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

@crazydemo

Copy link
Copy Markdown
Collaborator Author

/bot skip --comment "the failed stages is not related to this PR"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #55661 [ skip ] triggered by Bot. Commit: a10f3be Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #55661 [ skip ] completed with state SUCCESS. Commit: a10f3be
Skipping testing for commit a10f3be

Link to invocation

chenfeiz0326 added a commit to chenfeiz0326/TensorRT-LLM that referenced this pull request Jun 29, 2026
…ils (NVIDIA#15592)"

This reverts commit 7cc568c.

Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
chenfeiz0326 pushed a commit to chenfeiz0326/TensorRT-LLM that referenced this pull request Jun 29, 2026
…DIA#15592)

Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
BrianLi23 pushed a commit to BrianLi23/TensorRT-LLM that referenced this pull request Jul 9, 2026
…DIA#15592)

Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants