[https://nvbugs/6070421][fix] Prefer pre-compiled cubin over NVRTC in trtllm-gen FMHA run() - #12969
[https://nvbugs/6070421][fix] Prefer pre-compiled cubin over NVRTC in trtllm-gen FMHA run()#12969galagam wants to merge 5 commits into
Conversation
galagam
added
the
gagam-slack-watch
📝 WalkthroughWalkthroughAdds an early cubin lookup in the FMHA kernel path to avoid NVRTC when a matching precompiled kernel exists, and introduces a reduced-model helper plus a small single-GPU smoke test (bf16, fp8) for NemotronSuperV3 with corresponding test-list entries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/bot run --stage-list "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-Post-Merge-1,DGX_B200-8_GPUs-AutoDeploy-Post-Merge-1" |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/fmhaKernels.h (1)
1-2:⚠️ Potential issue | 🟡 MinorUpdate the NVIDIA copyright year.
This file is being modified in 2026, but the header still ends at 2025.
📝 Proposed fix
- * Copyright (c) 2020-2025, NVIDIA CORPORATION. All rights reserved. + * Copyright (c) 2020-2026, NVIDIA CORPORATION. All rights reserved.As per coding guidelines, "Include NVIDIA copyright header on ALL new files (update year on modified files)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/fmhaKernels.h` around lines 1 - 2, Update the file header year range in the top comment to include 2026 (e.g., change "2020-2025" to "2020-2026") so the NVIDIA copyright header reflects the file modification year; locate the header comment at the top of trtllmGenKernels/fmha/fmhaKernels.h and adjust the year range accordingly.
🧹 Nitpick comments (1)
tests/integration/defs/accuracy/test_llm_api_autodeploy.py (1)
164-165: Use explicit optional typing formodel_pathparameter.Change
model_path: str = Nonetomodel_path: str | None = Noneto comply with explicit optional type annotation style.Suggested patch
-def reduced_model_kwargs(num_hidden_layers: int, - model_path: str = None) -> dict: +def reduced_model_kwargs(num_hidden_layers: int, + model_path: str | None = None) -> dict:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/defs/accuracy/test_llm_api_autodeploy.py` around lines 164 - 165, The parameter annotation for reduced_model_kwargs should use explicit optional typing; change the signature of reduced_model_kwargs to annotate model_path as optional with the union operator (model_path: str | None = None) instead of model_path: str = None so the function uses an explicit Optional type for model_path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/defs/accuracy/test_llm_api_autodeploy.py`:
- Line 624: The test uses the decorator skip_pre_hopper (seen at
`@skip_pre_hopper`) but it isn't imported; add a top-level import for
skip_pre_hopper in tests/integration/defs/accuracy/test_llm_api_autodeploy.py
(alongside the other test imports) by importing the decorator from its defining
module (where your other test helpers/decorators are imported from) so the
NameError during test collection is resolved.
---
Outside diff comments:
In `@cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/fmhaKernels.h`:
- Around line 1-2: Update the file header year range in the top comment to
include 2026 (e.g., change "2020-2025" to "2020-2026") so the NVIDIA copyright
header reflects the file modification year; locate the header comment at the top
of trtllmGenKernels/fmha/fmhaKernels.h and adjust the year range accordingly.
---
Nitpick comments:
In `@tests/integration/defs/accuracy/test_llm_api_autodeploy.py`:
- Around line 164-165: The parameter annotation for reduced_model_kwargs should
use explicit optional typing; change the signature of reduced_model_kwargs to
annotate model_path as optional with the union operator (model_path: str | None
= None) instead of model_path: str = None so the function uses an explicit
Optional type for model_path.
🪄 Autofix (Beta)
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: Pro
Run ID: 9680dbfe-7c13-45ae-bd2c-17869c2298e7
📒 Files selected for processing (3)
cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/fmhaKernels.htests/integration/defs/accuracy/test_llm_api_autodeploy.pytests/integration/test_lists/test-db/l0_b200.yml
|
PR_Github #42891 [ run ] triggered by Bot. Commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/integration/defs/accuracy/test_llm_api_autodeploy.py (1)
624-624:⚠️ Potential issue | 🔴 CriticalImport
skip_pre_hopperbefore using the decorator.Line 624 still fails collection with
NameErrorbecause this module never importsskip_pre_hopper.🛠️ Suggested fix
-from ..conftest import get_device_count, llm_models_root, skip_pre_blackwell +from ..conftest import ( + get_device_count, + llm_models_root, + skip_pre_blackwell, + skip_pre_hopper, +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/defs/accuracy/test_llm_api_autodeploy.py` at line 624, The NameError occurs because the decorator symbol skip_pre_hopper is used at top of the test (decorator on the function/class) but never imported; fix by adding an import for skip_pre_hopper at the top of the module so the decorator is defined before its use (add a line importing skip_pre_hopper from the test utilities module where it lives and ensure the import appears near other test helpers so the decorator on the test at line using `@skip_pre_hopper` resolves).
🧹 Nitpick comments (2)
cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/fmhaKernels.h (1)
294-304: Factor the cubin probe into a shared helper.
checkIfKernelExist()already performs the samealgoFilterForCubinPath()+hashFromFmhaOptions()+mFunctions.find()sequence. Keeping that lookup duplicated here makes the dispatcher andrun()easy to drift apart again the next time the cubin key changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/fmhaKernels.h` around lines 294 - 304, The duplicate cubin-probe logic should be replaced by the existing helper: remove the manual sequence in the block that builds cubinCheckOptions and calls algoFilterForCubinPath + hashFromFmhaOptions + mFunctions.find, and instead call the shared checkIfKernelExist() helper to decide shouldUseNvrtc; locate the sequence around the shouldUseNvrtc branch where FmhaOptions cubinCheckOptions is created and replace it with a call to checkIfKernelExist(cubinCheckOptions) (or pass options to checkIfKernelExist) so the dispatcher and run() use the same cubin key logic and avoid duplication between algoFilterForCubinPath, hashFromFmhaOptions and mFunctions lookups.tests/integration/defs/accuracy/test_llm_api_autodeploy.py (1)
636-645: Make the attention backend requirement explicit in code, not implicit in YAML.While
super_v3.yamldoes specifyattn_backend: trtllm(line 6), the test at lines 636–645 never explicitly sets it inkwargs. This creates implicit coupling to the YAML file: ifsuper_v3.yamlis later modified or a different config is used, the test silently switches backends without the code making this requirement obvious.Since this PR guards
TllmGenFmhaKernel::run(), addkwargs["attn_backend"] = "trtllm"before callingAutoDeployLLMto make the backend requirement explicit and testable at code review time.Suggested change
model_path = self.MODEL_PATHS[dtype] kwargs = {} + kwargs["attn_backend"] = "trtllm" kwargs.update( reduced_model_kwargs(num_hidden_layers=16, model_path=model_path))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/defs/accuracy/test_llm_api_autodeploy.py` around lines 636 - 645, The test relies on an attention backend from YAML but doesn't set it in code — before invoking AutoDeployLLM, add kwargs["attn_backend"] = "trtllm" so the requirement is explicit; update the block that builds kwargs (which currently uses reduced_model_kwargs(...) and MODEL_PATHS[dtype] and CONFIG_YAML) to assign the attn_backend key prior to calling AutoDeployLLM.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/defs/accuracy/test_llm_api_autodeploy.py`:
- Around line 164-165: The parameter type for model_path in the
reduced_model_kwargs function is annotated as str but its default is None;
update the annotation to str | None (i.e., make model_path: str | None) so the
type matches the default value and any static type checks; locate the
reduced_model_kwargs definition to apply this change.
---
Duplicate comments:
In `@tests/integration/defs/accuracy/test_llm_api_autodeploy.py`:
- Line 624: The NameError occurs because the decorator symbol skip_pre_hopper is
used at top of the test (decorator on the function/class) but never imported;
fix by adding an import for skip_pre_hopper at the top of the module so the
decorator is defined before its use (add a line importing skip_pre_hopper from
the test utilities module where it lives and ensure the import appears near
other test helpers so the decorator on the test at line using `@skip_pre_hopper`
resolves).
---
Nitpick comments:
In `@cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/fmhaKernels.h`:
- Around line 294-304: The duplicate cubin-probe logic should be replaced by the
existing helper: remove the manual sequence in the block that builds
cubinCheckOptions and calls algoFilterForCubinPath + hashFromFmhaOptions +
mFunctions.find, and instead call the shared checkIfKernelExist() helper to
decide shouldUseNvrtc; locate the sequence around the shouldUseNvrtc branch
where FmhaOptions cubinCheckOptions is created and replace it with a call to
checkIfKernelExist(cubinCheckOptions) (or pass options to checkIfKernelExist) so
the dispatcher and run() use the same cubin key logic and avoid duplication
between algoFilterForCubinPath, hashFromFmhaOptions and mFunctions lookups.
In `@tests/integration/defs/accuracy/test_llm_api_autodeploy.py`:
- Around line 636-645: The test relies on an attention backend from YAML but
doesn't set it in code — before invoking AutoDeployLLM, add
kwargs["attn_backend"] = "trtllm" so the requirement is explicit; update the
block that builds kwargs (which currently uses reduced_model_kwargs(...) and
MODEL_PATHS[dtype] and CONFIG_YAML) to assign the attn_backend key prior to
calling AutoDeployLLM.
🪄 Autofix (Beta)
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: Pro
Run ID: d00247a2-c703-46f8-b8fd-6914e8a280d9
📒 Files selected for processing (3)
cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/fmhaKernels.htests/integration/defs/accuracy/test_llm_api_autodeploy.pytests/integration/test_lists/test-db/l0_b200.yml
|
/bot kill |
|
PR_Github #42891 [ run ] completed with state
|
|
PR_Github #42893 [ kill ] triggered by Bot. Commit: |
|
PR_Github #42893 [ kill ] completed with state |
|
/bot run --stage-list "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-Post-Merge-1,DGX_B200-8_GPUs-AutoDeploy-Post-Merge-1" |
|
PR_Github #42896 [ run ] triggered by Bot. Commit: |
|
PR_Github #42896 [ run ] completed with state
|
… trtllm-gen FMHA run() account for sm_100f Persistent SwapsAbForGen cubins being compatible with sm_100 GPUs via isSMCompatible(). When a matching cubin exists in mFunctions, NVRTC is now skipped in favor of it. Signed-off-by: Gal Hubara Agam <96368689+galagam@users.noreply.github.com>
Add test_functional_small to TestNemotronSuperV3 which overrides num_hidden_layers (and layers_block_type) via model_kwargs to fit the model on a single GPU. This provides pre-merge coverage for large models that otherwise only run in multi-GPU CI. Signed-off-by: Gal Hubara Agam <96368689+galagam@users.noreply.github.com>
Signed-off-by: Gal Hubara-Agam <96368689+galagam@users.noreply.github.com>
this is only required for accuracy reference lookup Signed-off-by: Gal Hubara-Agam <96368689+galagam@users.noreply.github.com>
6f8ab99 to
7821177
Compare
|
/bot run --stage-list "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-Post-Merge-1,DGX_B200-8_GPUs-AutoDeploy-Post-Merge-1" |
|
PR_Github #42955 [ run ] triggered by Bot. Commit: |
|
PR_Github #42955 [ run ] completed with state
|
|
/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1,DGX_B200-8_GPUs-AutoDeploy-Post-Merge-1" |
|
PR_Github #42985 [ run ] triggered by Bot. Commit: |
|
PR_Github #42985 [ run ] completed with state
|
| - accuracy/test_llm_api_autodeploy.py::TestNemotronNanoV3::test_accuracy[fp8-1-trtllm] | ||
| - accuracy/test_llm_api_autodeploy.py::TestNemotronNanoV3::test_accuracy[nvfp4-1-trtllm] | ||
| - accuracy/test_llm_api_autodeploy.py::TestNemotronSuperV3::test_accuracy[nvfp4-1-attn_dp_off-trtllm] | ||
| - accuracy/test_llm_api_autodeploy.py::TestNemotronSuperV3::test_functional_small[bf16] |
There was a problem hiding this comment.
please add the tests into qa test list too.
|
Issue turned out to be an env issue and not a bug. |
4 participants
Description
PR #12612 introduced a
shouldUseNvrtcpath inTllmGenFmhaKernel::run()that unconditionally bypasses cubin lookup forSwapsMmaAbForGenerationkernels. On sm_100 (B200) GPUs, this triggers NVRTC compilation for kernels that already have compatible sm_100f pre-compiled cubins - causing a crash when cuda.h is not on the include path.Fix: Before entering the NVRTC path, check whether a matching cubin exists in mFunctions. If one is found, skip NVRTC and use the cubin directly.
Failing tests fixed:
tests/integration/defs/accuracy/test_llm_api_autodeploy.py::TestNemotronSuperV3::test_accuracy[fp8-*
tests/integration/defs/accuracy/test_llm_api_pytorch.py::TestNemotronV3Super::test_fp8_4gpus
Added TestNemotronSuperV3::test_functional_small[fp8] - a single-GPU smoke test that loads a layer-reduced (16-layer) Super V3 FP8 model using reduced_model_kwargs(), a reusable helper that truncates both num_hidden_layers and layers_block_type to fit large hybrid models on one device. Added to l0_b200.yml for pre-merge coverage.
Test Coverage
N/A - fixing existing tests
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
To see a list of available CI bot commands, please comment
/bot help.Summary by CodeRabbit
Improvements
Tests