Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Initial Java Support for GDS to KvikIO #396
base: branch-24.12
Are you sure you want to change the base?
Add Initial Java Support for GDS to KvikIO #396
Changes from 21 commits
83044ff
62649e6
161b260
30aa3dc
299bdb6
1162efc
3f29546
b3dd38f
fe91427
4ad08c6
83875b6
4120e4c
888d6a7
d337a16
833ad32
93598a8
661fcce
7610edb
3eb3daa
13b9a05
e3f8448
fa749a3
222426c
c704108
69b6d39
40a588f
fabd3c1
c29ac7b
157867f
acc6777
cfa183a
2d41f10
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there a reason to use
openjdk=8.*
? It appears that the latest isopenjdk=22.*
.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.
No particular reason, I just happened to have 8 installed and used that. I will update to 22
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.
JDK8 is a very much a least common denominator. It is no longer supported without paying a lot of money to oracle though.
https://www.oracle.com/java/technologies/java-se-support-roadmap.html
and oracle is trying very hard to push people away from older versions of java.
java 11, 17, and 21 are long term support versions that are currently GA.
Java generally has really good backwards compatibility so almost anything compiled with JDK8 can run on anything newer. But newer code is not backwards compatible with older runtimes. But you can ask newer compilers to output binary code that is compatible with older JDK versions. That is what we do with the Spark plugin.
We also support whatever Spark supports for their default binary release. So for older versions of Spark it is JDK8. For some of the latest versions that is JDK17, but because of how we ask the compiler to output older binary versions it really becomes a requirement that they have at least that version of java installed.
I wouldn't update to jdk22 unless you also decide which version of java you want to be minimally compatible with and ask the compiler to output compatible .class files.
Also just another point to think about jcuda still uses openjdk8. This is probably to have as much compatibility as possible. https://github.com/jcuda/jcuda-main/blob/master/BUILDING.md
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.
Thanks for that context, that makes a lot of sense. I have not been following the broader Java versioning ecosystem so was unaware of much of this. I think it is likely we will want to pick one of the versions with long term support, I would guess 17 or 21, but will think on it some more before making an update.
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.
This is where you can set what the output target is that 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.
@aslobodaNV The first real failure: this path is not the correct path to
nvcc
in the CI environment, which installedcuda-nvcc
withconda
. Can you just callnvcc
?Error log here:
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.
Similarly, the include paths should be located in the conda environment. The CUDA packages use a special "targets" layout. For example,
libcufile-dev
will installcufile.h
into${CONDA_PREFIX}/targets/x86_64-linux/include/cufile.h
.The
openjdk
package installs some headers into${CONDA_PREFIX}/include
and some into${CONDA_PREFIX}/include/linux
.CMake knows how to detect and parse this layout of the CUDA Toolkit but I do not know how Maven or other build systems are supposed to consume it. I would ask the Spark team, perhaps they know: @revans2, do you have insight here?
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.
Totally fine with committing the update to just call nvcc instead of the path, I just didn't happen to have nvcc on my path when I wrote that. Let me look into the include paths and reach out to Bobby if I get stuck.
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.
nit: catching Throwable is not ideal. It can include all kinds of unrecoverable errors. At a minimum you probably want to print out why this could not be initialized in the error message. That way you can debug that a dependency was missing/etc.
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.
Would you be able to point me to an example of best practices in this area? Happy to change it further from my latest update.
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 marked it as a nit because it is probably fine in this case. In fact when I went back to look at the CUDF code, I did the exact same thing 5 years ago.
https://github.com/rapidsai/cudf/blob/4dbb8a354a9d4f0b4d82a5bf9747409c6304358f/java/src/main/java/ai/rapids/cudf/NativeDepsLoader.java#L74-L83
It is just generally not good practice to catch a
Throwable
, becauseThrowable
includes allError
s andError
s are generally considered to not be recoverable. But really the main thing here is giving the user enough information that they can debug why it is failing. You are printing the error message, but the full stack trace would be better. That is the main thing that I would suggest you do.