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

Use new rapids-logger library #17899

Open
wants to merge 22 commits into
base: branch-25.04
Choose a base branch
from

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Feb 2, 2025

Description

Contributes to rapidsai/build-planning#104.

Checklist

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

@vyasr vyasr added improvement Improvement / enhancement to an existing function breaking Breaking change labels Feb 2, 2025
@vyasr vyasr self-assigned this Feb 2, 2025
Copy link

copy-pr-bot bot commented Feb 2, 2025

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.

@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. CMake CMake build issue labels Feb 2, 2025
@vyasr
Copy link
Contributor Author

vyasr commented Feb 2, 2025

/ok to test

@vyasr vyasr force-pushed the feat/rapids_logger_library branch from 8095a12 to d359c37 Compare February 2, 2025 07:22
@vyasr
Copy link
Contributor Author

vyasr commented Feb 2, 2025

/ok to test

@vyasr vyasr force-pushed the feat/rapids_logger_library branch from d359c37 to c6cd7a2 Compare February 2, 2025 07:40
@vyasr
Copy link
Contributor Author

vyasr commented Feb 2, 2025

/ok to test

@vyasr
Copy link
Contributor Author

vyasr commented Feb 2, 2025

/ok to test

@vyasr
Copy link
Contributor Author

vyasr commented Feb 2, 2025

/ok to test

@vyasr
Copy link
Contributor Author

vyasr commented Feb 2, 2025

/ok to test

@vyasr
Copy link
Contributor Author

vyasr commented Feb 3, 2025

/ok to test

@vyasr
Copy link
Contributor Author

vyasr commented Feb 4, 2025

/ok to test

@vyasr vyasr marked this pull request as ready for review February 4, 2025 20:12
@vyasr vyasr requested review from a team as code owners February 4, 2025 20:12
@vyasr
Copy link
Contributor Author

vyasr commented Feb 4, 2025

I'm working on a local repro of the spark-rapids-jni build. I won't merge this before I can figure out if there's a real problem there. The code in this PR itself should be good to go, though.

Copy link
Contributor

@Matt711 Matt711 left a comment

Choose a reason for hiding this comment

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

Approving the python changes. I had one small cmake suggestion.

@@ -12,6 +12,8 @@
# the License.
# =============================================================================
file(READ "${CMAKE_CURRENT_LIST_DIR}/VERSION" _rapids_version)
set(rapids-cmake-repo "vyasr/rapids-cmake")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that right?

Suggested change
set(rapids-cmake-repo "vyasr/rapids-cmake")
set(rapids-cmake-repo "feat/rapids-cmake")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is correct. It's pointing at my fork of rapids-cmake. The line below is the branch. However, this will get reverted before we merge, it's only there right now for testing purposes.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 5, 2025

Something is off for the installation of the static library build of rapids-logger. I'll look into that tomorrow.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 5, 2025

I resolved the spark-rapids issue in rapidsai/rapids-cmake@ae4cb7f. I just kicked off an updated build of rmm based on that commit, then I'll rerun CI here for a final test and then we should be good to start merging up the chain.

cpp/include/cudf/logger.hpp Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from davidwendt February 5, 2025 21:41
@vyasr vyasr requested a review from davidwendt February 5, 2025 22:47
@vyasr
Copy link
Contributor Author

vyasr commented Feb 6, 2025

I resolved the spark-rapids issue in rapidsai/rapids-cmake@ae4cb7f. I just kicked off an updated build of rmm based on that commit, then I'll rerun CI here for a final test and then we should be good to start merging up the chain.

The remaining spark-rapids-jni issue won't be fixed until the upstream PRs in rapids-cmake and rmm get merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants