[TRTLLM-12838][infra] enhance function-level code coverage for catching subprocess data#16229
Open
crazydemo wants to merge 6 commits into
Open
[TRTLLM-12838][infra] enhance function-level code coverage for catching subprocess data#16229crazydemo wants to merge 6 commits into
crazydemo wants to merge 6 commits into
Conversation
…ge via sys.monitoring Collect per-test code coverage on single-GPU L0 stages to feed change-based test selection. A sys.monitoring PY_START tracker (cbts_pystart.py) records, per test, the product functions each test enters; overhead scales with functions entered, not lines, so it is far cheaper than line tracing. sitecustomize bootstraps the tracker in every process (incl. subprocesses / mpi4py pool workers / long-lived servers); cbts_plugin drives per-test context switching. Per-process data is written as binary SQLite (skipped by the artifact guardword/secret scanners) and merged by pystart_report.py into an indexed touch DB, a per-file split HTML report, and a file/function coverage rate against tensorrt_llm/. Gated by isCbtsStage() to single-GPU non-perf/TensorRT/CPP/AutoDeploy stages (multi-GPU enabled incrementally later); ENABLE_CBTS_COVERAGE is a global kill-switch. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
…bprocess imports The cbts-mpi-patcher daemon imported pytest (via cbts_plugin) at process startup, concurrently with a plain CLI subprocess's own imports (e.g. trtllm-bench importing tensorrt_llm -> huggingface_hub). CPython's partial-module deadlock-avoidance then handed the main thread a half-initialized huggingface_hub.utils -> NameError name sys is not defined, failing test_offline_benchmark. Fix: (1) cbts_plugin binds pytest and its hooks only when pytest is already loaded, so importing it for install_mpi_pool_patch never drags in the pytest graph; (2) the mpi-patcher daemon defers every import until the target modules are already in sys.modules, so it never runs a concurrent import during the host's startup phase; (3) harden the PY_START callback and tracker startup so a tracker fault can never propagate into host code. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
The full single-GPU run produces thousands of per-process .cbtscov.*.sqlite files with millions of touch records; pystart_report loaded them all into in-memory dicts/sets (per_test + file_funcs), which at full scale can reach multiple GB. Stream every input file straight into the output SQLite (dedup on disk via a UNIQUE index, one file's rows in memory at a time), compute counts/rate with SQL, and render the split HTML by querying per file. Peak RSS is ~29 MB for a 4M-row / 464 MB stress input. CLI and outputs unchanged. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
…uardword scan The merged cbts_touchmap.sqlite was uploaded raw. SQLite stores its TEXT columns as plaintext, so the per-test (file, qualname) rows carry product paths (wan, marlin, ...) verbatim in the file bytes; the publish-artifacts guardword scanner byte-matches raw uploaded files and flagged them. The scanner does not recurse into archives (the HTML report .tar.gz with the same qualnames was never flagged), so bundle the touch DB into cbts_pystart_report.tar.gz alongside the report and upload only the tarball. Correct the stale 'binary sqlite is skipped' notes in cbts_pystart.py and README. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
…c sed Collapse the multi-line comment blocks across the CBTS coverage utils and Jenkins glue to single lines, fix stale .json/post-merge references, and remove the no-op @TRTLLM_SRC_PATH@ sed (the template has no such placeholder). Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
crazydemo
force-pushed
the
cbts-coverage-utils
branch
from
eb56591 to
ed6dd1c
Compare
Contributor
📝 WalkthroughWalkthroughThe PR replaces generic coverage collection with CBTS PY_START coverage, adds per-test runtime tracking and reporting utilities, gates instrumentation to eligible Jenkins stages, and publishes merged CBTS touch maps and HTML reports. ChangesCBTS coverage
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Jenkins
participant Pytest
participant sitecustomize
participant PyStartTracker
participant pystart_report.py
participant ArtifactStore
Jenkins->>Pytest: launch eligible stage with cbts_plugin
Pytest->>sitecustomize: initialize CBTS coverage
Pytest->>PyStartTracker: switch test context
PyStartTracker->>PyStartTracker: record PY_START entries
PyStartTracker->>ArtifactStore: save per-process SQLite files
Jenkins->>pystart_report.py: merge .cbtscov.*.sqlite files
pystart_report.py->>ArtifactStore: write touch DB and HTML report
Jenkins->>ArtifactStore: upload CBTS report archive
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…ge pipeline Enable per-test coverage only when JOB_NAME carries L0_PostMerge, instead of on every pipeline. Single-GPU stage selection is unchanged (the existing Perf / TensorRT / CPP / AutoDeploy / multi-GPU skip logic). Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Collaborator
Author
|
Description This PR adds CBTS Layer C — per-test function/class-level coverage capture for single-GPU L0 stages — and gates it to the official post-merge pipeline only. What it does
Post-merge execution-time overhead Measured on L0_MergeRequest #47070 (CBTS on) vs L0_PostMerge #2827 (no CBTS), zero product-code diff, per-case matched on l_e2e_time_ms (both sides PASSED), 41,232 matched cases. corresponding pipeline: https://prod.blsm.nvidia.com/sw-tensorrt-top-1/job/LLM/job/main/job/L0_MergeRequest_PR/47070/ Total matched-case time by stage (large-sample stages, ≥1500 cases — variance averaged out)
Test Coverage CI-infrastructure change with no product code path. Validated via:
|
Collaborator
Author
|
/bot run |
![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)
jenkins/scripts/cbts/coverage_utils/pystart_report.py (1)
29-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to all new Python functions.
The repository’s Python guidelines require parameter and return annotations, but every function in this new module is currently unannotated. Add annotations throughout the public and internal helpers.
As per coding guidelines, Python functions must always be annotated.
Also applies to: 35-50, 52-80, 83-107, 120-121, 124-189, 192-222, 225-303
🤖 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_utils/pystart_report.py` around lines 29 - 32, The new functions in pystart_report.py, including canon and all helpers in the noted ranges, lack required type annotations. Add parameter and return type annotations to every public and internal function, using appropriate concrete or generic types consistent with each function’s inputs and outputs.Source: Coding guidelines
jenkins/scripts/cbts/coverage_utils/make_coveragerc.sh (1)
35-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
make_coveragerc.shhelper
jenkins/L0_Test.groovyrenders.coveragercinline, and this script has no in-repo callers. Keeping both paths creates two sources of truth for the same template; remove the helper or route the pipeline through it consistently.🤖 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_utils/make_coveragerc.sh` around lines 35 - 41, Remove the unused make_coveragerc.sh helper and any associated references, since jenkins/L0_Test.groovy already renders .coveragerc inline; do not maintain two implementations of the same generation logic.
🤖 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 1245-1264: Add the standard NVIDIA copyright/license header to the
modified Groovy file, using the latest meaningful modification year (including
the current year if applicable), while preserving the existing pipeline logic.
- Around line 1245-1251: Update the coverage-file collection commands in the
CBTS coverage stage to remove the blanket “|| true” suppression and propagate or
explicitly report find/mv failures. Restrict the find operation to source result
locations while excluding cov/ so generated files are not reprocessed, and
ensure collection errors fail clearly before the fileCount check in the
surrounding coverage logic.
In `@jenkins/scripts/cbts/coverage_utils/coveragerc.template`:
- Line 1: The new coveragerc.template file is missing the required NVIDIA
copyright header. Prepend the standard repository NVIDIA copyright comment block
before the existing configuration comment, preserving the template’s valid
configparser syntax and the existing [run] settings.
In `@jenkins/scripts/cbts/coverage_utils/pystart_report.py`:
- Around line 120-121: The _page_name function generates colliding filenames by
replacing non-alphanumeric characters with underscores. Make page names
collision-free by appending a stable digest derived from the original file path,
or by using an injective encoding, while preserving the .html suffix and
updating any dependent links consistently.
- Around line 64-75: In the file-processing loop around `_iter_rows`, `canon`,
and the `con.executemany` calls, restrict exception recovery to input parsing
errors only (such as `OSError` and `ValueError`). Ensure `sqlite3.Error` raised
by destination-database writes propagates and aborts the merge instead of being
swallowed.
- Around line 246-249: Update the temporary database path logic in the report
entrypoint around merge_to_sqlite: when a.out_sqlite is unset, create a unique
path using tempfile.mkstemp() or NamedTemporaryFile(delete=False), close the
returned file descriptor, and track that exact path for cleanup after
processing. Preserve the user-provided output path unchanged and ensure only the
invocation-specific temporary database is removed.
- Around line 83-107: Update enumerate_defs() to recursively traverse all AST
child nodes, including functions and methods inside control-flow blocks and
nested classes, while preserving qualified names consistent with PY_START;
refactor visit() so it receives the file identifier explicitly instead of
closing over rel, resolving Ruff B023.
In `@jenkins/scripts/cbts/coverage_utils/README.md`:
- Around line 1-6: Add the standard mandated NVIDIA copyright and license header
at the beginning of the README, before the title and existing documentation
content.
In `@jenkins/scripts/slurm_run.sh`:
- Line 37: Fix the whitespace replacement for trtllmWhlPath and the matching
sanitization pattern later in jenkins/scripts/slurm_run.sh: use extended sed
regex with sed -E, escape the repetition operator appropriately, or replace it
with shell substitution so consecutive whitespace characters are converted to
underscores and the resulting paths are sanitized correctly.
---
Nitpick comments:
In `@jenkins/scripts/cbts/coverage_utils/make_coveragerc.sh`:
- Around line 35-41: Remove the unused make_coveragerc.sh helper and any
associated references, since jenkins/L0_Test.groovy already renders .coveragerc
inline; do not maintain two implementations of the same generation logic.
In `@jenkins/scripts/cbts/coverage_utils/pystart_report.py`:
- Around line 29-32: The new functions in pystart_report.py, including canon and
all helpers in the noted ranges, lack required type annotations. Add parameter
and return type annotations to every public and internal function, using
appropriate concrete or generic types consistent with each function’s inputs and
outputs.
🪄 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: 2acd0c0a-1d71-431a-aaff-472e166451c1
📒 Files selected for processing (10)
jenkins/L0_MergeRequest.groovyjenkins/L0_Test.groovyjenkins/scripts/cbts/coverage_utils/README.mdjenkins/scripts/cbts/coverage_utils/cbts_plugin.pyjenkins/scripts/cbts/coverage_utils/cbts_pystart.pyjenkins/scripts/cbts/coverage_utils/coveragerc.templatejenkins/scripts/cbts/coverage_utils/make_coveragerc.shjenkins/scripts/cbts/coverage_utils/pystart_report.pyjenkins/scripts/cbts/coverage_utils/sitecustomize.pyjenkins/scripts/slurm_run.sh
Comment on lines
+1245
to
1251
| // Collect the per-process PY_START data files from every stage's results tarball. | ||
| sh "find . -type f -name '.cbtscov.*.sqlite' -exec mv -t cov/ {} + || true" | ||
| sh "cd cov && ls -la" | ||
| def fileCount = sh(returnStdout: true, script: 'find cov -name ".cbtscov.*.sqlite" | wc -l').replaceAll("\\s","").toInteger() | ||
| if (fileCount == 0) { | ||
| echo "Test coverage is skipped because there is no test data file." | ||
| echo "CBTS coverage skipped: no PY_START data files." | ||
| return |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not hide coverage-file collection failures.
|| true masks every find/mv error. A partial move can still produce a report from incomplete data, while a total failure is reported as “no PY_START data files” and silently skipped. Fail or explicitly report collection errors, and exclude cov/ from the search to avoid reprocessing generated files.
🤖 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 1245 - 1251, Update the
coverage-file collection commands in the CBTS coverage stage to remove the
blanket “|| true” suppression and propagate or explicitly report find/mv
failures. Restrict the find operation to source result locations while excluding
cov/ so generated files are not reprocessed, and ensure collection errors fail
clearly before the fileCount check in the surrounding coverage logic.
Comment on lines
+1245
to
+1264
| // Collect the per-process PY_START data files from every stage's results tarball. | ||
| sh "find . -type f -name '.cbtscov.*.sqlite' -exec mv -t cov/ {} + || true" | ||
| sh "cd cov && ls -la" | ||
| def fileCount = sh(returnStdout: true, script: 'find cov -name ".cbtscov.*.sqlite" | wc -l').replaceAll("\\s","").toInteger() | ||
| if (fileCount == 0) { | ||
| echo "Test coverage is skipped because there is no test data file." | ||
| echo "CBTS coverage skipped: no PY_START data files." | ||
| return | ||
| } | ||
| trtllm_utils.llmExecStepWithRetry(pipeline, script: "pip3 install coverage") | ||
| sh "coverage --version" | ||
|
|
||
| sh "cp llm/examples/openai_triton/manual_plugin/fmha_triton.py llm/examples/openai_triton/plugin_autogen/" | ||
| def coverageConfigFile = "cov/.coveragerc" | ||
| // Merge into the indexed touch DB, a per-file HTML report, and the coverage rate. | ||
| sh """ | ||
| echo '[paths]' > ${coverageConfigFile} | ||
| echo 'source1=\n ${CUR_PATH}/llm/examples/\n */TensorRT-LLM/src/examples/' >> ${coverageConfigFile} | ||
| echo 'source2=\n ${CUR_PATH}/llm/tensorrt_llm/\n */tensorrt_llm/' >> ${coverageConfigFile} | ||
| cat ${coverageConfigFile} | ||
| python3 llm/jenkins/scripts/cbts/coverage_utils/pystart_report.py \ | ||
| --glob 'cov/.cbtscov.*.sqlite' \ | ||
| --out-sqlite cov/cbts_touchmap.sqlite \ | ||
| --out-dir cov/cbts_report \ | ||
| --source-root llm/tensorrt_llm | ||
| """ | ||
|
|
||
| sh "cd cov && coverage combine" | ||
| sh "cd cov && find . -type f" | ||
| sh "cd cov && coverage report -i" // -i: ignore errors. Ignore the error that the source code file cannot be found. | ||
| sh "cd cov && coverage html -d test_coverage_html -i" | ||
| trtllm_utils.uploadArtifacts("cov/test_coverage_html/*", "${UPLOAD_PATH}/test-results/coverage-report/") | ||
| echo "Test coverage report: https://urm.nvidia.com/artifactory/${UPLOAD_PATH}/test-results/coverage-report/index.html" | ||
| // Upload compressed only: the guardword scanner byte-matches raw files but not archives. | ||
| sh "cd cov && tar czf cbts_pystart_report.tar.gz cbts_touchmap.sqlite cbts_report" | ||
| trtllm_utils.uploadArtifacts("cov/cbts_pystart_report.tar.gz", "${UPLOAD_PATH}/cbts-coverage/") | ||
| echo "CBTS coverage (touch DB + report): https://urm.nvidia.com/artifactory/${UPLOAD_PATH}/cbts-coverage/cbts_pystart_report.tar.gz" |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required NVIDIA copyright header.
This modified Groovy file has no NVIDIA copyright/license header or current-year update.
As per coding guidelines, all modified files must contain an NVIDIA copyright header with the latest meaningful modification year.
🤖 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 1245 - 1264, Add the standard
NVIDIA copyright/license header to the modified Groovy file, using the latest
meaningful modification year (including the current year if applicable), while
preserving the existing pipeline logic.
Source: Coding guidelines
| @@ -0,0 +1,6 @@ | |||
| # CBTS runtime config template; only [run] source and data_file are consumed by sitecustomize.py. | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing NVIDIA copyright header on this new file.
Per repo guidelines, all new files must carry the NVIDIA copyright header. Since # lines are treated as comments by configparser, prepending the header block is safe and won't affect the [run] parsing done by sitecustomize.py.
As per coding guidelines: "Add an NVIDIA copyright header to all new files."
🤖 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_utils/coveragerc.template` at line 1, The new
coveragerc.template file is missing the required NVIDIA copyright header.
Prepend the standard repository NVIDIA copyright comment block before the
existing configuration comment, preserving the template’s valid configparser
syntax and the existing [run] settings.
Source: Coding guidelines
Comment on lines
+64
to
+75
| for fp in files: | ||
| batch = [] | ||
| try: | ||
| for test, file, qual in _iter_rows(fp): | ||
| batch.append((test, canon(file), qual)) | ||
| if len(batch) >= 20000: | ||
| con.executemany("INSERT OR IGNORE INTO touch VALUES (?, ?, ?)", batch) | ||
| batch = [] | ||
| if batch: | ||
| con.executemany("INSERT OR IGNORE INTO touch VALUES (?, ?, ?)", batch) | ||
| except (OSError, ValueError, sqlite3.Error): | ||
| continue |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not swallow destination-database failures.
The sqlite3.Error handler covers both malformed input files and writes to the output database. A locked, corrupt, or full destination can therefore be skipped as if it were bad input, producing and publishing an incomplete touch DB with misleading coverage rates. Scope recovery to input parsing and let destination write failures abort the merge.
🤖 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_utils/pystart_report.py` around lines 64 - 75,
In the file-processing loop around `_iter_rows`, `canon`, and the
`con.executemany` calls, restrict exception recovery to input parsing errors
only (such as `OSError` and `ValueError`). Ensure `sqlite3.Error` raised by
destination-database writes propagates and aborts the merge instead of being
swallowed.
Comment on lines
+83
to
+107
| def enumerate_defs(source_root): | ||
| """Return (coverable_files, funcs) for the coverage-rate denominator, matching what the tracker records.""" | ||
| funcs = set() | ||
| for dirpath, _dirs, names in os.walk(source_root): | ||
| for nm in names: | ||
| if not nm.endswith(".py"): | ||
| continue | ||
| full = os.path.join(dirpath, nm) | ||
| rel = canon(os.path.abspath(full)) | ||
| try: | ||
| tree = ast.parse(open(full, encoding="utf-8").read()) | ||
| except (OSError, SyntaxError): | ||
| continue | ||
|
|
||
| def visit(node, prefix): | ||
| for child in node.body: | ||
| if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)): | ||
| funcs.add((rel, prefix + child.name)) | ||
| elif isinstance(child, ast.ClassDef): | ||
| for m in child.body: | ||
| if isinstance(m, (ast.FunctionDef, ast.AsyncFunctionDef)): | ||
| funcs.add((rel, f"{prefix}{child.name}.{m.name}")) | ||
|
|
||
| visit(tree, "") | ||
| return {f for f, _q in funcs}, funcs |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'co_qualname|PY_START' jenkins/scripts/cbts/coverage_utils/cbts_pystart.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 1433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== file list ==\n'
git ls-files jenkins/scripts/cbts/coverage_utils
printf '\n== outline pystart_report.py ==\n'
ast-grep outline jenkins/scripts/cbts/coverage_utils/pystart_report.py --view expanded
printf '\n== relevant lines pystart_report.py ==\n'
sed -n '1,220p' jenkins/scripts/cbts/coverage_utils/pystart_report.py
printf '\n== relevant lines cbts_pystart.py ==\n'
sed -n '1,220p' jenkins/scripts/cbts/coverage_utils/cbts_pystart.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 14859
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' jenkins/scripts/cbts/coverage_utils/pystart_report.py
sed -n '1,220p' jenkins/scripts/cbts/coverage_utils/cbts_pystart.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 14013
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba jenkins/scripts/cbts/coverage_utils/pystart_report.py | sed -n '1,220p'
printf '\n---\n'
nl -ba jenkins/scripts/cbts/coverage_utils/cbts_pystart.py | sed -n '1,180p'Repository: NVIDIA/TensorRT-LLM
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== pystart_report.py ==\n'
nl -ba jenkins/scripts/cbts/coverage_utils/pystart_report.py | sed -n '1,220p'
printf '\n== references to all_funcs / enumerate_defs ==\n'
rg -n -C2 'all_funcs|enumerate_defs|funcs' jenkins/scripts/cbts/coverage_utils
printf '\n== tracker qualname filtering ==\n'
nl -ba jenkins/scripts/cbts/coverage_utils/cbts_pystart.py | sed -n '45,90p'Repository: NVIDIA/TensorRT-LLM
Length of output: 221
Walk the AST recursively. enumerate_defs() only visits direct module bodies and direct class bodies, so it drops functions/methods nested under control-flow blocks or nested classes even though PY_START still records them. That makes the coverage denominator too small. The inner visit() also closes over rel, which is what Ruff B023 is flagging.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 92-92: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(full, encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 Ruff (0.15.20)
[warning] 100-100: Function definition does not bind loop variable rel
(B023)
[warning] 104-104: Function definition does not bind loop variable rel
(B023)
🤖 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_utils/pystart_report.py` around lines 83 - 107,
Update enumerate_defs() to recursively traverse all AST child nodes, including
functions and methods inside control-flow blocks and nested classes, while
preserving qualified names consistent with PY_START; refactor visit() so it
receives the file identifier explicitly instead of closing over rel, resolving
Ruff B023.
Source: Linters/SAST tools
Comment on lines
+120
to
+121
| def _page_name(file): | ||
| return re.sub(r"[^A-Za-z0-9]", "_", file) + ".html" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make HTML page names collision-free.
Replacing every non-alphanumeric character with _ maps distinct files such as a-b.py and a_b.py to the same page. The later write overwrites the earlier report, and both index links show incorrect content. Append a stable digest of the original path or use an injective encoding.
🤖 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_utils/pystart_report.py` around lines 120 -
121, The _page_name function generates colliding filenames by replacing
non-alphanumeric characters with underscores. Make page names collision-free by
appending a stable digest derived from the original file path, or by using an
injective encoding, while preserving the .html suffix and updating any dependent
links consistently.
Comment on lines
+246
to
+249
| # Merge streams into a SQLite DB (bounded memory). Use --out-sqlite as that DB directly, else a temp file. | ||
| keep = a.out_sqlite is not None | ||
| db_path = a.out_sqlite if keep else os.path.join(tempfile.gettempdir(), "cbts_merge_tmp.sqlite") | ||
| con, n_data_files = merge_to_sqlite(a.glob, db_path) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use a unique temporary database per invocation.
When --out-sqlite is omitted, every process uses /tmp/cbts_merge_tmp.sqlite and removes it before opening it. Concurrent report jobs can delete or overwrite each other’s database and publish mixed or empty artifacts. Use tempfile.mkstemp()/NamedTemporaryFile(delete=False) and clean up that specific path.
🤖 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_utils/pystart_report.py` around lines 246 -
249, Update the temporary database path logic in the report entrypoint around
merge_to_sqlite: when a.out_sqlite is unset, create a unique path using
tempfile.mkstemp() or NamedTemporaryFile(delete=False), close the returned file
descriptor, and track that exact path for cleanup after processing. Preserve the
user-provided output path unchanged and ensure only the invocation-specific
temporary database is removed.
Comment on lines
+1
to
+6
| # CBTS Layer C — Coverage Utils | ||
|
|
||
| CI tooling that captures per-test **function/class-level** coverage (which product functions each | ||
| test entered), including subprocesses, on single-GPU L0 stages. CI infrastructure only — nothing | ||
| ships in the product wheel, and every file is a no-op unless `CBTS_COVERAGE_CONFIG` is set in the | ||
| environment. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required NVIDIA copyright header.
This new Markdown file does not include the mandated NVIDIA copyright/license header.
As per coding guidelines, all new files must include the NVIDIA copyright header.
🤖 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_utils/README.md` around lines 1 - 6, Add the
standard mandated NVIDIA copyright and license header at the beginning of the
README, before the title and existing documentation content.
Source: Coding guidelines
| # get trtllm wheel path and add to pytest command | ||
| # Wheel path for the CBTS .coveragerc @TRTLLM_WHEEL_PATH@ substitution below. | ||
| trtllmWhlPath=$(pip3 show tensorrt_llm | grep Location | cut -d ' ' -f 2) | ||
| trtllmWhlPath=$(echo "$trtllmWhlPath" | sed 's/[[:space:]]+/_/g') |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- jenkins/scripts/slurm_run.sh (selected lines) ---\n'
nl -ba jenkins/scripts/slurm_run.sh | sed -n '1,140p'
printf '\n--- grep for similar sed expressions ---\n'
rg -n "sed 's/\[\[:space:\]\]\+/_/g'" jenkins/scripts/slurm_run.sh || true
printf '\n--- probe sed BRE vs ERE behavior ---\n'
python3 - <<'PY'
import subprocess, textwrap
tests = [
("BRE default", ["sed", "s/[[:space:]]+/_/g"], "a b"),
("ERE", ["sed", "-E", "s/[[:space:]]+/_/g"], "a b"),
("BRE escaped plus", ["sed", "s/[[:space:]]\\+/_/g"], "a b"),
]
for name, cmd, inp in tests:
p = subprocess.run(cmd, input=inp.encode(), stdout=subprocess.PIPE, stderr=subprocess.PIPE)
print(name, "->", p.stdout.decode().strip(), "| rc=", p.returncode, "| stderr=", p.stderr.decode().strip())
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding context and test the regex semantics directly.
nl -ba jenkins/scripts/slurm_run.sh | sed -n '30,60p'
python3 - <<'PY'
import subprocess
cases = [
("default BRE", ["sed", "s/[[:space:]]+/_/g"], "a b"),
("ERE", ["sed", "-E", "s/[[:space:]]+/_/g"], "a b"),
("BRE escaped +", ["sed", "s/[[:space:]]\\+/_/g"], "a b"),
]
for label, cmd, data in cases:
p = subprocess.run(cmd, input=data.encode(), stdout=subprocess.PIPE, stderr=subprocess.PIPE)
print(f"{label}: {p.stdout.decode().strip()!r} rc={p.returncode} stderr={p.stderr.decode().strip()!r}")
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- jenkins/scripts/slurm_run.sh (lines 30-60) ---'
sed -n '30,60p' jenkins/scripts/slurm_run.sh
echo
echo '--- matching sed expressions ---'
grep -n "sed 's/\[\[:space:\]\]\+/_/g'" jenkins/scripts/slurm_run.sh || true
echo
echo '--- regex behavior probe ---'
python3 - <<'PY'
import subprocess
cases = [
("default BRE", ["sed", "s/[[:space:]]+/_/g"], "a b"),
("ERE", ["sed", "-E", "s/[[:space:]]+/_/g"], "a b"),
("BRE escaped +", ["sed", "s/[[:space:]]\\+/_/g"], "a b"),
]
for label, cmd, data in cases:
p = subprocess.run(cmd, input=data.encode(), stdout=subprocess.PIPE, stderr=subprocess.PIPE)
print(f"{label}: stdout={p.stdout.decode().rstrip()!r} rc={p.returncode} stderr={p.stderr.decode().rstrip()!r}")
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 1822
Fix the whitespace-collapse regex sed is using basic regex here, so [[:space:]]+ treats + literally and leaves spaces unchanged. That makes the wheel-path and LD_LIBRARY_PATH sanitization ineffective; switch to sed -E, escape +, or use shell substitution. Same pattern appears later in the file.
🧰 Tools
🪛 Shellcheck (0.11.0)
[style] 37-37: See if you can use ${variable//search/replace} instead.
(SC2001)
🤖 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/slurm_run.sh` at line 37, Fix the whitespace replacement for
trtllmWhlPath and the matching sanitization pattern later in
jenkins/scripts/slurm_run.sh: use extended sed regex with sed -E, escape the
repetition operator appropriately, or replace it with shell substitution so
consecutive whitespace characters are converted to underscores and the resulting
paths are sanitized correctly.
Source: Linters/SAST tools
Collaborator
|
PR_Github #58645 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #58645 [ run ] completed with state
|
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
2 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
Bug Fixes
Documentation
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.