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

[fea] Expose the arena mr to the Python interface. #1711

Open
wants to merge 18 commits into
base: branch-24.12
Choose a base branch
from

Conversation

trivialfis
Copy link
Member

@trivialfis trivialfis commented Oct 26, 2024

Description

Close #830 .

  • Add the arena allocator to the public Python interface.
  • Small changes to the logger initialization to avoid exposing spdlog in the shared objects.

Checklist

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

@trivialfis trivialfis requested a review from a team as a code owner October 26, 2024 11:20
@github-actions github-actions bot added the Python Related to RMM Python API label Oct 26, 2024
@trivialfis trivialfis requested a review from a team as a code owner October 26, 2024 12:54
@github-actions github-actions bot added the cpp Pertains to C++ code label Oct 26, 2024
@trivialfis trivialfis marked this pull request as draft October 26, 2024 16:11
@trivialfis trivialfis marked this pull request as ready for review October 26, 2024 17:19
namespace RMM_NAMESPACE {
namespace mr::detail::arena {

namespace rmm::mr::detail::arena {
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the __attribute__((visibility)) of this namespace from "default" to "hidden". Is this deliberate, or an inadvertent editor-induced change?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's deliberate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, OK, can we just hide that one symbol? Add RMM_HIDDEN for that symbol.

See discussion in #1652, particularly, we need to be careful about visibility of objects that might make it in to the public namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for sharing. I will check whether the exception can be thrown from there. Considering that the namespace is detail and is only used by the MR, which is exposed in the public, it should be fine. That said, I will double check.
Who doesn't love c++ linking, so much fun.

Copy link
Member Author

Choose a reason for hiding this comment

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

The visibility is controlled by namespace instead of translation unit.

Copy link
Member

Choose a reason for hiding this comment

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

oh I had it backwards. I thought this change was exposing more, but it's actually hiding more.

Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest that we make the hiding explicit?

namespace RMM_HIDDEN rmm::mr::detail::arena {

?

Copy link
Member Author

@trivialfis trivialfis Oct 31, 2024

Choose a reason for hiding this comment

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

I'm more than happy to make the change. But do you want to do it for the entire repository or do you want me to make the change to this specific file? Or maybe the CMake hidden visibility option is sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

Switched to hiding only the spdlog function as suggested offline.

@wence- wence- added the non-breaking Non-breaking change label Oct 31, 2024
@wence- wence- added the improvement Improvement / enhancement to an existing function label Oct 31, 2024
@wence-
Copy link
Contributor

wence- commented Oct 31, 2024

Thanks!

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

Successfully merging this pull request may close these issues.

[FEA] Python interface for arena_memory_resource
3 participants