build: add CUDA_PYTHON_TOOLCHAIN override for compiler/linker selection - #2903
Conversation
Introduce a CUDA_PYTHON_TOOLCHAIN build-time env var (mirroring CUDA_PYTHON_PARALLEL_LEVEL) that switches the C/C++ compiler and linker as a unit. Valid values: gnu (default on Linux), llvm (clang + lld) on Linux, msvc (default on Windows). The defaults reproduce the previous build behavior exactly and do not touch CC/CXX, so an externally-set compiler (e.g. the sccache wrapper used in CI) keeps working. Each build_hooks.py gains two helpers: - _resolve_toolchain(): reads CUDA_PYTHON_TOOLCHAIN, validates it against the platform's allowed set, and returns the toolchain name plus its cc/cxx and extra_compile_args/extra_link_args. A non-default toolchain sets CC/CXX/LDSHARED so distutils' customize_compiler picks up clang/lld. - _check_toolchain_available(): a preflight that probes the toolchain's tools on PATH and fails fast with a helpful message (tool name, install hint, and how to fall back) instead of a cryptic compile error. cuda_bindings/setup.py drops the now-redundant _is_clang strip: the llvm flag set from _resolve_toolchain is clang-correct from the start (no -fpermissive, no -fno-var-tracking-assignments). No workflow changes; the default path composes with the existing hardcoded CC='sccache cc' in CI. CI toolchain selection and the CUDA_PYTHON_COMPILER_LAUNCHER companion var land in a follow-up.
github-actions
Bot
added
cuda.bindings
mdboom
left a comment
mdboom
left a comment
There was a problem hiding this comment.
No real objection to this as-is, but it would be nice to reduce the duplication between the two build_hooks.py scripts somehow if possible.
I think in the long run, we will want to migrate to a more robust build backend, the top contender for which is probably scikit-build-core (based on CMake). In that environment, you wouldn't hardcode the flags to run explicitly, but have the configure step figure out which ones are available etc. My worry is that if we implement this now, we would have to duplicate this somehow inside of a better build system for backward compatibility and it may not make sense there. Would it be better to migrate now?
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Removing the _is_clang handling breaks the externally supplied clang path that this PR says should keep working. When CUDA_PYTHON_TOOLCHAIN is unset, _resolve_toolchain() selects gnu and keeps the GCC-only -fno-var-tracking-assignments; because the default path intentionally does not override CC/CXX, a caller with CC=clang / CXX=clang++ now gets that unsupported flag. Previously build_ext detected clang and removed it. Please preserve compiler-based flag filtering on the default path (or resolve flags from the actual external compiler) and add a regression with externally supplied clang and no toolchain override.
Extract the toolchain logic that is identical across the two build_hooks.py files (constants, name resolution/validation, env application, preflight) into a single shared block delimited by 'begin/end shared toolchain helpers' markers, duplicated verbatim with a 'keep in sync' comment. This mirrors the existing precedent set by _import_get_cuda_path_or_home. The per-package _resolve_toolchain() is now a thin wrapper that calls the shared _resolve_toolchain_name()/_apply_toolchain_env() and assembles only its own package-specific flags (cuda.bindings: c++14, -fpermissive, -O3; cuda.core: c++17, -O2). The cc/cxx compiler mapping moves into a shared _TOOLCHAIN_COMPILERS table, so it is no longer re-assigned per branch. PEP 517 build isolation forbids a shared module (the sibling package is not installed in the isolated build env), so the block is duplicated rather than imported. A new test_shared_toolchain_block_is_in_sync enforces the byte-identical invariant so drift is caught locally. No behavior change: the defaults (gnu/msvc) reproduce the previous build flags exactly, and all existing build-hooks tests still pass.
b0289f6 to
67ac9fc
Compare
I tried extracting the shared toolchain helpers into a small distributable package (
FWIW there is precedent:
Hm, I don't think we promise backward compatibility for how the packages are built. Am I missing your point? |
…clang regression When CUDA_PYTHON_TOOLCHAIN is unset on Linux, infer the toolchain from the externally-set CC/CXX (CXX preferred, fall back to CC): a value containing 'clang' selects llvm, else gnu. This fixes the regression reported by sylvesterkaczmarek: previously the default 'gnu' flag set (incl. -fno-var-tracking-assignments) reached an externally-supplied clang because the default path did not override CC/CXX and the old _is_clang strip was removed. Now clang is inferred and the llvm flag set (no gcc-only flags, -fuse-ld=lld) is used, and the external compiler is left in place, so a wrapper like CC='sccache clang' survives and gets the llvm flags. When CUDA_PYTHON_TOOLCHAIN is set it takes precedence over an externally- set CC: a mismatch warns (CC only; CXX commonly defaults to 'c++' and is not a reliable user-intent signal) and the external CC is overridden. The shared toolchain helpers remain byte-identical across the two build_hooks.py via the 'keep in sync' markers. _resolve_toolchain_name now returns an 'explicit' flag so _apply_toolchain_env only overrides CC/CXX when the toolchain was chosen explicitly (not inferred). Tests: regression tests for externally-supplied CC=clang (infer llvm, no gcc-only flags, CC survives), CC=gcc (infer gnu), and the explicit mismatch/no-mismatch cases. A module-level autouse fixture cleans CC/CXX/LDSHARED per test because _apply_toolchain_env sets them directly in os.environ, which monkeypatch does not revert.
67ac9fc to
f8245a5
Compare
|
/ok to test f8245a5 |
…om dedup)
The dedup refactor accidentally changed gcc's '-std=c++17' (equals) to
'-std:c++17' (colon) in cuda_core/build_hooks.py's gnu and llvm
branches. gcc rejects the colon form ('unrecognized command-line
option'), breaking all Linux gcc builds of cuda.core (the pixi
smoke build and the linux-aarch64 wheel builds). cuda.bindings was
unaffected (it uses -std=c++14, correctly). Restore the equals form.
|
/ok to test 8ce0a8b |
This comment has been minimized.
This comment has been minimized.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
I am very concerned in this. Whose problem are we addressing here? We don't have any CI infra to validate this (and full-fledged source build support is not something that we commit to -- 99.9% of our users should go use wheels or conda packages, not building from source). |
|
@sylvesterkaczmarek Are you AI agent? Are you actually using Clang to build cuda-bindings (and why?), or did you just leave random drive-by comments? |
My point was that this environment variable becomes part of the interface that others will use to build our package in their automation, and we might have to continue to support that surface indefinitely. |
I see. I believe we both agree that this is not something we promise nor want to promise. We could be more explicit about that in |
…-derives from env
|
/ok to test adebd02 |
|
/ok to test b281903 |
|
/ok to test baeff17 |
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
codex gpt-5.6-sol ultra review findings and suggested fixes
Reviewed PR head: baeff1783669135a754c75491896058ad1392790
The suggested fixes are organized as three cherry-pickable commits on rwgk/review/toolchain-override-backend2. Together they are tree-identical to the version exercised by the upstream CI run.
[P1] Scope build stamps to the extension ABI
The new configuration stamps were single files in each checkout (.build-toolchain and .build-config). Multiple Python interpreters can reuse that checkout while producing different extension ABIs. One interpreter could therefore overwrite the checkout-global stamp for another interpreter.
For example, after Python A builds configuration X, Python B builds configuration Y, and Python A requests Y, Python A sees B's Y stamp. Its existing X extension can look up to date by timestamp, so build_ext is not forced even though Python A never built Y. The same issue applies to architecture and free-threaded versus GIL-enabled builds whenever the checkout is shared.
Suggested fix: include Python's EXT_SUFFIX in each stamp filename. EXT_SUFFIX carries the interpreter ABI and platform extension identity, including distinctions such as CPython version, Windows architecture, and free-threaded builds. Keep the existing configuration value inside each ABI-specific stamp.
- Fix commit:
c5be9b69d6c5aa619e07f334e8e5bd7ba4338bb3(build: scope configuration stamps to extension ABI) - Bindings implementation:
cuda_bindings/build_hooks.py - Core implementation:
cuda_core/build_hooks.py - Regression coverage verifies that two mocked extension ABIs resolve to different stamp paths in both packages.
[P1] Stamp the exact core configuration that actually completed
cuda_core checked the requested debug mode in the PEP 517 backend, but wrote the completed-build stamp later from setuptools' build_ext.debug. Those values are not guaranteed to match. In particular, a backend debug build could be recorded as optimized. A later optimized request could then accept the debug artifact as current instead of forcing a rebuild.
Re-deriving the other stamp inputs at write time also made the stamp describe current ambient state rather than the exact configuration prepared earlier in the build.
Suggested fix: make _build_cuda_core() return the exact configuration key it checked, carry that immutable key through the backend, and record it only after the wheel build succeeds. For editable builds, write the stamp only after the .pth patch succeeds as well. Remove the inaccurate setup.py-side write.
- Fix commit:
d40f92a00f19cc684027b48f13d35370003988a3(build(core): stamp the exact successful configuration) - Exact-key recording:
cuda_core/build_hooks.py - Wheel and editable success ordering:
cuda_core/build_hooks.py - Regression coverage checks the precise key and ordering for successful wheels and editables, plus the absence of a stamp after wheel or editable-patch failure.
[P2] Isolate toolchain environment mutations in build-hook tests
The build-hook tests modify CUDA_PYTHON_TOOLCHAIN, CC, CXX, and LDSHARED. Several paths assign directly to os.environ, so ordinary monkeypatch cleanup does not necessarily know the original state of every variable. Values can leak between tests or escape the module, making results order-dependent and capable of contaminating later build tests.
Suggested fix: add an autouse fixture in each build-hook test module that snapshots these four variables, clears them before each test, and restores the exact original state afterward, including the distinction between an unset variable and a set value.
- Fix commit:
60b55cef872b4cc21045128613debc114e80a6c6(tests: isolate toolchain environment changes) - Bindings fixture:
cuda_bindings/tests/test_build_hooks.py - Core fixture:
cuda_core/tests/test_build_hooks.py
Validation
cuda_bindings/tests/test_build_hooks.py: 15 passed.cuda_core/tests/test_build_hooks.py: 49 passed.- All pre-commit hooks applicable to the five changed files passed.
- The clean three-commit branch has tree
b416652ea2dc6f81dcd5696d625113ec514999e7, identical to the CI-tested branch. - In upstream
ci.ymlrun 35691868451, all 104 source, build, and test jobs passed; one matrix job was skipped. - The run's only substantive failure was the pre-existing docs job: manual
workflow_dispatchruns have no pull-request event, whilebuild-docs.ymlunconditionally asksget_pr_numberfor a PR on non-release branches. The finalCheck job statusaggregator consequently failed because it requires docs success. Rerunning without creating a PR would deterministically produce the same result;.githubis unchanged by these fixes.
|
/ok to test f0a1b78 |
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
Approving, based on a codex gpt-5.6-sol ultra re-review (no findings anymore).
leofang
left a comment
leofang
left a comment
There was a problem hiding this comment.
Approved, with the following understanding per offline discussions:
- This is not meant to be public-facing and we are the sole consumer of this feature (for internal bring-up purposes)
- A follow-up PR will be sent (by me or Ralf J) to use symlink or other approaches to ensure we have a single source of truth for the build hook (and its tests).
| if explicit and cc is not None: | ||
| os.environ["CC"] = cc | ||
| os.environ["CXX"] = cxx | ||
| os.environ["LDSHARED"] = f"{cxx} -shared" |
There was a problem hiding this comment.
It might be better that this accommodates with all LDSHARED extras from sysconfig. A sane Python build frontend would read from there.
|
Per offline discussion: if a happy-path CI wheel build passes, the build-system code it exercised is by definition working, so tests asserting that same happy path are redundant. Keep only tests that assert behavior a green wheel build would not surface — error paths, cross-configuration transitions, cross-Python-ABI scoping, failure paths, and data variants beyond the one CI uses. Removed (behavior exercised by any green wheel build): - Shared mixin: `test_default_does_not_touch_env`, `test_default_preserves_existing_cc`, `test_default_is_noop`. - Per-package `test_gnu_sets_env_and_flags` (bindings and core). - `test_cuda_path_is_resolved_before_importing_bindings` — namespace ordering; wheel build fails at import if wrong. - `TestBuildToolchainStamp` / `TestBuildConfigStamp` `test_missing_stamp_forces_rebuild` — every clean-checkout CI build triggers the missing-stamp path. - `TestBuildHookStamping.test_wheel_records_exact_prepared_config_after_success` + `test_editable_records_default_config_after_patch` — every green wheel/editable exercises the record path (failure-path guards below it are kept, since CI does not fail on purpose). - `TestGeneratedSourceDirIsKeyed.test_dir_is_anchored_not_relative_to_cwd`, `TestSetuptoolsSourcePaths.test_absolute_sources_are_made_relative`, `TestExtensionDepends.test_headers_under_module_directories_only`, `TestForceReachesBuildExt.test_flag_set_forces_rebuild` — all implicit in every green wheel build. Kept (not surfaced by any single successful CI wheel): - Toolchain error paths (`test_case_insensitive`, `test_invalid_value_raises`, `test_llvm_missing_tool_lists_install_hint`). - `test_llvm_present_passes` and `test_llvm_overrides_external_cc` (no llvm CI). - All `test_changed_*_forces_rebuild` / `test_same_*_does_not_force` transitions. - `test_stamp_path_is_scoped_to_extension_abi` (cross-Python-ABI). - `test_failed_wheel_build_does_not_record_config`, `test_failed_editable_patch_does_not_record_config` (failure paths). - Parametrized `TestGetCudaMajorVersion` data variants and error paths. - `test_majors_use_different_dirs` (cross-CUDA-major). - `TestExtensionSources` edge cases, `TestParallelSourceCompilation` hook edge cases. - `test_gnu_keeps_gcc_only_flags` (bindings): regression guard for the `_is_clang` removal in NVIDIA#2903, catches a specific stale-flag scenario wheel builds would not. Pass counts: bindings 34 → 29, core 71 → 59; 17 fewer test bodies total, 213 lines removed.
…untime-floor Conflicts: the toolchain and Cython-cache helpers from NVIDIA#2903 and NVIDIA#2933 landed next to this branch's floor and header-check helpers in both build_hooks.py files and their tests. Both sets are kept. cuda.core's build now stamps the build configuration (main) and checks the cuda-bindings floor and header (this branch) at the same point; the cuda-bindings tests file merges main's toolchain tests with this branch's header-check tests.
5 participants
Description
Introduces a
CUDA_PYTHON_TOOLCHAINbuild-time override (mirroringCUDA_PYTHON_PARALLEL_LEVEL) that switches the C/C++ compiler and linker as a unit. Valid values:gnu(default on Linux),llvm(clang + lld) on Linux,msvc(default on Windows). The defaults reproduce the previous build behavior exactly.Motivation: we want to be able to experiment with alternative toolchains without committing to them yet. Based on experiments, switching to
llvmreduces build time.Naming
gnu/llvm/msvcname toolchain families (compiler + linker + binutils), not just a compiler, which matches switching compiler and linker together.llvm(clang + lld) is preferred overclangbecause the linker also switches to lld;clangwould under-describe that. Values are case-insensitive; an invalid value or platform mismatch raises a clear error listing the platform's valid values.Resolution rules
Only
CUDA_PYTHON_TOOLCHAINgoverns toolchain selection;CC/CXXare never read.CUDA_PYTHON_TOOLCHAINunset — the platform default (gnuon Linux,msvcon Windows). The backend does not touchCC/CXX, so an externally-set compiler (e.g.CC="sccache cc"in CI) keeps working, and the default flag set is applied.CUDA_PYTHON_TOOLCHAIN=gnu— selects gcc/g++ explicitly.CC/CXX/LDSHAREDare overridden togcc/g++so the gnu-specific flags (including-fpermissive,-fno-var-tracking-assignmentsin cuda.bindings) reach the actual GNU compiler even on systems wherecc/c++alias to clang.CUDA_PYTHON_TOOLCHAIN=llvm— selects clang/clang++ and lld.CC/CXX/LDSHAREDare overridden toclang/clang++;-fuse-ld=lldis added to link args. The gcc-only flags (e.g.-fpermissive,-fno-var-tracking-assignments) are dropped since clang rejects them.Preflight
_check_toolchain_availableprobes the toolchain's tools onPATHand fails fast with a helpful message (tool name, install hint, and how to fall back). No-op for the implicit platform default. Forllvm: checksclang,clang++, andld.lld.Stale-binary protection
A stale
.sofrom a previous toolchain looks perfectly fresh to setuptools' mtime check. Two stamps prevent this:cuda_core: generalizes the existing.build-cuda-majorstamp into.build-configcovering(cuda_major, toolchain, debug, coverage).build_extis forced and the cythonizebuild_diris keyed by the full config tag when any of these change.cuda_bindings: adds a new minimal.build-toolchainstamp.build_extis forced when the toolchain changes (the cythonize step is toolchain-independent, so itsbuild_diris left flat).Changes
cuda_bindings/build_hooks.py,cuda_core/build_hooks.py: add the toolchain helpers. The genuinely-shared logic (constants, name resolution/validation, env application, preflight) is in a single block delimited by# --- begin/end shared toolchain helpers ---markers, duplicated verbatim with a "keep in sync" comment — mirroring the existing_import_get_cuda_path_or_homeprecedent. PEP 517 build isolation forbids a shared module (the sibling package is not installed in the isolated build env), so the block is duplicated rather than imported. The per-package_resolve_toolchain()is a thin wrapper that assembles only its own package-specific flags (cuda.bindings: c++14,-fpermissive,-O3; cuda.core: c++17,-O2).cuda_bindings/setup.py: dropped the now-redundant_is_clangstrip (_resolve_toolchainselects the correct flag set upfront); addedforce_build_extcheck andrecord_build_toolchain()call.cuda_core/setup.py: updated to callrecord_build_config()(wasrecord_build_major()).toolshed/check_build_hooks_sync.py+.pre-commit-config.yaml: new pre-commit hook (check-build-hooks-sync) that verifies the shared block is byte-identical across both files at commit time, replacing the test that was fragile outside a full monorepo checkout.test_build_hooks.pyfiles.CUDA_PYTHON_TOOLCHAINintentionally not documented inenvironment_variables.rst(no support guarantee; keeps the surface from becoming relied-upon automation).Build paths verified
All current build entry points keep working: cibuildwheel (
build-wheel.yml, LinuxCC="sccache cc", Windows MSVC),python -m build/pip wheel(test-sdist-linux.yml,test-sdist-windows.yml),coverage.yml, pixipixi-build-python, the Cython-testbuild_tests.pydrivers, and localrebuild-cuda-python. The default path (unsetCUDA_PYTHON_TOOLCHAIN) is unchanged.Checklist