-
Notifications
You must be signed in to change notification settings - Fork 617
[wip]Dev wangqk test ci #4510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[wip]Dev wangqk test ci #4510
Conversation
|
👋 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.
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 a significant amount of code to support custom ACLNN operators for Ascend NPUs. This includes a new CMake-based build system under csrc/, build scripts, and the source code for two custom operators: dispatch_gmm_combine_decode and grouped_matmul_swiglu_quant_weight_nz_tensor_list. The changes also integrate these custom operators into the Python package by updating setup.py and binding them in torch_binding.cpp. My review has identified a critical issue in the path construction for loading custom ops, which would prevent them from being found at runtime. I've also found a couple of high-severity issues in the build scripts that could lead to build failures or unexpected behavior. Additionally, linting has been disabled for the new csrc directory, which should be addressed before merging.
| CUSTOM_OPP_PATH = os.path.join(CUR_DIR, "vllm_ascend", "_cann_ops_custom", | ||
| "vendors", "customize") | ||
| CUSTOM_LIB_PATH = os.path.join(CUSTOM_OPP_PATH, "op_api", "lib") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path to custom operators seems to be constructed incorrectly. CUR_DIR is .../vllm_ascend, so os.path.join(CUR_DIR, "vllm_ascend", "_cann_ops_custom", ...) will result in a wrong path .../vllm_ascend/vllm_ascend/_cann_ops_custom/.... It should probably be os.path.join(CUR_DIR, "_cann_ops_custom", ...) since _cann_ops_custom is installed inside the vllm_ascend package directory.
| CUSTOM_OPP_PATH = os.path.join(CUR_DIR, "vllm_ascend", "_cann_ops_custom", | |
| "vendors", "customize") | |
| CUSTOM_LIB_PATH = os.path.join(CUSTOM_OPP_PATH, "op_api", "lib") | |
| CUSTOM_OPP_PATH = os.path.join(CUR_DIR, "_cann_ops_custom", | |
| "vendors", "customize") | |
| CUSTOM_LIB_PATH = os.path.join(CUSTOM_OPP_PATH, "op_api", "lib") |
| args: [ | ||
| --toml, pyproject.toml, | ||
| '--skip', 'tests/e2e/multicard/test_torchair_graph_mode.py,csrc/mla_preprocess/**,tests/prompts/**,./benchmarks/sonnet.txt,*tests/lora/data/**,build/**,./vllm_ascend.egg-info/**,.github/**,typos.toml', | ||
| '--skip', 'tests/e2e/multicard/test_torchair_graph_mode.py,csrc/**,tests/prompts/**,./benchmarks/sonnet.txt,*tests/lora/data/**,build/**,./vllm_ascend.egg-info/**,.github/**,typos.toml', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| args: [ | ||
| "--force-exclude", | ||
| "--exclude", "csrc/mla_preprocess/**" | ||
| "--exclude", "csrc/**" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| while [[ $# -gt 0 ]]; do | ||
| case $1 in | ||
| -h|--help) | ||
| help_info | ||
| exit | ||
| ;; | ||
| -n|--op-name) | ||
| ascend_op_name="$2" | ||
| shift 2 | ||
| ;; | ||
| -c|--compute-unit) | ||
| ascend_compute_unit="$2" | ||
| shift 2 | ||
| ;; | ||
| *) | ||
| help_info | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script's help message mentions --cov and --verbose options, but they are not handled in the argument parsing loop. The variables COV and VERBOSE are initialized to false but never updated. This is a bug that makes these options non-functional.
while [[ $# -gt 0 ]]; do
case $1 in
-h|--help)
help_info
exit
;;
-n|--op-name)
ascend_op_name="$2"
shift 2
;;
-c|--compute-unit)
ascend_compute_unit="$2"
shift 2
;;
--cov)
COV="true"
shift
;;
--verbose)
VERBOSE="true"
shift
;;
*)
help_info
exit 1
;;
esac
done| -DOP_DEBUG_CONFIG=${OP_DEBUG_CONFIG} \ | ||
| -DASCEND_OP_NAME=${ASCEND_OP_NAME} | ||
|
|
||
| make ${JOB_NUM} prepare_build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script uses make directly to build the prepare_build target. This is not generator-agnostic and will fail if a user configures CMake with a different generator, such as Ninja. It's better to use cmake --build . which works with any generator.
| make ${JOB_NUM} prepare_build | |
| cmake --build . --target prepare_build -- ${JOB_NUM} |
b11db9d to
c13aa74
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
c13aa74 to
afad30b
Compare
5779f2e to
06dfe27
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
06dfe27 to
4a212b2
Compare
565f254 to
8011f0e
Compare
Signed-off-by: wangqiankun13 <[email protected]>
8011f0e to
7a92173
Compare
What this PR does / why we need it?
Does this PR introduce any user-facing change?
How was this patch tested?