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

Cibw2 #28

Closed
wants to merge 37 commits into from
Closed

Cibw2 #28

wants to merge 37 commits into from

Conversation

andyfaff
Copy link
Owner

@andyfaff andyfaff commented Aug 1, 2022

TODO:

  • figure out how to place extra license file into wheels, re-enable test that license is bundled
  • consider what tests to run on wheels (just running scipy.test() now)
  • macOS wheels. Can currently build macosx_x86_64 on the macos-10.15 image. This will stop working soon as the image is deprecated on github actions. This will requires update of openBLAS chain to be made available. Currently blocked from building the macosx_arm64 via cross compilation as I can't get the gfortran compiler to work.
  • re-enable azure workflow
  • Sort out conditions for when the wheels workflow happens. Will currently occur on workflow_dispatch (i.e. you manually start the workflow, scheduled (once a week), if the commit message contains [wheels build], or if there is a tag pushed which begins with 'v', and doesn't end with 'dev0'.
  • win_AMD64!
  • Figure out what ILP64 is, and what should be done with those kinds of environment variables
  • Figure out where they get uploaded to.
  • Work out where to get BLAS from on windows, https://github.com/scipy/scipy-ci-artifacts/raw/main/openblas_32_if.zip or https://anaconda.org/multibuild-wheels-staging/openblas-libs. It seems like the former is preferred with the mingw toolchain, otherwise DLLs don't get loaded properly.
  • discuss how delvewheel repairs windows wheels. It makes an accompanying directory in site-packages called scipy.libs which will contain a name-mangled libopenblas.dll. It also patches scipy.__init__ to load the DLL from scipy.libs at import. I don't think it's possible to place .libs within the scipy directory itself.
  • figure out how to get the git hash into the filename. I would have thought this would dealt with during the build process.

@andyfaff
Copy link
Owner Author

andyfaff commented Aug 2, 2022

@rgommers, when creating wheels there are extra license files to be included in the wheel (e.g. https://github.com/andyfaff/scipy/blob/cibw2/tools/wheels/LICENSE_linux.txt). I've tried placing them in the scipy directory when the wheel is built (https://github.com/andyfaff/scipy/blob/cibw2/tools/wheels/cibw_before_build.sh#L8), but they don't end up in the wheel. Do I have to modify a config file somewhere to make sure they're bundled?

@rgommers
Copy link

rgommers commented Aug 2, 2022

@rgommers, when creating wheels there are extra license files to be included in the wheel (e.g. https://github.com/andyfaff/scipy/blob/cibw2/tools/wheels/LICENSE_linux.txt). I've tried placing them in the scipy directory when the wheel is built (https://github.com/andyfaff/scipy/blob/cibw2/tools/wheels/cibw_before_build.sh#L8), but they don't end up in the wheel. Do I have to modify a config file somewhere to make sure they're bundled?

They should not be bundled, they should be appended to the main license file, as done in https://github.com/MacPython/scipy-wheels/blob/master/patch_code.sh.

What we need with cibuildwheel is pretty much the same as numpy already has, so you can use that as a reference: https://github.com/numpy/numpy/blob/main/tools/wheels/cibw_before_build.sh

@rgommers
Copy link

rgommers commented Aug 2, 2022

workout how to fuse macOS wheels (cibuildwheel tries to make fat binary)

I'd prefer to not spend any more effort and CI time on that. universal2 wheels are pretty much useless; there is no way to even install them from PyPI with pip. NumPy also doesn't have universal2 wheels anymore, and that has not been a problem for anyone so far.

@rgommers
Copy link

rgommers commented Aug 2, 2022

Figure out what ILP64 is, and what should be done with those kinds of environment variables

You can leave this one alone; it's not yet supported in the Meson build and anyway we have never shipped wheels using ILP64. What it is is a specific ABI flavor of 64-bit BLAS.

@rgommers
Copy link

rgommers commented Aug 2, 2022

Figure out where they get uploaded to.

This you can copy from the numpy cibuildwheel config; there are two Anaconda buckets containing nightly and pre-release wheels.

@andyfaff
Copy link
Owner Author

andyfaff commented Aug 2, 2022

@rgommers the current stumbling block is scipy#14812 (comment). Trying to cross compile for macOS-arm64 on github actions requires a cross-file for meson. I'm not sure how to provide that to cibuildwheel (which is then calling python -m build ., or python -m pip build .) so that it's eventually picked up by meson.

cibuildwheel seems to be creating new venvs, so I don't know where to place the cross file ahead of time.

@rgommers
Copy link

rgommers commented Aug 2, 2022

I'll work on that, I think https://github.com/FFY00/meson-python/pull/122 should enable that. It'd be useful though to add one cross-compilation CI job for SciPy itself before making things more complex with the extra cibuildwheel layer on top. Can we progress everything other than the arm64 cross-compile job here, and do cross-compilation separately? There's enough other things to get right here that it makes sense to do it in >1 PR I think.

@andyfaff
Copy link
Owner Author

andyfaff commented Aug 2, 2022

Iinux and macos-x86_64 are probably very close to working. Not touched windows yet.

@andyfaff
Copy link
Owner Author

andyfaff commented Aug 3, 2022

@rgommers @mattip in this test run (L274) I'm finding that the 'default' gfortran setup for macos-x86_64, obtained from here (gfortran-4.9.0) don't seem to be able to work when the CI is running on a macos-11 GH actions machine; it was able to work on macos-10.15.

I transitioned to macos-11 in this PR because there's a scheduled brownout on macos-10.15. Furthermore macos-10.15 is going to be removed from github actions at the end of the month.

I'm not sure if the issue is because that gfortran is so old, or because there's some other flags interfering, or there's an issue with meson.

EDIT:
when don't install that old version of gfortran, but instead use the gfortran supplied with gh actions:

    GFORTRAN=$(type -p gfortran-9)
    sudo ln -s $GFORTRAN /usr/local/bin/gfortran

then the build proceeds as far as the delocate step, albeit with warnings along the lines of ld: warning: dylib (/usr/local/Cellar/gcc@9/9.5.0/lib/gcc/9/libgfortran.dylib) was built for newer macOS version (11.5) than being linked (10.9). The delocate step fails because the libopenblas.dylib needs an older version of libgfortran.dylib.
It may be possible to use newer gfortran compilers, but I think gfortran-4.9 may be the last one to support 10.9. Using a more recent gfortran would require the openblas dylibs sourced from https://anaconda.org/multibuild-wheels-staging/openblas-libs to be rebuilt.

EDIT 2:
I tried creating a small test program just after the fortran compiler had been installed to see if it was meson's fault, or with the compiler:

  $printf '      program thing\n      write(*,*) '\''hello world'\''\n      end\n'
  $ gfortran test.f -o a.out
  ld: library not found for -lSystem

It therefore seems that gfortran-4.9.0 doesn't work on the macos-11 image. If we want to make wheels using cibuildwheel it may be that we're going to have to update our gfortran tooling, and our openblas libs.

@rgommers
Copy link

rgommers commented Aug 3, 2022

It may be possible to use newer gfortran compilers, but I think gfortran-4.9 may be the last one to support 10.9.

We should be okay bumping the minimum version to macOS 10.13 or 10.14; IIRC that was necessary anyway for some other reasons. Also, our minimum GCC is 8 now, so I think it's time to get rid of that ancient Gfortran (also Cc @matthew-brett).

Rebuilding the openblas binaries should not be hard; we just need to figure out which Gfortran to use. I think the answer is MacPython/gfortran-install#7.

@andyfaff
Copy link
Owner Author

andyfaff commented Aug 4, 2022

@rgommers, I'm having troubles with the windows build picking up where openblas is. Before cibuildwheel starts building a CIBW_BEFORE_BUILD script is run. As part of the script I obtain the precompiled openblas bundle. In this script I use pkg-config to check that the utility can find the openblas library. Everything is good so far, pkg-config finds the library (L166).

However, when cibuildwheel starts meson isn't picking up any installed openblas (L281). This is despite the correct environment variable being set L9.

I've spent a while trying to figure this out, so any insight you can provide would be welcome.

(you have to provide environment variables to be passed through CIBW to the build environment as CIBW_ENVIRONMENT: PKG_CONFIG_PATH=/blah).

@mattip
Copy link

mattip commented Aug 4, 2022

Here is the pkgconfig error line
Run-time dependency openblas found: NO (tried pkgconfig and cmake)

When the before build script checks pkgconfig it explicitly sets a PKG_CONFIG_PATH
PKG_CONFIG_PATH=/c/opt/openblas/if_32/64/lib/pkgconfig pkg-config --list-all.
How does the actual wheel build get that value? In the regular windows meson build it is done by
echo "PKG_CONFIG_PATH=c:\opt\openblas\if_32\64\lib\pkgconfig;" >> $env:GITHUB_ENV

.github/workflows/wheels.yml Outdated Show resolved Hide resolved
@rgommers
Copy link

rgommers commented Aug 4, 2022

Hmm, it's probably just the environment variable or the path somehow, but so far I've not spotted an issue in the logic.

CIBW_ENVIRONMENT: PKG_CONFIG_PATH=c:\\opt\\openblas\\if_32\\64\\lib\\pkgconfig

gets translated by cibuildwheel to

PKG_CONFIG_PATH=/mnt/c/opt/openblas/if_32/64/lib/pkgconfig

seems fine.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@rgommers
Copy link

rgommers commented Aug 4, 2022

This is now a single line:

PKG_CONFIG_PATH=/c/opt/openblas/if_32/64/lib/pkgconfig pkg-config --list-all

To check that CIBW_ENVIRONMENT: PKG_CONFIG_PATH=... propagates as intended, how about running
pkg-config --list-all --debug in the install-rtools step?

@andyfaff
Copy link
Owner Author

andyfaff commented Aug 4, 2022

@rgommers, @mattip, I spent several hours banging my head against the wall. THen about 5 mins after I made my last comment I discovered the solution to the issue.

In this PR the CIBW_BEFORE_BUILD script is run as bash, even on a Windows machine. I am unfamiliar with bash scripts running on windows. In that bash script I was unzipping the openblas into /mnt/c/opt. However, when considered as a windows path (such as that used by meson in the build process) it turns out that this is not the same as c:/opt. When I unzipped the openblas into /c/opt it suddenly started working.

@andyfaff
Copy link
Owner Author

andyfaff commented Aug 4, 2022

@rgommers, there are a whole lot of these types of warning:

    C:\Users\runneradmin\AppData\Local\Temp\pip-build-env-54nwlb9c\overlay\Lib\site-packages\numpy\core\include/numpy/npy_common.h:168:20: warning: '_fseeki64' redeclared without dllimport attribute after being referenced with dll linkage
      168 | extern int __cdecl _fseeki64(FILE *, long long, int);
          |                    ^~~~~~~~~

Not sure how to resolve that.

@rgommers
Copy link

rgommers commented Aug 4, 2022

Please ignore those warnings. I have already fixed them in NumPy, that's why they don't show up in our regular Meson on Windows CI. However, given that we have to build against older NumPy releases for wheels, there isn't much that we can do about these warnings there.

@andyfaff andyfaff force-pushed the cibw2 branch 2 times, most recently from 3f0fb59 to 949791b Compare August 6, 2022 00:43
@andyfaff
Copy link
Owner Author

andyfaff commented Aug 6, 2022

@rgommers, it's looking healthier now. Window AMD64, Linux x86_64 and aarch64 being built. Leaving macOS for now because the Fortran compiler/openBLAS aspect needs updating. The only issue is that the Windows wheels are all enormous, ~150 Mb.

@rgommers
Copy link

rgommers commented Aug 6, 2022

@andyfaff very nice! Re Windows wheel size, did you already look at why? Perhaps the stripping isn't done automatically. It should be strip[ing, that's the default set by meson-python, but we haven't built Windows wheels so perhaps something is going wrong there. Either that, or we're vendoring something large I guess.

@andyfaff
Copy link
Owner Author

andyfaff commented Aug 6, 2022

A lot of the pyd files are on the order of 20 Mb. I'm unfamiliar with stripping, I'll read up on it.

@rgommers
Copy link

rgommers commented Aug 6, 2022

A lot of the pyd files are on the order of 20 Mb. I'm unfamiliar with stripping, I'll read up on it.

What numpy.distutils did was always build with debugging info enabled (which really bloats an extension) and then manually strip each extension afterwards with strip or a related utility. For Meson we do it differently; it has command-line flags to control this, and with python dev.py build we use the default (which includes debug info) while if we build wheels through meson-python we disable debug info: https://github.com/FFY00/meson-python/blob/main/mesonpy/__init__.py#L498-L499. That looks fine in the CI logs though.

@matthew-brett have you seen this before perhaps?

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.

9 participants