[TRTLLM-9197][infra] Move thirdparty stuff to it's own listfile #8986
Conversation
cheshirekow
requested review from
Shixiaowei02,
SimengLiu-nv,
laikhtewari and
schetlur-nv
📝 WalkthroughWalkthroughThis PR migrates dependency management from git submodules to CMake's FetchContent mechanism. All submodule entries are removed from .gitmodules, a new 3rdparty/CMakeLists.txt declares dependencies via FetchContent, and downstream CMakeLists.txt files are updated to reference dependencies from build-directory paths instead of source-tree submodule paths. Changes
Sequence DiagramsequenceDiagram
participant CMake as CMake Build
participant RootCMake as cpp/CMakeLists.txt
participant ThirdParty as 3rdparty/CMakeLists.txt
participant FetchContent as FetchContent
participant TargetCMake as Target CMakeLists.txt<br/>(e.g., kernels/)
CMake->>RootCMake: Configure project
RootCMake->>ThirdParty: add_subdirectory(3rdparty)
ThirdParty->>FetchContent: FetchContent_Declare(...) for each lib
ThirdParty->>FetchContent: FetchContent_MakeAvailable(cutlass, json, etc.)
FetchContent-->>ThirdParty: Download to ${CMAKE_BINARY_DIR}/_deps/
RootCMake->>RootCMake: FetchContent_MakeAvailable(...) additional libs
RootCMake->>TargetCMake: include(CMakeLists.txt)
TargetCMake->>TargetCMake: Reference ${CMAKE_BINARY_DIR}/_deps/lib-src/
TargetCMake-->>CMake: Resolved include paths, sources
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cpp/tensorrt_llm/flash_mla/CMakeLists.txt (1)
14-19: Update error messages to reflect CMake FetchContent-based dependency fetching; verify nested submodule initialization.Lines 17 and 25 contain outdated guidance referencing
git submodule update, which is not applicable when dependencies are fetched via CMake's FetchContent. Users cannot execute git commands in the CMake-downloaded directory at${CMAKE_BINARY_DIR}/_deps/flashmla-src.Additionally, the code at line 22 checks for nested cutlass at
${FLASH_MLA_SOURCE_DIR}/csrc/cutlass/include, but CMake's FetchContent does not automatically initialize nested git submodules within the cloned repository. Verify that flashmla's nested dependencies (cutlass, fmt) are properly initialized during the FetchContent fetch phase, or update error messages to indicate configuration failures rather than manual git operations.cpp/tensorrt_llm/deep_gemm/CMakeLists.txt (1)
14-19: Update error messages and/or configure FetchContent to handle nested submodules.The deepgemm FetchContent declaration in 3rdparty/CMakeLists.txt (lines 39-44) lacks the
GIT_SUBMODULESoption. FetchContent historically has had inconsistent handling of git submodules across CMake versions and does not guarantee submodule initialization without explicit configuration.Meanwhile, deep_gemm/CMakeLists.txt expects nested dependencies (cutlass at line 22, fmt at line 112) that will not be fetched without proper configuration, yet error messages at lines 17 and 25 direct users to run
git submodule update --init --recursive—commands that cannot work on a CMake-fetched resource.Fix:
- Add
GIT_SUBMODULES_RECURSE ONto the deepgemm FetchContent_Declare in 3rdparty/CMakeLists.txt, or- Update error messages in deep_gemm/CMakeLists.txt (lines 14-19, 22-26) to reflect CMake-based fetching and remove misleading git commands.
cpp/CMakeLists.txt (2)
265-283: Critical: NVTX include path mismatch between conditional fetch and unconditional inclusion.FetchContent_MakeAvailable(nvtx) is called conditionally only when NVTX_DISABLE is false (lines 265–267), but the nvtx include directory is added unconditionally at line 280. Since NVTX_DISABLE defaults to ON (line 37), the nvtx source won't be fetched, but the build will attempt to reference
${CMAKE_BINARY_DIR}/_deps/nvtx-src/include, which won't exist.The include directory at line 280 must be guarded with the same conditional as the FetchContent_MakeAvailable call.
-include_directories( - SYSTEM - ${CUDAToolkit_INCLUDE_DIRS} - ${CUDAToolkit_INCLUDE_DIRS}/cccl - ${CUDNN_ROOT_DIR}/include - $<TARGET_PROPERTY:TensorRT::NvInfer,INTERFACE_INCLUDE_DIRECTORIES> +if(NOT NVTX_DISABLE) + include_directories( + SYSTEM + ${CMAKE_BINARY_DIR}/_deps/nvtx-src/include) +endif() + +include_directories( + SYSTEM + ${CUDAToolkit_INCLUDE_DIRS} + ${CUDAToolkit_INCLUDE_DIRS}/cccl + ${CUDNN_ROOT_DIR}/include + $<TARGET_PROPERTY:TensorRT::NvInfer,INTERFACE_INCLUDE_DIRECTORIES> ${CMAKE_BINARY_DIR}/_deps/nvtx-src/include ${CMAKE_BINARY_DIR}/_deps/cutlass-src/include(Alternatively, always call
FetchContent_MakeAvailable(nvtx)unconditionally, not just when NVTX is enabled, if the headers are needed regardless of the NVTX_DISABLE flag.)
496-498: Critical: Line 497 references submodule path being removed in this PR.Line 497 still references
${3RDPARTY_DIR}/NVTX/include, which is the old submodule path. Per the PR description, submodules are being removed and replaced with FetchContent. The nvtx path should be updated to use the FetchContent directory:${CMAKE_BINARY_DIR}/_deps/nvtx-src/include.- set(nvtx3_dir ${3RDPARTY_DIR}/NVTX/include) + set(nvtx3_dir ${CMAKE_BINARY_DIR}/_deps/nvtx-src/include)
🧹 Nitpick comments (2)
cpp/CMakeLists.txt (2)
550-559: Avoid in-place file modification for build-time fetch_rapids.cmake substitution.The code modifies
fetch_rapids.cmakein place (lines 550–559) when GITHUB_MIRROR is set. This pattern is fragile:
- Multiple concurrent CMake invocations could race on the file write.
- Repeated builds may behave unexpectedly if the substitution persists.
Consider using CMake's configure-time variable substitution (e.g., via
file(CONFIGURE_FILE ...)or custom wrapper) or applying patches at configure time rather than modifying fetched sources in place.
574-574: Minor: Message formatting inconsistency.Line 574 has a spacing/formatting issue in the message string.
- message("ucxx build:" ${UCXX_BUILD_OUTPUT}) + message("ucxx build: ${UCXX_BUILD_OUTPUT}")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.gitmodules(0 hunks)3rdparty/CMakeLists.txt(1 hunks)3rdparty/DeepGEMM(0 hunks)3rdparty/NVTX(0 hunks)3rdparty/cppzmq(0 hunks)3rdparty/cutlass(0 hunks)3rdparty/cxxopts(0 hunks)3rdparty/flash-mla(0 hunks)3rdparty/json(0 hunks)3rdparty/nanobind(0 hunks)3rdparty/pybind11(0 hunks)3rdparty/ucxx(0 hunks)3rdparty/xgrammar(0 hunks)benchmarks/cpp/CMakeLists.txt(1 hunks)cpp/CMakeLists.txt(4 hunks)cpp/kernels/xqa/CMakeLists.txt(0 hunks)cpp/micro_benchmarks/CMakeLists.txt(0 hunks)cpp/tensorrt_llm/batch_manager/CMakeLists.txt(2 hunks)cpp/tensorrt_llm/deep_ep/CMakeLists.txt(1 hunks)cpp/tensorrt_llm/deep_gemm/CMakeLists.txt(1 hunks)cpp/tensorrt_llm/executor/cache_transmission/ucx_utils/CMakeLists.txt(1 hunks)cpp/tensorrt_llm/flash_mla/CMakeLists.txt(1 hunks)cpp/tensorrt_llm/kernels/cutlass_kernels/CMakeLists.txt(2 hunks)cpp/tests/CMakeLists.txt(1 hunks)examples/cpp/executor/CMakeLists.txt(2 hunks)triton_backend/inflight_batcher_llm/CMakeLists.txt(2 hunks)
💤 Files with no reviewable changes (14)
- 3rdparty/cutlass
- 3rdparty/json
- cpp/micro_benchmarks/CMakeLists.txt
- 3rdparty/DeepGEMM
- 3rdparty/NVTX
- 3rdparty/xgrammar
- 3rdparty/flash-mla
- 3rdparty/nanobind
- .gitmodules
- 3rdparty/cxxopts
- 3rdparty/ucxx
- 3rdparty/pybind11
- 3rdparty/cppzmq
- cpp/kernels/xqa/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: xinhe-nv
Repo: NVIDIA/TensorRT-LLM PR: 8534
File: scripts/format_test_list.py:1-6
Timestamp: 2025-10-22T06:53:47.017Z
Learning: The file `scripts/format_test_list.py` in the TensorRT-LLM repository does not require the NVIDIA Apache-2.0 copyright header.
Learnt from: tomeras91
Repo: NVIDIA/TensorRT-LLM PR: 6796
File: tensorrt_llm/_torch/pyexecutor/mamba_cache_manager.py:201-201
Timestamp: 2025-08-11T13:58:57.678Z
Learning: When a PR is intended for pure code movement/refactoring (moving code from one file to another), avoid suggesting code changes including linting fixes, as the goal is to keep the code exactly as it was in the original location.
📚 Learning: 2025-08-21T21:48:35.135Z
Learnt from: djns99
Repo: NVIDIA/TensorRT-LLM PR: 7104
File: cpp/tensorrt_llm/cutlass_extensions/include/cutlass_extensions/epilogue/fusion/sm90_visitor_scatter.hpp:399-417
Timestamp: 2025-08-21T21:48:35.135Z
Learning: CUTLASS extensions in TensorRT-LLM (located under cpp/tensorrt_llm/cutlass_extensions/) are designed to integrate with and extend functionality in the external CUTLASS repository. When analyzing these extensions, their consumers and functionality wiring may exist in the CUTLASS codebase rather than within TensorRT-LLM itself.
Applied to files:
triton_backend/inflight_batcher_llm/CMakeLists.txtcpp/tests/CMakeLists.txtcpp/tensorrt_llm/kernels/cutlass_kernels/CMakeLists.txt
📚 Learning: 2025-08-08T05:06:31.596Z
Learnt from: sklevtsov-nvidia
Repo: NVIDIA/TensorRT-LLM PR: 3294
File: cpp/tensorrt_llm/cutlass_extensions/include/cutlass_extensions/epilogue/fusion/sm90_visitor_scatter.hpp:36-36
Timestamp: 2025-08-08T05:06:31.596Z
Learning: CUTLASS extension files (under cpp/tensorrt_llm/cutlass_extensions/) follow CUTLASS coding style conventions, including using #pragma once instead of TRTLLM_ prefixed header guards, even though they are .hpp files.
Applied to files:
triton_backend/inflight_batcher_llm/CMakeLists.txtcpp/tests/CMakeLists.txtcpp/tensorrt_llm/kernels/cutlass_kernels/CMakeLists.txtcpp/CMakeLists.txt
📚 Learning: 2025-08-22T01:54:35.850Z
Learnt from: djns99
Repo: NVIDIA/TensorRT-LLM PR: 7104
File: cpp/tensorrt_llm/kernels/cutlass_kernels/include/moe_kernels.h:999-1000
Timestamp: 2025-08-22T01:54:35.850Z
Learning: The `internal_cutlass_kernels` directory in TensorRT-LLM is a mirror of an internal NVIDIA repository and maintains its own implementation and API that may diverge from the public `cutlass_kernels` version. API inconsistencies between these two directories are intentional and by design, not bugs to be fixed.
Applied to files:
triton_backend/inflight_batcher_llm/CMakeLists.txtcpp/tensorrt_llm/kernels/cutlass_kernels/CMakeLists.txt
📚 Learning: 2025-09-23T15:01:00.070Z
Learnt from: nv-lschneider
Repo: NVIDIA/TensorRT-LLM PR: 7910
File: cpp/tensorrt_llm/kernels/nccl_device/config.cu:15-17
Timestamp: 2025-09-23T15:01:00.070Z
Learning: In TensorRT-LLM NCCL device kernels, the <sstream> header is not needed as an explicit include in config.cu because it's provided transitively through other headers. Local compilation testing confirms this works without the explicit include.
Applied to files:
triton_backend/inflight_batcher_llm/CMakeLists.txtcpp/tensorrt_llm/kernels/cutlass_kernels/CMakeLists.txtcpp/tensorrt_llm/deep_gemm/CMakeLists.txtcpp/tensorrt_llm/executor/cache_transmission/ucx_utils/CMakeLists.txt
📚 Learning: 2025-09-23T15:01:00.070Z
Learnt from: nv-lschneider
Repo: NVIDIA/TensorRT-LLM PR: 7910
File: cpp/tensorrt_llm/kernels/nccl_device/config.cu:15-17
Timestamp: 2025-09-23T15:01:00.070Z
Learning: In TensorRT-LLM NCCL device kernels (cpp/tensorrt_llm/kernels/nccl_device/config.cu), std::ostringstream is used but <sstream> doesn't need to be explicitly included because it's provided transitively through other headers like tensorrt_llm/common/cudaUtils.h or config.h. Local compilation testing confirms this works without the explicit include.
Applied to files:
cpp/tensorrt_llm/kernels/cutlass_kernels/CMakeLists.txtcpp/tensorrt_llm/executor/cache_transmission/ucx_utils/CMakeLists.txtexamples/cpp/executor/CMakeLists.txt
📚 Learning: 2025-08-18T09:08:07.687Z
Learnt from: tongyuantongyu
Repo: NVIDIA/TensorRT-LLM PR: 6984
File: cpp/tensorrt_llm/CMakeLists.txt:297-299
Timestamp: 2025-08-18T09:08:07.687Z
Learning: In the TensorRT-LLM project, artifacts are manually copied rather than installed via `cmake --install`, so INSTALL_RPATH properties are not needed - only BUILD_RPATH affects the final artifacts.
Applied to files:
cpp/tensorrt_llm/kernels/cutlass_kernels/CMakeLists.txtcpp/tensorrt_llm/deep_gemm/CMakeLists.txtcpp/tensorrt_llm/executor/cache_transmission/ucx_utils/CMakeLists.txt3rdparty/CMakeLists.txtcpp/CMakeLists.txtexamples/cpp/executor/CMakeLists.txt
📚 Learning: 2025-08-20T15:04:42.885Z
Learnt from: dbari
Repo: NVIDIA/TensorRT-LLM PR: 7095
File: docker/Dockerfile.multi:168-168
Timestamp: 2025-08-20T15:04:42.885Z
Learning: In docker/Dockerfile.multi, wildcard COPY for benchmarks (${CPP_BUILD_DIR}/benchmarks/*Benchmark) is intentionally used instead of directory copy because the benchmarks directory contains various other build artifacts during C++ builds, and only specific benchmark executables should be copied to the final image.
Applied to files:
benchmarks/cpp/CMakeLists.txt
📚 Learning: 2025-09-16T09:30:09.716Z
Learnt from: tongyuantongyu
Repo: NVIDIA/TensorRT-LLM PR: 7763
File: cpp/tensorrt_llm/CMakeLists.txt:297-301
Timestamp: 2025-09-16T09:30:09.716Z
Learning: In the TensorRT-LLM project, NCCL libraries are loaded earlier by PyTorch libraries or the bindings library, so the main shared library doesn't need NCCL paths in its RPATH - the libraries will already be available in the process address space when needed.
Applied to files:
cpp/CMakeLists.txtexamples/cpp/executor/CMakeLists.txt
📚 Learning: 2025-09-23T15:13:48.819Z
Learnt from: nv-lschneider
Repo: NVIDIA/TensorRT-LLM PR: 7910
File: cpp/tensorrt_llm/kernels/nccl_device/multimem.h:20-30
Timestamp: 2025-09-23T15:13:48.819Z
Learning: TRT-LLM targets modern CUDA toolkits that support FP8 datatypes, so cuda_fp8.h can be included unconditionally without version guards in TRT-LLM code.
Applied to files:
cpp/CMakeLists.txtexamples/cpp/executor/CMakeLists.txt
📚 Learning: 2025-08-01T15:14:45.673Z
Learnt from: yibinl-nvidia
Repo: NVIDIA/TensorRT-LLM PR: 6506
File: examples/models/core/mixtral/requirements.txt:3-3
Timestamp: 2025-08-01T15:14:45.673Z
Learning: In TensorRT-LLM, examples directory can have different dependency versions than the root requirements.txt file. Version conflicts between root and examples dependencies are acceptable because examples are designed to be standalone and self-contained.
Applied to files:
examples/cpp/executor/CMakeLists.txt
📚 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:
examples/cpp/executor/CMakeLists.txt
⏰ 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). (2)
- GitHub Check: Pre-commit Check
- GitHub Check: Check PR Checklist Resolution
🔇 Additional comments (13)
cpp/tensorrt_llm/executor/cache_transmission/ucx_utils/CMakeLists.txt (1)
8-8: LGTM! The include path update correctly aligns with the new FetchContent-based dependency layout.cpp/tensorrt_llm/flash_mla/CMakeLists.txt (1)
22-26: Verify nested submodule dependencies are available. This check assumes cutlass and fmt are available within the flashmla source tree. Confirm that the FetchContent declaration in 3rdparty/CMakeLists.txt ensures these nested dependencies are initialized before this CMakeLists.txt is processed.examples/cpp/executor/CMakeLists.txt (2)
94-95: Verify FetchContent initialization order. Lines 94–95 add the 3rdparty subdirectory and immediately callFetchContent_MakeAvailable(cxxopts). Confirm that:
- The 3rdparty/CMakeLists.txt contains
FetchContent_Declare(cxxopts ...)(but not MakeAvailable).- This ensure that line 95 successfully populates cxxopts before the conditional guard at line 130.
130-132: Verify conditional guard logic. TheNOT TARGET cxxopts::cxxoptsguard is a safe fallback, but ifFetchContent_MakeAvailable(cxxopts)at line 95 succeeds, the condition at line 130 should be false and this block skipped. Verify that the FetchContent declaration in 3rdparty/CMakeLists.txt defines the target correctly so the guard works as intended.triton_backend/inflight_batcher_llm/CMakeLists.txt (1)
212-221: LGTM! The cutlass_source_dir variable correctly uses the new binary-dir path. Verify that the 3rdparty/CMakeLists.txt declares and makes available the cutlass dependency before this file is processed.Confirm that the parent CMakeLists that includes this file also includes 3rdparty/CMakeLists.txt and calls
FetchContent_MakeAvailable(cutlass).benchmarks/cpp/CMakeLists.txt (1)
22-25: LGTM! The conditional logic correctly provides a fallback add_subdirectory if the cxxopts target is not already available. This pattern is safe and flexible.cpp/tensorrt_llm/kernels/cutlass_kernels/CMakeLists.txt (2)
30-40: LGTM! The cutlass_source_dir variable is properly introduced and consistently used throughout. The error message at line 39 is also correctly updated to reference the new location.
222-222: LGTM! The include path is correctly updated to use the new cutlass source directory.cpp/tests/CMakeLists.txt (1)
22-27: LGTM! The cutlass_source_dir variable is introduced and the include paths are correctly updated to reference the new location.3rdparty/CMakeLists.txt (1)
1-114: Centralized third-party dependencies declarations look well-structured.The FetchContent_Declare blocks are properly formatted with appropriate GIT_SHALLOW optimizations, SOURCE_SUBDIR directives to prevent automatic subdirectory addition, and the GITHUB_MIRROR environment-variable pattern provides good flexibility for enterprise mirror scenarios. The global DEEP_EP_COMMIT property correctly enables downstream access. No issues identified.
cpp/tensorrt_llm/deep_ep/CMakeLists.txt (1)
1-1: Property retrieval correctly depends on 3rdparty subdirectory initialization.The
get_property(DEEP_EP_COMMIT GLOBAL PROPERTY DEEP_EP_COMMIT)call will succeed because the 3rdparty subdirectory is added early in cpp/CMakeLists.txt (line 246), ensuring the global property is set before this CMakeLists.txt is processed. The refactor properly centralizes version management.cpp/tensorrt_llm/batch_manager/CMakeLists.txt (1)
59-85: Xgrammar paths correctly updated to FetchContent directory structure.The xgrammar_source_dir variable and associated glob/include-directory paths correctly reflect the FetchContent build-directory layout. Consistent with the centralized fetch declaration at 3rdparty/CMakeLists.txt:107–114.
cpp/CMakeLists.txt (1)
248-271: FetchContent declarations and MakeAvailable calls are properly coordinated.All FetchContent_MakeAvailable invocations reference corresponding declarations in 3rdparty/CMakeLists.txt, and conditional calls align with build options. No orphaned or missing dependencies identified.
cheshirekow
force-pushed
the
cmake-thirdparty-refactor
branch
from
1c8114f to
6660dbc
Compare
|
/bot run |
|
PR_Github #23875 [ run ] triggered by Bot. Commit: |
cheshirekow
force-pushed
the
cmake-thirdparty-refactor
branch
from
6660dbc to
d17fd51
Compare
|
/bot run |
|
PR_Github #24331 [ run ] triggered by Bot. Commit: |
|
PR_Github #24331 [ run ] completed with state |
|
/bot run |
|
PR_Github #24424 [ run ] triggered by Bot. Commit: |
|
PR_Github #24424 [ run ] completed with state |
cheshirekow
force-pushed
the
cmake-thirdparty-refactor
branch
from
d17fd51 to
8fa2afb
Compare
|
/bot run |
|
PR_Github #24493 [ run ] triggered by Bot. Commit: |
|
PR_Github #24493 [ run ] completed with state |
|
/bot run |
|
PR_Github #24519 [ run ] triggered by Bot. Commit: |
|
PR_Github #24519 [ run ] completed with state |
|
/bot run |
|
PR_Github #24566 [ run ] triggered by Bot. Commit: |
|
PR_Github #24566 [ run ] completed with state |
|
/bot run |
|
PR_Github #24732 [ run ] triggered by Bot. Commit: |
|
PR_Github #24732 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #24932 [ run ] triggered by Bot. Commit: |
|
PR_Github #24932 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #25078 [ run ] triggered by Bot. Commit: |
SimengLiu-nv
left a comment
SimengLiu-nv
left a comment
There was a problem hiding this comment.
Not an expert on cmake. Wonder if the message below need to be changed accordingly:
message(
FATAL_ERROR
"FlashMLA submodule not found at ${FLASH_MLA_SOURCE_DIR}. Please run: git submodule update --init --recursive"
)
|
PR_Github #25078 [ run ] completed with state |
cheshirekow
force-pushed
the
cmake-thirdparty-refactor
branch
from
8fa2afb to
d09d172
Compare
@SimengLiu-nv -- I removed the stale checks in the latest version. Thank you for noticing them! |
|
/bot run --skip-test --disable-fail-fast |
1 similar comment
|
/bot run --skip-test --disable-fail-fast |
|
PR_Github #25176 [ run ] triggered by Bot. Commit: |
|
PR_Github #25176 [ run ] completed with state |
In this change we consolidate all third party code registrations into a the common file 3rdparty/CMakeLists.txt using cmake's FetchContent API. This helps to streamline both the process of adding new dependencies, as well as the process of auditing and tracing those dependencies. Because FetchContent_Declare doesn't actually add rules to the build, we can conveniently register all third party dependencies in this one place regardless of whether or not that dependency is integrated or enabled for a particular build. Because several third party dependencies are moved to this build-time fetching, they are also removed as git submodules in this change. Signed-off-by: Josh Bialkowski <[email protected]>
cheshirekow
force-pushed
the
cmake-thirdparty-refactor
branch
from
d09d172 to
6e0eda6
Compare
|
/bloom skip --comment "union of pipelines pass" |
|
/bot skip --comment "union of pipelines pass" |
|
PR_Github #25255 [ skip ] triggered by Bot. Commit: |
|
PR_Github #25255 [ skip ] completed with state |
5 participants
Summary by CodeRabbit
Description
In this change we consolidate all third party code registrations into a the common file 3rdparty/CMakeLists.txt using cmake's FetchContent API. This helps to streamline both the process of adding new dependencies, as well as the process of auditing and tracing those dependencies.
Because FetchContent_Declare doesn't actually add rules to the build, we can conveniently register all third party dependencies in this one place regardless of whether or not that dependency is integrated or enabled for a particular build.
Because several third party dependencies are moved to this build-time fetching, they are also removed as git submodules in this change.
Test Coverage
There is no functional change in this Pull Request. It should be effectively a no-op from the perspective of the individual build commands. It affects only how third party source code is fetched and layed-out during the cmake configure steps. Builds and Tests exercised in CI give us high confidence that there are no unforseen side-effects.
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.