[Feat.]: Support 310P device run qwen2.5/3 dense and qwen2.5vl models - #5774
Tflowers-0129 wants to merge 7 commits into
Conversation
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
There was a problem hiding this comment.
Code Review
This pull request adds support for the 310P device, enabling it to run specific qwen models. The changes are well-structured, primarily introducing 310P-specific implementations within a new _310p directory and using conditional logic to activate them. My review identified a critical issue in the attention implementation where some attention states are unhandled, which could lead to incorrect behavior. Additionally, I've pointed out a high-severity issue concerning a hardcoded sequence length that might impact model compatibility. Addressing these points will improve the robustness and correctness of the 310P support.
| def forward_impl(self, query, key, value, kv_cache, attn_metadata, output): | ||
| if attn_metadata.attn_state == AscendAttentionState.DecodeOnly: | ||
| output = self.forward_paged_attention(query, attn_metadata, output) | ||
|
|
||
| if attn_metadata.attn_state == AscendAttentionState.PrefillNoCache: | ||
| num_tokens = query.shape[0] | ||
| q = query[:num_tokens] | ||
| k = key[:num_tokens] | ||
| v = value[:num_tokens] | ||
| out = self._forward_prefill_310p_fallback(q, k, v, attn_metadata, output) | ||
| output[:num_tokens] = out | ||
|
|
||
| return output |
There was a problem hiding this comment.
The forward_impl method only handles DecodeOnly and PrefillNoCache attention states. Other states from the AscendAttentionState enum, such as PrefillCacheHit, ChunkedPrefill, and SpecDecoding, are not handled. This will cause incorrect behavior when these attention states occur, as the function will return the output tensor without processing it. This is a critical bug that needs to be addressed. Additionally, the use of two separate if statements is incorrect; an if/elif structure should be used to ensure only one path is taken.
| def forward_impl(self, query, key, value, kv_cache, attn_metadata, output): | |
| if attn_metadata.attn_state == AscendAttentionState.DecodeOnly: | |
| output = self.forward_paged_attention(query, attn_metadata, output) | |
| if attn_metadata.attn_state == AscendAttentionState.PrefillNoCache: | |
| num_tokens = query.shape[0] | |
| q = query[:num_tokens] | |
| k = key[:num_tokens] | |
| v = value[:num_tokens] | |
| out = self._forward_prefill_310p_fallback(q, k, v, attn_metadata, output) | |
| output[:num_tokens] = out | |
| return output | |
| def forward_impl(self, query, key, value, kv_cache, attn_metadata, output): | |
| if attn_metadata.attn_state == AscendAttentionState.DecodeOnly: | |
| output = self.forward_paged_attention(query, attn_metadata, output) | |
| elif attn_metadata.attn_state == AscendAttentionState.PrefillNoCache: | |
| num_tokens = query.shape[0] | |
| q = query[:num_tokens] | |
| k = key[:num_tokens] | |
| v = value[:num_tokens] | |
| out = self._forward_prefill_310p_fallback(q, k, v, attn_metadata, output) | |
| output[:num_tokens] = out | |
| else: | |
| raise NotImplementedError( | |
| f"Attention state {attn_metadata.attn_state} is not yet supported in AscendAttentionBackendImpl310." | |
| ) | |
| return output |
| def get_splitfuse_attn_mask(self) -> torch.Tensor: | ||
| return self._get_fp16_mask(2048) | ||
|
|
||
| def get_attention_mask(self, model_config) -> torch.Tensor: | ||
| if getattr(model_config, "runner_type", None) == "pooling": | ||
| return self._base.get_attn_mask(2048, torch.bool) | ||
| return self.get_splitfuse_attn_mask() |
There was a problem hiding this comment.
The methods get_splitfuse_attn_mask and get_attention_mask use a hardcoded sequence length of 2048. This could lead to incorrect behavior or errors for models with a different maximum sequence length. It would be more robust to derive this value from the model_config, for example, by using model_config.max_model_len.
| def get_splitfuse_attn_mask(self) -> torch.Tensor: | |
| return self._get_fp16_mask(2048) | |
| def get_attention_mask(self, model_config) -> torch.Tensor: | |
| if getattr(model_config, "runner_type", None) == "pooling": | |
| return self._base.get_attn_mask(2048, torch.bool) | |
| return self.get_splitfuse_attn_mask() | |
| def get_splitfuse_attn_mask(self, max_seq_len: int) -> torch.Tensor: | |
| return self._get_fp16_mask(max_seq_len) | |
| def get_attention_mask(self, model_config) -> torch.Tensor: | |
| # Fallback to 2048 if max_model_len is not available. | |
| max_seq_len = getattr(model_config, "max_model_len", 2048) | |
| if getattr(model_config, "runner_type", None) == "pooling": | |
| return self._base.get_attn_mask(max_seq_len, torch.bool) | |
| return self.get_splitfuse_attn_mask(max_seq_len) |
Signed-off-by: Shaoxu Cheng <2906339855@qq.com>
What this PR does / why we need it?
Does this PR introduce any user-facing change?
How was this patch tested?