-
Notifications
You must be signed in to change notification settings - Fork 927
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
[REVIEW] Python redesign for libcudf++ #3254
Conversation
…ibcudfxx-pythonexample
…ibcudfxx-pythonexample
python/cudf/setup.py
Outdated
|
||
CUDA_HOME = os.environ.get("CUDA_HOME", False) | ||
if not CUDA_HOME: | ||
CUDA_HOME = os.path.dirname(os.path.dirname(shutil.which("cuda-gdb"))) |
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.
It's possible that the user doesn't have cuda-gdb
on their path, so CUDA_HOME
could still not be found at this point. Might want to check it again and if not found, raise an exception telling the user to set CUDA_HOME
.
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.
It's also possible the user has set the CUDA_HOME
environment variable to a path which doesn't exist. @shwina is this needed long term or is this just a temporary workaround similar to cub / libcudacxx below?
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.
@raydouglass any opposition to us tackling this in a follow up pr?
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.
@kkraus14 Nope, it's just a nice-to-have to error out early on in my opinion since the user will get a compile error later that should make it obvious the CUDA includes are missing.
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.
dask_cudf-specific changes LGTM - Thanks for the hard work on this @shwina !
Co-Authored-By: jakirkham <[email protected]>
Co-Authored-By: jakirkham <[email protected]>
Co-Authored-By: jakirkham <[email protected]>
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.
LGTM. Thanks for all of your hard work on this @shwina! 😄
See the "redesign document" here.
See the corresponding RMM PR rapidsai/rmm#163
Buffer
to usermm.DeviceBuffer
instead ofrmm.device_array
underneathColumn
to Cython and enable it to talk to both legacylibcudf
and newlibcudf++
[ ] Add example of porting Python functionality to libcudf++