[None][infra] Recognize SM107 (Rubin) in build config and arch detection - #17336
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe change adds CUDA architecture 107 support, classifies SM100 through SM109 as one family, and adds integration-test markers for Rubin architecture boundaries. ChangesSM107 support
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
tests/integration/defs/conftest.py (1)
1588-1588: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd boundary tests for
is_sm_100f.
- Added, modified, or removed test functions: none.
- Affected existing tests are listed in the CI and QA files.
skip_no_rubinandskip_pre_rubinhave no consumers.- Coverage verdict: insufficient. Add tests for SM99, SM100, SM107, SM109, and SM110. No test-list changes are required for the unused Rubin markers.
🤖 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 `@tests/integration/defs/conftest.py` at line 1588, Update the tests covering is_sm_100f to exercise the boundary and representative cases SM99, SM100, SM107, SM109, and SM110, verifying only SM100 through SM109 are accepted. Do not modify test lists for the unused skip_no_rubin or skip_pre_rubin markers.Source: Path instructions
🤖 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 `@cpp/include/tensorrt_llm/common/cudaUtils.h`:
- Around line 309-312: Update isSM100Family so getSMVersion() is evaluated only
when the optional sm argument is absent, preserving the explicit-version path
without any CUDA query. Keep the existing SM100-family range check unchanged.
---
Nitpick comments:
In `@tests/integration/defs/conftest.py`:
- Line 1588: Update the tests covering is_sm_100f to exercise the boundary and
representative cases SM99, SM100, SM107, SM109, and SM110, verifying only SM100
through SM109 are accepted. Do not modify test lists for the unused
skip_no_rubin or skip_pre_rubin markers.
🪄 Autofix
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: ead0271e-1c2d-4f67-84c8-b181a0cbe797
📒 Files selected for processing (4)
cpp/cmake/modules/cuda_configuration.cmakecpp/include/tensorrt_llm/common/cudaUtils.htensorrt_llm/_utils.pytests/integration/defs/conftest.py
|
/bot run |
|
PR_Github #64234 [ run ] triggered by Bot. Commit: |
|
PR_Github #64234 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64361 [ run ] triggered by Bot. Commit: |
|
PR_Github #64361 [ run ] completed with state
|
820efa6 to
c05e989
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --disable-fail-fast |
|
/bot run --disable-fail-fast |
|
PR_Github #64511 [ run ] triggered by Bot. Commit: |
|
PR_Github #64511 [ run ] completed with state
|
73b8b0d to
d489ac2
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #64631 [ run ] triggered by Bot. Commit: |
|
PR_Github #64631 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64742 [ run ] triggered by Bot. Commit: |
|
PR_Github #64742 [ run ] completed with state
|
|
@NVIDIA/trt-llm-perf-devs can you help look at why the CI always fail in this test: unittest/tools/test_layer_wise_benchmarks.py ? This is link to the CI Agent Failure Analysis. |
Enable SM107 (Rubin, compute capability 10.7) as a recognized architecture in the build system and architecture-detection helpers. - cuda_configuration.cmake: add 107 to the default architecture list, the compatibility base, and the list of architectures with kernels. Map 107 to the SM100 family (100f) so Rubin reuses SM100-family kernels. - cudaUtils.h / _utils.py: extend the SM100-family predicate to cover the 10.x range, and let isSM100Family() accept an explicit SM version. - conftest.py: add skip_no_rubin / skip_pre_rubin test markers. The architecture append is gated on CUDA >= 13.4, which is the first toolkit that supports sm_107 (PTX ISA 9.4). Earlier toolkits reject the architecture: CUDA 13.3 nvcc reports "Unsupported gpu architecture 'sm_107'". Verified on B200 (sm_100) that this is a no-op for Blackwell: with CUDA 13.3 the resolved architecture list, family list and CMAKE_CUDA_ARCHITECTURES are byte-identical to the unpatched tree, a full sm_100 build succeeds, and tests/unittest/_torch/thop/parallel/test_fp8_quantize.py and test_fp8_block_scale_gemm.py pass (354 passed, 65 skipped, 0 failed). Signed-off-by: Chulian Zhang <851104+zhangcl@users.noreply.github.com>
std::optional::value_or() evaluates its argument unconditionally, so isSM100Family(sm) still called getSMVersion() even when an explicit SM version was supplied. That defeats the purpose of the parameter and makes the call throw on hosts with no CUDA device, since getSMVersion() runs check_cuda_error(). Use a conditional so the fallback is only evaluated when the caller did not supply a version. The no-argument path, which is what every current caller uses, is unchanged. Reported by CodeRabbit on NVIDIA#17336. Signed-off-by: Chulian Zhang <851104+zhangcl@users.noreply.github.com>
Follow-up to review feedback on NVIDIA#17336. Adding 107 to ARCHITECTURES_COMPATIBILITY_BASE made every kernel build a separate sm_107 variant, since that list selects the architectures code is actually generated for. Rubin does not need its own kernel set: sm_100f cubins run on sm_107, and SM103 is already handled this way. Drop 107 from the compatibility base so it folds into 100 like 103 does, and drop the special case that appended 100f to the families list. The special case only existed to compensate for the base change: once 107 folds into 100, CMAKE_CUDA_ARCHITECTURES_NORMALIZED_LIST contains 100 and the families list gets 100f on its own. Verified with an isolated harness over setup_cuda_architectures(): CUDA 13.4 (simulated), CMAKE_CUDA_ARCHITECTURES=all before: FAMILIES [100f;120f;107f;100f] ARCHITECTURES 80-real;86-real;90a-real;100f-real;120f-real;107f-real after: FAMILIES [100f;120f] ARCHITECTURES 80-real;86-real;90a-real;100f-real;120f-real CUDA 13.2 (real): unchanged, 107 absent as before. The before case also shows 100f appended twice, which the removal fixes. Kernels that benefit from 107a can opt in per target via add_cuda_architectures / set_cuda_architectures, as cutlass_kernels does for FP4 GroupGEMM with 100f/103a/120f. Signed-off-by: Chulian Zhang <851104+zhangcl@users.noreply.github.com>
d489ac2 to
383b9ca
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --disable-fail-fast |
|
PR_Github #64851 [ run ] triggered by Bot. Commit: |
|
PR_Github #64851 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #65011 [ run ] triggered by Bot. Commit: |
|
PR_Github #65011 [ run ] completed with state |
…ion (NVIDIA#17336) Signed-off-by: Chulian Zhang <851104+zhangcl@users.noreply.github.com> Co-authored-by: Chulian Zhang <851104+zhangcl@users.noreply.github.com> Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com>
7 participants
Enable SM107 (Rubin, compute capability 10.7) as a recognized architecture in the build system.
Dev Engineer Review
100f) kernel family.isSM100Family.skip_no_rubinandskip_pre_rubinmarkers.QA Engineer Review
tests/integration/defs/conftest.py.test-db/orqa/test-list changes were provided.Description
Test Coverage
No new test functions were added: this PR adds architecture recognition only,
and the new path (SM107) cannot execute without Rubin hardware. Existing
coverage was used to verify the change is a no-op on Blackwell.
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.