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

Switch to using rapids-logger as a library #765

Merged
merged 13 commits into from
Feb 10, 2025

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Jan 31, 2025

Description

This PR changes rapids_cpm_rapids_logger to use the new shared library version of rapids_logger instead of the old C++ file generation approach. In addition to changing the versions.json file, there are a couple of associated changes to the function and also some small cleanup.

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.
  • The cmake-format.json is up to date with these changes.
  • I have added new files under rapids-cmake/
    • I have added include guards (include_guard(GLOBAL))
    • I have added the associated docs/ rst file and update the api.rst

@vyasr vyasr added breaking Introduces a breaking change improvement Improves an existing functionality labels Jan 31, 2025
@vyasr vyasr self-assigned this Jan 31, 2025
@vyasr vyasr force-pushed the feat/rapids_logger_library branch from 3b2c8a3 to 076af41 Compare January 31, 2025 23:57
@vyasr
Copy link
Contributor Author

vyasr commented Jan 31, 2025

I'm expecting tests to fail for the moment. I'll update those once I have RAPIDS builds working with this change.

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

vyasr commented Feb 10, 2025

The remaining failures here are because there are tests that pull rmm and there is a catch-22 with this PR and rapidsai/rmm#1808. I'll request an admin merge here so that I can rerun and get CI passing on the rmm PR to merge that one.

@raydouglass raydouglass merged commit 4082b35 into rapidsai:branch-25.04 Feb 10, 2025
12 of 15 checks passed
@vyasr vyasr deleted the feat/rapids_logger_library branch February 10, 2025 21:57
rapids-bot bot pushed a commit that referenced this pull request Feb 11, 2025
#765 required an admin merge on this repository to break a circular dependency between this repository and rmm because rapids-cmake provides a centralized `rapids_cpm_rmm` function. Unfortunately, because of how certain tests in this repo were set up, they could not be tested against a fork (like #765 did) without manually copying over branches to the fork, so that was not done. Now that the rmm PR is merged, we can see that there are a few issues to be ironed out with the tests here. In particular, since rmm no longer clones fmt, the pin testing needs to be updated to not look for it.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #775
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces a breaking change improvement Improves an existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants