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.

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

Upgrade nvcomp to 4.1.0.6 #525

Merged
merged 4 commits into from
Oct 30, 2024
Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Oct 29, 2024

This updates cudf to use nvcomp 4.1.0.6.

The version is updated in rapids-cmake in rapidsai/rapids-cmake#709.

@bdice bdice marked this pull request as ready for review October 30, 2024 13:37
@bdice bdice requested review from a team as code owners October 30, 2024 13:37
@bdice bdice requested a review from raydouglass October 30, 2024 13:37
cpp/cmake/rapids_config.cmake Outdated Show resolved Hide resolved
@bdice bdice changed the title Test nvcomp 4.1.0.6 Upgrade nvcomp to 4.1.0.6 Oct 30, 2024
@bdice bdice added feature request New feature or request non-breaking Introduces a non-breaking change labels Oct 30, 2024
@bdice bdice self-assigned this Oct 30, 2024
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Looks to me like this is not ready yet. From wheel tests:

Module 'kvikio.zarr' was found, but when imported by pytest it raised:
ImportError('/pyenv/versions/3.10.15/lib/python3.10/site-packages/kvikio/_lib/libnvcomp.cpython-310-x86_64-linux-gnu.so: undefined symbol: _ZN6nvcomp14BitcompManagerC1EmRK30nvcompBatchedBitcompFormatOptsP11CUstream_stiNS_14ChecksumPolicyENS_13BitstreamKindE')

(build link)

@jameslamb
Copy link
Member

Maybe libkvikio also needs a runtime dependency on the nvidia-nvcomp wheels, similar to what was done with libcudf` in rapidsai/cudf#16946?

https://github.com/rapidsai/cudf/blob/5ee7d7caaf459e7d30597e6ea2dd1904c50d12fc/python/libcudf/pyproject.toml#L40-L42

@bdice
Copy link
Contributor Author

bdice commented Oct 30, 2024

@jameslamb Sorry for my lack of communication here. This is only failing now because I reverted rapids-cmake testing changes in 62265bb. The tests passed in 45e7389. I am going to rerun CI after merging the rapids-cmake PR, but you can review the CI run from the previous commit.

@bdice
Copy link
Contributor Author

bdice commented Oct 30, 2024

Also: We have the nvcomp wheel dependency we need here:

"nvidia-nvcomp==4.1.0.6",

libkvikio is header-only, and the nvcomp dependency is really in kvikio (Cython), not libkvikio C++, so we don't need this nvcomp wheel dependency in the libkvikio wheel.

@jameslamb jameslamb self-requested a review October 30, 2024 14:33
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Ah ok got it, I misunderstood what was happening there. Thanks for the explanation.

@bdice
Copy link
Contributor Author

bdice commented Oct 30, 2024

/merge

@rapids-bot rapids-bot bot merged commit 607c6a5 into rapidsai:branch-24.12 Oct 30, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants