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.

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

Update Java nvcomp JNI bindings to nvcomp 2.x API #9384

Merged
merged 7 commits into from
Oct 26, 2021
Merged

Update Java nvcomp JNI bindings to nvcomp 2.x API #9384

merged 7 commits into from
Oct 26, 2021

Conversation

jbrennan333
Copy link
Contributor

This PR Closes #8336

The Java nvcomp JNI bindings are updated to the nvcomp 2.x API and the nvcomp library is no longer built as part of the Java build. It is reused from the dependency that libcudf is using.

Note that the nvcomp 1.x API is no longer supported by the CUDF Java JNI, so this is a breaking change. Spark-Rapids currently depends on the nvcomp 1.x JNI. I will be putting up a PR for spark-rapids that changes it to use this 2.x JNI, which will need to be merged soon after this PR to minimize incompatibilities.

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

1 similar comment
@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@github-actions github-actions bot added CMake CMake build issue Java Affects Java cuDF API. labels Oct 6, 2021
@jlowe jlowe added breaking Breaking change improvement Improvement / enhancement to an existing function labels Oct 6, 2021
@jlowe
Copy link
Contributor

jlowe commented Oct 6, 2021

ok to test

@jbrennan333 jbrennan333 mentioned this pull request Oct 6, 2021
Merged
abellina
abellina requested changes Oct 11, 2021
View reviewed changes
Copy link
Contributor

@abellina abellina left a comment

Choose a reason for hiding this comment

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

Overall it's looking really good. My comments are mostly around clarification. I'll do another pass when you want me to look again.

java/src/main/java/ai/rapids/cudf/Cuda.java Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/Cuda.java Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Compressor.java Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Compressor.java Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Compressor.java Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Compressor.java Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Compressor.java Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Compressor.java Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Compressor.java Outdated Show resolved Hide resolved
@jbrennan333
Copy link
Contributor Author

Thanks @jlowe and @abellina for the reviews and great comments. I am making suggested changes and will update the PR once I have tested.

@jbrennan333
Copy link
Contributor Author

I have pushed a commit with changes to address the review comments from @jlowe and @abellina. In addition to those changes I removed some interfaces in NvcompJni.java and NvcompJni.cpp that are deprecated in nvcomp-2.x. We don't use them.

@jbrennan333
Copy link
Contributor Author

ok to test

@jbrennan333 jbrennan333 self-assigned this Oct 13, 2021
@jbrennan333 jbrennan333 marked this pull request as ready for review October 13, 2021 16:59
@jbrennan333 jbrennan333 requested a review from a team as a code owner October 13, 2021 16:59
@jbrennan333
Copy link
Contributor Author

ok to test

@hyperbolic2346 hyperbolic2346 added the 3 - Ready for Review Ready for review by team label Oct 13, 2021
@hyperbolic2346
Copy link
Contributor

ok to test

1 similar comment
@ajschmidt8
Copy link
Member

ok to test

@codecov
Copy link

codecov bot commented Oct 14, 2021 •
edited
Loading

Codecov Report

Merging #9384 (d17c715) into branch-21.12 (ab4bfaa) will decrease coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.12    #9384      +/-   ##
================================================
- Coverage         10.79%   10.65%   -0.14%     
================================================
  Files               116      117       +1     
  Lines             18869    19760     +891     
================================================
+ Hits               2036     2106      +70     
- Misses            16833    17654     +821
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 0.00% <ø> (ø)
python/cudf/cudf/_lib/__init__.py 0.00% <ø> (ø)
python/cudf/cudf/io/csv.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/hdf.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/orc.py 0.00% <0.00%> (ø)
python/cudf/cudf/_version.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/abc.py 0.00% <0.00%> (ø)
python/cudf/cudf/api/types.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/dlpack.py 0.00% <0.00%> (ø)
... and 63 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cff71ff...d17c715. Read the comment docs.

abellina
abellina reviewed Oct 15, 2021
View reviewed changes
Copy link
Contributor

@abellina abellina left a comment

Choose a reason for hiding this comment

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

I have a few last comments, mostly nits.

java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Compressor.java Outdated Show resolved Hide resolved
java/src/main/native/src/check_nvcomp_output_sizes.cu Show resolved Hide resolved
java/src/test/java/ai/rapids/cudf/nvcomp/NvcompTest.java Outdated Show resolved Hide resolved
java/src/test/java/ai/rapids/cudf/nvcomp/NvcompTest.java Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Compressor.java Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Compressor.java Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Compressor.java Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/nvcomp/BatchedLZ4Decompressor.java Show resolved Hide resolved
@jbrennan333
Copy link
Contributor Author

I pushed another commit to change places where chunkSizes were using ints instead of longs. This was based on comments in NVIDIA/spark-rapids#3757

abellina
abellina reviewed Oct 19, 2021
View reviewed changes
Copy link
Contributor

@abellina abellina left a comment

Choose a reason for hiding this comment

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

I had one last nit, otherwise LGTM

@jbrennan333
Copy link
Contributor Author

Thanks @abellina! I have updated to restore the copyFromHostBuffer along with a fix for the unit test and your other comments.

@abellina
Copy link
Contributor

@jlowe, when you get a chance, could you give this another look?

@jbrennan333
Copy link
Contributor Author

@gpucibot merge

1 similar comment
@jlowe
Copy link
Contributor

jlowe commented Oct 26, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 063c982 into rapidsai:branch-21.12 Oct 26, 2021
@jlowe jlowe mentioned this pull request Nov 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abellina abellina abellina approved these changes

@jlowe jlowe jlowe approved these changes

Assignees

@jbrennan333 jbrennan333

Labels
3 - Ready for Review Ready for review by team breaking Breaking change CMake CMake build issue improvement Improvement / enhancement to an existing function Java Affects Java cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Update Java nvcomp bindings to nvcomp 2.x API
6 participants
@jbrennan333 @GPUtester @jlowe @hyperbolic2346 @ajschmidt8 @abellina