-
Notifications
You must be signed in to change notification settings - Fork 266
Use cuda-pathfinder in build-system.requires
#1817
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?
Changes from 4 commits
01f33cd
0fc4597
3d422de
73e840e
6892ae0
5a06d6f
b779d0d
6facaa5
f1ef076
3cb0313
a9995ac
cb5ffbb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,13 +33,40 @@ | |
| _extensions = None | ||
|
|
||
|
|
||
| # Please keep in sync with the copy in cuda_core/build_hooks.py. | ||
| def _import_get_cuda_path_or_home(): | ||
| """Import get_cuda_path_or_home, working around PEP 517 namespace shadowing. | ||
|
|
||
| In isolated build environments, backend-path=["."] causes the ``cuda`` | ||
| namespace package to resolve to only the project's ``cuda/`` directory, | ||
| hiding ``cuda.pathfinder`` installed in the build-env's site-packages. | ||
| Fix by replacing ``cuda.__path__`` with a plain list that includes the | ||
| site-packages ``cuda/`` directory. | ||
| """ | ||
| try: | ||
| import cuda.pathfinder | ||
| except ModuleNotFoundError: | ||
| import cuda | ||
|
|
||
| for p in sys.path: | ||
| sp_cuda = os.path.join(p, "cuda") | ||
| if os.path.isdir(os.path.join(sp_cuda, "pathfinder")): | ||
| cuda.__path__ = list(cuda.__path__) + [sp_cuda] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mutating If we keep this overall approach, I think the safer version is to resolve the installed from importlib import metadata
from pathlib import Path
# after narrowing the ModuleNotFoundError as above
import cuda
dist = metadata.distribution("cuda-pathfinder")
site_cuda = str(dist.locate_file(Path("cuda")))
cuda_paths = list(cuda.__path__)
if site_cuda not in cuda_paths:
cuda.__path__ = cuda_paths + [site_cuda]
from cuda.pathfinder import get_cuda_path_or_home
return get_cuda_path_or_homeThat still uses the namespace workaround, but it removes the If you'd rather avoid
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, much better, thanks! Done: commit 6facaa5
The answer to this goes deeper, I rewrote the PR description to make this clearer. Quoting from there:
We don't want to advertise a helper function that reaches into pathfinder internals.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately it looks like we have to backtrack to the We tried replacing the Diagnostic output (linux-64, py3.12)What happens
Conclusion
CI run with diagnostics: https://github.com/NVIDIA/cuda-python/actions/runs/23809723643
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another Cursor writeup, based on the diagnostic results: Regarding the two concerns about the "First hit wins" ordering: In this context there is exactly one Sticky And as shown in the diagnostic run, |
||
| break | ||
|
Comment on lines
+52
to
+56
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Path hacking of any sort like this is a code smell. At the very least the docstring is missing a concrete example of why this is needed.
is doing a ton of work there. Isolated how? What is In what real-world scenarios does this actually happen where there's not a viable alternative? I'm really skeptical that path hacking like this is necessary.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| else: | ||
| raise ModuleNotFoundError( | ||
| "cuda-pathfinder is not installed in the build environment. " | ||
| "Ensure 'cuda-pathfinder>=1.5' is in build-system.requires." | ||
|
Comment on lines
+59
to
+60
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: Does this msg make sense? We already updated pyproject.toml below. I suppose you meant to say: "--no-build-isolation is used, but cuda-pathfinder is not installed in the build environment." ? Because if the build isolation is in use, the build-time dependency is taken care of by the build frontend. |
||
| ) | ||
| import cuda.pathfinder | ||
|
|
||
| return cuda.pathfinder.get_cuda_path_or_home | ||
|
|
||
|
|
||
| @functools.cache | ||
| def _get_cuda_path() -> str: | ||
| # Not using cuda.pathfinder.get_cuda_path_or_home() here because this | ||
| # build backend runs in an isolated venv where the cuda namespace package | ||
| # from backend-path shadows the installed cuda-pathfinder. See #1803 for | ||
| # a workaround to apply after cuda-pathfinder >= 1.5 is released. | ||
| cuda_path = os.environ.get("CUDA_PATH", os.environ.get("CUDA_HOME")) | ||
| get_cuda_path_or_home = _import_get_cuda_path_or_home() | ||
| cuda_path = get_cuda_path_or_home() | ||
|
Comment on lines
68
to
+70
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am concerned about audibility. With pathfinder being used at build time, it could be possible that if CUDA headers are installed via pip or conda, they'd be used, but we are less certain (than using In CuPy the treatment we introduced was to hard-wire
so that we can inspect it at test/release times. Can we do the same thing? |
||
| if not cuda_path: | ||
| raise RuntimeError("Environment variable CUDA_PATH or CUDA_HOME is not set") | ||
| print("CUDA path:", cuda_path) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ requires = [ | |
| "setuptools_scm[simple]>=8", | ||
| "cython>=3.2,<3.3", | ||
| "pyclibrary>=0.1.7", | ||
| "cuda-pathfinder>=1.5", | ||
| ] | ||
| build-backend = "build_hooks" | ||
| backend-path = ["."] | ||
|
|
@@ -31,7 +32,7 @@ classifiers = [ | |
| "Environment :: GPU :: NVIDIA CUDA", | ||
| ] | ||
| dynamic = ["version", "readme"] | ||
| dependencies = ["cuda-pathfinder >=1.4.2"] | ||
| dependencies = ["cuda-pathfinder >=1.5"] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: We don't need to bump the run-time dependency; build-time (as done above) is enough |
||
|
|
||
| [project.optional-dependencies] | ||
| all = [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,13 +28,40 @@ | |
| COMPILE_FOR_COVERAGE = bool(int(os.environ.get("CUDA_PYTHON_COVERAGE", "0"))) | ||
|
|
||
|
|
||
| # Please keep in sync with the copy in cuda_bindings/build_hooks.py. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should avoid duplicating implementations. Could we not hoist this into a separate file and import from both?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agreed
No 😭 In the words of Cursor Opus 4.6 1M Thinking: There's no reasonable way to share code between the two
The only alternatives are worse: a new PyPI package just for this, The "please keep in sync" comments are the pragmatic solution here.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have other existing cases of "please keep in sync" that are impractical to avoid. It just crossed my mind: a pre-commit check to enforce that they stay in sync is probably pretty easy to implement. |
||
| def _import_get_cuda_path_or_home(): | ||
| """Import get_cuda_path_or_home, working around PEP 517 namespace shadowing. | ||
|
|
||
| In isolated build environments, backend-path=["."] causes the ``cuda`` | ||
| namespace package to resolve to only the project's ``cuda/`` directory, | ||
| hiding ``cuda.pathfinder`` installed in the build-env's site-packages. | ||
| Fix by replacing ``cuda.__path__`` with a plain list that includes the | ||
| site-packages ``cuda/`` directory. | ||
| """ | ||
| try: | ||
| import cuda.pathfinder | ||
| except ModuleNotFoundError: | ||
| import cuda | ||
|
|
||
| for p in sys.path: | ||
| sp_cuda = os.path.join(p, "cuda") | ||
| if os.path.isdir(os.path.join(sp_cuda, "pathfinder")): | ||
| cuda.__path__ = list(cuda.__path__) + [sp_cuda] | ||
| break | ||
| else: | ||
| raise ModuleNotFoundError( | ||
| "cuda-pathfinder is not installed in the build environment. " | ||
| "Ensure 'cuda-pathfinder>=1.5' is in build-system.requires." | ||
| ) | ||
| import cuda.pathfinder | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we include some message explaining that we iterated through sys.path looking for pathfinder and couldn't find it? That seems to be important information that would be missing from just throwing we couldn't import pathfinder module.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done: commit 3d422de |
||
|
|
||
| return cuda.pathfinder.get_cuda_path_or_home | ||
|
|
||
|
|
||
| @functools.cache | ||
| def _get_cuda_path() -> str: | ||
| # Not using cuda.pathfinder.get_cuda_path_or_home() here because this | ||
| # build backend runs in an isolated venv where the cuda namespace package | ||
| # from backend-path shadows the installed cuda-pathfinder. See #1803 for | ||
| # a workaround to apply after cuda-pathfinder >= 1.5 is released. | ||
| cuda_path = os.environ.get("CUDA_PATH", os.environ.get("CUDA_HOME")) | ||
| get_cuda_path_or_home = _import_get_cuda_path_or_home() | ||
| cuda_path = get_cuda_path_or_home() | ||
| if not cuda_path: | ||
| raise RuntimeError("Environment variable CUDA_PATH or CUDA_HOME is not set") | ||
| print("CUDA path:", cuda_path) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
This
except ModuleNotFoundErroris too broad: it will also catchModuleNotFoundErrors raised from insidecuda.pathfinderitself. Ifcuda.pathfinderis installed but one of its own imports is broken, we silently reinterpret that as the namespace-shadowing case and take the workaround path.I'd narrow the fallback to the exact failure we expect here:
That preserves the real traceback for genuine
cuda.pathfinderimport bugs and keeps the workaround limited to thecuda.pathfinder-not-found case.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.
Thanks for catching this (no pun intended)!
Done: commit b779d0d