Skip to content

[None] [feat] Add Tencent HunYuanMoEV1 model support#5521

Merged
byshiue merged 7 commits into
NVIDIA:mainfrom
qianbiaoxiang:support_hunyuan_moe
Aug 14, 2025
Merged

[None] [feat] Add Tencent HunYuanMoEV1 model support#5521
byshiue merged 7 commits into
NVIDIA:mainfrom
qianbiaoxiang:support_hunyuan_moe

Conversation

@qianbiaoxiang

@qianbiaoxiang qianbiaoxiang commented Jun 26, 2025

Copy link
Copy Markdown
Contributor

Description

Currently, the Hunyuan inference team supports the Hunyuan-A13B model. By adding the modeling_hunyuan_moe.py related files, it supports the model of HunYuanMoEV1ForCausalLM.

We have validated the accuracy of this PR,HunYuan (new MoE LLM model from Tencent) will open source these days.

Thanks~

Test Coverage

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.

Details

run [--disable-fail-fast --skip-test --stage-list "A10-1, xxx" --gpu-type "A30, H100_PCIe" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-[Post-Merge]-1, xxx"]

Launch build/test pipelines. All previously running jobs will be killed.

--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-1, xxx" (OPTIONAL) : Only run the specified test stages. Examples: "A10-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.

--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. Will also run 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-[Post-Merge]-1, xxx" (OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-[Post-Merge]-1, xxx".

For guidance on mapping tests to stage names, see docs/source/reference/ci-overview.md.

kill

kill

Kill all running builds associated with pull request.

skip

skip --comment COMMENT

Skip 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-pipeline

Reuse 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.

Summary by CodeRabbit

  • New Features

    • Advanced LLM quickstart: new CLI flag to optionally apply chat-template formatting to prompts.
    • New HunYuan MoE causal LM model exposed for Torch users.
    • Tokenizer: ability to retrieve chat templates.
    • RoPE: dynamic scaling mode with configurable alpha and added QK-normalization support for attention.
  • Bug Fixes

    • Safer MLA detection before enabling flash MLA.
    • Clearer tokenizer loading warnings with exception details.

@qianbiaoxiang
qianbiaoxiang requested review from a team as code owners June 26, 2025 14:46
@juney-nvidia juney-nvidia added Community want to contribute PRs initiated from Community Community Engagement help/insights needed from community labels Jun 26, 2025
@wm2012011492
wm2012011492 force-pushed the support_hunyuan_moe branch from 99a91f8 to f81cd35 Compare June 30, 2025 02:47
@wm2012011492

Copy link
Copy Markdown
Collaborator

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #10260 [ run ] triggered by Bot

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #10260 [ run ] completed with state FAILURE
/LLM/main/L0_MergeRequest_PR pipeline #7581 completed with status: 'FAILURE'

@sorenwu
sorenwu requested review from a team as code owners August 12, 2025 03:03
@byshiue
byshiue removed request for a team August 12, 2025 08:43

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (8)
tensorrt_llm/llmapi/tokenizer.py (1)

60-64: Align API with HF, add docstring, and use keyword args to avoid positional drift

Rename the parameter to match HF’s get_chat_template(default=..., tools=...), return Optional[str] to reflect potential absence, add a concise docstring, and call the underlying API with keyword arguments.

-    def get_chat_template(self,
-                          chat_template: Optional[str] = None,
-                          tools: Optional[List[Dict]] = None) -> str:
-        return self.tokenizer.get_chat_template(chat_template, tools)
+    def get_chat_template(
+            self,
+            default: Optional[str] = None,
+            tools: Optional[List[Dict]] = None) -> Optional[str]:
+        """Return the tokenizer chat-template string if available.
+
+        Args:
+            default: Optional fallback template to return if the tokenizer has no built-in template.
+            tools: Optional tool schema to augment the template, if supported by the tokenizer.
+
+        Returns:
+            The chat-template string, or None if not available and no default is provided.
+        """
+        return self.tokenizer.get_chat_template(default=default, tools=tools)
examples/llm-api/quickstart_advanced.py (1)

111-113: Add help text for the new CLI flag

Provide a helpful description consistent with other args to improve UX and self-documentation.

-    parser.add_argument('--apply_chat_template',
-                        default=False,
-                        action='store_true')
+    parser.add_argument('--apply_chat_template',
+                        default=False,
+                        action='store_true',
+                        help='Apply the tokenizer chat template to prompts '
+                             '(tokenize=False, add_generation_prompt=True). '
+                             'Requires a tokenizer with a chat template.')
tensorrt_llm/_torch/attention_backend/interface.py (1)

341-349: Add alpha to RopeParams: document intent and be mindful of hashing

Adding alpha is good for dynamic RoPE scaling. Since RopeParams is unsafe_hash=True and used as a cache key, any later mutation of alpha after caching will cause hard-to-debug issues. Consider:

  • Documenting alpha’s semantics in RopeParams (Google-style docstring).
  • Treat RopeParams as immutable after from_config, or make the dataclass frozen to prevent post-cache mutations.

Would you like me to convert RopeParams to a frozen dataclass and adjust construction accordingly?

tensorrt_llm/_torch/model_config.py (1)

154-163: Avoid duplicated MLA gating logic; reuse is_mla(config)

This block duplicates the predicate implemented in is_mla(config). Prefer delegating to is_mla(self.pretrained_config) to keep a single source of truth and avoid divergence if the predicate evolves.

Apply this refactor:

-        if self.attn_backend == 'TRTLLM':
-            if hasattr(
-                    self.pretrained_config, "kv_lora_rank"
-            ) and self.pretrained_config.kv_lora_rank is not None and hasattr(
-                    self.pretrained_config, "qk_rope_head_dim"
-            ) and self.pretrained_config.qk_rope_head_dim is not None:
+        if self.attn_backend == 'TRTLLM' and is_mla(self.pretrained_config):
tensorrt_llm/_torch/modules/attention.py (1)

144-144: Add documentation for the new parameter

The docstring should be updated to include documentation for the qk_norm_type parameter.

             config (Optional[ModelConfig]): The model configuration.
-            qk_norm_type (QkNormType): The type of QK normalization.
             q_scaling (float): The scaling factor for the qk_scale. The definition is $O = softmax(QK^T * qk_scale) * V, qk_scale = 1 / (sqrt(head_dim) * q_scaling)$. The default value is 1.0.
+            qk_norm_type (QkNormType): The type of QK normalization to apply. Options are none (no normalization), pre_rope (before RoPE), or post_rope (after RoPE). Default is none.
             attention_chunk_size (Optional[int]): See [Chunked Attention] below.
tensorrt_llm/_torch/models/modeling_hunyuan_moe.py (3)

215-215: Improve readability of MoE layer condition

The condition for determining if a layer should use MoE is complex and could benefit from better formatting.

-        is_moe_single_node = is_experts_valid and layer_idx >= config.moe_layer_num_skipped  # only support one node yet
+        # Only support single node MoE for now
+        is_moe_single_node = (is_experts_valid and 
+                              layer_idx >= config.moe_layer_num_skipped)

376-385: Potential inefficiency in weight duplication

The weight duplication is performed for every k_proj and v_proj weight during loading. Consider checking if duplication is necessary based on the tensor shape first.

                         if new_name in ['k_proj', 'v_proj']:
+                            # Only duplicate if weight needs duplication
+                            needs_duplication = any(
+                                k in fw and fw[k].shape[0] != self.config.num_key_value_heads * head_dim
+                                for k in ["weight", "bias"] if k in fw
+                            )
+                            if needs_duplication:
                             fw = {
                                 k:
                                 duplicate_kv_weight(
                                     weight=v[:],
                                     num_kv_heads=v[:].shape[0] // head_dim,
                                     tensor_parallel_size=tp_size)
                                 if k in ["weight", "bias"] else v
                                 for k, v in fw.items()
                             }

389-403: Improve weight name mapping for MoE modules

The weight name remapping logic could be made more maintainable using a dictionary.

                     if isinstance(module, CutlassFusedMoE) or isinstance(
                             module, VanillaMoE):
                         # model.layers.{idx}.mlp.experts
                         updated_module_weights = {}
+                        weight_name_map = {
+                            "gate_proj": "w1",
+                            "up_proj": "w3", 
+                            "down_proj": "w2"
+                        }
                         for weight_name, weight_value in module_weights.items():
-                            new_weight_name = weight_name.replace(
-                                "gate_proj",
-                                "w1").replace("up_proj",
-                                              "w3").replace("down_proj", "w2")
+                            new_weight_name = weight_name
+                            for old_name, new_name in weight_name_map.items():
+                                new_weight_name = new_weight_name.replace(old_name, new_name)
                             updated_module_weights[
                                 new_weight_name] = weight_value
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dc4dd1d and 0ac773c.

📒 Files selected for processing (9)
  • examples/llm-api/quickstart_advanced.py (2 hunks)
  • tensorrt_llm/_torch/attention_backend/interface.py (3 hunks)
  • tensorrt_llm/_torch/model_config.py (1 hunks)
  • tensorrt_llm/_torch/models/__init__.py (2 hunks)
  • tensorrt_llm/_torch/models/modeling_hunyuan_moe.py (1 hunks)
  • tensorrt_llm/_torch/modules/attention.py (8 hunks)
  • tensorrt_llm/_torch/pyexecutor/config_utils.py (1 hunks)
  • tensorrt_llm/functional.py (1 hunks)
  • tensorrt_llm/llmapi/tokenizer.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)

**/*.py: Python code should conform to Python 3.8+.
Indent Python code with 4 spaces. Do not use tabs.
Always maintain the namespace when importing in Python, even if only one class or function from a module is used.
Python filenames should use snake_case (e.g., some_file.py).
Python classes should use PascalCase (e.g., class SomeClass).
Python functions and methods should use snake_case (e.g., def my_awesome_function():).
Python local variables should use snake_case. Prefix k for variable names that start with a number (e.g., k_99th_percentile).
Python global variables should use upper snake_case and prefix G (e.g., G_MY_GLOBAL).
Python constants should use upper snake_case (e.g., MY_CONSTANT).
Avoid shadowing variables declared in an outer scope in Python.
Initialize all externally visible members of a Python class in the constructor.
For interfaces that may be used outside a Python file, prefer docstrings over comments.
Comments in Python should be reserved for code within a function, or interfaces that are local to a file.
Use Google style docstrings for Python classes and functions, which can be parsed by Sphinx.
Attributes and variables in Python can be documented inline; attribute docstrings will be rendered under the class docstring.
Avoid using reflection in Python when functionality can be easily achieved without it.
When using try-except blocks in Python, limit the except to the smallest set of errors possible.
When using try-except blocks to handle multiple possible variable types in Python, keep the body of the try as small as possible, using the else block to implement the logic.

Files:

  • tensorrt_llm/_torch/attention_backend/interface.py
  • tensorrt_llm/_torch/model_config.py
  • tensorrt_llm/_torch/pyexecutor/config_utils.py
  • tensorrt_llm/functional.py
  • tensorrt_llm/llmapi/tokenizer.py
  • tensorrt_llm/_torch/models/modeling_hunyuan_moe.py
  • examples/llm-api/quickstart_advanced.py
  • tensorrt_llm/_torch/models/__init__.py
  • tensorrt_llm/_torch/modules/attention.py
**/*.{cpp,h,hpp,cc,cxx,cu,py}

📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)

All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.

Files:

  • tensorrt_llm/_torch/attention_backend/interface.py
  • tensorrt_llm/_torch/model_config.py
  • tensorrt_llm/_torch/pyexecutor/config_utils.py
  • tensorrt_llm/functional.py
  • tensorrt_llm/llmapi/tokenizer.py
  • tensorrt_llm/_torch/models/modeling_hunyuan_moe.py
  • examples/llm-api/quickstart_advanced.py
  • tensorrt_llm/_torch/models/__init__.py
  • tensorrt_llm/_torch/modules/attention.py
⏰ 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: Pre-commit Check
🔇 Additional comments (13)
tensorrt_llm/_torch/attention_backend/interface.py (2)

385-394: Config parsing for alpha looks correct

Reading alpha from rope_scaling with a default of 1.0 aligns with the dynamic RoPE path needs. No issues.


465-472: Propagation of alpha into rope_scaling_config is correct

Passing alpha alongside the other rope parameters ensures the functional path can compute dynamic scaling without fishing back into config. LGTM.

tensorrt_llm/_torch/pyexecutor/config_utils.py (1)

8-12: Stricter MLA enablement check is a sensible improvement

Switching from assert-based behavior to an explicit non-None check for kv_lora_rank and qk_rope_head_dim makes the predicate safer and prevents surprising assertion failures. Looks good.

tensorrt_llm/_torch/models/__init__.py (1)

42-44: Ensure exported names are defined

After importing Gemma3Model as suggested above, the all entry becomes valid. Without the import, star-imports or explicit imports for Gemma3Model will fail.

Would you like me to scan call sites to ensure no other missing exports were introduced?

tensorrt_llm/_torch/modules/attention.py (7)

3-3: LGTM!

The import of IntEnum is appropriate for defining the QkNormType enumeration.


31-38: LGTM!

The QkNormType enum definition is well-structured with clear documentation and appropriate values for different QK normalization modes.


126-126: LGTM!

The qk_norm_type parameter is appropriately placed after q_scaling with a sensible default value of QkNormType.none.


171-171: LGTM!

The qk_norm_type attribute is properly stored as an instance variable.


273-275: LGTM!

The rope fusion logic correctly disables fusion when qk_norm_type is post_rope, as applying QK normalization after RoPE is incompatible with fused RoPE operations.


524-527: LGTM!

The apply_qk_norm method properly raises NotImplementedError with a clear message, following the template method pattern for subclasses to implement their specific normalization logic.


447-455: QKV split logic and reconciliation verified

The convert_qkv method cleanly handles both unsplit (k, v = None) and fused QKV cases—splitting when needed and re-fusing when support_fused_qkv is true—so all downstream attention paths get the correct q, k, v shapes. No changes required.

tensorrt_llm/_torch/models/modeling_hunyuan_moe.py (2)

29-94: LGTM!

The HunyuanMoE class is well-structured with proper initialization of the gate, experts, and shared MLP components. The forward method correctly combines the outputs of the shared expert and MoE experts.


312-318: Logical error in input validation

The XOR operation check has incorrect logic - it should ensure exactly one input is provided.

-        if (input_ids is None) ^ (inputs_embeds is not None):
+        if not ((input_ids is None) ^ (inputs_embeds is None)):
             raise ValueError(
                 "You cannot specify both input_ids and inputs_embeds at the same time, and must specify either one"
             )

Likely an incorrect or invalid review comment.

Comment on lines +279 to 288
new_prompts = []
if args.apply_chat_template:
for prompt in prompts:
messages = [{"role": "user", "content": f"{prompt}"}]
new_prompts.append(
llm.tokenizer.apply_chat_template(messages,
tokenize=False,
add_generation_prompt=True))
prompts = new_prompts
outputs = llm.generate(prompts, sampling_params)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Guard against tokenizers without a chat template and reduce scope of temporaries

Without a guard, enabling the flag on models lacking a chat template will raise and crash the example. Also, move new_prompts into the if-block to reduce scope.

You can verify this by running with a tokenizer that lacks a chat template; the current code will raise on apply_chat_template.

-    new_prompts = []
-    if args.apply_chat_template:
-        for prompt in prompts:
-            messages = [{"role": "user", "content": f"{prompt}"}]
-            new_prompts.append(
-                llm.tokenizer.apply_chat_template(messages,
-                                                  tokenize=False,
-                                                  add_generation_prompt=True))
-        prompts = new_prompts
+    if args.apply_chat_template:
+        try:
+            # Validate template availability (will raise if unsupported)
+            _ = llm.tokenizer.get_chat_template()
+            new_prompts = []
+            for prompt in prompts:
+                messages = [{"role": "user", "content": prompt}]
+                new_prompts.append(
+                    llm.tokenizer.apply_chat_template(
+                        messages, tokenize=False, add_generation_prompt=True
+                    )
+                )
+            prompts = new_prompts
+        except Exception as e:
+            print(f"[warn] --apply_chat_template ignored: {e}")

If you prefer structured logging instead of print, add this import at the top of the file and replace the print with logger.warning(...):

from tensorrt_llm.logger import logger

🏁 Script executed:

#!/bin/bash
set -e

echo "Searching for apply_chat_template definitions and references:"
rg "apply_chat_template" -n

echo
echo "Searching for get_chat_template definitions and references:"
rg "get_chat_template" -n

Length of output: 10606


🏁 Script executed:

#!/bin/bash
set -e

echo "Inspecting tensorrt_llm/llmapi/tokenizer.py around get/apply_chat_template definitions:"
rg -n "def get_chat_template" -C3 tensorrt_llm/llmapi/tokenizer.py
rg -n "def apply_chat_template" -C3 tensorrt_llm/llmapi/tokenizer.py

echo
echo "Inspecting tensorrt_llm/inputs/utils.py around get/apply_chat_template definitions:"
rg -n "def get_chat_template" -C3 tensorrt_llm/inputs/utils.py
rg -n "def apply_chat_template" -C3 tensorrt_llm/inputs/utils.py

Length of output: 1389


🏁 Script executed:

#!/bin/bash
set -e

echo "Extracting full `apply_chat_template` from `tensorrt_llm/llmapi/tokenizer.py`:"
rg -n "def apply_chat_template" -C10 tensorrt_llm/llmapi/tokenizer.py

echo
echo "Extracting `get_chat_template` and `apply_chat_template` from `tensorrt_llm/inputs/utils.py`:"
rg -n "def get_chat_template" -C10 tensorrt_llm/inputs/utils.py
rg -n "def apply_chat_template" -C10 tensorrt_llm/inputs/utils.py

Length of output: 1986


🏁 Script executed:

#!/bin/bash
set -e

echo "Showing `get_chat_template` definition in tensorrt_llm/inputs/utils.py:"
rg -n 'def get_chat_template' -C5 tensorrt_llm/inputs/utils.py

echo
echo "Showing `apply_chat_template` definition in tensorrt_llm/inputs/utils.py:"
rg -n 'def apply_chat_template' -C5 tensorrt_llm/inputs/utils.py

Length of output: 317


🏁 Script executed:

#!/bin/bash
set -e

echo "Locating TokenizerBase and its chat-template methods in inputs/utils.py"
rg -n "class TokenizerBase" -C5 tensorrt_llm/inputs/utils.py

echo
echo "Definition of get_chat_template in inputs/utils.py:"
rg -n "def get_chat_template" -C10 tensorrt_llm/inputs/utils.py

echo
echo "Definition of apply_chat_template in inputs/utils.py:"
rg -n "def apply_chat_template" -C10 tensorrt_llm/inputs/utils.py

Length of output: 224


#!/bin/bash
set -e

echo "Displaying lines 400-460 from tensorrt_llm/inputs/utils.py:"
sed -n '400,460p' tensorrt_llm/inputs/utils.py

Guard chat-template usage and limit new_prompts scope in quickstart_advanced.py

Move the declaration of new_prompts into the if block and wrap chat‐template calls in a try/except to skip unsupported tokenizers instead of crashing:

     if args.apply_chat_template:
-        # new_prompts declared too early
-        new_prompts = []
+        try:
+            # Ensure this tokenizer actually supports chat templates
+            _ = llm.tokenizer.get_chat_template()
+            new_prompts = []
             for prompt in prompts:
                 messages = [{"role": "user", "content": prompt}]
                 new_prompts.append(
                     llm.tokenizer.apply_chat_template(
                         messages,
                         tokenize=False,
                         add_generation_prompt=True,
                     )
                 )
             prompts = new_prompts
-        # no handling if unsupported → crash
+        except Exception as e:
+            # Skip templating on unsupported tokenizers
+            # (or replace print with logger.warning if preferred)
+            print(f"[warn] --apply_chat_template ignored: {e}")
     outputs = llm.generate(prompts, sampling_params)

If you’d rather use structured logging, add at the top:

from tensorrt_llm.logger import logger

and replace print(...) with:

logger.warning(f"--apply_chat_template ignored: {e}")
🤖 Prompt for AI Agents
In examples/llm-api/quickstart_advanced.py around lines 279 to 288, the variable
new_prompts is declared outside the chat-template guard and chat-template calls
can raise for tokenizers that don't support it; move the declaration of
new_prompts inside the if args.apply_chat_template block so its scope is
limited, and wrap the per-prompt llm.tokenizer.apply_chat_template call in a
try/except that catches the tokenizer error and continues (skipping that prompt
or appending the original prompt) instead of letting the script crash;
optionally import the package logger at the top and replace any print warnings
with logger.warning messages.

from .modeling_gemma3 import Gemma3ForCausalLM
from .modeling_gemma3vl import Gemma3VLM
from .modeling_gpt_oss import GptOssForCausalLM
from .modeling_hunyuan_moe import HunYuanMoEV1ForCausalLM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing import for Gemma3Model while exporting it in all

"Gemma3Model" is added to all but is not imported, which will break direct imports like from ...models import Gemma3Model. Import it from modeling_gemma3.

Apply this diff:

-from .modeling_gemma3 import Gemma3ForCausalLM
+from .modeling_gemma3 import Gemma3ForCausalLM, Gemma3Model

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In tensorrt_llm/_torch/models/__init__.py around line 11, add an import for
Gemma3Model so the symbol exported in __all__ is actually available;
specifically import Gemma3Model from .modeling_gemma3 (e.g. add the line `from
.modeling_gemma3 import Gemma3Model`) so `from ...models import Gemma3Model`
works as expected.

Comment on lines +96 to +170
class HunYuanAttention(Attention):

def __init__(
self,
model_config: ModelConfig[PretrainedConfig],
layer_idx: Optional[int] = None,
use_qk_norm: bool = True,
nope_layer: bool = False,
aux_stream: Optional[torch.cuda.Stream] = None,
):
config = model_config.pretrained_config

self.use_rope = not nope_layer
pos_embd_params = PositionalEmbeddingParams(
type=PositionEmbeddingType.rope_gpt_neox,
rope=RopeParams.from_config(config),
is_neox=True,
) if self.use_rope else None
self.use_qk_norm = use_qk_norm

super().__init__(
hidden_size=config.hidden_size,
num_attention_heads=config.num_attention_heads,
num_key_value_heads=config.num_key_value_heads,
max_position_embeddings=config.max_position_embeddings,
bias=config.attention_bias,
pos_embd_params=pos_embd_params,
qk_norm_type=QkNormType.post_rope
if use_qk_norm else QkNormType.none,
layer_idx=layer_idx,
dtype=config.torch_dtype,
config=model_config,
)

self.head_dim = config.hidden_size // config.num_attention_heads
self.query_layernorm = RMSNorm(hidden_size=self.head_dim,
eps=config.rms_norm_eps,
dtype=config.torch_dtype)
self.key_layernorm = RMSNorm(hidden_size=self.head_dim,
eps=config.rms_norm_eps,
dtype=config.torch_dtype)
self.aux_stream = aux_stream
self.ln_events = [torch.cuda.Event(), torch.cuda.Event()]

def apply_rope(self, q: torch.Tensor, k: Optional[torch.Tensor],
v: Optional[torch.Tensor], position_ids: torch.Tensor):
q, k, v = self.split_qkv(q, k, v)
if position_ids is not None:
q, k, v = super().apply_rope(q, k, v, position_ids)
# Llama4 applies QK norm after RoPE.
if self.use_qk_norm:
q, k = self.apply_qk_norm(q, k)

return q, k, v

def apply_qk_norm(self, q, k):

def q_l2norm():
return self.query_layernorm(q.reshape(-1, self.head_dim)).reshape(
-1, self.q_size)

def k_l2norm():
return self.key_layernorm(k.reshape(-1, self.head_dim)).reshape(
-1, self.kv_size)

q, k = maybe_execute_in_parallel(
q_l2norm,
k_l2norm,
self.ln_events[0],
self.ln_events[1],
self.aux_stream,
)

return q, k

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Verify auxiliary stream usage in apply_qk_norm

The apply_qk_norm method uses auxiliary streams for parallel execution but doesn't check if self.aux_stream is None before use.

     def apply_qk_norm(self, q, k):
 
         def q_l2norm():
             return self.query_layernorm(q.reshape(-1, self.head_dim)).reshape(
                 -1, self.q_size)
 
         def k_l2norm():
             return self.key_layernorm(k.reshape(-1, self.head_dim)).reshape(
                 -1, self.kv_size)
 
-        q, k = maybe_execute_in_parallel(
-            q_l2norm,
-            k_l2norm,
-            self.ln_events[0],
-            self.ln_events[1],
-            self.aux_stream,
-        )
+        if self.aux_stream is not None:
+            q, k = maybe_execute_in_parallel(
+                q_l2norm,
+                k_l2norm,
+                self.ln_events[0],
+                self.ln_events[1],
+                self.aux_stream,
+            )
+        else:
+            q = q_l2norm()
+            k = k_l2norm()
 
         return q, k
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class HunYuanAttention(Attention):
def __init__(
self,
model_config: ModelConfig[PretrainedConfig],
layer_idx: Optional[int] = None,
use_qk_norm: bool = True,
nope_layer: bool = False,
aux_stream: Optional[torch.cuda.Stream] = None,
):
config = model_config.pretrained_config
self.use_rope = not nope_layer
pos_embd_params = PositionalEmbeddingParams(
type=PositionEmbeddingType.rope_gpt_neox,
rope=RopeParams.from_config(config),
is_neox=True,
) if self.use_rope else None
self.use_qk_norm = use_qk_norm
super().__init__(
hidden_size=config.hidden_size,
num_attention_heads=config.num_attention_heads,
num_key_value_heads=config.num_key_value_heads,
max_position_embeddings=config.max_position_embeddings,
bias=config.attention_bias,
pos_embd_params=pos_embd_params,
qk_norm_type=QkNormType.post_rope
if use_qk_norm else QkNormType.none,
layer_idx=layer_idx,
dtype=config.torch_dtype,
config=model_config,
)
self.head_dim = config.hidden_size // config.num_attention_heads
self.query_layernorm = RMSNorm(hidden_size=self.head_dim,
eps=config.rms_norm_eps,
dtype=config.torch_dtype)
self.key_layernorm = RMSNorm(hidden_size=self.head_dim,
eps=config.rms_norm_eps,
dtype=config.torch_dtype)
self.aux_stream = aux_stream
self.ln_events = [torch.cuda.Event(), torch.cuda.Event()]
def apply_rope(self, q: torch.Tensor, k: Optional[torch.Tensor],
v: Optional[torch.Tensor], position_ids: torch.Tensor):
q, k, v = self.split_qkv(q, k, v)
if position_ids is not None:
q, k, v = super().apply_rope(q, k, v, position_ids)
# Llama4 applies QK norm after RoPE.
if self.use_qk_norm:
q, k = self.apply_qk_norm(q, k)
return q, k, v
def apply_qk_norm(self, q, k):
def q_l2norm():
return self.query_layernorm(q.reshape(-1, self.head_dim)).reshape(
-1, self.q_size)
def k_l2norm():
return self.key_layernorm(k.reshape(-1, self.head_dim)).reshape(
-1, self.kv_size)
q, k = maybe_execute_in_parallel(
q_l2norm,
k_l2norm,
self.ln_events[0],
self.ln_events[1],
self.aux_stream,
)
return q, k
def apply_qk_norm(self, q, k):
def q_l2norm():
return self.query_layernorm(q.reshape(-1, self.head_dim)).reshape(
-1, self.q_size)
def k_l2norm():
return self.key_layernorm(k.reshape(-1, self.head_dim)).reshape(
-1, self.kv_size)
if self.aux_stream is not None:
q, k = maybe_execute_in_parallel(
q_l2norm,
k_l2norm,
self.ln_events[0],
self.ln_events[1],
self.aux_stream,
)
else:
q = q_l2norm()
k = k_l2norm()
return q, k
🤖 Prompt for AI Agents
In tensorrt_llm/_torch/models/modeling_hunyuan_moe.py around lines 96-170,
apply_qk_norm unconditionally uses maybe_execute_in_parallel with
self.aux_stream and cuda events even when aux_stream is None; modify the method
to check if self.aux_stream is not None and only call maybe_execute_in_parallel
with the two events and aux_stream in that case, otherwise execute q_l2norm()
and k_l2norm() synchronously (or in a simple sequential manner) and return their
results, ensuring no cuda stream or event is dereferenced when aux_stream is
None.

Comment on lines +141 to +149
v: Optional[torch.Tensor], position_ids: torch.Tensor):
q, k, v = self.split_qkv(q, k, v)
if position_ids is not None:
q, k, v = super().apply_rope(q, k, v, position_ids)
# Llama4 applies QK norm after RoPE.
if self.use_qk_norm:
q, k = self.apply_qk_norm(q, k)

return q, k, v

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Inconsistent method signature with parent class

The apply_rope method has a different signature than the parent class, missing the return type hint.

     def apply_rope(self, q: torch.Tensor, k: Optional[torch.Tensor],
-                   v: Optional[torch.Tensor], position_ids: torch.Tensor):
+                   v: Optional[torch.Tensor], position_ids: torch.Tensor) -> tuple:
         q, k, v = self.split_qkv(q, k, v)
         if position_ids is not None:
             q, k, v = super().apply_rope(q, k, v, position_ids)
         # Llama4 applies QK norm after RoPE.
         if self.use_qk_norm:
             q, k = self.apply_qk_norm(q, k)
 
         return q, k, v
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
v: Optional[torch.Tensor], position_ids: torch.Tensor):
q, k, v = self.split_qkv(q, k, v)
if position_ids is not None:
q, k, v = super().apply_rope(q, k, v, position_ids)
# Llama4 applies QK norm after RoPE.
if self.use_qk_norm:
q, k = self.apply_qk_norm(q, k)
return q, k, v
def apply_rope(self, q: torch.Tensor, k: Optional[torch.Tensor],
v: Optional[torch.Tensor], position_ids: torch.Tensor) -> tuple:
q, k, v = self.split_qkv(q, k, v)
if position_ids is not None:
q, k, v = super().apply_rope(q, k, v, position_ids)
# Llama4 applies QK norm after RoPE.
if self.use_qk_norm:
q, k = self.apply_qk_norm(q, k)
return q, k, v
🤖 Prompt for AI Agents
In tensorrt_llm/_torch/models/modeling_hunyuan_moe.py around lines 141-149, the
apply_rope method signature is missing the explicit return type hint and
therefore is inconsistent with the parent class; update the method signature to
include the same return type annotation as the parent (e.g., ->
Tuple[torch.Tensor, torch.Tensor, Optional[torch.Tensor]]), ensure typing.Tuple
and typing.Optional are imported (or use from typing import Tuple, Optional),
and keep the existing parameter types unchanged so the implementation matches
the parent's contract.

hidden_size=model_config.pretrained_config.hidden_size,
vocab_size=model_config.pretrained_config.vocab_size)
self._execution_stats = None
print("---debug model_config: ", model_config)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove debug print statement

Debug print statement should be removed from production code.

         self._execution_stats = None
-        print("---debug model_config: ", model_config)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
print("---debug model_config: ", model_config)
self._execution_stats = None
🤖 Prompt for AI Agents
In tensorrt_llm/_torch/models/modeling_hunyuan_moe.py around line 344, remove
the debug print statement ("print(\"---debug model_config: \", model_config)")
from production code; replace it with no output or, if needed for future
debugging, use the configured logger at an appropriate level (e.g.,
logger.debug(...)) so the information can be enabled/disabled via logging
configuration.

Comment on lines +4737 to 4746
elif scale_type == RotaryScalingType.dynamic:
# Make sure scaling_alpha exists in rope_scaling
# Ref: https://huggingface.co/tencent/Hunyuan-A13B-Instruct-FP8/blob/main/modeling_hunyuan.py#L346
assert rope_scaling_config[
"alpha"] is not None, "rope_scaling_config.alpha must be provided."
scaling_alpha = rope_scaling_config["alpha"]
adjusted_base = theta * (scaling_alpha**(dim / (dim - 2)))
inv_freq = 1.0 / (adjusted_base**(
np.arange(0, dim, 2, dtype=dtype) / dim)).astype(dtype)
else:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Dynamic RoPE: guard against None rope_scaling_config and invalid dim; fix potential crash

The current code indexes rope_scaling_config["alpha"] without first ensuring rope_scaling_config is not None. This can raise a TypeError before the assert runs. Also, dim == 2 leads to division by zero in dim/(dim-2). Enforce preconditions before computing adjusted_base.

Apply this diff:

-        elif scale_type == RotaryScalingType.dynamic:
-            # Make sure scaling_alpha exists in rope_scaling
-            # Ref: https://huggingface.co/tencent/Hunyuan-A13B-Instruct-FP8/blob/main/modeling_hunyuan.py#L346
-            assert rope_scaling_config[
-                "alpha"] is not None, "rope_scaling_config.alpha must be provided."
-            scaling_alpha = rope_scaling_config["alpha"]
-            adjusted_base = theta * (scaling_alpha**(dim / (dim - 2)))
-            inv_freq = 1.0 / (adjusted_base**(
-                np.arange(0, dim, 2, dtype=dtype) / dim)).astype(dtype)
+        elif scale_type == RotaryScalingType.dynamic:
+            # Ref: https://huggingface.co/tencent/Hunyuan-A13B-Instruct-FP8/blob/main/modeling_hunyuan.py#L346
+            assert rope_scaling_config is not None, "rope_scaling_config must be provided for dynamic rotary scaling."
+            scaling_alpha = rope_scaling_config.get("alpha", None)
+            assert scaling_alpha is not None, "rope_scaling_config['alpha'] must be provided for dynamic rotary scaling."
+            assert scaling_alpha > 0.0, "rope_scaling_config['alpha'] must be > 0.0."
+            assert dim > 2, "dim must be > 2 for dynamic rotary scaling (to avoid division by zero)."
+            adjusted_base = theta * (scaling_alpha ** (dim / (dim - 2)))
+            inv_freq = (1.0 / (adjusted_base ** (np.arange(0, dim, 2) / dim))).astype(dtype)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
elif scale_type == RotaryScalingType.dynamic:
# Make sure scaling_alpha exists in rope_scaling
# Ref: https://huggingface.co/tencent/Hunyuan-A13B-Instruct-FP8/blob/main/modeling_hunyuan.py#L346
assert rope_scaling_config[
"alpha"] is not None, "rope_scaling_config.alpha must be provided."
scaling_alpha = rope_scaling_config["alpha"]
adjusted_base = theta * (scaling_alpha**(dim / (dim - 2)))
inv_freq = 1.0 / (adjusted_base**(
np.arange(0, dim, 2, dtype=dtype) / dim)).astype(dtype)
else:
elif scale_type == RotaryScalingType.dynamic:
# Ref: https://huggingface.co/tencent/Hunyuan-A13B-Instruct-FP8/blob/main/modeling_hunyuan.py#L346
assert rope_scaling_config is not None, "rope_scaling_config must be provided for dynamic rotary scaling."
scaling_alpha = rope_scaling_config.get("alpha", None)
assert scaling_alpha is not None, "rope_scaling_config['alpha'] must be provided for dynamic rotary scaling."
assert scaling_alpha > 0.0, "rope_scaling_config['alpha'] must be > 0.0."
assert dim > 2, "dim must be > 2 for dynamic rotary scaling (to avoid division by zero)."
adjusted_base = theta * (scaling_alpha ** (dim / (dim - 2)))
inv_freq = (1.0 / (adjusted_base ** (np.arange(0, dim, 2) / dim))).astype(dtype)
else:
🤖 Prompt for AI Agents
In tensorrt_llm/functional.py around lines 4737 to 4746, the dynamic RoPE branch
currently indexes rope_scaling_config["alpha"] without ensuring
rope_scaling_config is not None and performs dim/(dim-2) which will divide by
zero when dim == 2; before computing adjusted_base, add explicit guards: verify
rope_scaling_config is not None, verify "alpha" is present and not None (or
raise a ValueError with a clear message), and validate that dim > 2 (raise
ValueError if not); only after these preconditions compute scaling_alpha,
adjusted_base, and inv_freq so the assert cannot be bypassed and
division-by-zero is prevented.

Comment on lines +361 to 365
except Exception as e:
logger.warning(
f"Failed to load hf tokenizer from {model_dir}, encounter error: {e}"
)
return None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Narrow the exception scope and include traceback for debuggability

Limit the caught exceptions to expected loader failures and include the traceback in logs for easier diagnostics.

-    except Exception as e:
-        logger.warning(
-            f"Failed to load hf tokenizer from {model_dir}, encounter error: {e}"
-        )
+    except (OSError, ValueError, RuntimeError) as e:
+        logger.warning(
+            f"Failed to load HF tokenizer from {model_dir}: {e}",
+            exc_info=True,
+        )
         return None
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
except Exception as e:
logger.warning(
f"Failed to load hf tokenizer from {model_dir}, encounter error: {e}"
)
return None
except (OSError, ValueError, RuntimeError) as e:
logger.warning(
f"Failed to load HF tokenizer from {model_dir}: {e}",
exc_info=True,
)
return None
🤖 Prompt for AI Agents
In tensorrt_llm/llmapi/tokenizer.py around lines 361 to 365, the current except
Exception is too broad and omits traceback; change it to catch only the expected
loader errors (for example OSError, ValueError, and any specific
tokenizer/transformers exceptions used in your project) and log the failure with
traceback by using logger.exception(...) or logger.warning(..., exc_info=True)
so the error message includes the stacktrace, then return None as before.

Comment thread tensorrt_llm/_torch/models/__init__.py Outdated
q, k = self.rotary_emb(position_ids, [q, k])
if self.qk_norm_type == QkNormType.post_rope:
q, k = self.apply_qk_norm(q, k)
#q, k, v = self.apply_rope(q, k, v, position_ids)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep the apply_rope?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep the apply_rope?

ok

Comment thread tensorrt_llm/_torch/model_config.py Outdated
Signed-off-by: sorenwu <sorenwu@tencent.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🔭 Outside diff range comments (4)
tensorrt_llm/_torch/model_config.py (3)

10-16: Duplicate logger import shadows the earlier symbol

You import logger twice (module vs. object), which is ambiguous and can lead to confusion or subtle bugs due to shadowing.

Apply this diff to keep a single unambiguous import:

-from tensorrt_llm import logger
 from tensorrt_llm._torch.pyexecutor.config_utils import is_nemotron_hybrid
 from tensorrt_llm._utils import get_sm_version, torch_dtype_to_binding
 from tensorrt_llm.bindings import LayerType as LayerTypeCpp
 from tensorrt_llm.functional import AllReduceStrategy
 from tensorrt_llm.logger import logger

163-171: Bug: Attribute name mismatch (per_layer_quant_configs vs. quant_config_dict)

get_quant_config references self.per_layer_quant_configs, but the dataclass defines quant_config_dict and from_pretrained sets quant_config_dict=layer_quant_config. This will raise at runtime.

Use the correct attribute name:

     def get_quant_config(self, name: Optional[str] = None) -> QuantConfig:
-        if name is None or self.per_layer_quant_configs is None:
+        if name is None or self.quant_config_dict is None:
             return self.quant_config

-        if name in self.per_layer_quant_configs:
-            return self.per_layer_quant_configs[name]
+        if name in self.quant_config_dict:
+            return self.quant_config_dict[name]

1-5: Missing NVIDIA copyright header

Per repository guidelines, prepend the NVIDIA copyright header.

Apply at the top of the file:

+# Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved.
+
 import json
 import os
 from dataclasses import dataclass, field
 from pathlib import Path
 from typing import Dict, Generic, List, Optional, TypeVar
tensorrt_llm/_torch/pyexecutor/config_utils.py (1)

1-1: Missing NVIDIA copyright header

Please prepend the standard header.

+# Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved.
+
 def is_nemotron_hybrid(config):
🧹 Nitpick comments (1)
tensorrt_llm/_torch/model_config.py (1)

154-161: MLA gating uses truthiness; confirm 0-valued configs are intended to disable MLA

Current check treats 0 as False. If kv_lora_rank=0 is a valid config that should still allow the head_dim computation and gating, this change alters behavior vs. the prior hasattr/is-not-None semantics.

Consider switching to explicit None checks to preserve previous behavior, or clearly enforce >0 if that’s the intent.

-            if getattr(self.pretrained_config,
-                       "kv_lora_rank", None) and getattr(
-                           self.pretrained_config, "qk_rope_head_dim", None):
+            if (getattr(self.pretrained_config, "kv_lora_rank", None) is not None
+                    and getattr(self.pretrained_config, "qk_rope_head_dim", None) is not None):
                 head_dim = self.pretrained_config.kv_lora_rank + self.pretrained_config.qk_rope_head_dim
                 if head_dim == 576 and torch.cuda.get_device_capability() == (
                         9, 0):
                     return True

Optionally, centralize this check by reusing is_mla(config) to avoid duplication and drift with pyexecutor/config_utils.py:

# outside selected lines (import near the existing is_nemotron_hybrid import)
from tensorrt_llm._torch.pyexecutor.config_utils import is_mla
# ...then...
if self.attn_backend == 'TRTLLM':
    if is_mla(self.pretrained_config):
        ...
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0ac773c and 387818d.

📒 Files selected for processing (5)
  • tensorrt_llm/_torch/model_config.py (1 hunks)
  • tensorrt_llm/_torch/models/__init__.py (2 hunks)
  • tensorrt_llm/_torch/models/modeling_hunyuan_moe.py (1 hunks)
  • tensorrt_llm/_torch/modules/attention.py (1 hunks)
  • tensorrt_llm/_torch/pyexecutor/config_utils.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • tensorrt_llm/_torch/modules/attention.py
  • tensorrt_llm/_torch/models/init.py
  • tensorrt_llm/_torch/models/modeling_hunyuan_moe.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)

**/*.py: Python code must target Python 3.8+
Python indentation: 4 spaces, no tabs
Maintain module namespace in imports (from package.subpackage import foo; then use foo.SomeClass())
Python file names use snake_case
Python class names use PascalCase
Python functions/methods and local variables use snake_case; variables starting with a number get k_ prefix (e.g., k_99th_percentile)
Global variables use G_ prefixed UPPER_SNAKE_CASE (e.g., G_MY_GLOBAL)
Constants use UPPER_SNAKE_CASE in Python
Avoid shadowing variables from outer scopes in Python
Initialize all externally visible members of a Python class in init
Prefer docstrings for interfaces used outside a file; comments for local code
Use Google-style docstrings for classes and functions (Sphinx-parsable)
Document attributes/variables inline with short docstrings
Avoid reflection when simple alternatives exist (e.g., prefer explicit parameters over dict(**locals()))
In try/except, catch the narrowest exceptions possible
For duck-typing with try/except, keep try body minimal and put logic in else

Files:

  • tensorrt_llm/_torch/model_config.py
  • tensorrt_llm/_torch/pyexecutor/config_utils.py
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}

📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)

Prepend NVIDIA copyright header (current year) to all source files

Files:

  • tensorrt_llm/_torch/model_config.py
  • tensorrt_llm/_torch/pyexecutor/config_utils.py
⏰ 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). (1)
  • GitHub Check: Pre-commit Check

Comment on lines +8 to 11
if getattr(config, "kv_lora_rank", None) and getattr(
config, "qk_rope_head_dim", None):
return True
return False

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Make is_mla robust to 0 values and match intended semantics

Truthiness means kv_lora_rank=0 or qk_rope_head_dim=0 will disable MLA. If the intent is “exists and not None,” change to explicit None-check; if the intent is “> 0,” enforce that. This also keeps behavior aligned with model_config.enable_flash_mla.

Suggested fix for the “not None” semantics:

-def is_mla(config):
-    if getattr(config, "kv_lora_rank", None) and getattr(
-            config, "qk_rope_head_dim", None):
-        return True
-    return False
+def is_mla(config):
+    return (
+        getattr(config, "kv_lora_rank", None) is not None
+        and getattr(config, "qk_rope_head_dim", None) is not None
+    )

If the intent is “strictly positive,” use:

return (
    (getattr(config, "kv_lora_rank", None) or 0) > 0
    and (getattr(config, "qk_rope_head_dim", None) or 0) > 0
)

Please confirm which semantics you want so we can update model_config.enable_flash_mla consistently.


🏁 Script executed:

#!/bin/bash
set -e

echo "=== Preview of tensorrt_llm/_torch/pyexecutor/config_utils.py ==="
sed -n '1,200p' tensorrt_llm/_torch/pyexecutor/config_utils.py || echo "File not found"

echo
echo "=== Definition of is_mla ==="
rg -n "def is_mla" -A5 tensorrt_llm/_torch/pyexecutor/config_utils.py || echo "No definition found"

echo
echo "=== Occurrences of model_config.enable_flash_mla ==="
rg -n "enable_flash_mla" -A5 -t py || echo "No occurrences found"

echo
echo "=== Calls to is_mla in the repository ==="
rg -n "is_mla" -A5 -t py || echo "No usages found"

Length of output: 30832


🏁 Script executed:

#!/bin/bash
set -e

echo "=== enable_flash_mla implementation in tensorrt_llm/_torch/model_config.py ==="
sed -n '140,200p' tensorrt_llm/_torch/model_config.py

Length of output: 2663


Enforce “> 0” Head-Dim Checks in is_mla
The rest of the code (e.g. attention_backend/utils assertions and model_config.enable_flash_mla) assumes kv_lora_rank and qk_rope_head_dim must both be strictly positive. Update is_mla accordingly.

• File to update: tensorrt_llm/_torch/pyexecutor/config_utils.py
• Replace the truthiness check with explicit “> 0” semantics, guarding against None or 0.

Suggested diff:

-def is_mla(config):
-    if getattr(config, "kv_lora_rank", None) and getattr(
-            config, "qk_rope_head_dim", None):
-        return True
-    return False
+def is_mla(config):
+    # Require both ranks to be strictly positive (None or 0 means “not MLA”)
+    kv_rank = getattr(config, "kv_lora_rank", None) or 0
+    qk_dim = getattr(config, "qk_rope_head_dim", None) or 0
+    return kv_rank > 0 and qk_dim > 0
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if getattr(config, "kv_lora_rank", None) and getattr(
config, "qk_rope_head_dim", None):
return True
return False
def is_mla(config):
# Require both ranks to be strictly positive (None or 0 means “not MLA”)
kv_rank = getattr(config, "kv_lora_rank", None) or 0
qk_dim = getattr(config, "qk_rope_head_dim", None) or 0
return kv_rank > 0 and qk_dim > 0
🤖 Prompt for AI Agents
In tensorrt_llm/_torch/pyexecutor/config_utils.py around lines 8 to 11, the
is_mla check uses truthiness for config.kv_lora_rank and config.qk_rope_head_dim
but callers expect they be strictly positive; change the condition to explicitly
verify both values are greater than 0 (e.g., use getattr(config, "kv_lora_rank",
0) > 0 and getattr(config, "qk_rope_head_dim", 0) > 0) so None or 0 are treated
as false and only strictly positive values return True.

@byshiue

byshiue commented Aug 13, 2025

Copy link
Copy Markdown
Collaborator

/bot run

1 similar comment
@byshiue

byshiue commented Aug 13, 2025

Copy link
Copy Markdown
Collaborator

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #15060 [ run ] triggered by Bot

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #15060 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #11371 completed with status: 'FAILURE'

@byshiue

byshiue commented Aug 13, 2025

Copy link
Copy Markdown
Collaborator

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #15089 [ run ] triggered by Bot

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #15089 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #11392 completed with status: 'FAILURE'

@byshiue

byshiue commented Aug 13, 2025

Copy link
Copy Markdown
Collaborator

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #15102 [ run ] triggered by Bot

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #15102 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #11402 completed with status: 'FAILURE'

@byshiue

byshiue commented Aug 13, 2025

Copy link
Copy Markdown
Collaborator

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #15193 [ run ] triggered by Bot

@byshiue

byshiue commented Aug 14, 2025

Copy link
Copy Markdown
Collaborator

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #15298 [ run ] triggered by Bot

@QiJune QiJune left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #15298 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #11550 completed with status: 'SUCCESS'
Pipeline passed with automatic retried tests. Check the rerun report for details.

@byshiue

byshiue commented Aug 14, 2025

Copy link
Copy Markdown
Collaborator

There are too many conservation due to wrong commit at the beginning. So, I bypass the merge after checking the comments we give, and ignore the comments of robot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community Engagement help/insights needed from community Community want to contribute PRs initiated from Community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants