[TRTLLM-12838][infra] CBTS: coverage-based test selection#16776
[TRTLLM-12838][infra] CBTS: coverage-based test selection#16776crazydemo wants to merge 2 commits into
Conversation
|
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:
WalkthroughCBTS adds optional coverage-database test narrowing, maps changed Python lines to qualified names, preserves impacted and untrusted tests, removes safe YAML entries, exposes structured diagnostics, and propagates coverage settings through Jenkins and dry-run execution. ChangesCoverage-tier CBTS selection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JenkinsPipeline
participant main_py
participant CoverageSelector
participant TouchDB
participant coverage_tier_py
participant YAMLTestDB
JenkinsPipeline->>main_py: pass coverage database path
main_py->>CoverageSelector: run initial selection
CoverageSelector-->>main_py: fallback result and selector state
main_py->>TouchDB: open coverage database
main_py->>coverage_tier_py: apply coverage tier
coverage_tier_py->>TouchDB: query impacted and skippable tests
coverage_tier_py-->>main_py: narrowed stages and diagnostics
main_py->>YAMLTestDB: write narrowed test database
Suggested reviewers: 🚥 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jenkins/L0_MergeRequest.groovy (2)
809-815: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
reasons.join('; ')will now print raw MaptoString()output.
result.reasonsused to be a list of strings;main.pyin this PR turns every entry into a structuredMap({source: ..., reason: ..., ...}). This log line still does a plain.join('; '), so the "CBTS: deferring" console message will render as GroovyMapliterals (e.g.[source:fallback, reason:unhandled_files, files:[...]]) instead of readable text.🩹 Suggested fix: format each structured reason before joining
- pipeline.echo("CBTS: deferring — Python returned scope=null. " + - "Reasons: ${result.reasons.join('; ')}") + def reasonStr = result.reasons.collect { r -> + if (r instanceof Map) { + def src = r.source ?: "?" + def rest = r.findAll { k, v -> k != "source" }.collect { k, v -> "${k}=${v}" }.join(", ") + return rest ? "[${src}] ${rest}" : "[${src}]" + } + return r.toString() + }.join('; ') + pipeline.echo("CBTS: deferring — Python returned scope=null. Reasons: ${reasonStr}")🤖 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 809 - 815, Update the scope-null handling in the pipeline flow around _cbtsParseSelectionResult and _cbtsReportDecision so result.reasons maps are converted to readable reason text before joining. Reuse the structured fields, including source and reason and any relevant details, rather than calling join directly on the maps; preserve the existing deferring message and fallback decision behavior.
954-968: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winCoverage-tier multi-GPU re-enable is broken:
_cbtsParseSelectionResultnever forwardsenable_multi_gpu/coverage_dropped_stagesto the Groovy consumer.main.py'sSelectionResult.to_json()emits these two new fields, but the Groovy-side JSON parser explicitly whitelists a fixed set of keys that doesn't include them, so they never reachtestFilter[(CBTS_RESULT)].
jenkins/L0_MergeRequest.groovy#L954-L968: addenable_multi_gpu: data.enable_multi_gpu ?: falseandcoverage_dropped_stages: data.coverage_dropped_stages ?: []to the map literal returned by_cbtsParseSelectionResult.jenkins/L0_Test.groovy#L5714-L5718: no code change needed here — this consumer is correct and will start working oncecbts.enable_multi_gpuis actually populated by the fix above; currently it always evaluates falsy, so multi-GPU stages are never re-added under coverage-tier selection even whenMULTI_GPU_FILE_CHANGEDis true.🤖 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 954 - 968, The Groovy parser _cbtsParseSelectionResult in jenkins/L0_MergeRequest.groovy:954-968 must forward enable_multi_gpu with a false default and coverage_dropped_stages with an empty-list default in its returned map. Make no code change to jenkins/L0_Test.groovy:5714-5718; its consumer is already correct and will work once the parser populates cbts.enable_multi_gpu.
🧹 Nitpick comments (2)
jenkins/scripts/cbts/coverage_selection/qualname_map.py (1)
79-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComplete the public API docstrings.
jenkins/scripts/cbts/coverage_selection/qualname_map.py#L79-L86: documentsource,lines, and theok=Falseparse-failure result using Google-styleArgsandReturns.jenkins/scripts/cbts/coverage_selection/selector.py#L108-L112: documentresidual_files,diffs, and allCoverageResultoutcomes using Google-style sections.As per coding guidelines, “Prefer docstrings for external interfaces, use Google-style docstrings, document public function arguments.”
🤖 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_selection/qualname_map.py` around lines 79 - 86, The public API docstrings are missing parameter and result documentation. In jenkins/scripts/cbts/coverage_selection/qualname_map.py lines 79-86, update qualnames_for_lines with Google-style Args for source and lines and Returns describing the qualname set and the ok=False result when parsing fails. In jenkins/scripts/cbts/coverage_selection/selector.py lines 108-112, update the affected public function’s docstring with Google-style Args for residual_files and diffs and Returns covering every possible CoverageResult outcome.Source: Coding guidelines
jenkins/scripts/cbts/coverage_tier.py (1)
175-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
must_run_reasonstally is computed but never surfaced.
_build_narrowingreturns aCounterof why each entry was kept (rule_kept/impacted/untrusted/no_data/coarse), andapply_coverage_tierstores it onCoverageTierResult.must_run_reasons(Line 239). Nothing downstream reads it —main.py's coverage-tier block only consumestier.detail,tier.affected_stages,tier.dropped,tier.removed. This diagnostic is silently discarded even though the PR explicitly aims to "emit structured selection reasons."♻️ Suggested fix: fold the tally into `detail`
detail={ "source": "coverage", "files": len(residual), "impacted": n_impacted, "untrusted": cov.n_untrusted, "removed_cases": n_removed, "dropped_stages": len(dropped), "outcome": "narrowed" if narrowed else "nothing_removable", + **({"must_run_reasons": dict(must_run_reasons)} if must_run_reasons else {}), **({"no_data_funcs": list(cov.no_data_funcs)} if cov.no_data_funcs else {}), },🤖 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_tier.py` around lines 175 - 251, Expose the must-run reason tally in the structured coverage-tier output by adding the existing must_run_reasons value returned from _build_narrowing to CoverageTierResult.detail in apply_coverage_tier. Preserve the existing must_run_reasons field and include the tally under a clear stable detail key so downstream main.py consumers receive it.
🤖 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/scripts/cbts/coverage_selection/selector.py`:
- Around line 18-32: The Ruff auto-fixes must be applied and staged in both
affected files: rerun the configured pre-commit hook for
jenkins/scripts/cbts/coverage_selection/selector.py lines 18-32 and
jenkins/scripts/cbts/tools/coverage_explain.py lines 30-43, then stage the
resulting edits so the release check passes.
In `@jenkins/scripts/cbts/coverage_tier.py`:
- Around line 18-42: Re-run the ruff pre-commit hook for coverage_tier.py,
retain all four auto-fixes it applies, then stage the modified file and create a
new commit containing those changes so the hook completes successfully.
In `@jenkins/scripts/cbts/tools/coverage_explain.py`:
- Around line 86-103: The coverage explanation must mirror
CoverageSelector.decide() safety gates. In
jenkins/scripts/cbts/tools/coverage_explain.py lines 86-103, check
db.file_has_touch_rows(cf) before reporting file or function impact; when false,
stop and report the coverage decision as unsupported. In lines 124-143, remove
the selector’s untrusted tests from skip_s and report those tests as
forced-kept, preserving the selector’s actual behavior.
---
Outside diff comments:
In `@jenkins/L0_MergeRequest.groovy`:
- Around line 809-815: Update the scope-null handling in the pipeline flow
around _cbtsParseSelectionResult and _cbtsReportDecision so result.reasons maps
are converted to readable reason text before joining. Reuse the structured
fields, including source and reason and any relevant details, rather than
calling join directly on the maps; preserve the existing deferring message and
fallback decision behavior.
- Around line 954-968: The Groovy parser _cbtsParseSelectionResult in
jenkins/L0_MergeRequest.groovy:954-968 must forward enable_multi_gpu with a
false default and coverage_dropped_stages with an empty-list default in its
returned map. Make no code change to jenkins/L0_Test.groovy:5714-5718; its
consumer is already correct and will work once the parser populates
cbts.enable_multi_gpu.
---
Nitpick comments:
In `@jenkins/scripts/cbts/coverage_selection/qualname_map.py`:
- Around line 79-86: The public API docstrings are missing parameter and result
documentation. In jenkins/scripts/cbts/coverage_selection/qualname_map.py lines
79-86, update qualnames_for_lines with Google-style Args for source and lines
and Returns describing the qualname set and the ok=False result when parsing
fails. In jenkins/scripts/cbts/coverage_selection/selector.py lines 108-112,
update the affected public function’s docstring with Google-style Args for
residual_files and diffs and Returns covering every possible CoverageResult
outcome.
In `@jenkins/scripts/cbts/coverage_tier.py`:
- Around line 175-251: Expose the must-run reason tally in the structured
coverage-tier output by adding the existing must_run_reasons value returned from
_build_narrowing to CoverageTierResult.detail in apply_coverage_tier. Preserve
the existing must_run_reasons field and include the tally under a clear stable
detail key so downstream main.py consumers receive it.
🪄 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: a4e8fe46-939f-4256-acec-6e03d0ed6157
📒 Files selected for processing (11)
jenkins/L0_MergeRequest.groovyjenkins/L0_Test.groovyjenkins/scripts/cbts/coverage_selection/qualname_map.pyjenkins/scripts/cbts/coverage_selection/selector.pyjenkins/scripts/cbts/coverage_tier.pyjenkins/scripts/cbts/main.pyjenkins/scripts/cbts/rules/base.pyjenkins/scripts/cbts/rules/tests_def_rule.pyjenkins/scripts/cbts/rules/waives_rule.pyjenkins/scripts/cbts/tools/coverage_explain.pyjenkins/scripts/cbts/tools/dryrun.py
| import math | ||
| import re | ||
| import sys | ||
| from collections import Counter | ||
| from dataclasses import dataclass, field | ||
| from pathlib import Path | ||
|
|
||
| import yaml | ||
| from blocks import ( | ||
| TARGET_SHARD_SECONDS, | ||
| Stage, | ||
| YAMLIndex, | ||
| _avg_duration, | ||
| _entry_applies_to_waive, | ||
| _entry_target, | ||
| _estimate_entries_seconds, | ||
| _target_in_filter_subtree, | ||
| block_matches_stage, | ||
| ) | ||
|
|
||
| from rules.base import PRInputs, RuleResult | ||
|
|
||
| sys.path.insert(0, str(Path(__file__).resolve().parent / "coverage_selection")) | ||
| from selector import CoverageSelector # noqa: E402 | ||
| from touch_db import TouchDB, db_key # noqa: E402 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Re-stage ruff's auto-fixes and re-commit.
The attached pipeline logs show ruff (pre-commit) modified this file and fixed 4 errors, but the hook still exited non-zero. The current diff does not include those auto-fixes.
Based on learnings and pipeline failures, as per coding guidelines: "If pre-commit hooks modify files, re-stage the modified files and commit again."
🤖 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_tier.py` around lines 18 - 42, Re-run the ruff
pre-commit hook for coverage_tier.py, retain all four auto-fixes it applies,
then stage the modified file and create a new commit containing those changes so
the hook completes successfully.
Sources: Coding guidelines, Pipeline failures
| for f in core: | ||
| cf = canon(f) | ||
| changed_files.add(cf) | ||
| diff = _git(repo, "diff", f"{args.sha}^", args.sha, "--", f, check=False) | ||
| lines = iter_diff_post_line_numbers(diff) | ||
| src = _src_at(repo, args.sha, f) | ||
| if not lines or src is None: | ||
| impact_files.add(cf) | ||
| impacted |= db.tests_touching_file(cf) | ||
| continue | ||
| qns, ok = qualnames_for_lines(src, lines) | ||
| if not ok: | ||
| impact_files.add(cf) | ||
| impacted |= db.tests_touching_file(cf) | ||
| continue | ||
| for q in qns: | ||
| impact_funcs.add((cf, q)) | ||
| impacted |= db.tests_touching_func(cf, q) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the explanation output aligned with selector safety gates. The tool can label cases as removed even when CoverageSelector.decide() refuses zero-touch files or forces untrusted tests to run.
jenkins/scripts/cbts/tools/coverage_explain.py#L86-L103: stop/report an unsupported coverage decision whendb.file_has_touch_rows(cf)is false.jenkins/scripts/cbts/tools/coverage_explain.py#L124-L143: exclude the selector’s untrusted tests fromskip_sand report them as forced-kept.
📍 Affects 1 file
jenkins/scripts/cbts/tools/coverage_explain.py#L86-L103(this comment)jenkins/scripts/cbts/tools/coverage_explain.py#L124-L143
🤖 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/tools/coverage_explain.py` around lines 86 - 103, The
coverage explanation must mirror CoverageSelector.decide() safety gates. In
jenkins/scripts/cbts/tools/coverage_explain.py lines 86-103, check
db.file_has_touch_rows(cf) before reporting file or function impact; when false,
stop and report the coverage decision as unsupported. In lines 124-143, remove
the selector’s untrusted tests from skip_s and report those tests as
forced-kept, preserving the selector’s actual behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/scripts/cbts/rules/out_of_scope_rule.py`:
- Line 59: Remove the broad "jenkins/" no-op prefix from the OutOfScopeRule
configuration before merging. If it is required for temporary validation, gate
it behind explicit test-only configuration so normal CBTS runs do not classify
every Jenkins change as out of scope.
In `@tensorrt_llm/_torch/pyexecutor/py_executor.py`:
- Line 106: Remove the temporary “[cbts-test] no-op touch to validate
coverage-based selection” comment from the production code, leaving the
surrounding implementation unchanged.
🪄 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: 23a719fe-5256-435e-8601-e5531343b7d7
📒 Files selected for processing (2)
jenkins/scripts/cbts/rules/out_of_scope_rule.pytensorrt_llm/_torch/pyexecutor/py_executor.py
| "tests/integration/defs/agg_unit_mem_df.csv", | ||
| "tests/microbenchmarks/", | ||
| "jenkins/scripts/cbts/", | ||
| "jenkins/", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Revert the broad jenkins/ no-op prefix before merging.
Because OutOfScopeRule is active in CBTS, this classifies every Jenkins change as out of scope and can suppress test-stage selection. Keep this only for the temporary validation run, or gate it behind explicit test-only configuration.
🤖 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/rules/out_of_scope_rule.py` at line 59, Remove the broad
"jenkins/" no-op prefix from the OutOfScopeRule configuration before merging. If
it is required for temporary validation, gate it behind explicit test-only
configuration so normal CBTS runs do not classify every Jenkins change as out of
scope.
|
|
||
| # Environment variable to specify iteration ranges for profiling start/stop. | ||
| # Format: "start1-stop1,start2-stop2,..." or single iterations "iter1,iter2,..." | ||
| # [cbts-test] no-op touch to validate coverage-based selection |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the temporary coverage marker before merging.
This comment only touches production code to trigger CBTS coverage selection and has no runtime purpose. Revert it after the validation run.
🤖 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 `@tensorrt_llm/_torch/pyexecutor/py_executor.py` at line 106, Remove the
temporary “[cbts-test] no-op touch to validate coverage-based selection” comment
from the production code, leaving the surrounding implementation unchanged.
8e99945 to
8ce112a
Compare
|
/bot run |
8ce112a to
34ba032
Compare
|
/bot run |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
jenkins/scripts/cbts/main.py (1)
390-392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid swallowing every coverage-tier failure.
This converts programming defects in Tier 2 into an ordinary fallback, making coverage regressions hard to detect. Catch expected operational failures at this boundary (or a dedicated coverage-tier exception) and preserve unexpected-error diagnostics.
As per coding guidelines, “Avoid broad exception handling; catch specific exceptions instead of using bare
except:.”🤖 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/main.py` around lines 390 - 392, Update the exception handling around the coverage-tier invocation in the Tier 2 flow to catch only expected operational failures or the dedicated coverage-tier exception, while preserving the existing fallback note and tier=None behavior for those cases. Do not convert unexpected programming errors into ordinary fallback; allow them to propagate with their diagnostics, and update the handler near the existing note and tier assignments rather than broadening error handling elsewhere.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@jenkins/scripts/cbts/main.py`:
- Around line 390-392: Update the exception handling around the coverage-tier
invocation in the Tier 2 flow to catch only expected operational failures or the
dedicated coverage-tier exception, while preserving the existing fallback note
and tier=None behavior for those cases. Do not convert unexpected programming
errors into ordinary fallback; allow them to propagate with their diagnostics,
and update the handler near the existing note and tier assignments rather than
broadening error handling elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 39c1ca16-ae60-4c99-b8e9-b0e33d4122af
📒 Files selected for processing (13)
jenkins/L0_MergeRequest.groovyjenkins/L0_Test.groovyjenkins/scripts/cbts/coverage_selection/qualname_map.pyjenkins/scripts/cbts/coverage_selection/selector.pyjenkins/scripts/cbts/coverage_tier.pyjenkins/scripts/cbts/main.pyjenkins/scripts/cbts/rules/base.pyjenkins/scripts/cbts/rules/out_of_scope_rule.pyjenkins/scripts/cbts/rules/tests_def_rule.pyjenkins/scripts/cbts/rules/waives_rule.pyjenkins/scripts/cbts/tools/coverage_explain.pyjenkins/scripts/cbts/tools/dryrun.pytensorrt_llm/_torch/pyexecutor/py_executor.py
🚧 Files skipped from review as they are similar to previous changes (10)
- jenkins/scripts/cbts/rules/tests_def_rule.py
- jenkins/scripts/cbts/rules/base.py
- jenkins/scripts/cbts/rules/out_of_scope_rule.py
- tensorrt_llm/_torch/pyexecutor/py_executor.py
- jenkins/scripts/cbts/rules/waives_rule.py
- jenkins/L0_MergeRequest.groovy
- jenkins/L0_Test.groovy
- jenkins/scripts/cbts/coverage_tier.py
- jenkins/scripts/cbts/coverage_selection/selector.py
- jenkins/scripts/cbts/coverage_selection/qualname_map.py
|
PR_Github #61223 [ run ] triggered by Bot. Commit: |
|
/bot kill |
|
PR_Github #61226 [ kill ] triggered by Bot. Commit: |
34ba032 to
28d14b0
Compare
|
PR_Github #61223 [ run ] completed with state |
|
PR_Github #61226 [ kill ] completed with state |
|
/bot run |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jenkins/scripts/cbts/main.py (1)
197-214: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
self.pairs/self.handledset only insiderun(), never initialized in__init__.They're read externally as
selector.pairs/selector.handledinmain()(lines 388), which makes them externally-visible class members. As per coding guidelines,"initialize externally visible class members in the constructor". CurrentlySelector.__init__(lines 198-199) only setsself.stages, sopairs/handledare absent from the object untilrun()executes, and any future caller that inspects them before callingrun()gets anAttributeError.♻️ Proposed fix
class Selector: def __init__(self, stages: dict[str, Stage]) -> None: self.stages = stages + self.pairs: list[tuple[Rule, RuleResult]] = [] + self.handled: set[str] = set()🤖 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/main.py` around lines 197 - 214, Initialize the externally visible Selector state in Selector.__init__ by setting pairs to an empty list of rule/result tuples and handled to an empty set of file names. Keep run() updating these members with the computed values before they are consumed by main().Source: Coding guidelines
🧹 Nitpick comments (1)
jenkins/scripts/cbts/tools/dryrun.py (1)
214-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates
_fmt_reasonfrommain.pyinstead of reusing it.This block re-implements the same dict-vs-non-dict reason formatting that
main.pyalready defines as_fmt_reason(r).report_cbts_decision.pyapparently imports/reuses that helper rather than reimplementing it;dryrun.pyshould do the same to avoid the two renderings drifting apart.♻️ Proposed fix
+from main import _fmt_reason # noqa: E402 + ... - lines.append("reasons:") - for r in result.get("reasons", []): - if isinstance(r, dict): - src = r.get("source", "?") - rest = ", ".join(f"{k}={v}" for k, v in r.items() if k != "source") - lines.append(f" - [{src}] {rest}" if rest else f" - [{src}]") - else: - lines.append(f" - {r}") + lines.append("reasons:") + lines.extend(f" - {_fmt_reason(r)}" for r in result.get("reasons", []))🤖 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/tools/dryrun.py` around lines 214 - 221, Replace the inline reason-formatting logic in the report-generation block with the existing _fmt_reason helper from main.py. Import or reuse that helper in dryrun.py and pass each reason to it, preserving the current “reasons:” output structure while ensuring formatting remains consistent.
🤖 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.
Outside diff comments:
In `@jenkins/scripts/cbts/main.py`:
- Around line 197-214: Initialize the externally visible Selector state in
Selector.__init__ by setting pairs to an empty list of rule/result tuples and
handled to an empty set of file names. Keep run() updating these members with
the computed values before they are consumed by main().
---
Nitpick comments:
In `@jenkins/scripts/cbts/tools/dryrun.py`:
- Around line 214-221: Replace the inline reason-formatting logic in the
report-generation block with the existing _fmt_reason helper from main.py.
Import or reuse that helper in dryrun.py and pass each reason to it, preserving
the current “reasons:” output structure while ensuring formatting remains
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c9b5ee70-2616-4357-9ad6-bde91f02cfce
📒 Files selected for processing (14)
jenkins/L0_MergeRequest.groovyjenkins/L0_Test.groovyjenkins/scripts/cbts/coverage_selection/qualname_map.pyjenkins/scripts/cbts/coverage_selection/selector.pyjenkins/scripts/cbts/coverage_tier.pyjenkins/scripts/cbts/main.pyjenkins/scripts/cbts/rules/base.pyjenkins/scripts/cbts/rules/out_of_scope_rule.pyjenkins/scripts/cbts/rules/tests_def_rule.pyjenkins/scripts/cbts/rules/waives_rule.pyjenkins/scripts/cbts/tools/coverage_explain.pyjenkins/scripts/cbts/tools/dryrun.pyjenkins/scripts/cbts/tools/report_cbts_decision.pytensorrt_llm/_torch/pyexecutor/py_executor.py
🚧 Files skipped from review as they are similar to previous changes (9)
- jenkins/scripts/cbts/rules/base.py
- tensorrt_llm/_torch/pyexecutor/py_executor.py
- jenkins/scripts/cbts/rules/tests_def_rule.py
- jenkins/L0_Test.groovy
- jenkins/L0_MergeRequest.groovy
- jenkins/scripts/cbts/rules/waives_rule.py
- jenkins/scripts/cbts/coverage_selection/qualname_map.py
- jenkins/scripts/cbts/coverage_selection/selector.py
- jenkins/scripts/cbts/coverage_tier.py
28d14b0 to
c43b960
Compare
|
PR_Github #61233 [ run ] triggered by Bot. Commit: |
|
PR_Github #61233 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61261 [ run ] triggered by Bot. Commit: |
|
PR_Github #61261 [ run ] completed with state
|
…ector) Add coverage-based test selection as Tier 2 in the CBTS decision pipeline. When Tier 1 (flat rules) defers to a full run on a residual of core-Python files, Tier 2 consults the merged touch DB to narrow per-stage test lists to only the cases provably reached by the changed functions. New files: - coverage_selection/selector.py: CoverageSelector — maps changed lines to qualnames via AST, queries the DB for impacted tests, returns per-stage impacted/skippable sets. - coverage_selection/qualname_map.py: AST-based line→qualname mapping with fallback to enclosing class/module importers for class-body changes. - coverage_selection/fixture.py: pytest fixture that resolves the CBTS touch DB path for unit tests. - coverage_tier.py: apply_coverage_tier() — orchestrates the selector, applies shared-block and rule-union correctness rules, sizes splits, emits structured reasons. - rules/product_data_rule.py: ProductDataRule — scopes pure product-data PRs (configs, curated examples, golden manifests) to productdataonly. - tools/coverage_explain.py: per-test explain tool showing which changed function triggered each must-run verdict. Modified: - main.py: wire Tier 2 (--coverage-db flag, CoverageTierResult → SelectionResult fields enable_multi_gpu / coverage_dropped_stages, structured reasons dict). - L0_MergeRequest.groovy: _cbtsCoverageAudit() returns the sqlite path; pass --coverage-db to main.py when the DB is available (enforce step). - L0_Test.groovy: re-add multiGpuJobs under baseline gate when enable_multi_gpu (coverage narrows single-GPU only; multi-GPU stays on MULTI_GPU_FILE_CHANGED). - dryrun.py: --coverage-db option to replay Tier 2 over historical commits. - rules/base.py, tests_def_rule.py, waives_rule.py: minor cleanups to expose block_filters and handled_files for Tier 2 consumption. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
…py_executor Test-only commit to exercise Tier 2 coverage selection in CI: - out_of_scope_rule: expand noop prefix from jenkins/scripts/cbts/ to jenkins/ so L0_MergeRequest.groovy and L0_Test.groovy are claimed as noop instead of falling through as non-core-Python residual. - py_executor.py: no-op comment touch to leave a core-Python file in the residual, triggering coverage tier. py_executor.py is also in getMultiGpuFileChanged, so MULTI_GPU_FILE_CHANGED=True → the enable_multi_gpu gate in L0_Test.groovy should re-add multi-GPU stages. Expected CI outcome: scope=coverage, enable_multi_gpu=True, multi-GPU stages present. Revert both changes before merging. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
c43b960 to
f42c618
Compare
|
/bot run |
|
PR_Github #61287 [ run ] triggered by Bot. Commit: |
|
PR_Github #61287 [ run ] completed with state
|
2 participants
Summary
Adds Tier-2 coverage-based test selection (CBTS) for residual core-Python changes. The Jenkins pipeline now runs
_cbtsCoverageAudit(pipeline)to discover an optional sqlite coverage DB path and, when present, threads it intojenkins/scripts/cbts/main.pyas--coverage-db(omitted when empty or when audit failures are non-fatal), preserving existing CBTS behavior when coverage is unavailable.Key new/updated coverage-tier behavior:
co_qualnames (jenkins/scripts/cbts/coverage_selection/qualname_map.py).jenkins/scripts/cbts/coverage_selection/selector.py).jenkins/scripts/cbts/coverage_tier.py).reasons(list of dicts) plus coverage-tier fields likeenable_multi_gpuandcoverage_dropped_stages(jenkins/scripts/cbts/main.py), with consistent rendering indryrun.pyandreport_cbts_decision.py.coverage_explain.pyto explain kept/removed coverage cases for a specific commit SHA, and extendsdryrun.pywith opt-in--coverage-dbreplay/formatting support.Supporting infra tweaks:
_cbtsCoverageAuditis converted from best-effort “shadow audit” into a return-value helper that yields the sqlite path on success and""otherwise; logs updated to reflect skipping Tier 2 when coverage artifacts can’t be found/processed (jenkins/L0_MergeRequest.groovy).parallelJobsFilteredonly whencbts.enable_multi_gpuis true andMULTI_GPU_FILE_CHANGEDis set (jenkins/L0_Test.groovy).jenkins/changes (jenkins/scripts/cbts/rules/out_of_scope_rule.py).tensorrt_llm/_torch/pyexecutor/py_executor.pyfor coverage-based validation.Dev Engineer Review
Correctness / behavior
jenkins/L0_MergeRequest.groovy:_cbtsCoverageAudit(pipeline)now returns the sqlite coverage artifact path on success and""on non-fatal failures;--coverage-dbis only appended to the Python invocation when the returned path is non-empty. Updated logging aligns with Tier 2 being skipped when the artifact is unavailable/unusable.jenkins/scripts/cbts/main.py: implements optional coverage-tier execution behind--coverage-db, updatesSelectionResultJSON withenable_multi_gpuandcoverage_dropped_stages, and migratesreasonsfromlist[str]tolist[dict]with structured rendering/printing.jenkins/scripts/cbts/coverage_selection/qualname_map.py: AST-based line→qualname attribution with scope selection rules; returns(set(), False)onSyntaxErrorandqualnames_for_lines(...)->(set[str], bool)to drive conservative downstream behavior.jenkins/scripts/cbts/coverage_selection/selector.py: validates residual residual inputs, derives post-diff line numbers, maps them to qualnames, queries TouchDB for impacted tests, caches “untrusted” tests, and avoids marking untrusted tests skippable unless impacted; uses conservative “all touching-file tests impacted” fallback when mapping/reads fail.jenkins/scripts/cbts/coverage_tier.py: narrows the test-db by removing only SAFE entries and accounting for must-run reasons (impacted/untrusted/no-data). Includes stage-dropping logic with explicit exclusions for multi-GPU and post-merge variants.jenkins/scripts/cbts/tools/coverage_explain.py: diff+qualname mapping and TouchDB-based classification of KEPT vs REMOVED nodeids for a specific SHA; includes optional kept-case printing and safe fallbacks.jenkins/scripts/cbts/tools/dryrun.py+jenkins/scripts/cbts/tools/report_cbts_decision.py: consistent formatting for structuredreasonsobjects (dict-based reasons rendered as[source] k=v, ...).Jenkins pipeline / scoping
jenkins/L0_Test.groovy: conditional re-add of multi-GPU stages intoparallelJobsFilteredgated bycbts.enable_multi_gpuandMULTI_GPU_FILE_CHANGED.jenkins/scripts/cbts/rules/out_of_scope_rule.py: treatsjenkins/as out-of-scope in addition to the existing more specific CBTS script prefix.Schema / API consistency
jenkins/scripts/cbts/rules/base.py: addsRuleResult.detail: dict[str, object]defaulting to empty dict for structured rule payloads.jenkins/scripts/cbts/rules/tests_def_rule.pyandwaives_rule.py: populatesdetailwith narrowed-path counts and waived added/removed counts respectively.jenkins/scripts/cbts/main.py+ tooling: updates reason formatting/serialization to match the structured schema end-to-end.Risk / areas to double-check
coverageDbPathis non-empty, and that empty/failed audit does not alter non-coverage CBTS semantics.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.