vllm-project / vllm-project/afd-plugin

[RFC]: Restructure csrc/npu into ascend_kernels / pybind / scripts around the CANN npu_op_* build system

Closed
#353 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

RFC
Dominant language
Python
Stars
228
Forks
48
Avg merge
1d 19h
Merged PRs (30d)
53

Description

Motivation

csrc/npu/ currently mixes three unrelated concerns in one flat namespace: the
ACLNN operator sources (a2e/, e2a/), the PyTorch extension
(torch_extension/ + aclnn_torch_adapter/), and the CANN build system
(build.sh, cmake/, build_aclnn.sh). The flat layout has four concrete
costs:

  1. Duplicated shared headers. comm_args.h, data_copy.h, and
    moe_distribute_base.h are vendored twice, once under a2e/op_kernel/ and
    once under e2a/op_kernel/. The two copies are byte-identical today, so any
    fix to the MoE distribute base must be applied twice with nothing keeping
    them in sync.
  2. Operator selection is hard-coded shell control flow. build_aclnn.sh
    passes -n "a2e;e2a" and maps SOC to operator set through case
    statements. Adding an operator or a SOC generation means editing shell, and
    nothing validates that a requested operator is buildable for the requested
    SOC.
  3. A vendored generic CANN build wrapper with no owner. build.sh is a
    ~200-line "build open project" script carrying ccache/bisheng handling and
    -DCUSTOM_ASCEND_CANN_PACKAGE_PATH plumbing that AFD never varies. It is
    CANN-licensed boilerplate maintained in-tree, so every CANN bump is a
    potential merge.
  4. The A5 tiling gate is fragile. AFD_TILING_HAS_COMM_ENGINE gates
    Mc2CcTilingConfig::SetCommEngine(3) for Ascend 950 in a2e.cpp and
    e2a.cpp. In the current tree it is wired into the code-generation compile
    options in a way that does not take effect (see Risks).

The operator half of this has an established upstream shape: the CANN
npu_op_* run-package build system, the same structure used by the
umdk/cam comm_operator ascend_kernels project. Aligning with it lets us
delete the vendored wrapper instead of maintaining it.

Proposed change

Split the directory along concern boundaries, and drive operator selection from
data instead of shell logic. Diff: 56 files, +1576/−3170.

csrc/npu/
  ascend_kernels/          # ACLNN operator run package (npu_op_* build system)
    CMakeLists.txt         #   npu_op_package(... TYPE RUN) driver
    AddCustom.json         #   msopgen input template
    operator_registry.json #   SOC -> operators map + per-op build metadata
    cmake_files/           #   cmake/, op_host/, op_kernel/ build fragments
    a2e/{op_api,op_host,op_kernel}
    e2a/{op_api,op_host,op_kernel}
    utils/op_kernel/       #   shared headers, deduplicated
  pybind/                  # PyTorch extension (_C_ascend)
    CMakeLists.txt
    torch_binding.cpp, torch_binding_meta.cpp
    pytorch_extension/     #   NPUBridge, NPUStorageImpl, op_api_common.h
  scripts/
    compile_ascend_proj.sh #   msopgen + cmake driver for one SOC
    select_ops.py          #   registry-driven operator resolution
    set_conf.py            #   patch generated CMakePresets.json
  build_aclnn.sh           # thin entry point, calls scripts/

Behavioral changes, called out because they are not pure moves:

  • Operator selection becomes registry-driven. operator_registry.json maps
    a SOC generation (ascend910_93, ascend950) to the operator directories it
    supports, plus per-operator metadata. select_ops.py validates the requested
    selection against the registry, rejects unknown SOCs, unknown operators, and
    empty entries, then applies the SHMEM availability filter. Adding a SOC
    generation becomes a registry entry rather than a case branch.
  • Op host sources consolidate. a2e_def.cpp / a2e_proto.cpp /
    a2e_tiling.cpp fold into a2e/op_host/a2e.cpp; e2a_tiling.cpp folds into
    e2a/op_host/e2a.cpp. op_api/ moves under each operator instead of being a
    sibling of op_host/.
  • Shared headers deduplicate into ascend_kernels/utils/op_kernel/, copied
    unconditionally by the build driver.
  • Run-package naming becomes deterministic. build_aclnn.sh previously
    globbed output/CANN-custom_ops*.run and required exactly one match. The new
    driver emits a fixed AFD_${SOC_VERSION}.run and the entry point asserts that
    exact path.
  • build.sh is deleted. Its ASCEND_CANN_PACKAGE_PATH resolution moves
    into compile_ascend_proj.sh (trying ASCEND_HOME_PATH, then
    ASCEND_TOOLKIT_HOME, then the default toolkit path). Parallelism becomes an
    explicit AFD_BUILD_JOBS (default 8) instead of 2 x nproc.
  • setup.py CMake source dir moves from csrc/npu/torch_extension to
    csrc/npu/pybind. The extension target name afd_plugin._C_ascend and the
    installed artifact layout are unchanged.
  • vendor_name is set to afd-plugin by set_conf.py, so the run package
    installs under vendors/afd-plugin/, preserving the existing
    afd_plugin/_cann_ops_custom/vendors/afd-plugin/op_api/lib rpath contract.
  • AFD_TILING_HAS_COMM_ENGINE ordering fixed in
    ascend_kernels/cmake_files/op_host/CMakeLists.txt — see Risks.
Plugin boundary

Everything here is plugin-owned in-tree C++/CMake; no vLLM source is touched.

  • Plugin-owned: all of csrc/npu/, the _C_ascend extension target, the
    afd_ascend Torch library namespace, and the ACLNN operator build.
  • Compat helper: none.
  • Compat patch: none. This change does not touch afd_plugin/compat/.
  • Explicit class path: unchanged — afd_plugin._C_ascend remains the import
    surface, and afd_plugin/_cann_ops_custom/ remains the install surface.
  • Build-time dependency change: the operator build now depends on the CANN
    npu_op_* CMake package (find_package(ASC REQUIRED)) and on msopgen,
    rather than on the in-tree cmake/ + build.sh pair. Reviewers should
    confirm the minimum CANN version this implies.
Risks and alternatives
  • A5 tiling define did not reach the code-generation path. In
    cmake_files/op_host/CMakeLists.txt, list(APPEND CODE_GEN_COMPILE_OPTS -DAFD_TILING_HAS_COMM_ENGINE=1) was followed by an unconditional
    set(CODE_GEN_COMPILE_OPTS -I${CMAKE_CURRENT_SOURCE_DIR}), which clobbered
    the append before npu_op_code_gen(... COMPILE_OPTIONS ...) consumed it.
    Confirmed with cmake -P: the 950 case produced [] instead of the expected
    [-I...;-DAFD_TILING_HAS_COMM_ENGINE=1]. This is not a compile failure — the
    #ifdef block only calls SetCommEngine(3) at runtime on
    SocVersion::ASCEND950 — so the effect is a silently skipped A5 tiling
    setting, which is exactly the kind of drift that is hard to attribute later.
    Fixed by initialising CODE_GEN_COMPILE_OPTS before the gate and appending
    inside it; re-verified with cmake -P that 950 now carries the define and
    ascend910_93 does not.
  • Loss of build knobs. build.sh offered ccache/bisheng acceleration,
    CHECK_COMPATIBLE, and --cov/--verbose. The new driver drops them. If any
    are load-bearing for CI or local NPU iteration, they need to be reintroduced
    before the old script is deleted.
  • Behavior change for consumers of the run package. Anything globbing
    CANN-custom_ops*.run breaks. In-tree the only glob was in build_aclnn.sh
    (updated), but downstream packaging scripts may assume the old name.
  • No functional change is intended to the a2e/e2a kernels or the pybind
    surface. Reviewers should treat any semantic diff in the moved sources as a
    bug, except the intentional TWO_DIMS magic-number extraction in
    e2a/op_host/e2a.cpp.
  • Minor: AddCustom.json and operator_registry.json carry no license
    header. Pre-commit's SPDX check covers only py/pyi/rs/proto/sh, so this is
    not CI-enforced; raising it only for reviewer awareness.
  • Alternatives considered: (a) keep the flat layout and only deduplicate the
    three shared headers — smaller diff, but leaves shell-encoded operator
    selection and the vendored build.sh in place; (b) keep build.sh and layer
    the registry on top — avoids the CANN npu_op_* dependency, but keeps two
    build systems alive. Option (a) is the lower-risk fallback if reviewers judge
    the CANN-version coupling too costly.
Feedback period

Two weeks from posting, or until the affected code owners sign off, whichever
comes first.

CC list

@hsliuustc0106 @jiangkuaixue123 (per .github/CODEOWNERS).

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by mapping csrc/npu/ against the proposed ascend_kernels, pybind, and scripts layout, then read build_aclnn.sh, cmake_files/op_host/CMakeLists.txt, and setup.py. Run the stated cmake -P check and inspect select_ops.py, set_conf.py, and operator_registry.json. Done means the build uses the registry-driven npu_op_* flow, preserves the pybind/install surface, and retains the documented A5 tiling behavior without the old build.sh.

Written by the indexing model from the issue text.

Assessment

Tech stack
cmake, cpp, python, shell
Domain
build-system, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.