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

Less verbose handling of config-settings #243

Closed
ejjordan opened this issue Mar 21, 2023 · 14 comments
Closed

Less verbose handling of config-settings #243

ejjordan opened this issue Mar 21, 2023 · 14 comments

Comments

@ejjordan
Copy link

A common use case for building software on HPC resources is that many CMake options need to be set, such as the following.

  • compilers
  • GPU backend
  • libraries to use or avoid
  • whether to use MPI, OpenMP, SIMD, etc

It also does not seem like there is or will be any consensus on config-settings from an implementing PEP 517 perspective given, for instance, the following discussions.

As such, since one of the stated goals of this project is to integrate CMake with python packaging via pip, it would be good to have a compact way to specify CMake options for running pip install ..

Currently, unless I am just thick, it seems that the only way to specify multiple CMake arguments is to write --config-settings=cmake.define.MPI=ON --config-settings=cmake.define.GPU=ON. Since for many of the types of options that need to be set there is not really a sensible default, these options generally need to be passed every time someone wants to build the package.

It would be good to have something like --config-settings=cmake.define.MPI=ON;GPU=ON or --config-settings=cmake.define{MPI:ON,GPU:ON} so that many options can be passed easily.

@henryiii
Copy link
Collaborator

The first step was to add -C to pip, and that has been done - the next version of pip will allow the much shorter/more readable -Ccmake.define.MPI=ON -Ccmake.define.GPU=ON. I've considered allowing a D shortcut, which would further reduce this to -CD.MPI=ON -CD.GPU=ON, but at the expense of readability.

FWIW, you can do this with args, --config-settings=cmake.args='-DMPI=ON;-DGPU=ON' does work. This was mostly implemented due to Pip's current (also fixed in the next version) ability to support multiple matching arguments as arrays.

Maybe something similar could/should be done for dicts.

@henryiii
Copy link
Collaborator

If you are setting a lot of arguments, maybe this is a good use for a preset or toolchain file? (not that we have any shortcuts for that, at least yet, #204)

@ejjordan
Copy link
Author

Wow thanks for the really quick feedback. I thought I had tried every combination of cmake.[define,args] with quotes and special characters but I guess I missed at least one, since --config-settings=cmake.args='-DMPI=ON;-DGPU=ON' worked.

I think a toolchain file is a good idea but I seem to recall that it was hard somehow to make sure that the toolchain was actually the same one used to build both the underlying project (the C++ code) and the python interface (which also has some C++ since the API in the underlying code is not very stable). It was quite some time ago that I played with that so I would need to revisit it before I could say more.

As far as presets, it would increase the maintenance burden I think, because then you would need to mirror all the custom CMake options that the project has, as well as the generic CMake options like compiler and prefix paths. I think it would be nicer to have a shorthand, but probably the shorthand you pointed out is sufficient.

@eirrgang
Copy link

One issue with a toolchain file is that it specifically implies "cross-compiling" with special meaning to CMake, which may not be what is intended. A distinct alternative is to provide a file to pre-initialize the CMake cache.

LAMMPS, for instance, ships with multiple presets files to deal with common variations.

Unfortunately, a user would need to be building from the source repository to make use of these with, e.g.

pip install . --config-settings=cmake.args=-Cpresets.cmake

@henryiii
Copy link
Collaborator

The one thing you can't do is use -C, as that's how scikit-build-core communicates to CMake, setting things like the Python location. We could add an option that allows a user to add a preset file to that generated file, though.

@eirrgang
Copy link

The one thing you can't do is use -C, as that's how scikit-build-core communicates to CMake, setting things like the Python location. We could add an option that allows a user to add a preset file to that generated file, though.

Ooh. That could be important. For example, clients of a gromacs installation have a notoriously hard time finding a compatible toolchain, MPI implementation, etc. and the gmxapi python package has recently migrated away from a CMake toolchains file in favor of -C, with a file generated by and installed with gromacs.

@eirrgang
Copy link

The one thing you can't do is use -C

I must have misunderstood you. When you said "maybe this is a good use for a preset or toolchain file", I assumed you were referencing the two cases of -Csome/cache/presets.cmake and --toolchain toolchain.cmake.

Were you talking about https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html ? I hadn't seen that before.

@henryiii
Copy link
Collaborator

Yes, I was. I haven't used it yet, but it's come up a few times.

We can implement custom handling for -C where we append or prepend our file with the given one. We also could possible look into writing out a cache file directly (but that requires we also be able to process a cache file if doing a rebuild).

@eirrgang
Copy link

eirrgang commented Mar 22, 2023

Whichever way it is approached, I guess there needs to be well-defined precedence for specifying CMAKE_C_COMPILER CMAKE_CXX_COMPILER, CMAKE_LINKER, and any other toolchain details that scikit-build-core might manipulate, with consideration for the many ways people are accustomed to providing them.

To the point of this issue, it would be very nice if they could be provided in one or more of the standard ways (toolchain file, cache initialization file, new style presets file) without breaking scikit-build-core.

@ejjordan
Copy link
Author

(but that requires we also be able to process a cache file if doing a rebuild)

Maybe I am misunderstanding the context here, but this seems to just work as long as a build-dir is specified.

@henryiii
Copy link
Collaborator

If anything changes (settings, temporary directories like site-packages in an isolated build), then we need to "update" the cache file. That's why we have to put "FORCE" on some of the entries, even though you aren't really supposed to.

@eirrgang
Copy link

The one thing you can't do is use -C, as that's how scikit-build-core communicates to CMake, setting things like the Python location. We could add an option that allows a user to add a preset file to that generated file, though.

There does not seem to be a conflict with providing multiple -C arguments to CMake, and scikit-build-core only uses it to set SKBUILD-specific things.

There is some potential for ambiguity if scikit-build-core does not parse and explicitly merge a user-provided file, but it is fairly minimal.

Also, I don't see any place where scikit-build-core specifies any tool chain details, like CMAKE_CXX_COMPILER.

In other words, -C seems to work as desired at the moment. Do you foresee future problems?

If you think it would be a bad idea to suggest that users reference a cache script, we need to hold off on migrating to scikit-build-core until we have a supportable way to convey tool chain details.

@henryiii
Copy link
Collaborator

Ah, I've never tried passing multiple -C arguments, that's great. No, I don't see a problem, and I'd assume CMake merges left to right like usual, so user provided scripts should override scikit-build-core if required.

@henryiii
Copy link
Collaborator

Pip 23.1 is out, and this is now simpler and identical between pip & build. Pip also now supports arrays by passing the same option multiple times, just like build always has.

I also noticed a mention of FORCE in the CMake docs released to -C files, so I think we are safe there as well. :)

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

No branches or pull requests

3 participants