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 #1808

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

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Feb 1, 2025

Description

This change helps completely insulate rmm (and transitively) the rest of RAPIDS from fmt and spdlog as dependencies, thereby solving a large number of issues around ABI stability, symbol visibility, package clobbering, and more. See rapidsai/build-planning#104 for more information.

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 breaking Breaking change improvement Improvement / enhancement to an existing function labels Feb 1, 2025
@vyasr vyasr self-assigned this Feb 1, 2025
Copy link

copy-pr-bot bot commented Feb 1, 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 CMake Python Related to RMM Python API conda cpp Pertains to C++ code ci labels Feb 1, 2025
@vyasr
Copy link
Contributor Author

vyasr commented Feb 1, 2025

/ok to test

8 similar comments
@vyasr
Copy link
Contributor Author

vyasr commented Feb 1, 2025

/ok to test

@vyasr
Copy link
Contributor Author

vyasr commented Feb 1, 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 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 vyasr force-pushed the feat/rapids_logger_library branch from a7b1c85 to 255d96a Compare February 2, 2025 07:12
@vyasr
Copy link
Contributor Author

vyasr commented Feb 2, 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
Comment on lines +15 to +18

set(rapids-cmake-repo "vyasr/rapids-cmake")
set(rapids-cmake-branch "feat/rapids_logger_library")

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 suggestion should be applied as a final change on this PR before merging.

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

python/rmm/rmm/__init__.py Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
benchmarks/replay/replay.cpp Show resolved Hide resolved
ci/build_wheel_python.sh Outdated Show resolved Hide resolved
ci/check_symbols.sh Show resolved Hide resolved
include/rmm/logger.hpp Outdated Show resolved Hide resolved
python/rmm/rmm/__init__.py Outdated Show resolved Hide resolved
python/rmm/rmm/librmm/_logger.pxd Show resolved Hide resolved
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.

I left a few comments I'd like answers on before approving. I especially want to be sure I understand why rmm is picking up a runtime dependency on librmm as part of this (I think I do, but left some questions about it).

ci/check_symbols.sh Show resolved Hide resolved
conda/recipes/librmm/meta.yaml Outdated Show resolved Hide resolved
python/librmm/librmm/__init__.py Outdated Show resolved Hide resolved
@vyasr vyasr requested review from jameslamb, Matt711 and bdice February 6, 2025 00:32
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.

Thanks, the python changes look good

python/rmm/rmm/__init__.py Outdated Show resolved Hide resolved
python/rmm/rmm/librmm/_logger.pxd 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.

I have a few small comments, mostly replies to existing threads. I have nothing else significant to add here. @vyasr Please take a pass or two through the open threads and get an approval from @jameslamb before merging.

@bdice bdice mentioned this pull request Feb 7, 2025
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.

I don't have any additional comments, changes look good to me! I'll help review the downstream PRs next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change ci CMake conda cpp Pertains to C++ code improvement Improvement / enhancement to an existing function Python Related to RMM Python API
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants