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.

Sign up for GitHub

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

Jump to bottom

Use new rapids-logger library #1808

Merged
merged 23 commits into from
Feb 10, 2025
Merged

Use new rapids-logger library #1808

merged 23 commits into from
Feb 10, 2025

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-pr-bot copy-pr-bot
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 requested a review from a team as a code owner February 4, 2025 20:12
vyasr
vyasr commented Feb 4, 2025
View reviewed changes
rapids_config.cmake Outdated Show resolved Hide resolved
Matt711
Matt711 reviewed Feb 5, 2025
View reviewed changes
python/rmm/rmm/__init__.py Outdated Show resolved Hide resolved
bdice
bdice reviewed Feb 5, 2025
View reviewed changes
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
jameslamb
jameslamb reviewed Feb 5, 2025
View reviewed changes
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
Matt711
Matt711 approved these changes Feb 6, 2025
View reviewed changes
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
bdice
bdice approved these changes Feb 7, 2025
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment •
edited
Loading

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
Open
jameslamb
jameslamb approved these changes Feb 7, 2025
View reviewed changes
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.

@vyasr vyasr mentioned this pull request Feb 10, 2025
Merged
7 tasks
@vyasr
Copy link
Contributor Author

vyasr commented Feb 10, 2025

/merge

@rapids-bot rapids-bot bot merged commit f0a784a into rapidsai:branch-25.04 Feb 10, 2025
62 checks passed
jameslamb
jameslamb reviewed Feb 10, 2025
View reviewed changes
dependencies.yaml
Comment on lines +370 to +371
# This index is needed for rapids_logger
- --extra-index-url=https://pypi.nvidia.com
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# This index is needed for rapids_logger
- --extra-index-url=https://pypi.nvidia.com

Sorry for not noticing this earlier, just caught it in cudf: rapidsai/cudf#17899 (comment)

Do we really need pypi.nvidia.com? I hope we could go straight to publishing releases of rapids-logger on pypi.org, like we do for libucx-cu{11,12}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Responded on there. Thanks, I'll file a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1820

@vyasr vyasr mentioned this pull request Feb 10, 2025
Merged
3 tasks
@vyasr vyasr deleted the feat/rapids_logger_library branch February 10, 2025 23:00
rapids-bot bot pushed a commit that referenced this pull request Feb 10, 2025
@vyasr
Remove unnecessary index (#1820)
ac8a99b 
Addresses #1808 (comment)

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

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

URL: #1820
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameslamb jameslamb jameslamb approved these changes

@bdice bdice bdice approved these changes

@Matt711 Matt711 Matt711 approved these changes

@KyleFromNVIDIA KyleFromNVIDIA Awaiting requested review from KyleFromNVIDIA KyleFromNVIDIA is a code owner automatically assigned from rapidsai/packaging-codeowners

@wence- wence- Awaiting requested review from wence- wence- is a code owner automatically assigned from rapidsai/rmm-cpp-codeowners

Assignees

@vyasr vyasr

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
RMM Project Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vyasr @bdice @jameslamb @Matt711