[TRTLLM-12152][infra] Init Rule Based Change Based Test Selection - #13382
Merged
Conversation
crazydemo
changed the title
Collaborator
Author
|
/bot run |
Collaborator
Author
|
/bot run |
Contributor
📝 WalkthroughWalkthroughThis PR introduces Change-Based Testing Selection (CBTS), a pre-merge CI tool that selectively filters Jenkins pipeline stages and tests based on PR changed files. CBTS integrates into the Groovy pipeline, executes a Python-based rules engine, and returns a decision that skips stages and tests outside the affected scope. Changes
Sequence Diagram(s)sequenceDiagram
participant PR as PR Metadata
participant Groovy as Groovy Pipeline<br/>(L0_MergeRequest)
participant CBTS as CBTS Python<br/>main.py
participant Rules as Rules Engine<br/>(WaivesRule)
participant YAMLIdx as YAMLIndex<br/>(blocks.py)
participant Decision as Pipeline Decision<br/>(launchStages)
PR->>Groovy: PR changed_files, diffs
Groovy->>CBTS: invoke with changed_files +<br/>diffs for needs_diff_for entries
CBTS->>YAMLIdx: load() test-db YAMLs<br/>and Groovy stages
YAMLIdx-->>CBTS: Stage & Block index
CBTS->>Rules: apply(PRInputs)
Rules->>YAMLIdx: blocks_containing_test()<br/>for each changed test_id
YAMLIdx-->>Rules: matching Block list
Rules->>YAMLIdx: block_matches_stage()<br/>for each Block vs Stage
YAMLIdx-->>Rules: match boolean
Rules-->>CBTS: RuleResult with<br/>affected_stages, affected_cpu_arch
CBTS-->>Groovy: SelectionResult JSON<br/>{scope, affected_cpu_arch, affected_stages}
Groovy->>Decision: consume cbts_result
Decision->>Decision: skip x86_64-Linux if<br/>"x86" not in affected_cpu_arch
Decision->>Decision: skip SBSA-Linux if<br/>"sbsa" not in affected_cpu_arch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
tests/integration/test_lists/waives.txt (1)
158-158: Use the shortnvbugsURL on touched entries.Line 158 already edits this waiver, so please normalize the bug link to the short
https://nvbugs/5580781form for consistency with the rest of this file.Based on learnings, in
tests/integration/test_lists/waives.txtprefer the short nvbug URL formathttps://nvbugs/XXXXover the full format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_lists/waives.txt` at line 158, Normalize the nvbugs link in the waiver entry for unittest/llmapi/test_memory_profiling.py::test_profile_kvcache SKIP by replacing the current/full bug URL with the short form https://nvbugs/5580781 so the waiver line uses the consistent short nvbugs URL format used elsewhere in the file.jenkins/scripts/cbts/README.md (1)
37-46: Add a language tag to the file-tree fence.
```textis enough here to satisfyMD040and keep docs lint quiet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jenkins/scripts/cbts/README.md` around lines 37 - 46, The fenced file-tree in jenkins/scripts/cbts/README.md should use a language tag to satisfy MD040; update the opening fence from ``` to ```text for the block that lists main.py, blocks.py, and the rules/* files (README.md, base.py, waives_rule.py) so the markdown linter stops flagging the code fence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jenkins/L0_MergeRequest.groovy`:
- Around line 1162-1167: The CBTS short-circuit currently returns early based
solely on cbts.scope and cbts.affected_cpu_arch, which can skip user-requested
manual reruns; update the conditional used in the CBTS check (the block
referencing testFilter[(CBTS_RESULT)] and variable cbts) to only perform the
early return when this is NOT a manual rerun by also verifying that no manual
rerun parameters are present (e.g., require params.stage_list and
params.gpu_type to both be absent before returning), and apply the same guard to
the other identical CBTS check elsewhere in the file (the second cbts
short-circuit).
- Around line 727-769: The catch-all around the CBTS flow currently swallows
InterruptedException and can convert an abort/timeout into a full run; update
the try/catch so interrupted aborts propagate: in the catch surrounding the
block that calls
_cbtsMatchesAnyPattern/getMergeRequestOneFileChanges/_cbtsParseSelectionResult,
detect if the caught exception is an InterruptedException (or
Thread.currentThread().isInterrupted()) and rethrow it (or re-interrupt the
thread) before handling other exceptions (which you can log and return null as
now).
In `@jenkins/L0_Test.groovy`:
- Around line 3854-3862: The CBTS block currently resets the candidate stages
from parallelJobs, which can re-enable stages previously removed by earlier
filters; instead, compute the affectedSet from
testFilter[CBTS_RESULT].affected_stages and intersect it with the existing
parallelJobsFiltered (falling back to parallelJobs if parallelJobsFiltered is
null) so that CBTS only narrows the already-selected stages; update the logic
around cbts, parallelJobsFiltered and parallelJobs to perform a set intersection
rather than assigning from parallelJobs.
In `@jenkins/scripts/cbts/blocks.py`:
- Around line 81-86: The mapping _test_to_blocks currently keys only the raw
YAML test string, so normalize and index both forms: for each test in tests (and
the block variable), compute the shared normalized id used by
waives_rule.parse_waives_diff (e.g. strip trailing SKIP/TIMEOUT annotations the
same way parse_waives_diff does) and call
self._test_to_blocks.setdefault(raw_test, []).append(block) as well as
self._test_to_blocks.setdefault(normalized_test, []).append(block); ensure the
normalization logic exactly mirrors waives_rule.parse_waives_diff to keep
lookups consistent.
- Around line 218-223: The branch handling map openings leaves current_arch
unchanged when _MAP_OPEN_RE matches but
_classify_map_var(open_match.group("var")) returns None; update that branch to
reset current_arch to None so unfamiliar Groovy maps don't preserve a previous
arch and the code will fall back to the stage-name heuristic (modify the block
that uses open_match, _classify_map_var, and current_arch to set current_arch =
None when detected is None).
In `@jenkins/scripts/cbts/README.md`:
- Around line 31-33: The README documents the "no decision" scope as scope: none
but the implementation emits JSON null for SelectionResult.scope; update the
README text to use null consistently (e.g., refer to scope: null and scope ==
null) wherever it currently says none (including the sections describing
waive-only behavior and the fallback contract) so docs match the
SelectionResult.scope JSON contract.
In `@jenkins/scripts/cbts/rules/base.py`:
- Around line 54-58: Change the class-level mutable default and the one-line
abstractmethod: replace needs_diff_for: list[str] = [] with an immutable default
(e.g., needs_diff_for: tuple[str, ...] = ()) or initialize it per-instance in
__init__ to avoid shared mutable state, and expand the abstractmethod
declaration for apply into a multi-line form (def apply(self, pr: PRInputs) ->
Optional[RuleResult]:\n ...) or raise NotImplementedError so it no longer
violates E704; update references to needs_diff_for and the apply method
accordingly.
In `@jenkins/scripts/cbts/rules/waives_rule.py`:
- Around line 103-138: Detect when affected_stage_names (computed from
yaml_index.blocks_containing_test and block_matches_stage) is empty and avoid
returning scope="waiveonly" in that case; instead compute a scope_value =
"waiveonly" if affected_stage_names is non-empty else "" (or the default
non-narrowing value) and pass scope_value into the RuleResult constructor along
with the existing handled_files, tests, affected_stages and reason so
typoed/stale unmapped waive ids do not cause Groovy consumers to narrow to zero
stages.
In `@tests/integration/test_lists/waives.txt`:
- Line 418: The waive entry "unittest/utils/test_util.py SKIP (CBTS v0
validation, revert before merge)" in tests/integration/test_lists/waives.txt is
temporary and must not remain; remove this line (or replace it with a
conditional/annotated TODO that is explicitly reverted) so the test is no longer
permanently skipped, and ensure the temporary CBTS v0 validation SKIP note is
documented and removed before merge; look for the exact string
"unittest/utils/test_util.py SKIP (CBTS v0 validation, revert before merge)" to
locate and update the waiver.
---
Nitpick comments:
In `@jenkins/scripts/cbts/README.md`:
- Around line 37-46: The fenced file-tree in jenkins/scripts/cbts/README.md
should use a language tag to satisfy MD040; update the opening fence from ``` to
```text for the block that lists main.py, blocks.py, and the rules/* files
(README.md, base.py, waives_rule.py) so the markdown linter stops flagging the
code fence.
In `@tests/integration/test_lists/waives.txt`:
- Line 158: Normalize the nvbugs link in the waiver entry for
unittest/llmapi/test_memory_profiling.py::test_profile_kvcache SKIP by replacing
the current/full bug URL with the short form https://nvbugs/5580781 so the
waiver line uses the consistent short nvbugs URL format used elsewhere in the
file.
🪄 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: Pro Plus
Run ID: e0a9a164-7f87-4758-9db1-bb031bf94c28
📒 Files selected for processing (9)
jenkins/L0_MergeRequest.groovyjenkins/L0_Test.groovyjenkins/scripts/cbts/README.mdjenkins/scripts/cbts/blocks.pyjenkins/scripts/cbts/main.pyjenkins/scripts/cbts/rules/README.mdjenkins/scripts/cbts/rules/base.pyjenkins/scripts/cbts/rules/waives_rule.pytests/integration/test_lists/waives.txt
Collaborator
|
PR_Github #45197 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #45198 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #45197 [ run ] completed with state |
Collaborator
|
PR_Github #45198 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #45309 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #45309 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #45346 [ run ] triggered by Bot. Commit: |
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #45371 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #45371 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #45382 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #45382 [ run ] completed with state |
… cases
Without this change, when CBTS narrows an affected block to a small
number of cases (e.g. 1), the stage's default pytest-split splits
(typically 3) still allocate that many machines: only one shard runs
the case and the other two start up, download wheel for ~10 min, then
exit pytest=5 with 'no tests collected'. Wasteful and noisy.
Add a per-stage narrowed-count heuristic with a hard-coded threshold of
20: when the count is below 20, collapse the stage to splits=1 (only
group 1 runs everything; groups 2..N skip without allocating machines).
At or above 20, the default splits stand and pytest-split parallelizes
normally.
Implementation:
- blocks.py: new compute_stage_test_counts() helper that mirrors
write_filtered_test_db's keep filter to count surviving entries per
affected stage (sum across blocks the stage's mako matches).
- main.py: SelectionResult gains affected_stage_test_counts; populated
after write_filtered_test_db; serialized into the stdout JSON.
- L0_MergeRequest.groovy:_cbtsParseSelectionResult: read the new field.
- L0_Test.groovy: _cbtsMaybeCollapseSplits() helper consults the count
and the 20-case threshold; called at the entry of runLLMTestlistOnSlurm
and runLLMTestlistOnPlatform. Returns either skip=true (caller does
early return; no machine allocated) or new splits/splitId values
(caller overwrites locals before existing dispatch).
Verified locally on the 3-waive testing PR: every affected stage has
count in {1, 5}, all collapse to splits=1; only group 1 of each stage
will run, no machines wasted on empty groups.
Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
…rtyException
The previous commit declared the threshold via top-level
`def _CBTS_SPLIT_COLLAPSE_THRESHOLD = 20` and referenced it inside
the helper method. Groovy script-level `def` does not enter the
binding visible to method bodies (same trap that bit pwd() / pipeline
earlier), so runtime threw
groovy.lang.MissingPropertyException: No such property:
_CBTS_SPLIT_COLLAPSE_THRESHOLD for class: WorkflowScript
Inline the literal 20 in the three reference sites and drop the
top-level def. The value is only used by this single helper, so
inlining is the simplest fix and avoids reintroducing the binding
issue.
Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
…d by split-collapse The split-collapse heuristic (commit 2e0fd25 / ee7cdd9) makes empty shards impossible in the CBTS path — count<20 collapses to splits=1 (single group runs all narrowed cases), and count>=20 keeps default splits with at least 7 cases per group. Either way, pytest never sees 'no tests collected' (exit 5) and no shard ends up empty. Three previously-added safety nets are therefore dead code: - slurm_run.sh: 'trap - ERR' workaround and CBTS exit-5 if-block. Both were added to tolerate empty SLURM shards. Restore the file to its state before the CBTS work touched it (the underlying 'set -E + ERR trap + set +e' bug is a pre-existing latent issue in this repo; scope-creep fixing it here is unnecessary now). - L0_Test.groovy:runLLMTestlistWithSbatch: drop the 'def cbtsActive' Groovy local and the corresponding 'export cbtsActive=$cbtsActive' in the slurm_launch.sh heredoc. They only fed the slurm_run.sh if-block above. - L0_Test.groovy:runLLMTestlistOnPlatformImpl 'noRegularTests && noIsolateTests' cbtsActive branch (commit 9820ed6). Restored to the original sanity-check 'error "No tests were executed..."' because the K8s path also can no longer hit the empty-shard condition, and a real misconfiguration outside CBTS still deserves a hard error. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
The README had drifted from the code over the past week. Bring it back in sync with eight gaps: - Consumption layers table now includes Layer 2.5 (split-collapse) and notes that Perf stages are excluded from Layer 2 narrowing. - Layer 3 row records that unaffected blocks are now dropped from the narrowed YAML (commit 2d6d9bc) — the prior 'each block's tests array filtered' wording understated this. - block_filters schema in 'Adding a new rule' switches from set[prefix] to dict[prefix, set[waive_id]] (commit cbe0c1f) so rule authors emit data the -k keyword guard can reuse. - New section 'Cross-job seed for stage agents' documents the cbts_input_json piggyback through testFilter, renderTestDB's Utils.createTempLocation + main.py rerun, and the 256 KB cap. - New section 'Split-collapse heuristic (Layer 2.5)' explains the threshold-20 collapse, the splitId == 1 / > 1 dispatch, and where the count comes from (compute_stage_test_counts). - Decision JSON example gains affected_stage_test_counts and a bullet describing it. - Lookup-algorithm section spells out that the -k keyword guard runs twice — once at lookup, again at write — with an example. - Fallback paths picks up two new bullets: 256 KB cap exceeded; and narrowed YAML missing/empty on the stage agent. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Reverts the `false &&` short-circuit added during CBTS testing to unblock CI under SLURM OOM (commit df6c083). The Debug build smoke stage is the gate that keeps Debug compilation from rotting; restoring it is required before merge. Stage keeps its original opt-out via `reuseArtifactPath`: when a PR reuses a prior build's artifacts, the Debug smoke stage still skips — no behavior change there. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Reverts commit df790e1's two test-only changes now that the end-to-end CBTS Layer 3 work has been validated: - waives.txt: drop the 3 SKIP entries that were added to exercise the param / function-only / -k keyword variants of Layer 3 narrowing. - waives_rule.py: WaivesRule.apply restores handled_files={WAIVES_FILE} in all three RuleResult sites (instead of claiming every changed file). With this restored, CBTS only fires on PRs whose ONLY change is waives.txt — the original v0 contract — instead of the testing hack that made it fire on this PR's CBTS-infra commits too. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Default off (CBTS stays on). When set, getCbtsResult returns null immediately so the existing filter chain runs the full baseline — useful when CBTS mis-selects or for sanity full runs. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Bot service argparser is maintained outside this repo, so an unregistered flag would be rejected by /bot. Drop the half-wired Jenkins side until the bot can recognize it. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Layer 2 in L0_Test.groovy was asymmetric: it filtered to Post-Merge stages when --post-merge was set, but the default /bot run case had no symmetric filter. A CBTS narrow that included Post-Merge stages would therefore run them on /bot run too -- running CI the user did not ask for, and on stages whose mako conditions don't hold pre-merge. Push the trigger-mode filter from Groovy into Python: - rules/base.py: PRInputs gains an optional post_merge bool. - L0_MergeRequest.groovy: getCbtsResult passes IS_POST_MERGE through cbts_input.json so Python knows the trigger mode. - main.py: after Selector.run + Layer 3 setup, filter affected_stages by Post-Merge stage-name presence vs pr.post_merge. Recompute affected_cpu_arch and affected_stage_test_counts against the filtered set. Stderr diagnostic gains a [trigger mode: X] header and lists stages dropped by the filter. - L0_Test.groovy Layer 2: simplified to a single mode-agnostic block. Always assigns parallelJobsFiltered from the CBTS subset (already pre-filtered by Python). Empty subset -> empty parallelJobsFiltered = no-op (does NOT fall through to baseline), symmetrically for both /bot run and /bot run --post-merge. Behavior matrix (CBTS narrow contents shown; result is what runs): Trigger Narrow Old New ---------- ---------------- -------------- -------------- /bot run all pre-merge pre-merge same /bot run mixed pre + post runs both (bug) runs only pre /bot run all post-merge runs post (bug) no-op --post-merge all post-merge post-merge same --post-merge mixed pre + post runs only post same --post-merge all pre-merge no-op same scope=null n/a baseline baseline Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
…ackageSanityCheck Earlier CBTS L1 (arch-track skip in L0_MergeRequest.groovy) collapsed the entire x86 / SBSA track — including Build — when no stage on that arch survived Python's trigger-mode filter. That overreached: a post-merge-only waive on /bot run produced a wheel-less no-op, and the runtime-named *-PackageSanityCheck-* stages (which the CBTS Python parser cannot see) were silently dropped any time CBTS narrowed. Tighten the scope: CBTS now narrows test cases only. - L0_MergeRequest.groovy: drop the CBTS arch-track short-circuit on both x86_64-Linux and SBSA-Linux. Build always runs so a wheel exists for sanity checks and post-merge consumers. - L0_Test.groovy Layer 2: keep the existing affected-stage filter, but force-include `*-PackageSanityCheck-*` (Perf still excluded). Sanity is a wheel/image gate that should always run after Build. - main.py: SelectionResult gains `trigger_mode_mismatch: bool`, set true when rules resolved stages but the trigger-mode filter dropped them all. Surfaced through cbts JSON and parsed in _cbtsParseSelectionResult so Layer 2 can log the "build + sanity only, no test cases" path explicitly instead of inferring it from `affected_stages.empty`. - README + comments: drop the Layer 1 row, document the Build-always invariant and the trigger-mode-mismatch fallback (analogous to `/bot run --stage-list ""`). New behavior matrix: Trigger Narrow Result ---------- ---------------- ------------------------------------ /bot run all pre-merge Build + affected pre + Sanity /bot run mixed pre + post Build + affected pre + Sanity /bot run all post-merge Build + Sanity (trigger_mode_mismatch) --post-merge all post-merge Build + affected post + Sanity --post-merge mixed pre + post Build + affected post + Sanity --post-merge all pre-merge Build + Sanity (trigger_mode_mismatch) scope=null n/a baseline (CBTS not consulted) Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
… tests, trigger_mode_mismatch)
Audit of the cbts/main.py JSON contract found three fields with no
load-bearing consumer after the previous "scope to test cases only"
refactor:
- `affected_cpu_arch`: was load-bearing only for the L1 arch-track skip
in L0_MergeRequest.groovy, which got removed when CBTS stopped
affecting Build. The remaining single echo "archs=..." was diagnostic
fluff.
- `tests`: only ever consumed as `result.affected_tests.size()` in one
echo line — never used for any decision. RuleResult.tests was
symmetric dead weight.
- `trigger_mode_mismatch`: fully derivable from `affected_stages.empty`
on the Groovy side (the Selector safety net guarantees pre-filter
affected_stages is non-empty whenever scope!=null, so post-filter
emptiness unambiguously means trigger-mode mismatch).
Drop all three. JSON keys go from 8 to 5; SelectionResult dataclass
sheds two fields; RuleResult sheds one; the WaivesRule constructors
drop the now-unused `tests=` argument; Selector.run skips the
aggregation/derivation; main() skips the post-filter recompute; stderr
diagnostic loses two now-trivial blocks; the L0_MergeRequest summary
echo collapses to `scope=..., stages=N`; Layer 2 in L0_Test reads
`affectedSet.isEmpty()` directly. README example JSON and rule-author
guide updated accordingly.
scope and reasons stay — they live at different abstraction levels
(coarse machine label + None-as-baseline gate vs free-form why-string,
especially for scope=null defer messages) and are not redundant.
Final JSON contract (5 keys):
{
"scope": "waiveonly" | null,
"affected_stages": [...],
"reasons": [...],
"test_db_dir_override": "cbts_test_db" | null,
"affected_stage_test_counts": {...}
}
Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Drop rationale, history, and thinking-process commentary from the CBTS files. Keep one-line descriptions of what the current code does; move deeper context to commit messages and the README's relevant section. No behavior change. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Two TESTING-only changes to exercise CBTS Layer 2 / Layer 3 in CI on this infra PR: 1. jenkins/scripts/cbts/main.py — comment out the unhandled-files defer in Selector.run so CBTS doesn't bail on this PR's jenkins/ and cbts/ infra changes. 2. waives.txt — change the placeholder bug id on unittest/llmapi/test_memory_profiling.py::test_profile_kvcache to 9999000 so the diff is parseable by parse_waives_diff. That waive maps to one cheap pre-merge stage (A100X-PyTorch-1). Expected behavior on /bot run: - Now we will run stages: A100X-PyTorch-1 + *-PackageSanityCheck-* - A100X-PyTorch-1 renders the narrowed test list (count=1) - Layer 2.5 collapses pytest-split to splits=1 Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
…anity yaml
For scope=waiveonly PRs, PackageSanityCheck (3-4 GPU pods, ~30 GPU·min)
was running unconditionally even though waives.txt is test-infra and
the wheel is byte-identical to main. The exception is when a waive
targets a test that itself lives in l0_sanity_check.yml — sanity
needs to run there to verify the SKIP behavior takes effect (10 of
18 sanity tests are cross-listed in l0_l40s/l0_gb203/l0_h100, so this
case is the majority for sanity-test waives).
Express the policy per-rule, not in Groovy:
- RuleResult.sanity_relevant (default True = safe): rules opt out
when their handled changes have nothing the wheel-sanity check
would verify.
- WaivesRule.apply sets sanity_relevant=any(b.yaml_stem ==
"l0_sanity_check" for b in affected_blocks). Most waives don't
match sanity blocks → False → sanity skipped.
- Selector.run aggregates sanity_required = any(r.sanity_relevant)
across fired rules.
- JSON gains sanity_required: bool. _cbtsParseSelectionResult parses
with explicit null-check so `false` survives Groovy coercion.
- Layer 2 in L0_Test.groovy gates the PackageSanityCheck force-keep
on cbts.sanity_required; trigger mode is irrelevant.
Trigger mode (--post-merge) is orthogonal — sanity stages have no
"Post-Merge" in their name and aren't filtered by the trigger-mode
logic. sanity_required is the sole gate.
Behavior matrix (CBTS path; baseline unchanged):
scope=waiveonly, waive on non-sanity test → sanity SKIPPED (saves
~30 GPU·min)
scope=waiveonly, waive on sanity-only test → safety net fires
(affected_stages={}), scope=None, baseline runs sanity
scope=waiveonly, waive cross-listed → sanity_required=True, sanity
runs with narrowed list
scope=waiveonly, trigger-mode mismatch + sanity not required →
true no-op
scope=null → CBTS not consulted, baseline behavior
Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
…equired Make the PerfSanity exclusion explicit per-rule rather than a blanket regex drop in Layer 2. Mirrors the sanity_required design. - RuleResult.perfsanity_relevant (default True = safe). - WaivesRule sets it False (waives.txt doesn't affect perf benchmarks). - Selector aggregates any() → SelectionResult.perfsanity_required. - JSON gains perfsanity_required: bool. _cbtsParseSelectionResult parses with explicit null-check. - Layer 2 regex changes from /Perf/ to /-Perf-/ so PerfSanity is no longer auto-dropped; force-keep PerfSanity when perfsanity_required. Behavior for waiveonly is unchanged: perfsanity_required=False, so PerfSanity stays excluded under both /bot run and /bot run --post-merge. The contract is now ready for v1+ rules (e.g., CoreCodeRule) that need perf coverage to opt in by leaving perfsanity_relevant at default True. Trigger-mode gating for force-kept *-PerfSanity-Post-Merge-* under plain /bot run is deferred to the first v1 rule that sets the flag True; not needed today since waiveonly always sets it False. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Restore the production state: - jenkins/scripts/cbts/main.py: re-enable the unhandled-files defer in Selector.run that the TESTING commit had commented out. - tests/integration/test_lists/waives.txt: drop the two TESTING-only waives marked with placeholder bug IDs #9999001 and #9999002. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
The generic catch (Exception e) in getCbtsResult swallowed Jenkins abort signals, preventing the pipeline from terminating. Catch InterruptedException first and rethrow, matching the pattern used elsewhere in this file. Other failures still fall back to a full run. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
crazydemo
enabled auto-merge (squash)
May 8, 2026 07:16
Collaborator
Author
|
/bot skip --comment "only add a throw" |
Collaborator
|
PR_Github #47344 [ skip ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #47344 [ skip ] completed with state |
yufeiwu-nv
pushed a commit
to yufeiwu-nv/TensorRT-LLM
that referenced
this pull request
May 19, 2026
…IDIA#13382) Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
1 task
1 task
This was referenced Jul 24, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
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)
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.