Skip to content

Reiew pr #1159 and refactor codes#1231

Merged
helloyongyang merged 16 commits into
mainfrom
dev_pr1159
Jul 9, 2026
Merged

Reiew pr #1159 and refactor codes#1231
helloyongyang merged 16 commits into
mainfrom
dev_pr1159

Conversation

@Watebear

@Watebear Watebear commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces support for the RainFusion sparse attention acceleration algorithm on Ascend NPU, along with NPU-optimized implementations of LayerNorm, RMSNorm, RoPE, and Flash Attention. It also adds NPU support for RIFE video frame interpolation and provides configuration and execution scripts for the Wan2.2 MoE I2V Distill pipeline. The review feedback highlights critical issues where torch.npu.is_available() is called without importing torch_npu first, which will lead to runtime errors. Additionally, a layout mismatch was identified in the NPU RainFusion dense attention operator, and a cleaner tensor flattening approach was suggested in the RainFusion attention implementation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +1 to +6
import importlib.util

import torch

device = torch.device("cuda" if torch.cuda.is_available() else "cpu")
_HAS_NPU = importlib.util.find_spec("torch_npu") is not None and torch.npu.is_available()
device = torch.device("cuda") if torch.cuda.is_available() else (torch.device("npu") if _HAS_NPU else torch.device("cpu"))

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.

high

Using importlib.util.find_spec("torch_npu") to check for NPU availability without actually importing torch_npu will result in an AttributeError: module 'torch' has no attribute 'npu' when torch.npu.is_available() is called. In PyTorch, the npu attribute is dynamically registered on the torch module only after torch_npu is imported. Please import torch_npu inside a try-except block to safely check NPU availability.

Suggested change
import importlib.util
import torch
device = torch.device("cuda" if torch.cuda.is_available() else "cpu")
_HAS_NPU = importlib.util.find_spec("torch_npu") is not None and torch.npu.is_available()
device = torch.device("cuda") if torch.cuda.is_available() else (torch.device("npu") if _HAS_NPU else torch.device("cpu"))
import torch
try:
import torch_npu
_HAS_NPU = torch.npu.is_available()
except ImportError:
_HAS_NPU = False
device = torch.device("cuda") if torch.cuda.is_available() else (torch.device("npu") if _HAS_NPU else torch.device("cpu"))

Comment on lines +20 to +21
_HAS_NPU = importlib.util.find_spec("torch_npu") is not None and torch.npu.is_available()
self.device = torch.device("cuda") if torch.cuda.is_available() else (torch.device("npu") if _HAS_NPU else torch.device("cpu"))

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.

high

Using importlib.util.find_spec("torch_npu") to check for NPU availability without actually importing torch_npu will result in an AttributeError: module 'torch' has no attribute 'npu' when torch.npu.is_available() is called. In PyTorch, the npu attribute is dynamically registered on the torch module only after torch_npu is imported. Please import torch_npu inside a try-except block to safely check NPU availability.

Suggested change
_HAS_NPU = importlib.util.find_spec("torch_npu") is not None and torch.npu.is_available()
self.device = torch.device("cuda") if torch.cuda.is_available() else (torch.device("npu") if _HAS_NPU else torch.device("cpu"))
try:
import torch_npu
_HAS_NPU = torch.npu.is_available()
except ImportError:
_HAS_NPU = False
self.device = torch.device("cuda") if torch.cuda.is_available() else (torch.device("npu") if _HAS_NPU else torch.device("cpu"))

Comment on lines +10 to +11
_HAS_NPU = importlib.util.find_spec("torch_npu") is not None and torch.npu.is_available()
device = torch.device("cuda") if torch.cuda.is_available() else (torch.device("npu") if _HAS_NPU else torch.device("cpu"))

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.

high

Using importlib.util.find_spec("torch_npu") to check for NPU availability without actually importing torch_npu will result in an AttributeError: module 'torch' has no attribute 'npu' when torch.npu.is_available() is called. In PyTorch, the npu attribute is dynamically registered on the torch module only after torch_npu is imported. Please import torch_npu inside a try-except block to safely check NPU availability.

try:
    import torch_npu
    _HAS_NPU = torch.npu.is_available()
except ImportError:
    _HAS_NPU = False
device = torch.device("cuda") if torch.cuda.is_available() else (torch.device("npu") if _HAS_NPU else torch.device("cpu"))

Comment on lines +27 to +28
def dense_attention(self, q, k, v):
return attention_forward(q, k, v, opt_mode="manual", op_type="ascend_laser_attention", layout="BNSD")

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.

high

In dense_attention, layout="BNSD" is passed to attention_forward, but the input tensors q, k, v are in BSND layout (shape (batch, seqlen, num_heads, head_dim)). This layout mismatch will cause incorrect results or crashes. Since attention_forward supports BSND layout, please change layout="BNSD" to layout="BSND".

Suggested change
def dense_attention(self, q, k, v):
return attention_forward(q, k, v, opt_mode="manual", op_type="ascend_laser_attention", layout="BNSD")
def dense_attention(self, q, k, v):
return attention_forward(q, k, v, opt_mode="manual", op_type="ascend_laser_attention", layout="BSND")

)
out = self.do_tensor_inv_rearrange(out, h_res_len, w_res_len)

out = out.squeeze(0).reshape(out.shape[1], -1)

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.

medium

Using out.shape[1] of the pre-squeezed tensor in the same expression where out is reassigned is slightly confusing and prone to bugs if the tensor shape changes. It is much cleaner and more idiomatic to use out = out.squeeze(0).flatten(1).

Suggested change
out = out.squeeze(0).reshape(out.shape[1], -1)
out = out.squeeze(0).flatten(1)

@helloyongyang helloyongyang merged commit 6f436da into main Jul 9, 2026
2 checks passed
@helloyongyang helloyongyang deleted the dev_pr1159 branch July 9, 2026 03:21
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.

3 participants