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

Enable roundtrip for nvCOMP batch codecs. #253

Merged
merged 8 commits into from
Aug 1, 2023
Merged

Enable roundtrip for nvCOMP batch codecs. #253

merged 8 commits into from
Aug 1, 2023

Conversation

Alexey-Kamenev
Copy link
Contributor

This PR enables a roundtrip between codecs other than LZ4. For LZ4 codec, there is already such support.

The "roundtrip" here means ability to compress/store data using default numcodecs codec (e.g. zstd) and decompress using nvCOMP batch codecs. Reverse would also be true: ability to compress data using nvCOMP and decompress using CPU numcodecs codec.

The original implementation of the nvCOMP codec was based on the assumption, taken from numcodecs LZ4 codec, that in order for the codecs to be compatible, each codec must write a small header (4 bytes) in the compressed chunk, which contains the original, uncompressed size of the data. However, after subsequent analysis of other numcodecs codecs, it turned out LZ4 was actually an exception! No other codec writes a header or any additional data to the compressed chunk.

This PR:

  • moves the header creation down to a specific algorithm, LZ4
  • switches to nvCOMP nvcompBatched*GetDecompressSizeAsync family of functions to get the original size of data during decompression.

@Alexey-Kamenev Alexey-Kamenev requested a review from a team as a code owner July 19, 2023 18:39
@rapids-bot
Copy link

rapids-bot bot commented Jul 19, 2023

Pull requests from external contributors require approval from a rapidsai organization member with write permissions or greater before CI can begin.

@wence-
Copy link
Contributor

wence- commented Jul 25, 2023

/ok to test

wence-
wence- requested changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Thanks, overall I think this looks good. One thing I wonder is whether we should just do all of the pointer manipulation in host buffers and then do a single round of H2D memcopies before calling into nvcomp. Since I imagine the number of batches is not that large this is probably faster than lots of tiny (explicit and implicit) memcopies back and forth.

WDYT?

python/kvikio/_lib/libnvcomp_ll.pyx Outdated Show resolved Hide resolved
python/kvikio/nvcomp_codec.py Show resolved Hide resolved
python/kvikio/nvcomp_codec.py Outdated Show resolved Hide resolved
@wence- wence- added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jul 26, 2023
wence-
wence- reviewed Jul 27, 2023
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Thanks! Two minor typographic fixes, plus (apologies I didn't spot this first time round), I wonder if we want to use memcpy2DAsync to avoid the batch loop and kick everything off in one go. I don't think that's a necessary change though, so leave it up to you.

python/kvikio/_lib/libnvcomp_ll.pyx Outdated Show resolved Hide resolved
python/kvikio/_lib/libnvcomp_ll.pyx Outdated Show resolved Hide resolved
python/kvikio/_lib/libnvcomp_ll.pyx Show resolved Hide resolved
python/kvikio/_lib/libnvcomp_ll.pyx Show resolved Hide resolved
python/kvikio/_lib/libnvcomp_ll.pyx Outdated Show resolved Hide resolved
@wence-
Copy link
Contributor

wence- commented Jul 27, 2023

/ok to test

@wence-
Copy link
Contributor

wence- commented Jul 28, 2023

/ok to test

@wence-
Copy link
Contributor

wence- commented Jul 28, 2023

Thanks very much for this! We've just gone into code freeze so this will need to go to 23.10 rather than 23.08. Could you please retarget to branch-23.10

@Alexey-Kamenev Alexey-Kamenev changed the base branch from branch-23.08 to branch-23.10 July 28, 2023 16:53
@Alexey-Kamenev
Copy link
Contributor Author

Thank you for the review! I've updated the PR's target branch to branch-23.10.

@wence-
Copy link
Contributor

wence- commented Jul 28, 2023

/ok to test

madsbk
madsbk approved these changes Jul 31, 2023
View reviewed changes
Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Alexey-Kamenev

wence-
wence- approved these changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the updates, looks good to me too.

@wence-
Copy link
Contributor

wence- commented Jul 31, 2023

/ok to test

@Alexey-Kamenev
Copy link
Contributor Author

Thank you for the review, @wence- and @madsbk !
Let me know if there is anything else I need to do to get it merged.

@madsbk
Copy link
Member

madsbk commented Aug 1, 2023

/merge

@rapids-bot rapids-bot bot merged commit 98fca77 into rapidsai:branch-23.10 Aug 1, 2023
@madsbk
Copy link
Member

madsbk commented Aug 1, 2023

Thanks @Alexey-Kamenev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wence- wence- wence- approved these changes

@madsbk madsbk madsbk approved these changes

Assignees
No one assigned
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Alexey-Kamenev @wence- @madsbk