-
Notifications
You must be signed in to change notification settings - Fork 871
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] add a cmake option to link to GDS/cuFile [skip ci] #6847
Conversation
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
We are in burn down, so technically shouldn't be opening new PRs in branch-0.17. But this one is isolated and low risk so we can allow it. |
@@ -0,0 +1,121 @@ | |||
#============================================================================= |
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.
We're going to need this file in libcudf as well where we should put this in a location where we can share the find module between the two.
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.
Do you want to do that now? I'm not sure about libcudf's timeline for integrating with cufile, so putting this in jni first and perhaps moving it later. I can move it now if you want.
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'd like the cmake find module to be placed somewhere general now, I.E. in this folder: https://github.com/rapidsai/cudf/tree/branch-0.17/cpp/cmake
It won't be used in the general libcudf cmake yet, but it sets things up to be used directly nicely in the future while presumably still allowing the JNI build to nicely use it now.
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.
Done.
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.
As discussed in an offline meeting, I am not a fan of builds that can silently pull in special features because it was present in the build environment. This is especially true for features that will impose significant extra requirements on the target environment of the artifact as in this case (i.e.: if build env finds cufile, the resulting jar can only run if cufile is found in the runtime env).
IMO there should not be anything implicit about adding cufile. The build should have a cmake option, defaulting to false, that controls whether the build should contain it. If the option is true and the build env cannot find cufile, build should fail.
Also I'd like to see the cufile support be optional at runtime even if support was built into the jar. This can be accomplished by having a separate jni.so library containing the cufile JNI bindings and only attempting to load that separate jni.so library when the Java classes specific to cufile support are loaded. That way if the application doesn't use interfaces requiring cufile, users don't need cufile in the environment. That avoids needing to add a classifier to the artifact detailing whether or not cufile is required to run it.
@jlowe cmake option added. PTAL |
Thanks, the option looks good, but I'd like to see cufile not being directly linked against libcudfjni.so. That means if cufile is turned on in the build, there's zero chance the jar will work in a runtime environment that doesn't have cufile. |
@jlowe removed the link to cuFile. |
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.
Approving this so I'm not blocking it over the break.
I'm not thrilled with the include paths having the possibility of cufile headers being available for libcudfjni.so. If not removed as part of this PR, I'd like to see it updated when the separate cufile jni shared lib is added later that enables optional cufile functionality at runtime.
@@ -277,7 +289,8 @@ include_directories("${THRUST_INCLUDE}" | |||
"${JNI_INCLUDE_DIRS}" | |||
"${CUDF_INCLUDE}" | |||
"${RMM_INCLUDE}" | |||
"${ARROW_INCLUDE}") | |||
"${ARROW_INCLUDE}" | |||
"$<$<BOOL:${cuFile_FOUND}>:${cuFile_INCLUDE_DIRS}>") |
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.
Ideally there should be separate include lists for libcudfjni.so and libcufilejni.so (or whatever we're going to call it) so we're not making it easy to create unsatisfied cufile symbols in libcudfjni.so.
If we're taking out the link for now then I question the utility of adding the includes.
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.
Using include_directories
is considered a cmake "antipattern" since it's a global function (https://cliutils.gitlab.io/modern-cmake/chapters/intro/dodonot.html). This file would need some cleanup before we can have different includes for different targets.
Add a cmake find module to locate
cuFile
. If found, add the include directory and link to the shared library.This shouldn't have any effect if
cuFile
is not installed locally.@jlowe @abellina