Skip to content
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] Added IDE changes for JNI files to have intelliSense and changes for rapids-compose [skip ci] #5659

Merged
merged 12 commits into from
Jul 21, 2020

Conversation

razajafri
Copy link
Contributor

This PR adds the ability to use intellisense with JNI code while using rapids-compose. Thank you @trxcllnt for all your help in setting this up

@razajafri razajafri requested a review from a team as a code owner July 9, 2020 00:35
@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

java/pom.xml Outdated
<CUDA_STATIC_RUNTIME>OFF</CUDA_STATIC_RUNTIME>
<PER_THREAD_DEFAULT_STREAM>OFF</PER_THREAD_DEFAULT_STREAM>
<project.build.directory>target/${CUDA_VERSION}</project.build.directory>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not exactly sure if we should be modifying this variable. @tgravescs @revans2 @jlowe please comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note, this isn't required for rapids-compose. The script in compose only cares about the C++ build dir, and is setting -Dnative.build.path= as part of the mvn package command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but this will help manage the java bindings in a similar way cpp build is managed.
I still need to add the branch name to this property

@razajafri razajafri changed the title added changes for jni files to have intelliSense [REVIEW] added changes for jni files to have intelliSense [skip ci] Jul 9, 2020
@razajafri razajafri changed the title [REVIEW] added changes for jni files to have intelliSense [skip ci] [REVIEW] added changes for jni files to have intelliSense and changes for rapids-compose [skip ci] Jul 9, 2020
@razajafri razajafri changed the title [REVIEW] added changes for jni files to have intelliSense and changes for rapids-compose [skip ci] [REVIEW] Added IDE changes for JNI files to have intelliSense and changes for rapids-compose [skip ci] Jul 9, 2020
Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

I am not a fan of us messing with the project.build.directory.

java/pom.xml Outdated Show resolved Hide resolved
java/pom.xml Outdated Show resolved Hide resolved
java/pom.xml Outdated Show resolved Hide resolved
java/src/main/native/CMakeLists.txt Outdated Show resolved Hide resolved
java/src/main/native/CMakeLists.txt Show resolved Hide resolved
@@ -129,6 +133,7 @@ set(CUDF_INCLUDE "${PROJECT_SOURCE_DIR}/../../../../cpp/include"

find_library(CUDF_LIBRARY "cudf"
HINTS "$ENV{CUDF_ROOT}/lib"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this necessary? If others are OK I would like to remove this. I don't think we need this to build on the host directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

It follows the conventions of the other libraries. and is where we would find libcudf if it were installed instead of just being built.

@razajafri
Copy link
Contributor Author

@revans2 @jlowe thanks for the review, is there anything else you want me to change?

@trxcllnt
Copy link
Contributor

trxcllnt commented Jul 9, 2020

tl;dr: these two commits are the minimal set of changes I've tested that work in compose:

  1. pom.xml
  2. CMakeLists.txt

@jlowe The changes to the CMakeLists.txt are to support source-building the RAPIDS projects against each other (without installing). We don't have an "install" location in the traditional sense, because we only run make and not make install.

The loose pattern in our CI and build.sh scripts is:

  • <PROJ>_HOME env vars point at the repository root
  • <PROJ>_ROOT env vars point at the install location, or occasionally the C++ build dir

I've replicated this pattern in rapids-compose for consistency:

RMM_HOME=rapids/rmm
RMM_ROOT=rapids/rmm/build/release
CUDF_HOME=rapids/cudf
CUDF_ROOT=rapids/cudf/cpp/build/release

With CMake I'd override the paths on the command line like this:

cmake -D RMM_INCLUDE=$RMM_HOME/include \
      -D RMM_LIBRARY=$RMM_ROOT/librmm.so \
      -D CUDF_INCLUDE=$CUDF_HOME/cpp/include \
      -D CUDF_LIBRARY=$CUDF_ROOT/libcudf.so \
    ..

But @razajafri and I couldn't find a good way to use mvn package and pass those arguments onto CMake. We can modify pom.xml to forward on each CLI argument (like -DARROW_STATIC_LIB), but there isn't a good default for each of those vars if you invoke mvn package standalone. Defaulting to nothing causes CMake to think the empty string should be the location.

So the alternative is to have the CMakeLists.txt look in additional locations via environment variables. Neither RMM nor cuDF have lib or include directories in their C++ build dirs, which is why we set RMM_INCLUDE and CUDF_INCLUDE to their source locations.

@jlowe
Copy link
Member

jlowe commented Jul 16, 2020

Thanks for the clarification, @trxcllnt. That helps explain why $RMM_HOME was showing up in the JNI CMakeLists.txt but not the cpp version.

@razajafri
Copy link
Contributor Author

@jlowe do you have any other comments?

###################################################################################################
# - CUDF ------------------------------------------------------------------------------------------

set(CUDF_INCLUDE "${PROJECT_SOURCE_DIR}/../../../../cpp/include"
"${PROJECT_SOURCE_DIR}/../../../../cpp/src/")

find_library(CUDF_LIBRARY "cudf"
HINTS "$ENV{CUDF_ROOT}/lib"
HINTS "$ENV{CUDF_ROOT}"
Copy link
Member

Choose a reason for hiding this comment

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

$CUDF_ROOT/lib should remain in case the user set this environment variable to be the install prefix.

@razajafri razajafri merged commit 79bd435 into rapidsai:branch-0.15 Jul 21, 2020
@vyasr vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs cuDF (Java) Reviewer labels Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Needs Review Waiting for reviewer to review or respond Java Affects Java cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants