Repository navigation
Conversation
Conda's `cuda-nvcc` inserts it's own CCCL in a way that setuptools ends up placing them before our includes. However, using the system CCCL means we find a version that isn't (currently) compatible. That means that in such a setup, the old "TMA doesn't link" bug resurfaces. This is a somewhat weird work-around, but it was the best idea, I could think of...
github-actions
Bot
added
the
cuda.core
📝 Summary
Priority: ⬇️ Low Change: Bug fix Merge Risk: 🔵 Low · up to
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
078222e2-d909-4ee6-9f89-e4d2826b80ca
📒 Files selected for processing: 2
cuda_core/build_hooks.pycuda_core/tests/test_build_hooks.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| cccl_flags = shlex.join([f"-I{path}" for path in include_dirs]) | ||
| default_flags = sysconfig.get_config_var("CFLAGS") or "" | ||
| for name in ("CFLAGS", "CXXFLAGS", "CPPFLAGS"): | ||
| flags = os.environ.get(name, "" if name == "CPPFLAGS" else default_flags) | ||
| if flags != cccl_flags and not flags.startswith(cccl_flags + " "): | ||
| os.environ[name] = f"{cccl_flags} {flags}" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
git diff --no-ext-diff --unified=80 3f58a7bee17cef175a614a591b97733c2c0f569d 3aefb73e4ef474692b984a9d8857438ed0b2fc08 -- cuda_core/build_hooks.py cuda_core/tests/test_build_hooks.py
printf '\n--- helper and callers ---\n'
rg -n -C 12 '_configure_cccl_include_flags|_build_cuda_core|CFLAGS|CXXFLAGS|CPPFLAGS' cuda_core/build_hooks.py cuda_core/tests/test_build_hooks.py
printf '\n--- repository guidance ---\n'
find .. -name AGENTS.md -o -name CONTRIBUTING.md -o -name '*guideline*' -o -name '*style*' | head -80Repository: NVIDIA/cuda-python
Length of output: 41763
🏁 Script executed:
set -e
printf '%s\n' '--- applicable guidance ---'
for f in AGENTS.md cuda_core/AGENTS.md cuda_core/tests/AGENTS.md CONTRIBUTING.md; do
if [ -f "$f" ]; then
printf '\n### %s\n' "$f"
nl -ba "$f"
fi
done
printf '%s\n' '--- head helper and callers ---'
rg -n -F -C 20 -- '_configure_cccl_include_flags' cuda_core/build_hooks.py cuda_core/tests/test_build_hooks.py
printf '%s\n' '--- all test references to helper/caller and environment flags ---'
rg -n -F -C 8 -e '_configure_cccl_include_flags' -e '_build_cuda_core(' -e 'CFLAGS' -e 'CXXFLAGS' -e 'CPPFLAGS' cuda_core/tests cuda_core | sed -n '1,1400p'
printf '%s\n' '--- test declarations and relevant blocks ---'
python3 - <<'PY'
from pathlib import Path
p = Path('cuda_core/tests/test_build_hooks.py')
for i, line in enumerate(p.read_text().splitlines(), 1):
if any(x in line for x in ('def test_', 'class Test', '_capture_cythonize_kwargs', '_build_cuda_core()', '_configure_cccl_include_flags')):
print(f'{i}:{line}')
PY
printf '%s\n' '--- changed test diff ---'
git diff --no-ext-diff --unified=30 3f58a7bee17cef175a614a591b97733c2c0f569d 3aefb73e4ef474692b984a9d8857438ed0b2fc08 -- cuda_core/tests/test_build_hooks.py
printf '%s\n' '--- exact head source ranges around helper and every _build_cuda_core test call ---'
nl -ba cuda_core/build_hooks.py | sed -n '440,510p'
nl -ba cuda_core/tests/test_build_hooks.py | sed -n '380,430p;840,910p;980,1060p;1060,1190p'Repository: NVIDIA/cuda-python
Length of output: 41996
🏁 Script executed:
set -e
printf '%s\n' '--- scoped guidance ---'
for f in cuda_core/AGENTS.md cuda_core/tests/AGENTS.md; do
printf '\n### %s\n' "$f"
nl -ba "$f"
done
printf '%s\n' '--- helper call and test assertions ---'
rg -n -F -C 6 -e '_configure_cccl_include_flags' -e 'os.environ' -e 'CFLAGS' -e 'CXXFLAGS' -e 'CPPFLAGS' cuda_core/tests/test_build_hooks.py
printf '%s\n' '--- indirect caller test blocks ---'
nl -ba cuda_core/tests/test_build_hooks.py | sed -n '390,425p;870,905p;995,1045p'
printf '%s\n' '--- caller source ---'
nl -ba cuda_core/build_hooks.py | sed -n '451,520p'Repository: NVIDIA/cuda-python
Length of output: 41756
suggestion: Add direct tests for CCCL flag ordering and repeated invocation.
_build_cuda_core() indirectly calls _configure_cccl_include_flags(), but existing tests do not assert the resulting environment flags. Add a focused test that supplies a competing -I flag, checks that the pinned CCCL path comes first, and calls the helper twice to check idempotence.
|
|
Restarted unrelated test failure Details |
|
I am still confused. We don’t need nvcc at build time. We only need its headers, and it’s how the |
|
The issue is But there is a point, maybe we should just close this. It seems rather a Note 100% sure, let's see where this goes: conda-forge/cuda-nvcc-feedstock#110 |
|
Hmmm OK you're referring to Does this affect CuPy too? 🤔 |
|
Looks like CuPy is affected too... Asking in the internal conda channel. |
| import sys | ||
| import tempfile | ||
| import zipfile | ||
| from distutils import sysconfig |
There was a problem hiding this comment.
distutils was removed from the standard library in Python 3.12, so on 3.12+ this import only resolves through setuptools' compatibility shim (_distutils_hack, activated by the distutils-precedence.pth file that setuptools installs). Because build_hooks is the build backend and this line runs before from setuptools import ..., the import depends on that .pth hook having been processed. On 3.12 it fails with ModuleNotFoundError: No module named 'distutils' under, for example, python -S or SETUPTOOLS_USE_DISTUTILS=stdlib.
The stdlib sysconfig module gives the same value (on 3.12, distutils.sysconfig.get_config_var("CFLAGS") == sysconfig.get_config_var("CFLAGS")), so the shim isn't needed here:
| import sys | |
| import tempfile | |
| import zipfile | |
| from distutils import sysconfig | |
| import sys | |
| import sysconfig | |
| import tempfile | |
| import zipfile |
This was fixed. The conda compiler is supposed to do the right thing now. @Andy-Jost @seberg let's retry and see if we can just close this PR. |
|
Well, I can confirm it fixed things for me. Although something is broken on my env (or conda), in that the file is named But closing. Freshly created conda envs should be fine after all. |
3 participants
Conda's
cuda-nvccinserts it's own CCCL in a way that setuptools ends up placing them before our includes.However, using the system CCCL means we find a version that isn't (currently) compatible.
That means that in such a setup, the old "TMA doesn't link" bug resurfaces.
This is a somewhat weird work-around, but it was the best idea, I could think of...
Closes gh-3098