Skip to content

Use nvcomp's snappy decompressor in avro reader - #9181

Merged
rapids-bot[bot] merged 42 commits into
NVIDIA:branch-21.12from
devavret:avro-reader-nvcomp-snappy
Sep 27, 2021
Merged

Use nvcomp's snappy decompressor in avro reader#9181
rapids-bot[bot] merged 42 commits into
NVIDIA:branch-21.12from
devavret:avro-reader-nvcomp-snappy

Conversation

@devavret

@devavret devavret commented Sep 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

devavret added 27 commits May 7, 2021 17:26
@devavret
Initial changes to get nvcomp integrated
db23741 
Cmake changes (excluding changes needed in nvcomp's cmake)
Replace cuIO's snappy compressor with nvcomp
@devavret
Using nvcomp provided max compressed buffer size
a5f3363
@devavret
Recover from error in nvcomp compressing and encode uncompressed.
61018aa
@devavret
review changes
64d7d1c
@devavret
Use nvcomp's snappy decompressor in parquet reader
9787c7c
@devavret
Replace accidental vector with uvector.
27764e7
@devavret
Provide the actual max uncomp page size to nvcomp's temp size estimat…
95a57ec 
…or rather than a hardcoded value
@devavret
cmake changes requested in review
cc9500a
@devavret
Merge branch 'parquet-writer-nvcomp-snappy' into parquet-reader-nvcom…
679eae1 
…p-snappy
@devavret
Merge branch 'branch-21.10' into parquet-writer-nvcomp-snappy
7989b9c
@devavret
Merge branch 'branch-21.10' into parquet-writer-nvcomp-snappy
f90409c
@devavret
Merge branch 'parquet-writer-nvcomp-snappy' into parquet-reader-nvcom…
63d22b0 
…p-snappy
@devavret
Update parquet writer to use nvcomp 2.1
40ebd1e
@devavret
One more cmake change related to updating nvcomp
4a2cb24
@devavret
Merge branch 'parquet-writer-nvcomp-snappy' into parquet-reader-nvcom…
1e6a1be 
…p-snappy
@devavret
Update nvcomp to version with fix for snappy decompressor
6019b0f
@devavret
Merge branch 'parquet-writer-nvcomp-snappy' into parquet-reader-nvcom…
79953a8 
…p-snappy
@devavret
Update reader to use new nvcomp API
d7b86b7
@devavret
Fix allocation size bug
140d3d0 
When writing statistics, there's not enough space allocated in chunk's compressed buffer.
This results in the compressed buffer being written into another chunk's memory.
@devavret
Merge branch 'parquet-writer-nvcomp-snappy' into parquet-reader-nvcom…
004d50b 
…p-snappy
@devavret
Merge branch 'branch-21.10' into parquet-writer-nvcomp-snappy
05f5343
@devavret
Update cmake to find nvcomp in new manner
62d92b4
@devavret
Merge branch 'parquet-writer-nvcomp-snappy' into avro-reader-nvcomp-s…
4155a89 
…nappy
@devavret
First commit to add nvcomp snappy decomp to avro
a0824e6 
Decompression works and newly added pytest passes.
Needs cleanup and benchmarking
@devavret
Cleanups
8645c68
@devavret
Add begin and end accessors to hd_vec.
ddff6fb 
And more cleanups
@devavret
Collect deflate decomp code into a single branch and add pytest for d…
26d9bbf 
…eflate codec
@devavret
devavret requested review from a team as code owners September 7, 2021 12:03
robertmaynard
robertmaynard reviewed Sep 8, 2021
View reviewed changes
Comment thread cpp/cmake/thirdparty/get_nvcomp.cmake Outdated
GITHUB_REPOSITORY NVIDIA/nvcomp
GIT_TAG 3a12516afdeab4ace01298031757f84b8dda81b7
# GIT_SHALLOW TRUE
OPTIONS "BUILD_TESTS OFF"

@robertmaynard robertmaynard Sep 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should add BUILD_STATIC ON to the nvcomp options. This will make it build as a static library and become embedded into cudf so we don't need to distribute another shared library along side cudf

@jlowe jlowe Sep 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will the nvcomp APIs be exported to users of libcudf.so? We have Java bindings to nvcomp in the cudf Java bindings (yeah, it's odd, long story). It would be nice to be able to leverage the lone instance of nvcomp rather than shipping a separate copy of nvcomp in the cudf jar.

@robertmaynard robertmaynard Sep 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If built statically we wouldn't be able to provide a stable nvcomp API since any function not used by libcudf.so would be removed by the linker.

If we need to package nvcomp as part of our dependencies we need to make sure of the following:

  • we are using the same nvcomp between the java bindings and cudf
  • we have a conda nvcomp package

@jlowe jlowe Sep 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah if it's complicated we'll just ship our own statically-linked nvcomp with the Java bindings.

@github-actions github-actions Bot added the conda label Sep 14, 2021
@devavret
devavret requested a review from a team as a code owner September 16, 2021 21:59
@github-actions github-actions Bot removed the CMake CMake build issue label Sep 16, 2021
nvdbaranec
nvdbaranec suggested changes Sep 20, 2021
View reviewed changes

@nvdbaranec nvdbaranec left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a couple of small things.

Comment thread cpp/src/io/avro/reader_impl.cu Outdated
Comment thread cpp/src/io/avro/reader_impl.cu Outdated
Comment thread cpp/src/io/avro/reader_impl.cu Outdated
Comment thread cpp/src/io/avro/reader_impl.cu Outdated
ajschmidt8
ajschmidt8 reviewed Sep 20, 2021
View reviewed changes
Comment thread conda/environments/cudf_dev_cuda11.0.yml
nvdbaranec
nvdbaranec suggested changes Sep 20, 2021
View reviewed changes
Comment thread cpp/src/io/avro/reader_impl.cu Outdated
Comment thread cpp/src/io/avro/reader_impl.cu Outdated
Comment thread cpp/src/io/avro/reader_impl.cu Outdated
@devavret devavret mentioned this pull request Sep 20, 2021
Merged
@ajschmidt8

ajschmidt8 commented Sep 21, 2021

Copy link
Copy Markdown
Member

rerun tests

@codecov

codecov Bot commented Sep 21, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.12@0cf2c5f). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head ad66a24 differs from pull request most recent head 03a278b. Consider uploading reports for the commit 03a278b to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.12    #9181   +/-   ##
===============================================
  Coverage                ?   10.89%           
===============================================
  Files                   ?      115           
  Lines                   ?    19087           
  Branches                ?        0           
===============================================
  Hits                    ?     2080           
  Misses                  ?    17007           
  Partials                ?        0

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 0cf2c5f...03a278b. Read the comment docs.

ajschmidt8
ajschmidt8 approved these changes Sep 21, 2021
View reviewed changes

@ajschmidt8 ajschmidt8 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

@devavret
devavret changed the base branch from branch-21.10 to branch-21.12 September 22, 2021 18:50
@devavret

devavret commented Sep 27, 2021

Copy link
Copy Markdown
Contributor Author

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit 42ffe78 into NVIDIA:branch-21.12 Sep 27, 2021
@vuule vuule added the cuIO cuIO issue label Sep 30, 2021
@vyasr vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs cuIO Reviewer labels Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robertmaynard robertmaynard robertmaynard left review comments
@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes
@galipremsagar galipremsagar galipremsagar approved these changes
@vuule vuule Awaiting requested review from vuule
@trxcllnt trxcllnt Awaiting requested review from trxcllnt
+2 more reviewers
@jlowe jlowe jlowe left review comments
@nvdbaranec nvdbaranec nvdbaranec approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond cuIO cuIO issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@devavret @ajschmidt8 @robertmaynard @jlowe @galipremsagar @nvdbaranec @vyasr @vuule @caryr35