Skip to content

Python bindings for Options.get()#303

Merged
rapids-bot[bot] merged 11 commits intorapidsai:branch-25.06from
madsbk:config_python_get
May 28, 2025
Merged

Python bindings for Options.get()#303
rapids-bot[bot] merged 11 commits intorapidsai:branch-25.06from
madsbk:config_python_get

Conversation

@madsbk
Copy link
Copy Markdown
Member

@madsbk madsbk commented May 23, 2025

Removing Options.get_or_assign(), which is now replaced by the more generic Options.get().

@madsbk madsbk self-assigned this May 23, 2025
@madsbk madsbk added breaking Introduces a breaking change improvement Improves an existing functionality labels May 23, 2025
@madsbk madsbk marked this pull request as ready for review May 23, 2025 14:58
@madsbk madsbk requested review from a team as code owners May 23, 2025 14:58
Copy link
Copy Markdown
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Looks reasonable. I didn't follow the cython / template stuff completely, but that seems to have been at least partly moved from config.pyx so presumably it's OK :)

Comment thread python/rapidsmpf/rapidsmpf/config.pyx Outdated
which must be one of the supported Python types: bool, int, float, or str.
The `default_value` is cast to the corresponding C++ type before being passed
to the underlying options system.
The option is cast to the specified `return_type`, which must be one
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should probably be

Suggested change
The option is cast to the specified `return_type`, which must be one
The option is cast to the specified ``return_type``, which must be one

I'm guessing this isn't included in our docs yet. Probably just need to add it to the index at https://github.com/rapidsai/rapidsmpf/blob/branch-25.06/docs/source/index.md?plain=1. LMK if you want to address that here & need help with it.

Likewise on L74 and L126 below.

Copy link
Copy Markdown
Member Author

@madsbk madsbk May 27, 2025

Choose a reason for hiding this comment

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

Let's do it in my next config PR, which will include a doc update

@madsbk
Copy link
Copy Markdown
Member Author

madsbk commented May 28, 2025

@rapidsai/rapidsmpf-cmake-codeowners, can I get one of you to review. The cmake changes a minor -- just adding new files to be compiled.

Copy link
Copy Markdown
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.

CMake approval.

@madsbk
Copy link
Copy Markdown
Member Author

madsbk commented May 28, 2025

/merge

@rapids-bot rapids-bot bot merged commit d384363 into rapidsai:branch-25.06 May 28, 2025
23 checks passed
@madsbk madsbk deleted the config_python_get branch May 28, 2025 15:18
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