Skip to content

feat: Add pp support for hybrid attn/mamba model - #4358

Merged
yuxianq merged 3 commits into
NVIDIA:mainfrom
yuxianq:pp-hybrid
May 19, 2025
Merged

feat: Add pp support for hybrid attn/mamba model#4358
yuxianq merged 3 commits into
NVIDIA:mainfrom
yuxianq:pp-hybrid

Conversation

@yuxianq

@yuxianq yuxianq commented May 15, 2025

Copy link
Copy Markdown
Collaborator

This PR adds pp support for the hybrid attn/mamba model by introducing layer masks to KV cache manager.

  • KVCacheManager/MambaCacheManager/MambaHybridCacheManager support layer_mask now, so that each manager can manage selected layers only.
  • Make MambaHybridLayer/NemotronHLayer inherits from DecoderLayer to support pp.
  • Use absolute layer_idx instead of relative layer_idx in the modeling files, we will translate it to the relative one inside KV cache manager.

Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com>
Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com>
@yuxianq
yuxianq marked this pull request as ready for review May 16, 2025 05:49
@yuxianq

yuxianq commented May 16, 2025

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast --add-multi-gpu-test

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #5461 [ run ] triggered by Bot

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #5461 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #3984 completed with status: 'SUCCESS'

@amukkara amukkara 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

@vegaluisjose

Copy link
Copy Markdown
Collaborator

Thanks @yuxianq , this looks great.

It would be great, however, to see if quality changes with this feature. We have another PR #4147 with this test.

@suyoggupta

@suyoggupta

Copy link
Copy Markdown
Collaborator

could you please add a brief PR description?

@suyoggupta
suyoggupta requested a review from Copilot May 16, 2025 21:23

Copilot AI 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.

Pull Request Overview

This PR adds pp support for the hybrid attn/mamba model by introducing layer masks and updating related interfaces.

  • Adds generation of layer_mask and mamba_layer_mask in tests for configuration consistency.
  • Updates function signatures and inheritance (e.g. NemotronHLayer now inherits from DecoderLayer and requires position_ids).
  • Modifies cache manager initializations to propagate new layer_mask parameters.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/unittest/_torch/modeling/test_modeling_nemotron_h.py Adds layer_mask and mamba_layer_mask for testing hybrid override patterns.
tensorrt_llm/_torch/pyexecutor/resource_manager.py Updates get_pp_layers and init to accept and process layer_mask.
tensorrt_llm/_torch/pyexecutor/_util.py Adjusts cache manager creation to include the new layer_mask parameters.
tensorrt_llm/_torch/models/modeling_nemotron_h.py Changes NemotronHLayer to inherit from DecoderLayer and updates layer indexing and forward signature with position_ids.
tensorrt_llm/_torch/models/modeling_mamba_hybrid.py Updates MambaHybridLayer and cache manager initializations with layer_mask support.
examples/pytorch/out_of_tree_example/modeling_opt.py Updates forward methods to include position_ids for decoder layers.
Comments suppressed due to low confidence (1)

tensorrt_llm/_torch/models/modeling_nemotron_h.py:173

  • The new simple enumeration for layer indexing replaces previously separate counters for each layer type; please verify that this change in indexing logic is intentional and that it correctly supports layer-specific configurations.
for layer_idx, layer_type in enumerate(config.hybrid_override_pattern):

Comment thread tensorrt_llm/_torch/pyexecutor/resource_manager.py Outdated
@suyoggupta

Copy link
Copy Markdown
Collaborator

Added copilot to also review this PR

@yuxianq

yuxianq commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

@vegaluisjose I have cherry-pick #4147 to this PR and the new test can pass locally.

Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com>
@yuxianq

yuxianq commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

could you please add a brief PR description?

@suyoggupta Added.

@yuxianq
yuxianq enabled auto-merge (squash) May 19, 2025 06:33
@yuxianq

yuxianq commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

/bot reuse-pipeline

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #5681 [ reuse-pipeline ] triggered by Bot

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #5681 [ reuse-pipeline ] completed with state SUCCESS
Reusing PR_Github #5461 for commit ba33051

@yuxianq
yuxianq merged commit cf6cd94 into NVIDIA:main May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants