[Bugfix] fix custom op GmmSwigluQuantWeightNzTensorList #4593
Conversation
Signed-off-by: QianChenxi <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request includes a bug fix and a performance optimization. In vllm_ascend/platform.py, a duplicated path segment in the construction of CUSTOM_OPP_PATH is corrected, which is a necessary fix. In csrc/torch_binding.cpp, at::zeros is replaced with at::empty for output tensor allocation, which is a good performance improvement. However, I've identified a related critical issue that should be addressed.
| at::Tensor output = at::empty({m, n/2}, x.options().dtype(at::kChar)); | ||
| at::Tensor output_scale = at::empty({m}, x.options().dtype(at::kFloat)); | ||
| at::Tensor output_offset = at::empty({m}, x.options().dtype(at::kFloat)); |
There was a problem hiding this comment.
The size of these output tensors depends on n, which is calculated on line 567 from the weight TensorList (weight[0].sizes()[1]). This is unsafe because if the weight TensorList is empty, accessing weight[0] will cause a crash. It's crucial to add a check to ensure weight is not empty before its elements are accessed.
For example, you could add the following check before line 567:
TORCH_CHECK(!weight.empty(), "weight tensor list cannot be empty");
|
👋 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. |
ChenxiQ
force-pushed
the
br_custom_op_fix
branch
from
a34ddcf to
71cf0c4
Compare
ChenxiQ
changed the title
ChenxiQ
force-pushed
the
br_custom_op_fix
branch
3 times, most recently
from
b1c3922 to
29191fb
Compare
Signed-off-by: QianChenxi <[email protected]>
ChenxiQ
force-pushed
the
br_custom_op_fix
branch
from
29191fb to
5486458
Compare
| global _CUSTOM_OP_ENABLED | ||
|
|
||
| # set custom ops path | ||
| CUR_DIR = os.path.dirname(os.path.realpath(__file__)) |
There was a problem hiding this comment.
@wangxiyuan This line causes the test tests/e2e/multicard/test_offline_inference_distributed.py::test_models_distributed_QwQ to fail.
os.path.realpath involves system calls that are not supported by torch.compile (Dynamo) during graph capture. This triggers a runtime crash when graph mode is enabled.
Maybe move the CUR_DIR calculation to the module level (global scope) to avoid this trace error and unnecessary re-calculation.
There was a problem hiding this comment.
yes, I found it. Thanks for reminding
…-project#4593)" This reverts commit 4588cda.
…#4593) ### What this PR does / why we need it? 1. Fixes the environment path used to locate custom op shared libraries. 2. Uses empty tensor initialization for op outputs instead of zero-initialization for better efficiency. - vLLM version: v0.11.2 - vLLM main: https://github.com/vllm-project/vllm/commit/v0.11.2 --------- Signed-off-by: QianChenxi <[email protected]>
…#4593) ### What this PR does / why we need it? 1. Fixes the environment path used to locate custom op shared libraries. 2. Uses empty tensor initialization for op outputs instead of zero-initialization for better efficiency. - vLLM version: v0.11.2 - vLLM main: https://github.com/vllm-project/vllm/commit/v0.11.2 --------- Signed-off-by: QianChenxi <[email protected]> Signed-off-by: Che Ruan <[email protected]>
…#4593) ### What this PR does / why we need it? 1. Fixes the environment path used to locate custom op shared libraries. 2. Uses empty tensor initialization for op outputs instead of zero-initialization for better efficiency. - vLLM version: v0.11.2 - vLLM main: https://github.com/vllm-project/vllm/commit/v0.11.2 --------- Signed-off-by: QianChenxi <[email protected]> Signed-off-by: Che Ruan <[email protected]>
…#4593) ### What this PR does / why we need it? 1. Fixes the environment path used to locate custom op shared libraries. 2. Uses empty tensor initialization for op outputs instead of zero-initialization for better efficiency. - vLLM version: v0.11.2 - vLLM main: https://github.com/vllm-project/vllm/commit/v0.11.2 --------- Signed-off-by: QianChenxi <[email protected]>
3 participants
What this PR does / why we need it?
Does this PR introduce any user-facing change?
How was this patch tested?