Repository navigation
[TRTLLM-17005][refactor] Component-local C++ Tests (Part 3a) - #21022
Conversation
|
/bot run |
|
PR_Github #80609 [ run ] triggered by Bot. Commit: |
|
PR_Github #80609 [ run ] completed with state
|
66f2c7f to
6067e22
Compare
|
/bot run |
|
PR_Github #80625 [ run ] triggered by Bot. Commit: |
|
PR_Github #80625 [ run ] completed with state
|
6067e22 to
e144d1b
Compare
|
/bot run |
|
PR_Github #80642 [ run ] triggered by Bot. Commit: |
|
PR_Github #80642 [ run ] completed with state
|
e144d1b to
9a8e8fd
Compare
|
/bot run |
|
PR_Github #80671 [ run ] triggered by Bot. Commit: |
|
PR_Github #80671 [ run ] completed with state
|
|
/bot run |
|
PR_Github #80816 [ run ] triggered by Bot. Commit: |
|
PR_Github #80816 [ run ] completed with state
|
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
|
| Layer / File(s) | Summary |
|---|---|
Define kernel component libraries cpp/tensorrt_llm/kernels/kernelComponents.cmake |
A new CMake helper validates arguments, creates static kernel libraries and namespaced aliases, and registers component targets with their sources and dependencies. |
Wire component libraries into kernel sources cpp/tensorrt_llm/kernels/cutlass_kernels/CMakeLists.txt, cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/CMakeLists.txt |
The MoE and FMHA source lists exclude files handled by component libraries and link the corresponding targets. MOE_CUTLASS_DIR is converted to an absolute path. |
Propagate and link kernel components cpp/tensorrt_llm/kernels/CMakeLists.txt, cpp/tensorrt_llm/CMakeLists.txt |
The kernel directory propagates component targets to its parent. The shared target links each component with WHOLE_ARCHIVE and sets its link-library override accordingly. |
Migrate kernel test declarations cpp/tests/unit_tests/kernels/CMakeLists.txt |
Kernel tests use add_tllm_gtest declarations with explicit owners, sources, dependencies, and labels. The NVFP4 cold-page test links the kernel and KV-cache host-memory libraries. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Suggested reviewers: qijune
Merge Risk: 
Minimal · previous review 73d65
-
No concrete build or test-wiring defect is established by the reviewed changes; reported CI failures still need their individual causes identified through normal validation.
Pre-merge checks: 
5
-
Full details: Description check
-
Explanation
The description clearly explains the CMake and test-wiring changes and includes the required sections and checklist. The Test Coverage section is empty and does not list specific tests or commands, but the overall description is sufficiently complete.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (3 skipped: 3 unsupported.)
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
@farazkh80, could you help to review this PR as trt-llm-torch-attention-devs? Thanks~ Assignment explanation
Affinity and workload are snapshots at selection, not live counters. Existing reservations on this PR are counted once. |
Keep the FMHA interface public while making the custom-mask kernel archive an implementation dependency. The shared library already retains the archive through its private whole-archive component linkage, so downstream consumers do not need it in the public link interface. Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
73d65ca to
14479fb
Compare
|
/bot run |
|
PR_Github #81169 [ run ] triggered by Bot. Commit: |
|
PR_Github #81169 [ run ] completed with state Resource maintenance in LLM/main/L0_Test-x86_64-Single-GPU #9590: L40S-PyTorch-2 is temporarily paused due to Kubernetes agent provisioning and node issues.
|
|
/bot run |
|
PR_Github #81185 [ run ] triggered by Bot. Commit: |
|
PR_Github #81185 [ run ] completed with state Resource maintenance in LLM/main/L0_Test-x86_64-Single-GPU #9103: L40S-PyTorch-2 is temporarily paused due to Kubernetes agent provisioning and node issues.
|
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Moving each test onto add_tllm_gtest with a narrow LINK_LIBRARIES list is a clear improvement. The nvfp4 cold-page test in particular no longer carries its hand-maintained list of common/*.cpp and kv_cache_manager_v2/utils/*.cpp sources.
Two things before merge:
- CI status: the PR history shows several failed or unstable pipelines. Please post a green
/bot runon the current head. This change moves sources between link units, so only a full build plus the migratedkernelsctest labels can show that nothing went missing or got duplicated inlibtensorrt_llm.so. - Description: the PR description is cut off partway through the list of extracted families ("MoE communication/loa…"). Please complete it, and say what "Part 3a" covers compared with the later parts, so a future reader can tell which kernel tests are still on
add_gtestand why.
|
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements. |
|
/bot run |
|
PR_Github #81219 [ run ] triggered by Bot. Commit: |
|
PR_Github #81219 [ run ] completed with state Resource maintenance in LLM/main/L0_Test-x86_64-Single-GPU #10088: L40S-PyTorch-2 is temporarily paused due to Kubernetes agent provisioning and node issues.
|
|
/bot run |
|
PR_Github #81255 [ run ] triggered by Bot. Commit: |
|
PR_Github #81255 [ run ] completed with state Resource maintenance in LLM/main/L0_Test-x86_64-Single-GPU #9613: L40S-PyTorch-2 is temporarily paused due to Kubernetes agent provisioning and node issues.
|
|
/bot run |
|
PR_Github #81337 [ run ] triggered by Bot. Commit: |
|
PR_Github #81337 [ run ] completed with state Resource maintenance in LLM/main/L0_Test-x86_64-Single-GPU #9637: L40S-PyTorch-2 is temporarily paused due to Kubernetes agent provisioning and node issues.
|
|
/bot run --disable-fail-fast |
|
PR_Github #81389 [ run ] triggered by Bot. Commit: |
|
PR_Github #81389 [ run ] completed with state |
….3: Restore Mistral Large 3 multimodal inference Forward-port of release/1.3 PR NVIDIA#19788 (release/1.3 commit 48c796c) to main. Adapted to main: adopts main's MultimodalItemMetadata contract from _build_mm_item_metadata and exposes it through get_mm_item_metadata, and the new tests call get_mm_item_metadata and compare against the tuples that MultimodalItemMetadata now stores. Also: the HF tokenizer load in MistralHFInputProcessor follows the trust_remote_code argument like the processor load next to it, and build_fmha_head104_tests requests dynamic NVRTC linking and passes plain architecture numbers (NVIDIA#20137) like the other configure-only gtest fixtures, so the target does not depend on a configure cache left by an earlier fixture. It also regenerates the FMHA sources, so a stale generation stamp cannot hide the head-104 generator changes, and fmhaHead104Test is registered with add_tllm_gtest (OWNER, LABELS, FULL_STACK) as main asks for new C++ tests (NVIDIA#21022). Signed-off-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com> Co-authored-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com> Signed-off-by: Simeng Liu <simengl@nvidia.com>
….3: Restore Mistral Large 3 multimodal inference Forward-port of release/1.3 PR NVIDIA#19788 (release/1.3 commit 48c796c) to main. Adapted to main: adopts main's MultimodalItemMetadata contract from _build_mm_item_metadata and exposes it through get_mm_item_metadata, and the new tests call get_mm_item_metadata and compare against the tuples that MultimodalItemMetadata now stores. Also: the HF tokenizer load in MistralHFInputProcessor follows the trust_remote_code argument like the processor load next to it, and build_fmha_head104_tests requests dynamic NVRTC linking and passes plain architecture numbers (NVIDIA#20137) like the other configure-only gtest fixtures, so the target does not depend on a configure cache left by an earlier fixture. It also regenerates the FMHA sources, so a stale generation stamp cannot hide the head-104 generator changes, and fmhaHead104Test is registered with add_tllm_gtest (OWNER, LABELS, FULL_STACK) as main asks for new C++ tests (NVIDIA#21022). Signed-off-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com> Co-authored-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com> Signed-off-by: Simeng Liu <simengl@nvidia.com>
….3: Restore Mistral Large 3 multimodal inference Forward-port of release/1.3 PR NVIDIA#19788 (release/1.3 commit 48c796c) to main. Adapted to main: adopts main's MultimodalItemMetadata contract from _build_mm_item_metadata and exposes it through get_mm_item_metadata, and the new tests call get_mm_item_metadata and compare against the tuples that MultimodalItemMetadata now stores. Also: the HF tokenizer load in MistralHFInputProcessor follows the trust_remote_code argument like the processor load next to it, and build_fmha_head104_tests requests dynamic NVRTC linking and passes plain architecture numbers (NVIDIA#20137) like the other configure-only gtest fixtures, so the target does not depend on a configure cache left by an earlier fixture. It also regenerates the FMHA sources, so a stale generation stamp cannot hide the head-104 generator changes, and fmhaHead104Test is registered with add_tllm_gtest (OWNER, LABELS, FULL_STACK) as main asks for new C++ tests (NVIDIA#21022). Signed-off-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com> Co-authored-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com> Signed-off-by: Simeng Liu <simengl@nvidia.com>
….3: Restore Mistral Large 3 multimodal inference Forward-port of release/1.3 PR NVIDIA#19788 (release/1.3 commit 48c796c) to main. Adapted to main: adopts main's MultimodalItemMetadata contract from _build_mm_item_metadata and exposes it through get_mm_item_metadata, and the new tests call get_mm_item_metadata and compare against the tuples that MultimodalItemMetadata now stores. Also: the HF tokenizer load in MistralHFInputProcessor follows the trust_remote_code argument like the processor load next to it, and build_fmha_head104_tests requests dynamic NVRTC linking and passes plain architecture numbers (NVIDIA#20137) like the other configure-only gtest fixtures, so the target does not depend on a configure cache left by an earlier fixture. It also regenerates the FMHA sources, so a stale generation stamp cannot hide the head-104 generator changes, and fmhaHead104Test is registered with add_tllm_gtest (OWNER, LABELS, FULL_STACK) as main asks for new C++ tests (NVIDIA#21022). Signed-off-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com> Co-authored-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com> Signed-off-by: Simeng Liu <simengl@nvidia.com>
Dev Engineer Review
The change extracts selected kernels into static component libraries and links them into
tensorrt_llmwith whole-archive semantics. It also updates CUTLASS and FMHA dependencies, source discovery, and kernel-test target wiring. The FMHA custom-mask archive remains a private implementation dependency. The supplied information does not establish test results for this source state.Review findings and severity counts are unavailable. CI reports failures for builds 66367, 66380, 66394, and 66423, and an unstable result for build 66548. The comments do not identify specific failing tests.
QA Engineer Review
cpp/tests/unit_tests/kernels/CMakeLists.txtmigrates kernel test targets toadd_tllm_gtestwith explicit owners, dependencies, source files, and labels. The registrations cover logits, NVFP4 cold-page, attention masks, RoPE and shift-K-cache, MLA, chunked prefill, sparse attention, MoE communication, load balancing, conditional MoE LoRA, and sparse KV cache. The NVFP4 cold-page test now links reusable component libraries instead of listing support sources and link settings directly.Changed test functions and IDs are not identified in the supplied information. No changes to
tests/integration/test_lists/are reported, so CI or manual-QA listing status cannot be established. Coverage verdict: needs follow-up. The supplied CI summary reports failures and an unstable pipeline, but does not identify specific failing tests.Per-File QA Perspective
cpp/tensorrt_llm/CMakeLists.txt: Whole-archive links retain component kernels in the shared library. Verify that each registered component is linked correctly.cpp/tensorrt_llm/kernels/CMakeLists.txt: Propagates the component target list to the parent scope. Verify that consumers receive the complete list.cpp/tensorrt_llm/kernels/cutlass_kernels/CMakeLists.txt: Changes LoRA source discovery and links the LoRA component when OSS MoE GEMM is enabled. Verify both conditional paths.cpp/tensorrt_llm/kernels/kernelComponents.cmake: Adds static component-library registration, source removal, and conditional MoE LoRA registration. Verify source ownership and dependency configuration.cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/CMakeLists.txt: ExcludesprepareCustomMask.cufrom recursive discovery and privately links the attention-mask component. Verify custom-mask behavior and source ownership.cpp/tests/unit_tests/kernels/CMakeLists.txt: Migrates kernel test targets and replaces manually listed NVFP4 support sources with component libraries. Verify dependencies, labels, and conditional MoE LoRA registration.Description
Implement component-local tests as real CMake target dependencies. This PR migrates some of the kernel tests.
add_tllm_gtest, with explicit component dependencies, ownership, and scheduling labels. This reduces their dependency on the full TensorRT-LLM library.All committed changes are CMake build/test wiring; kernel implementations and test bodies are unchanged.
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.