Upgrade to nvCOMP 5.0.0.6#19636
Conversation
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
To get the wheel builds passing you need to change all lines in |
We could use |
ci/build_wheel_libcudf.sh
Outdated
| # repair wheels and write to the location that artifact-uploading code expects to find them | ||
| python -m auditwheel repair \ | ||
| --exclude libnvcomp.so.4 \ | ||
| --exclude libnvcomp.so.* \ |
There was a problem hiding this comment.
@vuule Aha. This excludes nvcomp 5 from the cudf wheel. Previously we were safe excluding nvcomp 4 because the kvikio wheel was shipping the nvcomp shared library for us. We need to update kvikio to nvcomp 5 first, or just delete this line for testing to bundle nvcomp 5 into the cudf wheel.
There was a problem hiding this comment.
Now that I've removed the --exclude lines, the wheel build is back to failing.
Any help would be appreciated.
There was a problem hiding this comment.
Let's land rapidsai/kvikio#798 first and then come back to this.
vuule
added
feature request
|
/ok to test |
|
/ok to test 50a2820 |
vuule
added
DO NOT MERGE
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
Approving with one question about the FIXMEs.
| // FIXME how to use these statuses ? They are not used either in the corresponding | ||
| // decompressor. |
There was a problem hiding this comment.
How will we track this? Follow-up work?
There was a problem hiding this comment.
I merged this as-is from #19309
@firestarman?
There was a problem hiding this comment.
What does the new API write into this comp_statuses array? Compression status for each chunk?
There was a problem hiding this comment.
Just read the doc:
device_statuses – [out] Array with size num_chunks of statuses in device-accessible memory. This argument needs to be preallocated. For each chunk, if the compression is successful, the status will be set to
nvcompSuccess, and an error code otherwise.
So should we launch another kernel thrust::count_if(any error)?
There was a problem hiding this comment.
If we don't want to sync stream here, we probably need to return the entire status array for checking later on. Otherwise, we would not be able to check for compression status of each data chunk. The status returned from this async API is only checking for launching the kernel, not checking the compression results.
There was a problem hiding this comment.
yes, this is pretty much what we do in libcudf
since this is unrelated to nvcomp 5, I think this could be a follow-up. Also, not sure if you want to use the adapter here long-term.
| // FIXME how to use these statuses ? They are not used either in the corresponding | ||
| // decompressor. | ||
| auto comp_statuses = rmm::device_uvector<nvcompStatus_t>(batch_size, stream); |
There was a problem hiding this comment.
Similarly, we have the decompression status written into this array but here we don't check it to see if there is any failure.
ttnghia
left a comment
ttnghia
left a comment
There was a problem hiding this comment.
Java approval, pending the follow up work for checking batch comp/decomp status.
|
/ok to test |
@bdice, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test b45faae |
|
CI failed, probably because 9f0d3f0 was needed for a fix. I merged the upstream and will rerun CI. |
|
/ok to test 780610a |
|
/merge |
7 participants
Description
Issue #19686
Update the nvCOMP version and the JNI code that directly uses nvCOMP. The libcudf code is already compatible with this version.
Depends on rapidsai/rapids-cmake#896 and rapidsai/kvikio#800
Checklist