-
Notifications
You must be signed in to change notification settings - Fork 170
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
[FEA]: Introduce Python module with CCCL headers #3201
base: main
Are you sure you want to change the base?
Changes from 1 commit
daab580
ef9d5f4
bc116dc
2913ae0
7dbb82b
0703901
fc0e543
2e64345
82467cd
f13a96b
9ed6036
c9a4d96
df943c0
40c8389
e3c7867
acbd477
06f575f
e747768
499b191
62ce2d3
65c5a15
bffece6
585447c
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 |
---|---|---|
@@ -0,0 +1,2 @@ | ||
cuda/_include | ||
*egg-info |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# `cuda.cccl`: Experimental CUDA Core Compute Library Python module with CCCL headers | ||
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 should consider the name 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 have reservation on this, if considering the mirroring to conda packages (#3201 (comment)). 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. Ah, I waasn't aware there's a conda package called |
||
|
||
## Documentation | ||
|
||
Please visit the documentation here: https://nvidia.github.io/cccl/python.html. | ||
|
||
## Local development | ||
|
||
```bash | ||
pip3 install . | ||
``` | ||
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. Perhaps it's appropriate to document that this package is currently for internal use only and not meant to be used/installed explicitly. 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: c9a4d96 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. ALL RIGHTS RESERVED. | ||
# | ||
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
|
||
[build-system] | ||
requires = ["packaging", "setuptools>=61.0.0", "wheel"] | ||
build-backend = "setuptools.build_meta" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
# Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. ALL RIGHTS RESERVED. | ||
# | ||
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
|
||
import os | ||
import shutil | ||
|
||
from setuptools import Command, setup, find_namespace_packages | ||
from setuptools.command.build_py import build_py | ||
from wheel.bdist_wheel import bdist_wheel | ||
|
||
|
||
project_path = os.path.abspath(os.path.dirname(__file__)) | ||
cccl_path = os.path.abspath(os.path.join(project_path, "..", "..")) | ||
cccl_headers = [["cub", "cub"], ["libcudacxx", "include"], ["thrust", "thrust"]] | ||
ver = "0.1.2.8.0" | ||
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 think we need to use the CCCL version here, not CCCL Python modules' version. We should also not hard-code it, but instead read from CMakeLists which is the source of truth AFAIK, and for that setuptools might not be doing the job. @vyasr might have a simple example for how this can be done with scikit-build-core. 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. Ack. I added this is a bullet to the PR description. 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. Check out the dynamic metadata section, specifically the Regex tab. 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. You would need to rewrite everything here to use CMake instead of setuptools. Depending on what this module is trying to do that may or may not be beneficial. Do you need to run compilation of cuda_cccl/cooperative/parallel against CCCL headers? In that case it is almost certainly worthwhile, I wouldn't want to orchestrate that compilation using setuptools. 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.
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.
Based on
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 skimmed over the code and I am actually confused, because my impression is that the kernel compilation is still done at run time (JIT), and that the host logic can just be handled by a host compiler. @gevtushenko IIRC you built the prototype, any reason we have to use 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. Commit 2913ae0 adopts the established _version.py handling. 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. tl;dr I would suggest that if you have to do any compilation whatsoever beyond pure Cython you switch away from setuptools, but if you don't have any compiled modules at build time then stick to setuptools or use another backend that isn't designed for compilation (hatchling would be a great choice). 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.
In the offline call Georgii reminded me that there are some CUB structs that we need to pre-compile to pass around. Since generally CUB headers are not host compilable, NVCC has to be used, but we don't generate any GPU-specific code. |
||
|
||
|
||
with open("README.md") as f: | ||
long_description = f.read() | ||
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: this can be moved to pyproject.toml too, ex: 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 40c8389 |
||
|
||
|
||
class CustomBuildCommand(build_py): | ||
def run(self): | ||
self.run_command("package_cccl") | ||
build_py.run(self) | ||
|
||
|
||
class CustomWheelBuild(bdist_wheel): | ||
def run(self): | ||
self.run_command("package_cccl") | ||
super().run() | ||
|
||
|
||
class PackageCCCLCommand(Command): | ||
description = "Generate additional files" | ||
user_options = [] | ||
|
||
def initialize_options(self): | ||
pass | ||
|
||
def finalize_options(self): | ||
pass | ||
|
||
def run(self): | ||
for proj_dir, header_dir in cccl_headers: | ||
src_path = os.path.abspath(os.path.join(cccl_path, proj_dir, header_dir)) | ||
dst_path = os.path.join(project_path, "cuda", "_include", proj_dir) | ||
if os.path.exists(dst_path): | ||
shutil.rmtree(dst_path) | ||
shutil.copytree(src_path, dst_path) | ||
|
||
|
||
setup( | ||
name="cuda-cccl", | ||
version=ver, | ||
description="Experimental Package with CCCL headers to support JIT compilation", | ||
long_description=long_description, | ||
long_description_content_type="text/markdown", | ||
author="NVIDIA Corporation", | ||
classifiers=[ | ||
"Programming Language :: Python :: 3 :: Only", | ||
"Environment :: GPU :: NVIDIA CUDA", | ||
], | ||
packages=find_namespace_packages(include=["cuda.*"]), | ||
python_requires=">=3.9", | ||
cmdclass={ | ||
"package_cccl": PackageCCCLCommand, | ||
"build_py": CustomBuildCommand, | ||
"bdist_wheel": CustomWheelBuild, | ||
}, | ||
include_package_data=True, | ||
license="Apache-2.0 with LLVM exception", | ||
license_files=("../../LICENSE",), | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,2 @@ | ||
cuda/_include | ||
env | ||
*egg-info |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,3 @@ | ||
cuda/_include | ||
env | ||
*egg-info | ||
*so |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,16 @@ Please visit the documentation here: https://nvidia.github.io/cccl/python.html. | |
|
||
## Local development | ||
|
||
First-time installation: | ||
|
||
```bash | ||
pip3 install ./cuda_cccl | ||
pip3 install ./cuda_parallel[test] | ||
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. Can this be editable if necessary? Wouldn't a regular install here and then an editable install below would lead to two copies of the package in the environment? 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. It can! Thanks for asking. Previously I was incorrectly thinking that one pass without I restored the original README.md: commit 9ed6036
From what I can tell, the 2nd install clobbers the previous one. |
||
pytest -v ./cuda_parallel/tests/ | ||
``` | ||
|
||
For faster iterative development: | ||
|
||
```bash | ||
pip3 install -e .[test] | ||
pytest -v ./tests/ | ||
pip3 install -e ./cuda_parallel[test] | ||
``` |
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.
Q: Is it possible that we consolidate
.gitignore
files at the root directory and not have independent ones per sub dir...?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.
I created #3212 to look into this later.