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. |
Assign moe/loadBalance/moeLoadBalanceKernels.cu to the focused static library tllm::kernel_moe_load_balance, removing it from residual MoE sources while retaining shared-library exports and CUDA architectures. Declare common CUDA and environment utilities used by its cooperative and PDL launchers. Migrate moeLoadBalanceKernelTest to the load-balancing component and Threads::Threads. Its signal and expert-load tests allocate through CUDA directly and do not need runtime-buffer, communication or GEMM libraries. Register kernel aggregate ownership, GPU labels and a GPU resource lock. Validation: build and link with CUDA 13.4/SM80; verify the leaf Ninja graph, unchanged production-source ownership and discovered GPU resource locks. GPU execution requires hardware unavailable on this host. Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
Create tllm::kernel_moe_lora with explicit pointer-expansion, problem-builder and slot-expansion CUDA sources. Remove those sources from the CUTLASS MoE launcher object library and link the existing MoE GEMM backend to the new owner. Preserve OSS-MoE gating, CUDA architectures and shared-library exports; expose only CUTLASS headers needed by problem-size declarations. Migrate moeLoraPointerExpandTest, moeLoraProblemBuilderTest and moeLoraSlotExpandTest to the focused component. Their direct CUDA allocations need neither runtime buffers nor the full MoE GEMM backend. Register all three with kernel-tests, MoE LoRA/GPU labels and GPU locks. Validation: build and link all three leaves with CUDA 13.4/SM80; inspect their graphs to exclude the monolith, residual objects and Torch; verify original kernel/backend compilation counts and discovered GPU scheduling properties. GPU execution is unavailable on this host. Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
Move prepareCustomMask.cu from the generated FMHA object target into tllm::kernel_attention_mask with explicit source ownership. Link the FMHA backend to the new component, preserve shared-library exports, and declare the FMHA header interface and CUTLASS header requirements without inheriting generated kernels or prebuilt FMHA implementation. Migrate prepareCustomMaskTest to mask preparation and runtime buffers. Register it with kernel-tests, attention-mask/GPU labels and a GPU lock. Mask-format leaf builds now avoid generated FMHA, GEMM backends, Torch and the monolithic TensorRT-LLM shared library. Validation: build and link the leaf with CUDA 13.4/SM80; verify its Ninja graph excludes residual object libraries and the full stack, preserve production/backend source compilation counts, and inspect discovered GPU scheduling properties. GPU execution is unavailable on this host. Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
Remove REQUIRES_GPU from the migrated kernel test registrations and the explicit GPU resource lock from gemmSwigluKernelTestSm90Fp8. These tests do not need CTest to serialize execution through a shared GPU lock. Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
Name the component for its full scope, which includes unfused attention, GPT preprocessing, attention masks, and RoPE kernels. Update the three kernel test dependencies to use the new alias while retaining their existing scheduling labels. Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
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 |
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.