Skip to content

[native]: Enable Velox cuDF #25094

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[native]: Enable Velox cuDF #25094

wants to merge 1 commit into from

Conversation

majetideepak
Copy link
Collaborator

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... 
* ... 

Hive Connector Changes
* ... 
* ...

If release note is NOT required, use:

== NO RELEASE NOTE ==

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label May 12, 2025
@majetideepak majetideepak force-pushed the enable-cudf branch 3 times, most recently from d13712c to 7794347 Compare May 12, 2025 21:18
@majetideepak majetideepak marked this pull request as ready for review May 12, 2025 21:18
@majetideepak majetideepak requested a review from a team as a code owner May 12, 2025 21:18
@prestodb-ci prestodb-ci requested review from a team, BryanCutler and infvg and removed request for a team May 12, 2025 21:18
@majetideepak majetideepak changed the title [native]: Enable Velox cuDF integration [native]: Enable Velox cuDF May 12, 2025
@zoltan
Copy link

zoltan commented May 12, 2025

I'm able to build this PR without issues.

@majetideepak
Copy link
Collaborator Author

todo: Add build information to README

@majetideepak majetideepak requested review from steveburnett and a team as code owners May 13, 2025 15:55
@majetideepak majetideepak requested a review from ZacBlanco May 13, 2025 15:55
@majetideepak majetideepak force-pushed the enable-cudf branch 2 times, most recently from 221488b to 2933adb Compare May 13, 2025 16:00
@majetideepak
Copy link
Collaborator Author

This is now complete.

steveburnett
steveburnett requested changes May 13, 2025
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the doc! One tiny nit of a suggestion.

steveburnett
steveburnett approved these changes May 13, 2025
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (docs)

BryanCutler
BryanCutler approved these changes May 13, 2025
View reviewed changes
Copy link
Contributor

@BryanCutler BryanCutler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@majetideepak @zoltan @steveburnett
[native]: Enable Velox cuDF
98ca7a4 
Co-authored-by: Zoltan Arnold Nagy <[email protected]>
Co-authored-by: Steve Burnett <[email protected]>
@majetideepak majetideepak requested a review from czentgr May 13, 2025 18:16
@majetideepak
Copy link
Collaborator Author

@czentgr can you take a look? Thanks.

czentgr
czentgr reviewed May 13, 2025
View reviewed changes
presto-native-execution/CMakeLists.txt
add_compile_definitions(PRESTO_ENABLE_CUDF)
enable_language(CUDA)
# Determine CUDA_ARCHITECTURES automatically.
cmake_policy(SET CMP0104 NEW)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do this here if it is enabled also by the subproject (it is in the velox cmake as well when this is enabled).

presto-native-execution/README.md
In some environments, the CUDA_ARCHITECTURES and CUDA_COMPILER location must be explicitly set.
The make command will look like:

`CUDA_ARCHITECTURES=80 CUDA_COMPILER=/usr/local/cuda/bin/nvcc EXTRA_CMAKE_FLAGS=" -DPRESTO_ENABLE_CUDF=ON" make`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI. The VELOX CI uses CUDA_ARCHITECTURES=70 because this is the minimum support architecture.

presto-native-execution/scripts/dockerfiles/centos-dependency.dockerfile
rm -rf build

# put CUDA binaries on the PATH
ENV PATH=/usr/local/cuda/bin:${PATH}
Copy link
Contributor

@czentgr czentgr May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets see because in the Velox CI this is installed to /usr/local/cuda-12.8 after running the install_cuda function. So perhaps the location of the binaries changed since?
Edit: Turns out there are multiple symlinks. /usr/local/cuda is a symlink to /usr/local/cuda-12.8.

Turns out we missed to make sure this is built in the pipeline and I created facebookincubator/velox#13334 to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@czentgr czentgr czentgr left review comments

@BryanCutler BryanCutler BryanCutler approved these changes

@steveburnett steveburnett steveburnett approved these changes

@infvg infvg Awaiting requested review from infvg infvg was automatically assigned from prestodb/ibm-reviewers

@ZacBlanco ZacBlanco Awaiting requested review from ZacBlanco ZacBlanco is a code owner automatically assigned from prestodb/committers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
from:IBM PR from IBM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@majetideepak @zoltan @BryanCutler @steveburnett @czentgr @prestodb-ci