[Feature] Add Nvcomp python bindings to kvikio. #24

thomcom · 2022-03-09T01:47:44Z

This PR adds the basic bindings for Cascaded and Lz4, and a broken Snappy binding. I need to figure out how to import nvcomp properly before this will work with kvikio.

…ies added.

quasiben · 2022-03-09T20:25:04Z

Thanks @thomcom ! As part of this PR will you also include example notebooks and the benchmark plots you built earlier. The plots don't need to be committed in the PR but uploaded in the PR comments

… bit. Adding pure python wrappers for Cascade and Lz4 at this time.

thomcom · 2022-03-10T03:57:13Z

Thanks @quasiben I've added the files, I'll move them around later.

…ctually are, now.

thomcom · 2022-03-14T23:21:54Z

This is basically ready for review. Tomorrow I will add the notebook, benchmarks, and get the style updated correctly.

jakirkham · 2022-03-15T01:25:09Z

Do we need to do something to make sure nvCOMP is available on CI?

quasiben · 2022-03-15T13:45:12Z

Do we need to do something to make sure nvCOMP is available on CI?

I don't believe so as nvcomp tooling is tested as part of cuDF/libcuDF

@thomcom looks like there are some style check failures

jakirkham · 2022-03-15T15:08:51Z

I don't believe so as nvcomp tooling is tested as part of cuDF/libcuDF

How is the nvCOMP binding code here tested?

thomcom · 2022-03-15T17:52:47Z

@jakirkham the python bindings are tested here in compress->decompress tests for Cascaded and LZ4. python/tests/test_nvcomp.pyJ

jakirkham · 2022-03-15T18:09:14Z

Thanks. Was more asking are we running those on gpuCI here? Relatedly are there any gpuCI changes we need to include here to run the tests.

thomcom · 2022-03-15T18:27:28Z

Oh of course. I don't know if kvikio is running with any gpuCI yet, but there are tests! The nvcomp tests run automatically with the kvikio tests, so if kvikio is being run in gpuCI then so will nvcomp.

thomcom · 2022-03-15T18:32:07Z

These CI failures are mysterious to me.

jakirkham · 2022-03-15T18:38:38Z

Maybe try merging in branch-22.04 or pushing another commit? Seems like CI failed to checkout the PR. So would just give it another try. If it still fails, we can ask for help :)

thomcom · 2022-03-15T19:12:56Z

rerun tests

jakirkham · 2022-03-15T21:47:17Z

Think this CI error is capturing the issue that I'm wondering about

      building 'kvikio._lib.nvcomp' extension
      /usr/local/gcc9/bin/gcc -Wsign-compare -DNDEBUG -fwrapv -O3 -Wall -fPIC -O3 -I/workspace/.conda-bld/kvikio_1647375755170/work/cpp/include -I/workspace/.conda-bld/kvikio_1647375755170/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/include -I/usr/local/cuda/include -I/workspace/.conda-bld/kvikio_1647375755170/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/include/python3.8 -c kvikio/_lib/nvcomp.cpp -o build/temp.linux-x86_64-3.8/kvikio/_lib/nvcomp.o -std=c++17
      kvikio/_lib/nvcomp.cpp:776:10: fatal error: nvcomp.h: No such file or directory
        776 | #include "nvcomp.h"
            |          ^~~~~~~~~~
      compilation terminated.

Do we need to be cloning or downloading nvCOMP somewhere on CI first?

jakirkham · 2022-03-15T21:58:49Z

Thinking we need to add this code and then call it somewhere like this.

thomcom · 2022-03-15T22:46:32Z

I think we're already depending on cudf in this, are we not? There's a piece in setup.py about loading the headers from NVCOMP_HOME if they aren't installed. I will look again.

jakirkham

LGTM. Thanks Thomson! 😄

madsbk

Sorry for spamming the same suggestion, but I think some docstrings would be good. I doesn't have to anything lengthy. Just a short description, list of arguments/types, and maybe an url to the nvcomp doc when relevant.

Otherwise it looks good!

python/kvikio/nvcomp.py

…ork yet.

thomcom · 2022-03-24T22:34:56Z

@madsbk I added the docs that I forgot this morning. Thanks! I also dropped BatchedSnappy which has some issues I won't be working on presently.

madsbk

Thanks @thomcom, great work!

I took the liberty to rename nvcomp Cascade Benchmarks.ipynb -> nvcomp_cascade_benchmarks.ipynb, hope that is okay.

vyasr

Apologies for not reviewing sooner, I was waiting for repo permissions before I did much. I started making some comments on the pynvcomp files, but then I realized that those are taken from somewhere else. @thomcom did we modify them at all, or did we just copy them in and you just added nvcomp.py? I see at least a few discussions on those files around the Array type and the Enum import fix, so we've made nonzero modifications, but I'm not sure how much we intend to change them.

python/cmake/kvikio_python_helpers.cmake

python/cmake/thirdparty/get_nvcomp.cmake

python/kvikio/_lib/pynvcomp.pxd

python/kvikio/nvcomp.py

vyasr · 2022-03-25T16:48:22Z

python/kvikio/nvcomp.py

+        self.dtype = dtype
+        self.config = config
+        self.compressor = _lib._CascadedCompressor(
+            cp_to_nvcomp_dtype(self.dtype).value,
+            config.num_RLEs,
+            config.num_deltas,
+            config.use_bp,
+        )
+        self.decompressor = _lib._CascadedDecompressor()
+        self.s = cp.cuda.Stream()


Do we want all of these attributes to be visible to users, or are some of them internal details? Should they be marked as internal with underscores? Maybe we want immutable property-based access to some of them (i.e. a property without a setter)?

I think immutable is a good idea here, the fact that they are properties of the class is really just making them available as queryable parameters. They've been set in the underlying C++ class and are no longer accessible by any API, so I keep a record here.

Note for future work: Depending on whether users might actually need to access these properties, I would recommend either renaming them with underscores (e.g. self._decompressor) or turning them into properties instead of raw attributes.

python/kvikio/nvcomp.py

vyasr · 2022-03-25T16:53:40Z

python/kvikio/nvcomp.py

+        self.use_bp = use_bp
+
+
+class CascadedCompressor:


Another note you can feel free to push to future work, but it certainly seems like all of these compressors should inherit from an abstract parent class with a concrete constructor to standardize initializing the dtype, config, and stream parameters and abstract compress and decompress methods.

Indeed, I'd like to push it till later. :)

python/tests/test_nvcomp.py

vyasr · 2022-03-25T22:57:40Z

@thomcom my last request here would be to move pynvcomp.pxd to something like libnvcomp.pxd, then cimport it explicitly in pynvcomp.pyx. In Cython pxd and pyx files with the same name are interpreted in a very specific way, and in this case that relationship isn't accurate. In particular, there's no pure Cython in pynvcomp.pyx that you are trying to make cimportable in other Cython files, you're exclusively creating objects that are visible in pure Python. pynvcomp.pxd is not defining an interface for pynvcomp.pyx, it's importing the "symbols" from the C++ library. As such, it's misleading to have the names aligned.

thomcom · 2022-03-28T16:24:01Z

Hey @vyasr, your comment is interesting to me. My approach with .pxd and .pyx naming has always been that the .pxd file defines the interface to the existing C/C++ code, and the pyx implements the python bindings for that interface. In this case, the pure cython in pynvcomp.pyx is only the typecastings that convert between python objects and c pointers. It's not intended for cimport into other cython files, only to expose the C++ interface to python. Would you put the C++ interface and the python visible bindings into the .pyx file and not have a pxd file at all? I see that in _lib here there's a kvikio_cxx_api.pxd and a libkvikio.pyx, how about I rename into that pattern?

vyasr · 2022-03-28T16:37:56Z

You might be interested in looking at the first section on this page. Specifically, it's valuable to distinguish between use-cases 1 and 3. Use-case 1 is really what the pxd file in this PR is doing (exposing C++ code). In RAPIDS we rarely write pure Cython, it's almost always just a wrapper around C++, so we don't run into use-case 3 as much, but there are some good examples in cudf like column.pxd and copying.pxd.

Regarding the naming, I suggested the libFoo.pxd for C++ bindings and pyFoo.pyx or just Foo.pyx for the Cython since I think that's the common convention that I have observed across most other Cython packages that I've worked with. I'm fine with your proposal too since it maintains internal consistency in kvikio. Feel free to check with other devs too in case they express different preferences from mine.

madsbk · 2022-03-28T16:54:56Z

I see that in _lib here there's a kvikio_cxx_api.pxd and a libkvikio.pyx, how about I rename into that pattern?

I think this is a good idea. Then, if we want to change to another naming scheme like @vyasr suggest, we can change the name of both libraries at the same time.

…rds. Change CascadedOptions to defauled named args.

thomcom · 2022-03-28T18:21:09Z

All requests have been answered.

vyasr

@thomcom I've unresolved a pair of conversations to track potential future work, but otherwise looks good to go to me now!

…o fea-add-nvcomp

madsbk · 2022-03-29T11:20:32Z

Thanks @thomcom, it is merged.

Create first set of bindings and build command, need to get dependenc…

ff4d34d

…ies added.

thomcom added 3 commits March 9, 2022 16:59

Include cuda headers.

1e2759a

Add nvcomp to python module. Start cleaning up the cython for delivery.

6e771b0

Add Cascade benchmarks, batched snappy test, and update the library a…

d55bdc2

… bit. Adding pure python wrappers for Cascade and Lz4 at this time.

thomcom added 3 commits March 10, 2022 18:29

Got all the bindings working.

c4697df

What I meant was that Cascade Bindings are working now, which, they a…

51198e4

…ctually are, now.

Cascaded decompress wrapped and working.

bc82479

quasiben mentioned this pull request Mar 11, 2022

Integration into kvikIO carsonswope/py-nvcomp#3

Closed

thomcom added 3 commits March 11, 2022 12:17

Add lz4 bindings and cascade test.

cf6e500

Working basic round trip tests for LZ4 and Cascaded.

fc1ba4b

Parametrize and pass tests.

63578c7

Fix styles.

eaa2806

Missed isort of nvcomp.pyx

31b6b34

Permuting a file for rerun tests?

b4112ce

Try installing nvcomp.

38a49df

jakirkham approved these changes Mar 24, 2022

View reviewed changes

jakirkham requested review from madsbk and vyasr March 24, 2022 20:44

madsbk requested changes Mar 24, 2022

View reviewed changes

Add docstrings and get rid of BatchedSnappy compressor that doesn't w…

c76049d

…ork yet.

nvcomp Cascade Benchmarks.ipynb -> nvcomp_cascade_benchmarks.ipynb

aea11e9

madsbk approved these changes Mar 25, 2022

View reviewed changes

vyasr requested changes Mar 25, 2022

View reviewed changes

thomcom added 2 commits March 25, 2022 15:19

Take care of all of Vyas's questions except for aliases.

398f3b4

Remove that notebook. The other is the same, but better.

fe8ec24

Change pynvcomp cython naming scheme to reflext current kvikio standa…

f32b7c8

…rds. Change CascadedOptions to defauled named args.

vyasr approved these changes Mar 28, 2022

View reviewed changes

jakirkham requested a review from madsbk March 28, 2022 18:56

madsbk added 3 commits March 29, 2022 10:03

Merge branch 'branch-22.04' of https://github.com/rapidsai/kvikio int…

88e9f79

…o fea-add-nvcomp

typos

c5ac862

fixing format-all.py

4a1daf4

madsbk added improvement Improves an existing functionality non-breaking Introduces a non-breaking change python Affects the Python API of KvikIO labels Mar 29, 2022

madsbk approved these changes Mar 29, 2022

View reviewed changes

madsbk merged commit f0d2bed into rapidsai:branch-22.04 Mar 29, 2022

madsbk mentioned this pull request Mar 29, 2022

[Feature] Compression via Nvcomp #23

Closed

quasiben mentioned this pull request May 19, 2022

Python bindings implemented NVIDIA/nvcomp#33

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Add Nvcomp python bindings to kvikio. #24

[Feature] Add Nvcomp python bindings to kvikio. #24

thomcom commented Mar 9, 2022

quasiben commented Mar 9, 2022

thomcom commented Mar 10, 2022

thomcom commented Mar 14, 2022

jakirkham commented Mar 15, 2022

quasiben commented Mar 15, 2022

jakirkham commented Mar 15, 2022

thomcom commented Mar 15, 2022 •

edited

Loading

jakirkham commented Mar 15, 2022

thomcom commented Mar 15, 2022 •

edited

Loading

thomcom commented Mar 15, 2022

jakirkham commented Mar 15, 2022

thomcom commented Mar 15, 2022

jakirkham commented Mar 15, 2022

jakirkham commented Mar 15, 2022

thomcom commented Mar 15, 2022

jakirkham left a comment

madsbk left a comment

thomcom commented Mar 24, 2022

madsbk left a comment

vyasr left a comment

vyasr Mar 25, 2022

thomcom Mar 25, 2022

vyasr Mar 28, 2022

vyasr Mar 25, 2022

thomcom Mar 25, 2022

vyasr commented Mar 25, 2022

thomcom commented Mar 28, 2022

vyasr commented Mar 28, 2022

madsbk commented Mar 28, 2022

thomcom commented Mar 28, 2022

vyasr left a comment

madsbk commented Mar 29, 2022

[Feature] Add Nvcomp python bindings to kvikio. #24

[Feature] Add Nvcomp python bindings to kvikio. #24

Conversation

thomcom commented Mar 9, 2022

quasiben commented Mar 9, 2022

thomcom commented Mar 10, 2022

thomcom commented Mar 14, 2022

jakirkham commented Mar 15, 2022

quasiben commented Mar 15, 2022

jakirkham commented Mar 15, 2022

thomcom commented Mar 15, 2022 • edited Loading

jakirkham commented Mar 15, 2022

thomcom commented Mar 15, 2022 • edited Loading

thomcom commented Mar 15, 2022

jakirkham commented Mar 15, 2022

thomcom commented Mar 15, 2022

jakirkham commented Mar 15, 2022

jakirkham commented Mar 15, 2022

thomcom commented Mar 15, 2022

jakirkham left a comment

Choose a reason for hiding this comment

madsbk left a comment

Choose a reason for hiding this comment

thomcom commented Mar 24, 2022

madsbk left a comment

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

vyasr Mar 25, 2022

Choose a reason for hiding this comment

thomcom Mar 25, 2022

Choose a reason for hiding this comment

vyasr Mar 28, 2022

Choose a reason for hiding this comment

vyasr Mar 25, 2022

Choose a reason for hiding this comment

thomcom Mar 25, 2022

Choose a reason for hiding this comment

vyasr commented Mar 25, 2022

thomcom commented Mar 28, 2022

vyasr commented Mar 28, 2022

madsbk commented Mar 28, 2022

thomcom commented Mar 28, 2022

vyasr left a comment

Choose a reason for hiding this comment

madsbk commented Mar 29, 2022

thomcom commented Mar 15, 2022 •

edited

Loading

thomcom commented Mar 15, 2022 •

edited

Loading