Replies: 8 comments 18 replies
-
Environment variables should still overwrite config files. For example, I prefer a separate job for x86_64 and i686 for imagecodecs package (because of the time of build) |
Beta Was this translation helpful? Give feedback.
-
So, yes this has been something I've been toying with in my head for a while. But the main reason I've not actually done it is because I haven't seen a situation where it obviously better then env vars! When I looks at cibuildwheel configs around GitHub (search GitHub for CIBW_SKIP or similar) I see a lot of people using it alongside other CI features, like matrixes, or a test step before. And I think that if you're playing with your CI config, you're better of configuring cibuildwheel there. Mixing the two is probably a recipe for confusion. I'm forgetting something... Ah, I remember! Local running. You keep all your config in pyproject.toml , and then do Hmm. Is that worth it? It does cost us in simplicity. Documenting options gets harder. Our options suddenly have two names. Our examples have two syntaxes. We can no longer say simple things like 'cibuildwheel is configured with environment variables'. Maybe there's another upside to this that I've forgotten? (Aside from neatness and less ALL_CAPS) |
Beta Was this translation helpful? Give feedback.
-
So let's run through the options and see what might be useful in a pyproject.toml:
That's not a "slam dunk" either way. |
Beta Was this translation helpful? Give feedback.
-
This is a thread looking at some examples. Starting with the largest users (by number of stars), scikit-learn: CIBW_ENVIRONMENT: OMP_NUM_THREADS=2
OPENBLAS_NUM_THREADS=2
SKLEARN_SKIP_NETWORK_TESTS=1
SKLEARN_BUILD_PARALLEL=3
MACOSX_DEPLOYMENT_TARGET=10.13
CIBW_BUILD: cp${{ matrix.python }}-${{ matrix.platform_id }}
CIBW_REPAIR_WHEEL_COMMAND_WINDOWS: bash build_tools/github/repair_windows_wheels.sh {wheel} {dest_dir} ${{ matrix.bitness }}
CIBW_BEFORE_TEST_WINDOWS: bash build_tools/github/build_minimal_windows_image.sh ${{ matrix.python }} ${{ matrix.bitness }}
CIBW_TEST_REQUIRES: pytest pandas threadpoolctl
CIBW_TEST_COMMAND: bash {project}/build_tools/github/test_wheels.sh
CIBW_TEST_COMMAND_WINDOWS: bash {project}/build_tools/github/test_windows_wheels.sh ${{ matrix.python }} ${{ matrix.bitness }} Hmm, didn't realize that was valid YAML with the multiline setting, nice. Okay, this is what you could pull out: [tool.cibuildwheel.global]
test-requires = "pytest pandas threadpoolctl"
test-command = "bash {project}/build_tools/github/test_wheels.sh"
environment = [
"OMP_NUM_THREADS=2",
"OPENBLAS_NUM_THREADS=2",
"SKLEARN_SKIP_NETWORK_TESTS=1",
"SKLEARN_BUILD_PARALLEL=3",
"MACOSX_DEPLOYMENT_TARGET=10.13",
] This would have been much better if they didn't have so many commands with Score from -5 to 5Helpfulness Score: 1 (or -1 if environment was not pulled out) |
Beta Was this translation helpful? Give feedback.
-
Suggestion: how about refactoring loading values into a "config" style? We don't need to actually depend on a library for this (like confuse, which is the same idea but for YAML), but the structure might gain us a little cleanliness instead of losing a little. The idea would be that there would be a # Usage
config_options = ConfigOptions(platform)
config_options.get_var("CIBW_TEST_COMMAND", platform_variants=True)
# Design
class ConfigOptions:
def __init__(self, platform: str) -> None:
self.platform = platform
# Open defaults.toml and load tool.cibuildwheel.global, then update with tool.cibuildwheel.<platform>
# Open pyproject.toml and update with tool.cibuildwheel.global, then update with tool.cibuildwheel.<platform>
# Open .cibuildwheel.toml and update with tool.cibuildwheel.global, then update with tool.cibuildwheel.<platform>
self.config_dict = ... # result of above
def get_var(self, name: str, platform_variants: bool = False) -> Optional[str]:
# Get and return envvar for name or f"{name}_{self.platform)" if platform_variants if a var exists
key = name.removesuffix("CIBW_").lower().replace("_", "-") # Fix for Python < 3.9
return self.config_dict[key] I haven't thought this through completely yet, like if a "None" value should be stored in the defaults. |
Beta Was this translation helpful? Give feedback.
-
Crossposting for visibility: #564 I had the same idea and am suggesting a higher level approach for configuration that would live in |
Beta Was this translation helpful? Give feedback.
-
So it's about time to revisit this. Is there interest in a higher level approach? I'm pretty sure pip and build wouldn't be interested in dealing with the non-python parts, so it likely would be just us; not sure how important it is to build a standard, but would be happy to work with one if that's best. One important consideration that is even more important now that manylinux_2_24 is out and musllinux is planned would be the handling of linux variants. Let's say Edit: I notice that #564 had the manylinux version included in the section, which would likely solve this issue. |
Beta Was this translation helpful? Give feedback.
-
🎉 This feature was merged and released (as an alpha). Check out v2.0.0a3. Docs here: https://cibuildwheel.readthedocs.io/en/latest/options/#setting-options |
Beta Was this translation helpful? Give feedback.
-
I noticed while reading #179 (comment) that there was a thought at some point to drive (most) of cibuildwheel from a common config file. I'd like to play with an outline of what that might look like, and get some feedback and design discussion going on it (not for immediate implementation, but maybe a 2.0 idea?)
First, the name.
pyproject.toml
would be the natural name, since it's supposed to be the one place to control Python package configuration. Since we already have environment variable overrides, I don't think we need a.cibuildwheel.toml
or similar "alternate" config file for users who just don't want to usepyproject.toml
.Second, the sections. We are suppose to put things in
tool.cibuildwheel.*
, so I'd propose four sections:tool.cibuildwheel.global
,tool.cibuildwheel.windows
,tool.cibuildwheel.linux
, andtool.cibuildwheel.macos
, with the same structure as the currentCIBW_*
andCIBW_<PLATFORM>_*
options.Third, the names. I would power everything though the current interface, so each variable
CIBW_<SOME_NAME>
would have a toml setting with<some-name>
, lower case and with dashes.Fourth, precedence. I would always let environment variables override toml variables, since you might have to override an existing setting, or you might need a per-job setting for something like
archs
.Fifth, scope. We can easily add opt in/out, so should we evenly add everything that is a current environment variable, or should some settings be opted out?
Sixth, special case for CIBW_BUILD? Unlike all the other variables, BUILD, and to a lesser extent, SKIP, and possibly ARCHS are for driving the current job, not only describing "what can be done". If you want to split the built to avoid the short total job time (on things like Travis) or utilize the 10+ parallel builds on Azure/GHA, you have to use these variables in a matrix to do it. If you want to run two jobs with the same OS, you have to use one of these to not get exactly the same result (probably becoming more important with multiple arches and macOS ARM). So should we do something special here? Maybe selection by variables could be applied on top of configuration versions (but then there would be no way to add a skipped job without changing the pyproject.toml). Or maybe we could keep these out of the configuration file entirely; that's expecially natural for BUILD/SKIP, which are not even valid in the per-OS part anyway. But for simple packages that do fit within a single job's time, they might want to only use the file?
Benefits:
Most of the configuration could be completely independent of the CI, so you wouldn't have to depend on the method a CI provides for global environment variables, and could likely switch CI's more easily. If you support 2+ CIs, you no longer have to specify things multiple times.
Cons:
Splits the config into a separate file, forcing users to look in two places to find config. But this is a common problem for lots of packages (pytest, in fact, pretty much everything except black).
Beta Was this translation helpful? Give feedback.
All reactions