[TRTLLM-9228][infra] Verify thirdparty C++ process #9367
Conversation
📝 WalkthroughWalkthroughTwo new test modules are introduced to enforce third-party dependency management policies: one scans CMake files for unauthorized third-party integrations outside the designated 3rdparty directory, and another validates git submodule declarations. Both tests are integrated into the repository's test suite. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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)
3rdparty/test_git_modules.py (2)
1-5: Align documentation paths and fix minor typosThere are a few small consistency/clarity issues worth fixing:
- The process doc is referred to as both
cpp/3rdparty/cpp-thirdparty.mdand3rdparty/cpp-thirdparty.md. Please pick the canonical path (per the PR description it sounds like3rdparty/cpp-thirdparty.md) and use it consistently in:
- The module docstring (lines 1‑5).
- The ALLOWLIST comment (lines 16‑20).
- The logger error message (lines 69‑73, which currently mentions
cpp/3rdparty/...).- Typo at line 17:
sobmodules→submodules.- Slight grammar improvement in the docstring would make it read better, e.g. “This script audits the
.gitmodulesfile…” instead of “audit”.These are all non‑functional but will reduce confusion for future contributors.
Also applies to: 16-20, 69-74
90-100: Use the actual module docstring inargparse.ArgumentParserIn
main(), the parser is constructed withdescription="__doc__", which sets the description to the literal string"__doc__"instead of the module’s docstring. If you intended to reuse the top‑level docstring, this should be:-def main(): - parser = argparse.ArgumentParser(description="__doc__") +def main(): + parser = argparse.ArgumentParser(description=__doc__)Same pattern also appears in
3rdparty/test_cmake_third_party.py, so it’s worth updating both for consistency.3rdparty/test_cmake_third_party.py (3)
1-5: Make process doc references consistentThe module docstring points to
3rdparty/README.md, while the error log message points to3rdparty/cpp-thirdparty.md. Since this script is specifically about third‑party C++ integration, consider standardizing on the single, canonical doc (likely3rdparty/cpp-thirdparty.md, as mentioned in the PR description and the git‑modules test).A small tweak like:
-This script searches for cmake function invocations that might indicate -the addition of new third-party dependencies outside of the intended -process (3rdparty/README.md). +This script searches for CMake function invocations that might indicate +the addition of new third-party dependencies outside of the intended +process (see 3rdparty/cpp-thirdparty.md).would make the link between the policy and this enforcement script clearer.
Also applies to: 130-135
89-107: Consider ignoring commented lines to reduce false positives
yield_potential_thirdpartyflags any line containingFetchContent_DeclareorExternalProject_Add, regardless of whether it’s active code or a comment. That means just documenting the function names in comments will show up as violations.If you want to avoid this kind of false positive while keeping the logic simple, you could skip lines that are clearly commented out:
- with fullpath.open("r", encoding="utf-8") as infile: - for lineno, line in enumerate(infile): + with fullpath.open("r", encoding="utf-8") as infile: + for lineno, line in enumerate(infile): lineno += 1 # Make line numbers 1-based - - if "FetchContent_Declare" in line: + stripped = line.lstrip() + if stripped.startswith("#"): + continue + + if "FetchContent_Declare" in stripped: @@ - if "ExternalProject_Add" in line: + if "ExternalProject_Add" in stripped:Not mandatory, but it may make the check friendlier in practice by only flagging actual build logic.
149-159: Reuse the module docstring in the CLI description (same as git‑modules test)As in
test_git_modules.py,main()here usesdescription="__doc__", which sets the parser description to the literal string instead of the module’s docstring.Recommend changing to:
-def main(): - parser = argparse.ArgumentParser(description="__doc__") +def main(): + parser = argparse.ArgumentParser(description=__doc__)This keeps the CLI help text in sync with the module documentation and aligns both scripts.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
3rdparty/test_cmake_third_party.py(1 hunks)3rdparty/test_git_modules.py(1 hunks)tests/integration/test_lists/test-db/l0_a10.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-09T09:40:45.658Z
Learnt from: fredricz-20070104
Repo: NVIDIA/TensorRT-LLM PR: 7645
File: tests/integration/test_lists/qa/llm_function_core.txt:648-648
Timestamp: 2025-09-09T09:40:45.658Z
Learning: In TensorRT-LLM test lists, it's common and intentional for the same test to appear in multiple test list files when they serve different purposes (e.g., llm_function_core.txt for comprehensive core functionality testing and llm_function_core_sanity.txt for quick sanity checks). This duplication allows tests to be run in different testing contexts.
Applied to files:
tests/integration/test_lists/test-db/l0_a10.yml
📚 Learning: 2025-09-17T02:48:52.732Z
Learnt from: tongyuantongyu
Repo: NVIDIA/TensorRT-LLM PR: 7781
File: tests/integration/test_lists/waives.txt:313-313
Timestamp: 2025-09-17T02:48:52.732Z
Learning: In TensorRT-LLM, `tests/integration/test_lists/waives.txt` is specifically for waiving/skipping tests, while other test list files like those in `test-db/` and `qa/` directories are for different test execution contexts (pre-merge, post-merge, QA tests). The same test appearing in both waives.txt and execution list files is intentional - the test is part of test suites but will be skipped due to the waiver.
Applied to files:
tests/integration/test_lists/test-db/l0_a10.yml
📚 Learning: 2025-08-06T13:58:07.506Z
Learnt from: galagam
Repo: NVIDIA/TensorRT-LLM PR: 6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.
Applied to files:
tests/integration/test_lists/test-db/l0_a10.yml
🧬 Code graph analysis (2)
3rdparty/test_git_modules.py (1)
3rdparty/test_cmake_third_party.py (1)
main(149-159)
3rdparty/test_cmake_third_party.py (1)
3rdparty/test_git_modules.py (1)
main(90-100)
🪛 Ruff (0.14.5)
3rdparty/test_git_modules.py
31-31: Avoid specifying long messages outside the exception class
(TRY003)
35-35: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (3)
tests/integration/test_lists/test-db/l0_a10.yml (1)
79-81: Third‑party policy checks correctly integrated into l0_a10 pre‑merge listThe new CPU‑only third‑party policy tests are wired into the existing PyTorch pre‑merge job cleanly, with clear commenting and consistent formatting. No issues from the test‑list perspective.
3rdparty/test_git_modules.py (1)
79-87: Pytest entrypoint for.gitmodulescheck looks solidThe pytest wrapper cleanly resolves the repo root via
Path(__file__).parents[1] / ".gitmodules"and asserts on the checker’s return code. This aligns well with howcheck_modules_fileis designed and should integrate smoothly into the test suite.3rdparty/test_cmake_third_party.py (1)
139-147: Pytest wrapper for CMake third‑party violations looks goodThe
TestThirdPartyInCmakewrapper mirrors the git‑modules test pattern: it anchors on the repo root viaPath(__file__).parents[1], callscheck_sources, and asserts a zero exit code. This is a clean integration point for the policy check into the test suite.
cheshirekow
force-pushed
the
test-prevent-thirdparty-process
branch
from
76193b4 to
4f07f75
Compare
This change adds two tests to help ensure that thirdparty C++ code is integrated according to the process descripted in 3rdparty/cpp-thirdparty.md. The desired process requires folks to use FetchContent_Declare in 3rdparty/CMakeLists.txt. The two tests added here search the following deviations of the desired process: 1. uses of FetchContent_Declare or ExternalProject_Add outside of 3rdparty/CMakeLists.txt 2. new git-submodules added to the repo Both scripts have the ability to allow-list exemptions if there are any cases in the future where a deviation is warranted. The scripts live in 3rdparty/* where CODEOWNERS ensures that process reviewers will be required on any new exemptions added to the allowlists. Signed-off-by: Josh Bialkowski <[email protected]>
cheshirekow
force-pushed
the
test-prevent-thirdparty-process
branch
from
4f07f75 to
b9d7489
Compare
|
/bot run |
|
PR_Github #25598 [ run ] triggered by Bot. Commit: |
|
PR_Github #25598 [ run ] completed with state |
| - unittest/llmapi/apps/test_tool_parsers.py | ||
| - unittest/llmapi/apps/test_harmony_channel_validation.py | ||
| # third-party policy checks CPU-only | ||
| - 3rdparty/test_cmake_third_party.py |
There was a problem hiding this comment.
Will the tests run properly in the pipeline? I think all our test defs are under root_dir/tests/.
The pipeline failed at the test list validation stage.
3 participants
Description
This change adds two tests to help ensure that thirdparty C++ code is integrated according to the process descripted in
3rdparty/cpp-thirdparty.md. The desired process requires folks to use FetchContent_Declare in 3rdparty/CMakeLists.txt. The two tests added here search the following deviations of the desired process:
Both scripts have the ability to allow-list exemptions if there are any cases in the future where a deviation is warranted. The scripts live in 3rdparty/* where CODEOWNERS ensures that process reviewers will be required on any new exemptions added to the allowlists.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
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
/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...Provide a user friendly way for developers to interact with a Jenkins server.
Run
/bot [-h|--help]to print this help message.See details below for each supported subcommand.
run [--reuse-test (optional)pipeline-id --disable-fail-fast --skip-test --stage-list "A10-PyTorch-1, xxx" --gpu-type "A30, H100_PCIe" --test-backend "pytorch, cpp" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" --detailed-log --debug(experimental)]Launch build/test pipelines. All previously running jobs will be killed.
--reuse-test (optional)pipeline-id(OPTIONAL) : Allow the new pipeline to reuse build artifacts and skip successful test stages from a specified pipeline or the last pipeline if no pipeline-id is indicated. If the Git commit ID has changed, this option will be always ignored. The DEFAULT behavior of the bot is to reuse build artifacts and successful test results from the last pipeline.--disable-reuse-test(OPTIONAL) : Explicitly prevent the pipeline from reusing build artifacts and skipping successful test stages from a previous pipeline. Ensure that all builds and tests are run regardless of previous successes.--disable-fail-fast(OPTIONAL) : Disable fail fast on build/tests/infra failures.--skip-test(OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.--stage-list "A10-PyTorch-1, xxx"(OPTIONAL) : Only run the specified test stages. Examples: "A10-PyTorch-1, xxx". Note: Does NOT update GitHub check status.--gpu-type "A30, H100_PCIe"(OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.--test-backend "pytorch, cpp"(OPTIONAL) : Skip test stages which don't match the specified backends. Only support [pytorch, cpp, tensorrt, triton]. Examples: "pytorch, cpp" (does not run test stages with tensorrt or triton backend). Note: Does NOT update GitHub pipeline status.--only-multi-gpu-test(OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.--disable-multi-gpu-test(OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.--add-multi-gpu-test(OPTIONAL) : Force run the multi-GPU tests in addition to running L0 pre-merge pipeline.--post-merge(OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx"(OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx".--detailed-log(OPTIONAL) : Enable flushing out all logs to the Jenkins console. This will significantly increase the log volume and may slow down the job.--debug(OPTIONAL) : Experimental feature. Enable access to the CI container for debugging purpose. Note: Specify exactly one stage in thestage-listparameter to access the appropriate container environment. Note: Does NOT update GitHub check status.For guidance on mapping tests to stage names, see
docs/source/reference/ci-overview.mdand the
scripts/test_to_stage_mapping.pyhelper.kill
killKill all running builds associated with pull request.
skip
skip --comment COMMENTSkip testing for latest commit on pull request.
--comment "Reason for skipping build/test"is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.reuse-pipeline
reuse-pipelineReuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.