-
Notifications
You must be signed in to change notification settings - Fork 264
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
add cibuildwheel GHA #1283
add cibuildwheel GHA #1283
Conversation
b228041
to
49a7d4e
Compare
Ubuntu wheels are using
macOS is passing with the libs |
What about setting the cibuildwheel image to use But, in the existing wheel generation process (https://github.com/MacPython/netcdf4-python-wheels), netCDF and its dependencies are compiled from source, so currently netCDF 4.9.2 is being bundled for the wheels on PyPI.
EDIT: I didn't read in the original description that you already suggested building the dependencies from source |
I've done something similar for my nc-complex package: CI workflow, cibuildwheel config in the I used Note that I use my fork of netcdf-C because I'm waiting for PR #2762 to fix finding HDF5 correctly from CMake. Once that's in, using the lastest |
be9c828
to
214cbb6
Compare
I'll try to focus on building hdf5 and netcdf-c over the next week There are a few options I want to try:
As a conda-forge founder, core member, and maintainer of many geo/met/ocean packages, I'm heavily biased towards 2. What do you think @ZedThree, @jswhit, and @dopplershift? * we already do something similar for the Windows packages and I want to move the current Windows builds to this, regardless of the Linux and macOS choice, to keep things in a single repository. |
I suggest using I would definitely try avoiding building HDF5 from scratch if possible, as you might find you end up building a lot more of the stack! |
Looks like their hdf5 has |
What about the approach of using conda built libs? (ocefpaf/netcdf4-win-wheels#6) If you want to enable all the features of hdf5/netcdf-c it requires a lot of dependencies to be built - don't think we want to do all that here. |
Option 2 is a "modern" version of that where the main advantage is that it uses cibuildwheel+conda-forge clibs, and keep things in this repo, no need for third party ones. I'll try that and see if it works. |
ac1a5ab
to
7579696
Compare
.github/workflows/cibuildwheel.yml
Outdated
CIBW_REPAIR_WHEEL_COMMAND_WINDOWS: "delvewheel repair -w {dest_dir} {wheel}" | ||
CIBW_TEST_COMMAND: > | ||
python -c "import netCDF4; print(f'netCDF4 v{netCDF4.__version__}')" | ||
&& xcopy {project}\\test . /E/H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The {project}
directory is on Drive D:
, not C:
and for some odd reason the PATH variables change if you switch drives on Windows. It is easier just to copy them to C:
.
.github/workflows/cibuildwheel.yml
Outdated
CIBW_TEST_COMMAND: > | ||
python -c "import netCDF4; print(f'netCDF4 v{netCDF4.__version__}')" | ||
&& xcopy {project}\\test . /E/H | ||
&& python -m pip install --upgrade numpy cython packaging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably drop numpy, not sure why it wasn't installed with the wheel. Cython and packaging are test dependencies and we can figure out a more elegant way to handle this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move them to pyproject.toml
?
[project.optional-dependencies]
tests = [
"Cython",
"packaging",
]
5891d1b
to
0b5a475
Compare
Windows wheels using conda package is working 🎉 I'll try the same for Linux and macOS next week. Time to clock out a bit. |
203bc1d
to
371dc0f
Compare
@jswhit everything is passing but the devil is in the details as usual. I'd love for more eyes here. We could also try to consolidate some of the many environment variables and options, netcdf-c version, etc but I prefer to leave that for another PR. |
f5bddfe
to
63eae57
Compare
.github/workflows/cibuildwheel.yml
Outdated
@@ -70,7 +70,6 @@ jobs: | |||
&& sh .ci/build_deps.sh | |||
CIBW_BEFORE_BUILD_MACOS: brew install hdf5 netcdf | |||
CIBW_TEST_SKIP: "*_arm64" | |||
CIBW_TEST_REQUIRES: pytest cython |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like cibuildwheel still needs this regardless of it being declared in the pyporoject.toml. The levels of abstraction and isolation when building wheels is quite confusing. I guess we need to to a pip [test] install here. It has been to long that I used pip and I forget.
93eb57b
to
6305503
Compare
This looks great! Thank you @ocefpaf and @ZedThree. I'd like to get this merged soon so we can get a 1.6.5 release out with 3.12 wheels. The old MacPython wheel building repo is broken, and it looks like the multibuild package it relies on is no longer actively maintained. |
Just to say - for the record - we (the multibuild maintainers) think the build is broken because your multibuild is not up to date (see MacPython/netcdf4-python-wheels#23) - and we are maintaining multibuild - but no complaints from us for switching away. |
9014863
to
ab4381f
Compare
a245a9e
to
dcce9e0
Compare
d115346
to
57b50b6
Compare
03a9078
to
9adbfa4
Compare
9adbfa4
to
33eebb9
Compare
6ebf4e6
to
db5b321
Compare
.github/workflows/cibuildwheel.yml
Outdated
CIBW_ENVIRONMENT: MACOSX_DEPLOYMENT_TARGET=14.0 | ||
- os: macos-13 | ||
arch: x86_64 | ||
CIBW_ENVIRONMENT: MACOSX_DEPLOYMENT_TARGET=13.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are a bit high, most folks set a lower bar with MACOSX_DEPLOYMENT_TARGET=10.9
. If we aim to set 10.9 that we would need to build our own HDF5. I'm not sure how many people we are leaving behind by setting a higher deployment target, probably the same amount as brew. With that said, I'd like to try the HDF5 in another PR. This one is getting unwieldy already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like 13.0 was released in Oct 24, 2022 - this seems problematic to me. I think we would be leaving at least half our users behind with this restriction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some guidelines on adding hdf5 from another project. I'll try that but I don't know if I can make it before the next release. Indeed, brew is too new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I guess we could release 1.7.0 with the 13.0 restriction, and add new wheels to the pypi release later one the hdf5 build is working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a thought - macports supports older OSes than brew. Have no idea how hard that would be to get working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep that as a plan B then. I'll send if I can get a better alternative working first. Pytable's infrastructure seems pretty close to what we need here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would MACOSX_DEPLOYMENT_TARGET 11 work for you? That is the oldest image available and brew is working fine to install both hdf5 and netcdf on it compiling from source. I still want to pursue the approach used in pytables at a later time. just want to be sure I'll have all the proper dependencies used in the current multibuild.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a target of macos 11 would be fine, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a million @ocefpaf! Merging now...
5ab5403
to
1452d90
Compare
This is just a proof of concept to see if we can build wheels here. We probably want to build out own hdf5 and netcdf-c b/c the ones from CentOS are quite old probably the source of the failures we are seeing in the tests here. See https://github.com/MacPython/netcdf4-python-wheels/blob/master/config.sh#L13-L26 for the versions used in the muiltbuild.
xref.: #1204