[TRTLLM-12838][infra] CBTS coverage db audit - #16658
Conversation
Add the read-only touch-DB accessor (touch_db.py, implementing
TOUCH_DB_CONTRACT.md), an Artifactory fetch for the latest merged DB
(artifact.py), and a standalone audit tool (coverage_audit.py) that
reports the DB's health -- stage prefix, schema, scale, and the
incomplete-capture ("untrusted") rate -- plus this HEAD's coverage gap:
test cases that render on an instrumented single-GPU stage yet have no
DB row, so the selector could never skip them.
Wire an audit-first shadow step into getCbtsResult: each pre-merge
/bot run downloads the latest merged touch DB and logs the audit. It is
diagnostic only and does not change the CBTS decision -- coverage-based
selection is not yet enforced; the same download will feed the selector
in a follow-up. Everything is best-effort: a failed fetch or audit is
logged and skipped, never blocking CI.
Foundation for coverage-based test selection (CBTS Tier 2).
Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Address review findings on the coverage-DB audit shadow: - coverage_audit.py: guard the per-test footprint print so an empty or all-`test=''` DB reports "no usable per-test coverage" instead of crashing on min()/max(); classify near-empty vs worker-lost from the footprint so the by-reason histogram no longer mislabels tiny non-executor tests as worker-lost. - touch_db.py: TouchDB.meta() tolerates a missing `meta` table; add per_test_footprint() so the audit no longer reaches into the private `_conn`. - artifact.py: return None (not raise) when a downloaded tarball fails to extract, honoring the best-effort contract; drop the unreachable HTTPError 200/206 branch in _exists. - L0_MergeRequest.groovy: run `artifact.py --print-url || true` so a missing artifact takes the clean skip path instead of the generic catch. - Collapse multi-line comments and docstrings to one-line, current-behavior form. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
|
/bot run |
|
PR_Github #60585 [ run ] triggered by Bot. Commit: |
📝 WalkthroughWalkthroughThe PR adds tooling to locate merged CBTS touch database artifacts, inspect SQLite coverage data, generate coverage reports, and invoke the audit from Jenkins without changing CBTS selection behavior. ChangesCBTS coverage audit
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Jenkins as Jenkins pipeline
participant AuditHelper as _cbtsCoverageAudit
participant Artifact as artifact.py
participant Audit as coverage_audit.py
participant DB as cbts_touchmap.sqlite
Jenkins->>AuditHelper: invoke coverage audit
AuditHelper->>Artifact: resolve and download latest artifact
Artifact-->>AuditHelper: return SQLite path
AuditHelper->>Audit: run audit CLI
Audit->>DB: query coverage metadata and touch rows
DB-->>Audit: return coverage data
Audit-->>AuditHelper: emit report
AuditHelper-->>Jenkins: continue CBTS selection
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (5)
jenkins/scripts/cbts/coverage_selection/artifact.py (2)
115-125: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHarden tarball extraction against symlink/hardlink attacks.
tf.extract(member, dest_dir)runs without afilter=. Themember.namereassignment prevents a path-traversal via the name, but a malicious member could still be a symlink/hardlink whoselinknamepoints outsidedest_dir; extracting it would still honor that link target (CVE-2007-4559 class issue, the motivation for PEP 706'sdatafilter). Since this pulls a tarball from an internal Artifactory path, risk is currently low, but the fix is essentially free.🔒️ Proposed fix
- tf.extract(member, dest_dir) + tf.extract(member, dest_dir, filter="data")Per PEP 706/PEP 721, the
filterargument (anddatafilter) has been backported to all Python versions still supported by python.org, so this is safe to use unconditionally.🤖 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/artifact.py` around lines 115 - 125, Harden extract_touch_db by passing the PEP 706 data filter to tf.extract when extracting the selected member, preventing symlink and hardlink targets from escaping dest_dir. Preserve the existing member selection, renaming, return value, and missing-member behavior.
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent
Optional[X]vsX | Nonetyping style across the new coverage-audit modules. Both new modules import and usetyping.Optionalpervasively, whilecoverage_audit.py(added in the same PR) already uses theX | Nonestyle — one root cause (style drift within the new code), two sites to align.
jenkins/scripts/cbts/coverage_selection/artifact.py#L39-L39: drop theOptionalimport and switch allOptional[...]annotations (lines 54, 78, 98, 115, 128, 148) toX | None, including reconciling the mixedPath | str/Optional[str]/Optional[Path]styles withinfetch_latest_touch_db's own signature.jenkins/scripts/cbts/coverage_selection/touch_db.py#L28-L28: drop theOptionalimport and switch allOptional[...]annotations (lines 60, 70, 112, 119, 122) toX | None.🤖 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/artifact.py` at line 39, Align both new coverage-audit modules with the existing union typing style: in jenkins/scripts/cbts/coverage_selection/artifact.py:39-39, remove Optional and convert every Optional annotation, including the fetch_latest_touch_db signature, to X | None; make the same changes in jenkins/scripts/cbts/coverage_selection/touch_db.py:28-28 for all listed annotations, with no direct changes needed elsewhere.Source: Coding guidelines
jenkins/L0_MergeRequest.groovy (1)
848-874: 🧹 Nitpick | 🔵 TrivialNo standalone kill switch for the coverage audit.
Unlike
DISABLE_CBTS/ENABLE_CBTS_COVERAGE,_cbtsCoverageAudithas no dedicated flag to disable it independently — every qualifying pre-merge//bot runnow does a Jenkins REST call, up to 10 Artifactory ranged-GET probes, and awget+tardownload on top of existing CBTS work. The try/catch makes it non-blocking, but there's no lever to turn it off without a code change if it turns out to add meaningful latency or Artifactory load at scale.🤖 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 848 - 874, Add a dedicated configuration flag for `_cbtsCoverageAudit` that disables the audit before it performs artifact lookup or download work. Default the audit to its current enabled behavior, and have the existing pre-merge/`/bot run` caller honor this flag without affecting `DISABLE_CBTS` or `ENABLE_CBTS_COVERAGE`.jenkins/scripts/cbts/tools/coverage_audit.py (1)
39-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCross-module import of
touch_db's "private" constants.
_LAUNCH_MARKERS,_MIN_FUNCS,_SERVING_PATH_MARKERS, and_WORKER_SENTINELare underscore-prefixed intouch_db.py, signaling non-public status, yet this file imports them directly to feeduntrusted_tests(). That breaks the encapsulation the naming convention is meant to signal, and couples this CLI totouch_db's internal constant names.Consider promoting these to public constants (drop the leading underscore) in
touch_db.pyif they're meant to be part of its consumable API, or wrap the heuristic defaults behind a small public function/dataclass thatcoverage_audit.pycalls instead of importing the raw constants.As per coding guidelines, "Prefix non-public names with
_" — the intent of that convention is undermined when non-public names are imported across module boundaries.🤖 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_audit.py` around lines 39 - 47, Remove the cross-module imports of _LAUNCH_MARKERS, _MIN_FUNCS, _SERVING_PATH_MARKERS, and _WORKER_SENTINEL from coverage_audit.py by exposing the heuristic defaults through a public touch_db API, such as a function or dataclass, and update untrusted_tests() to use that API. Keep the defaults centralized in touch_db.py while preserving the existing behavior.Source: Coding guidelines
jenkins/scripts/cbts/coverage_selection/touch_db.py (1)
171-182: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant full-table
known_tests()scans acrossTouchDBmethods and their caller.known_by_stage()anduntrusted_tests()each independently re-run theSELECT DISTINCT test FROM touch WHERE test!=''scan thatcoverage_audit.py'smain()already ran to computeknown— one root cause (no way to pass a precomputed set in), three sites affected.
jenkins/scripts/cbts/coverage_selection/touch_db.py#L171-L182: add an optionalknown: set[str] | None = Noneparameter toknown_by_stage()and use it instead of callingself.known_tests()when provided.jenkins/scripts/cbts/coverage_selection/touch_db.py#L195-L232: thread the same optionalknownparameter throughuntrusted_tests()'sserving_path_markersbranch (line 221).jenkins/scripts/cbts/tools/coverage_audit.py#L79-L82: pass the already-computedknownset into bothdb.known_by_stage(known)anddb.untrusted_tests(..., known=known)calls.🤖 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/touch_db.py` around lines 171 - 182, The redundant full-table known_tests() scans should be eliminated by threading the precomputed known set through the affected APIs. In jenkins/scripts/cbts/coverage_selection/touch_db.py:171-182, add an optional known: set[str] | None parameter to known_by_stage() and use it instead of self.known_tests() when supplied; in jenkins/scripts/cbts/coverage_selection/touch_db.py:195-232, add and forward the same optional parameter through untrusted_tests(), including its serving_path_markers branch. In jenkins/scripts/cbts/tools/coverage_audit.py:79-82, pass the existing known set to both db.known_by_stage(known) and db.untrusted_tests(..., known=known).
🤖 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 866-867: Update the coverage audit invocation in the relevant
pipeline stage to remove the “cd ${LLM_ROOT} &&” prefix and run
jenkins/scripts/cbts/tools/coverage_audit.py from the workspace root. Keep the
--db argument using ${covDir}/cbts_touchmap.sqlite so it resolves to the
populated database path, and preserve the existing audit behavior and error
handling.
In `@jenkins/scripts/cbts/coverage_selection/artifact.py`:
- Around line 60-62: Update the exception handlers in _get, _exists, and
fetch_latest_touch_db to catch only the expected network and filesystem
exception types instead of broad Exception. Preserve each handler’s existing
error reporting and fallback return behavior, while allowing unrelated
programming errors to propagate.
---
Nitpick comments:
In `@jenkins/L0_MergeRequest.groovy`:
- Around line 848-874: Add a dedicated configuration flag for
`_cbtsCoverageAudit` that disables the audit before it performs artifact lookup
or download work. Default the audit to its current enabled behavior, and have
the existing pre-merge/`/bot run` caller honor this flag without affecting
`DISABLE_CBTS` or `ENABLE_CBTS_COVERAGE`.
In `@jenkins/scripts/cbts/coverage_selection/artifact.py`:
- Around line 115-125: Harden extract_touch_db by passing the PEP 706 data
filter to tf.extract when extracting the selected member, preventing symlink and
hardlink targets from escaping dest_dir. Preserve the existing member selection,
renaming, return value, and missing-member behavior.
- Line 39: Align both new coverage-audit modules with the existing union typing
style: in jenkins/scripts/cbts/coverage_selection/artifact.py:39-39, remove
Optional and convert every Optional annotation, including the
fetch_latest_touch_db signature, to X | None; make the same changes in
jenkins/scripts/cbts/coverage_selection/touch_db.py:28-28 for all listed
annotations, with no direct changes needed elsewhere.
In `@jenkins/scripts/cbts/coverage_selection/touch_db.py`:
- Around line 171-182: The redundant full-table known_tests() scans should be
eliminated by threading the precomputed known set through the affected APIs. In
jenkins/scripts/cbts/coverage_selection/touch_db.py:171-182, add an optional
known: set[str] | None parameter to known_by_stage() and use it instead of
self.known_tests() when supplied; in
jenkins/scripts/cbts/coverage_selection/touch_db.py:195-232, add and forward the
same optional parameter through untrusted_tests(), including its
serving_path_markers branch. In
jenkins/scripts/cbts/tools/coverage_audit.py:79-82, pass the existing known set
to both db.known_by_stage(known) and db.untrusted_tests(..., known=known).
In `@jenkins/scripts/cbts/tools/coverage_audit.py`:
- Around line 39-47: Remove the cross-module imports of _LAUNCH_MARKERS,
_MIN_FUNCS, _SERVING_PATH_MARKERS, and _WORKER_SENTINEL from coverage_audit.py
by exposing the heuristic defaults through a public touch_db API, such as a
function or dataclass, and update untrusted_tests() to use that API. Keep the
defaults centralized in touch_db.py while preserving the existing behavior.
🪄 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: 9b38e43e-62fb-46b6-9495-1aebfd5afb41
📒 Files selected for processing (4)
jenkins/L0_MergeRequest.groovyjenkins/scripts/cbts/coverage_selection/artifact.pyjenkins/scripts/cbts/coverage_selection/touch_db.pyjenkins/scripts/cbts/tools/coverage_audit.py
|
PR_Github #60585 [ run ] completed with state
|
_cbtsCoverageAudit ran coverage_audit.py with `cd ${LLM_ROOT}` while passing
`--db ${covDir}/...`, but covDir already includes ${LLM_ROOT}, so the path
resolved to ${LLM_ROOT}/${LLM_ROOT}/cbts_cov/... and sqlite3 could not open the
DB (OperationalError: unable to open database file). Run the script by its
${LLM_ROOT}-prefixed path without cd, matching the workspace-relative
mkdir/wget/tar steps in the same helper.
Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Replace the broad `except Exception` (BLE001-suppressed) in _get, _exists, and fetch_latest_touch_db with `except OSError`, which covers the expected network and filesystem failures (URLError/HTTPError, timeout, SSL, connection reset, BadGzipFile) while letting genuine programming errors propagate. Drops the three noqa suppressions. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
|
/bot run |
|
PR_Github #60661 [ run ] triggered by Bot. Commit: |
|
PR_Github #60661 [ run ] completed with state
|
|
/bot skip --comment "target functionality is confirmed ready, the failed cases have nothing to do with this pr" |
|
PR_Github #60689 [ skip ] triggered by Bot. Commit: |
|
PR_Github #60689 [ skip ] completed with state |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
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.