[build](feat) Added build entry setup_ascend.py - #1485
Conversation
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
|
|
/retry |
| elif arch == "arm64": | ||
| system_suffix = "ubuntu-arm64" | ||
| elif arch == "x64": | ||
| vglibc = tuple(map(int, platform.libc_ver()[1].split("."))) |
There was a problem hiding this comment.
[bug · high]
On systems without glibc (e.g., musl-based Alpine Linux), platform.libc_ver() may return an empty version string ('', ''). Calling ''.split('.') returns [''] and int('') raises a ValueError, aborting the entire build. This function should handle the empty-string case gracefully and return a sensible fallback or None.
Suggestion:
| vglibc = tuple(map(int, platform.libc_ver()[1].split("."))) | |
| libc_ver = platform.libc_ver()[1] | |
| if not libc_ver: | |
| return None | |
| vglibc = tuple(map(int, libc_ver.split("."))) |
| def _is_dev_mode(): | ||
| if os.environ.get("TRITON_WHEEL_VERSION_SUFFIX", ""): | ||
| return True | ||
| if "dev" in _get_default_version(): | ||
| return True |
There was a problem hiding this comment.
[other · low]
When neither condition is met, _is_dev_mode() implicitly returns None instead of False. While None is falsy and works in boolean contexts, the implicit return is unclear. Adding an explicit return False improves readability and makes the function contract unambiguous.
Suggestion:
| def _is_dev_mode(): | |
| if os.environ.get("TRITON_WHEEL_VERSION_SUFFIX", ""): | |
| return True | |
| if "dev" in _get_default_version(): | |
| return True | |
| def _is_dev_mode(): | |
| if os.environ.get("TRITON_WHEEL_VERSION_SUFFIX", ""): | |
| return True | |
| if "dev" in _get_default_version(): | |
| return True | |
| return False |
| try: | ||
| out = subprocess.check_output(["cmake", "--version"]) | ||
| except OSError: | ||
| raise RuntimeError("CMake must be installed to build the following extensions: " + | ||
| ", ".join(e.name for e in self.extensions)) |
There was a problem hiding this comment.
[bug · medium]
If the cmake binary exists but returns a non-zero exit code (e.g., corrupted installation), subprocess.check_output raises subprocess.CalledProcessError, which is not caught by the except OSError handler. This results in an unhandled exception rather than a helpful error message. Catch subprocess.CalledProcessError as well.
Suggestion:
| try: | |
| out = subprocess.check_output(["cmake", "--version"]) | |
| except OSError: | |
| raise RuntimeError("CMake must be installed to build the following extensions: " + | |
| ", ".join(e.name for e in self.extensions)) | |
| try: | |
| out = subprocess.check_output(["cmake", "--version"]) | |
| except (OSError, subprocess.CalledProcessError): | |
| raise RuntimeError("CMake must be installed to build the following extensions: " + | |
| ", ".join(e.name for e in self.extensions)) |
| bdist_wheel.run(self) | ||
|
|
||
| if is_manylinux: | ||
| file = glob.glob(os.path.join(self.dist_dir, "*-linux_*.whl"))[0] |
There was a problem hiding this comment.
[bug · medium]
glob.glob may return an empty list if no wheel file matches the pattern (e.g., if the wheel uses a different platform tag), causing an IndexError that crashes the build with an unhelpful message. Add a guard to check the result before indexing, and raise a meaningful error if no matching wheel is found.
Suggestion:
| file = glob.glob(os.path.join(self.dist_dir, "*-linux_*.whl"))[0] | |
| files = glob.glob(os.path.join(self.dist_dir, "*-linux_*.whl")) | |
| if not files: | |
| raise RuntimeError("No wheel file matching '*-linux_*.whl' found in dist_dir") | |
| file = files[0] |
| cd base | ||
| MAX_JOBS="$NUM_PROCS" python setup.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" | ||
| MAX_JOBS="$NUM_PROCS" python setup_ascend.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" |
There was a problem hiding this comment.
[bug · critical]
Critical bug: setup_ascend.py does not exist in the base branch checkout.
The "Checkout base" step (line 53) checks out the base/target branch via ref: ${{ github.base_ref }}. Since setup_ascend.py is a new file added by this PR (listed as ADDED in the changed files), it does not exist in the base branch. The build step cd base && python setup_ascend.py bdist_wheel ... will fail with a FileNotFoundError.
Comparison with other workflow fixes:
The other modified workflow Ascend950-ci.yml wisely uses a conditional check:
if [ -f setup_ascend.py ]; then
SETUP_PY="setup_ascend.py"
elif [ -f python/setup.py ]; then
SETUP_PY="setup.py"
else
echo "ERROR: setup.py or setup_ascend.py not found" >&2
exit 1
fiSuggestion: Apply the same pattern here. For the base branch build, fall back to setup.py since setup_ascend.py won't be there. Alternatively, only use setup_ascend.py for the PR checkout (which contains the new file) and keep using setup.py for the base checkout.
| cd base | ||
| MAX_JOBS="$NUM_PROCS" python setup.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" | ||
| MAX_JOBS="$NUM_PROCS" python setup_ascend.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" |
There was a problem hiding this comment.
[bug · critical]
Critical: setup_ascend.py does not exist in the base branch checkout, causing the build to fail.
The "Checkout base" step (line 53) checks out the base/target branch at ref: ${{ github.base_ref }}. Since setup_ascend.py is newly added in this PR (it's an ADDED file), it does not exist in the base branch checkout. Running python setup_ascend.py bdist_wheel inside the base/ directory will fail with FileNotFoundError.
Meanwhile, the base branch's setup.py still contains all the Ascend-specific build logic that hasn't yet been extracted (the extraction happens only in this PR), so the base build should continue using setup.py.
Fix: Revert this line back to python setup.py bdist_wheel, since setup.py is the correct entry point for the base branch. Only the PR checkout should use setup_ascend.py.
| cd pr | ||
| MAX_JOBS="$NUM_PROCS" python setup.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/pr/wheelhouse" | ||
| MAX_JOBS="$NUM_PROCS" python setup_ascend.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/pr/wheelhouse" |
There was a problem hiding this comment.
[bug · low]
Same critical issue applies to the PR build step — but this one will work since the PR checkout does contain setup_ascend.py.
However, for consistency and to avoid future breakage (if the file name changes again), consider adopting the same defensive -f check pattern as Ascend950-ci.yml here as well.
| cd pr | ||
| MAX_JOBS="$NUM_PROCS" python setup.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/pr/wheelhouse" | ||
| MAX_JOBS="$NUM_PROCS" python setup_ascend.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/pr/wheelhouse" |
There was a problem hiding this comment.
[bug · low]
The PR build step change to setup_ascend.py is correct.
The PR checkout contains the newly added setup_ascend.py, and the modified setup.py no longer includes the Ascend-specific build logic (patches, coverage, distributed submodule, etc.). So using python setup_ascend.py for the PR build is the right change.
However, for defensive programming and consistency with Ascend950-ci.yml, consider adding a fallback check (e.g., if [ -f setup_ascend.py ]; then ...) to handle any future structural changes gracefully.
| cd base | ||
| MAX_JOBS="$NUM_PROCS" python setup.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" | ||
| MAX_JOBS="$NUM_PROCS" python setup_ascend.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" |
There was a problem hiding this comment.
setup_ascend.py may not exist on the base branch, causing build failure
The "Build triton-ascend base" step (line 81) runs in the base directory, which was checked out via ref: ${{ github.base_ref }} (the target branch of the PR). Since setup_ascend.py is a new file being introduced in this PR, it does not exist on the base branch. This will cause the step to fail with FileNotFoundError.
In contrast, the parallel change in Ascend950-ci.yml uses a robust fallback pattern that checks for setup_ascend.py first and falls back to setup.py if not found. DynamicCVPipeline-ci.yml should adopt a similar approach for consistency and reliability.
Suggested fix:
Either use a check-then-fallback approach (as done in Ascend950-ci.yml), or ensure the base build continues to use setup.py while only the PR build uses setup_ascend.py.
Suggestion:
| cd base | |
| MAX_JOBS="$NUM_PROCS" python setup.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" | |
| MAX_JOBS="$NUM_PROCS" python setup_ascend.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" | |
| cd base | |
| if [ -f setup_ascend.py ]; then | |
| SETUP_PY="setup_ascend.py" | |
| elif [ -f setup.py ]; then | |
| SETUP_PY="setup.py" | |
| else | |
| echo "ERROR: setup_ascend.py or setup.py not found" >&2 | |
| exit 1 | |
| fi | |
| MAX_JOBS="$NUM_PROCS" python ${SETUP_PY} bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" |
| cd base | ||
| MAX_JOBS="$NUM_PROCS" python setup.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" | ||
| MAX_JOBS="$NUM_PROCS" python setup_ascend.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" |
There was a problem hiding this comment.
setup_ascend.py may not exist on the base branch, causing build failure
The "Build triton-ascend base" step (line 81) runs in the base directory, which was checked out via ref: ${{ github.base_ref }} (the target branch of the PR). Since setup_ascend.py is a new file being introduced in this PR, it does not exist on the base branch. This will cause the step to fail with FileNotFoundError.
In contrast, the parallel change in Ascend950-ci.yml uses a robust fallback pattern that checks for setup_ascend.py first and falls back to setup.py if not found. DynamicCVPipeline-ci.yml should adopt a similar approach for consistency and reliability.
Suggested fix:
Either use a check-then-fallback approach (as done in Ascend950-ci.yml), or ensure the base build continues to use setup.py while only the PR build uses setup_ascend.py.
Suggestion:
| cd base | |
| MAX_JOBS="$NUM_PROCS" python setup.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" | |
| MAX_JOBS="$NUM_PROCS" python setup_ascend.py bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" | |
| cd base | |
| if [ -f setup_ascend.py ]; then | |
| SETUP_PY="setup_ascend.py" | |
| elif [ -f setup.py ]; then | |
| SETUP_PY="setup.py" | |
| else | |
| echo "ERROR: setup_ascend.py or setup.py not found" >&2 | |
| exit 1 | |
| fi | |
| MAX_JOBS="$NUM_PROCS" python ${SETUP_PY} bdist_wheel --dist-dir="${GITHUB_WORKSPACE}/base/wheelhouse" |
Triton Ascend Build System: Non-Intrusive Refactoring Design
1. Background and Goals
When maintaining the
triton-ascendproject, Ascend NPU-specific logic needs tobe injected into the upstream Triton build flow (backend registration, LLVM
build, patch application, coverage tooling, distributed submodule, wheel
repair, etc.). These changes were previously written directly into
setup.pyin an intrusive manner, which caused:
rebasing or upgrading the upstream version;
setup.py, making it hard tomaintain and review;
build" paths.
Refactoring goals:
setup.py;setup_ascend.py;python setup.py ...runs the upstreamstandard build, while
python setup_ascend.py ...runs the Ascend build;dev patchis applied only in development scenarios and is skipped forreleases.
2. Final File Layout
setup.pysetup.pysetup_ascend.py3. Usage
4. Core Design of setup_ascend.py
4.1 Overall Flow
main()insetup_ascend.pyuses a "intercept setup() call -> apply patches-> re-invoke" monkey-patch pattern:
Key code (
setup_ascend.py:513):Why intercept
setuptools.setup?By temporarily replacing
setuptools.setupduring import, the upstream modulecan fully define all its functions and classes, while the arguments passed to
setup()are captured. After we apply our patches, we invoke the realsetup()with the modified arguments.4.2 Default Environment Variables (
_set_default_env_vars)TRITON_BUILD_WITH_CCACHEtrueTRITON_BUILD_WITH_CLANG_LLDtrueTRITON_BUILD_PROTONOFFTRITON_WHEEL_NAMEtriton-ascendTRITON_BUILD_DISTRIBUTEDOFFTRITON_APPEND_CMAKE_ARGS-DTRITON_BUILD_UT=OFFsetdefaultis used, so users can override these from the outside.4.3 Dev / Release Mode Detection (
_is_dev_mode)Detection priority:
TRITON_DEV_MODE=1-> force development mode (apply the dev patch);TRITON_RELEASE_MODE=1-> force release mode (skip the dev patch);release-> release mode; otherwise development mode.
4.4 Patch Application (
_apply_triton_ascend_patch)third_party/ascend/patch/triton-ascend-3.6.0.patch;triton-ascend-dev-3.6.0.patch(which touchespython/triton/runtime/autotuner.py);git checkout --on the affected files toavoid failures caused by applying a patch twice.
4.5 LLVM Package Info Override (
get_llvm_package_info)Override the upstream LLVM download source to point to the Huawei Cloud
Ascend-customized LLVM build:
Here
llvm_patch_hashis the first 8 hex digits of the SHA256 of the contentsof
third_party/ascend/patch/llvm_patch_*.patch. When the patches change, anew package is automatically fetched. If there is no corresponding package for
the current platform (e.g. Windows), it falls back to the original upstream
implementation.
4.6 CMakeBuild Override
Inherit from the upstream
CMakeBuildand override two methods:run():download_and_copy_dependencies();TRITON_ENABLE_COVERAGE_HITEST=1, configure the hitest coverageenvironment variables and append
-DTRITON_ENABLE_COVERAGE_HITEST=ONtoTRITON_APPEND_CMAKE_ARGS; otherwise clean up any residual hitestenvironment variables;
build_extension.build_extension():By temporarily wrapping
subprocess.check_call, Ascend-specific arguments areinjected during the cmake configuration phase (when
cmd[0] == "cmake"and
--buildis not present):-DASCENDNPU_IR_TAG=...(if set)-DLLVM_MAJOR_VERSION_22_COMPATIBLE=ON-DTRITON_BUILD_DISTRIBUTED=ON/OFFAfter the build completes,
_copy_ascend_tools()is called to copytriton-mlir-optandtriton-optinto the output directory and strip them(non-Windows).
4.7 BuildWheel (auditwheel Repair)
Override
bdist_wheel:add_links(external_only=True);IS_MANYLINUX=TRUE, callauditwheel repairafter building, taggingthe wheel with
manylinux_2_27_*andmanylinux_2_28_*, and delete theoriginal wheel.
4.8 Distributed Submodule Support
Three upstream functions are wrapped to include the
triton_distpackage(controlled by
TRITON_BUILD_DISTRIBUTED, default OFF):get_package_dirs(): append the package path mapping fortriton_dist;get_packages(): walkTriton-distributed-ascend/python/triton_disttoautomatically discover all subpackages;
add_links(): create a symlink forpython/triton_dist._ensure_distributed_submodule()ensures the git submodule is initializedbefore the build, and raises a clear error if it is missing.
4.9 setup() Argument Rewrite (
_build_setup_kwargs)On top of the captured upstream
kwargs, the following fields are overridden:nametriton-ascend(controlled byTRITON_WHEEL_NAME)versionversion.txt+ wheel suffix + git commit hash (non-manylinux)urlhttps://gitcode.com/Ascend/triton-ascend/long_descriptionREADME.mdinstall_requirestriton==3.6.0appended per architecturecmdclassbdist_wheel->BuildWheel,build_ext->CMakeBuildpackages/package_dirtriton_distare includedentry_pointsascendentry is present undertriton.backendspackage_data*.py/*.pyifortriton_dist5. Ascend Backend Registration
First lines of
_patch_module:BackendInstaller.prepare("ascend")scansthird_party/ascend/backend/andadds it as an in-tree backend (
is_external=False). As a result, the upstreamcmake configuration automatically includes ascend in
-DTRITON_CODEGEN_BACKENDS=ascend;nvidia;amd, and thetriton.backendsentry point registers
ascend = triton.backends.ascend.6. Coverage Tooling (hitest)
Takes effect only when
TRITON_ENABLE_COVERAGE_HITEST=1:HITEST_*/HitestHome/lltcovRootpathenvironmentvariables;
PATHandLD_LIBRARY_PATH;-DTRITON_ENABLE_COVERAGE_HITEST=ONviaTRITON_APPEND_CMAKE_ARGS;_clean_hitest_env()to remove all related variables sothe environment is not polluted.
Default key paths:
HITEST_HOME=/opt/hitest/linux_avatar_x86_64LLTCOV_ROOTPATH=/opt/covdata7. Upgrade and Maintenance Strategy
When the upstream version is upgraded:
setup.pywith the new upstreamsetup.py;python setup_ascend.py bdist_wheeland, based on any errors, adjustthe patch points in
setup_ascend.py(e.g. changed class/function names);third_party/ascend/patch/to adapt to thenew upstream code;
llvm_patch_hashchanges automatically, sothere is no need to manually bump the version.
Because all Ascend logic is isolated in
setup_ascend.pywith zero overlapwith upstream files, the conflict surface during upgrades is minimized.
8. Verification
All files pass the syntax check;
setup_ascendcan be imported normally andthe
mainfunction is callable.New contributor declaration
I am not making a trivial change, such as fixing a typo in a comment.
I have written a PR description following these
rules.
I have run
pre-commit run --from-ref origin/main --to-ref HEAD.Select one of the following.
/testforlittests/unittestfor C++ tests/python/testfor end-to-end testsFILL THIS IN.Select one of the following.
littests.littests I have added follow these best practices,including the "tests should be minimal" section. (Usually running Python code
and using the instructions it generates is not minimal.)