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

Specify a version for rapids_logger dependency #17573

Merged
merged 8 commits into from
Dec 11, 2024

Conversation

jlowe
Copy link
Contributor

@jlowe jlowe commented Dec 10, 2024

Description

#17307 broke builds that use the rapids-cmake pinned dependencies feature since no version was specified for the rapids_logger dependency. This adds a version string equal to the git tag so the dependency has a stated version.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@jlowe jlowe added bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue non-breaking Non-breaking change labels Dec 10, 2024
@jlowe jlowe self-assigned this Dec 10, 2024
@jlowe jlowe requested a review from a team as a code owner December 10, 2024 22:47
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

@vyasr Can you take a quick look? I am approving but I think your input would be helpful.

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@ttnghia
Copy link
Contributor

ttnghia commented Dec 11, 2024

I've just updated the commit hash to reflect the last PR in rapids-logger.

@vyasr
Copy link
Contributor

vyasr commented Dec 11, 2024

Wow Nghia was quick I just came here to make the same change that he did 😄

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Thanks! This looks fine to me for now. It's not the cleanest but we will have things in a cleaner state before the 25.02 release so I'm fine with patching in the version this way until then to ease the transition.

@vyasr
Copy link
Contributor

vyasr commented Dec 11, 2024

Nghia found another minor issue with rapids-logger that I'm going to resolve now so that we can bump again in this PR.

@vyasr vyasr added the DO NOT MERGE Hold off on merging; see PR for details label Dec 11, 2024
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented Dec 11, 2024

@ttnghia I pushed a fix to rapidsai/rapids-logger#3. Could you rebase your local changes onto Jason's cudf branch's latest commit and make sure that things build as you expect (you can link to cudf::cudf_logger without linking to cudf::cudf)?

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented Dec 11, 2024

CI is passing here. Once @ttnghia can confirm that the latest version fixes his issue we can merge the rapids-logger PR, update the commit hash in this PR (and revert the repo changes) then rerun and merge.

@ttnghia
Copy link
Contributor

ttnghia commented Dec 11, 2024

I confirm that the patch fixes all the previous bugs. spark-rapids-jni (with my patch in NVIDIA/spark-rapids-jni#2679) can build without issue.

@bdice
Copy link
Contributor

bdice commented Dec 11, 2024

This fix appears to be necessary for devcontainers, too. I am getting

[0/8] Performing download step (git clone) for 'rapids_logger-populate'
Cloning into 'rapids_logger-src'...
fatal: unable to read tree (14bb233d2420f7187a690f0bb528ec0420c70d48)
CMake Error at rapids_logger-subbuild/rapids_logger-populate-prefix/tmp/rapids_logger-populate-gitclone.cmake:61 (message):
  Failed to checkout tag: '14bb233d2420f7187a690f0bb528ec0420c70d48'

but it builds successfully with this PR.

@davidwendt
Copy link
Contributor

What happens here next? Is this dependent on merging rapidsai/rapids-logger#3 ?

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@bdice
Copy link
Contributor

bdice commented Dec 11, 2024

What happens here next? Is this dependent on merging rapidsai/rapids-logger#3 ?

Yes, I merged that fix and updated the commit hash here. If we get through a few CI builds, I think this will be good enough to admin-merge so we are unblocked.

@bdice bdice removed the DO NOT MERGE Hold off on merging; see PR for details label Dec 11, 2024
@AyodeAwe AyodeAwe merged commit cd3a79b into rapidsai:branch-25.02 Dec 11, 2024
100 of 102 checks passed
@bdice bdice mentioned this pull request Dec 11, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants