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

Prefix and filter command line options with rapidsai. #37

Merged
merged 5 commits into from
May 23, 2024

Conversation

KyleFromNVIDIA
Copy link
Contributor

Some build backends, like scikit-build-core, get confused if you pass them options that they don't recognize. Filter out RAPIDS' config options from getting to the build backend, and prefix them with rapidsai. to make the filtering easier.

Some build backends, like scikit-build-core, get confused if you
pass them options that they don't recognize. Filter out RAPIDS'
config options from getting to the build backend, and prefix them
with `rapidsai.` to make the filtering easier.
@KyleFromNVIDIA KyleFromNVIDIA requested a review from a team as a code owner May 22, 2024 21:57
@KyleFromNVIDIA
Copy link
Contributor Author

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.

Thanks for the context, I support this general idea.

I've noticed that scikit-build-core does not do this... at https://scikit-build-core.readthedocs.io/en/latest/configuration.html I see --config-settings=--config-settings=install.strip=false.

But it makes sense to me that projects like rapids-build-backend that sit in front of another build backend should namespace all their configuration this way... I like it!

Would you consider rapids.* instead of rapidsai.*? I think that's enough to pretty significantly reduce the risk of clashes with other build backends, and I don't think the "AI" is adding any information there. I hold that opinion very lightly, so if you don't find that convincing then I'm fine with leaving it as-is.

rapids_build_backend/impls.py Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented May 23, 2024

Happy to let James take point on reviewing, just dropping in to say that I definitely support this.

But it makes sense to me that projects like rapids-build-backend that sit in front of another build backend should namespace all their configuration this way... I like it!

There isn't really any precedent for this kind of backend adapter yet. I've seen it discussed once or twice by implementers of other backends, but not really in any concrete way. So we should feel comfortable defining best practices and conventions in this space as needed. We won't get much from looking for parallels in well-known preexisting build backends when it comes to this kind of issue that's specific to wrapping.

@KyleFromNVIDIA
Copy link
Contributor Author

I've noticed that scikit-build-core does not do this... at https://scikit-build-core.readthedocs.io/en/latest/configuration.html I see --config-settings=--config-settings=install.strip=false.

Interesting... at https://pypi.org/project/scikit-build-core/ I see:

All configuration options can be placed in pyproject.toml, passed via -C/--config-setting in build or -C/--config-settings in pip , or set as environment variables. tool.scikit-build is used in toml, skbuild. for -C options, or SKBUILD_* for environment variables.

And the scikit-build-core source confirms this:

https://github.com/scikit-build/scikit-build-core/blob/216aa14ea60ee90ba4cc9f8260cbd3ab1b1e782f/src/scikit_build_core/settings/skbuild_read_settings.py#L299

@KyleFromNVIDIA
Copy link
Contributor Author

Would you consider rapids.* instead of rapidsai.*?

I used rapidsai.* at https://github.com/rapidsai/pre-commit-hooks/blob/f9c4f63dda83c95333729f7961beb519469085a4/src/rapids_pre_commit_hooks/copyright.py#L141 for a Git config option, and I'd like to keep it consistent.

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.

Ok thanks for considering the suggestions, I have no other comments.

And thanks for the links about scikit-build-core, maybe the skbuild.* prefix didn't make it all the way through their docs whenever it was added. They might like a PR unifying those.

@vyasr
Copy link
Contributor

vyasr commented May 23, 2024

Ah right, I forgot they added namespacing support. It's in scikit-build/scikit-build-core#556. Yeah, I'm sure Henry would be happy with a PR updating the docs if you'd like to make one. The namespace is optional, so the docs would need to reflect that.

@KyleFromNVIDIA
Copy link
Contributor Author

I've opened scikit-build/scikit-build-core#754.

@henryiii
Copy link

By the way, you can turn off checking for unused options with strict-config. So -Cskbuild.strict-config=false (or identically -Cstrict-config=false) would ignore unrecognized options. But I think it's working as intended, anything that passes through yours and makes it to skbuild won't be handled and should be an error.

@KyleFromNVIDIA
Copy link
Contributor Author

@henryiii Thanks for the advice. I think we'll stick with prefixing and filtering our options with rapidsai. to avoid any confusion, warnings, etc.

rapids_build_backend/config.py Outdated Show resolved Hide resolved
rapids_build_backend/config.py Outdated Show resolved Hide resolved
@KyleFromNVIDIA KyleFromNVIDIA merged commit 0333e5f into rapidsai:main May 23, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants