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

maint: update CI wheel building script #20

Merged
merged 8 commits into from
Apr 21, 2022

Conversation

oscarbenjamin
Copy link
Collaborator

@oscarbenjamin oscarbenjamin commented Mar 14, 2022

Update in relation to #1.

Previously it wasn't possible to build python_flint with the most recent versions of Cython but that was fixed in b3eb354. That makes it possible to update all build dependencies and to create wheels for newer Python versions. This commit updates the build scripts that build wheels for OSX and Linux in GitHub Actions.

The result of these changes is that the latest versions of all build and runtime dependencies are currently being used and x86_64 bit wheels are being built for OSX and Linux for all supported CPython versions.

Changes:

  • Unpin the version of Cython used to build python_flint so that the newest version will always be used. At the time of writing that means using Cython 0.29.28 but newer versions will automatically be used when released.

  • Build 64 bit CPython wheels for OSX and Linux for all currently supported Python versions (3.7, 3.8, 3.9 and 3.10).

  • Fix the YASM version from 1.3.1 to 1.3.0. There does not seem to be a 1.3.1 version of YASM any more and 1.3.0 appears to be the newest version. It is unclear what happened to 1.3.1 which was previously used successfully.

  • Use the cibuildwheel GitHub Action for wheel building and bump version of cibuildwheel to 2.3.1 to get support for Python 3.10.

  • Bump versions of flint and arb to the latest releases (flint 2.8.4 and arb 2.22.0).

  • Check that all other dependencies are at their latest released versions (GMP 6.2.1 and MPIR 3.0.0 and MPFR 4.1.0).

Outstanding issues:

  • Wheels are still not built for Windows - this is the biggest omission. There are conda-forge recipes for building python-flint and it should be possible to adapt those.

  • Newer OSX runs on Apple's M1 hardware so the OSX wheels built here will not work there. It is not yet possible to run M1 hardware in GitHub Actions so it is not immediately clear how to build those wheels.

  • It is also possible to build 32 bit Linux wheels but these are less likely to be used. Building them significantly increases the build time because numpy does not provide 32 bit wheels and numpy is currently a build-time dependency. Linux wheels also consume a lot of disk space which is limited for artifacts in GitHub Actions.

  • I have not checked whether it is possible to build wheels for pypy which is another likely platform that could be supported.

  • The build-time dependency on numpy can probably be eliminated. A couple of small things are being used from numpy.distutils which is itself probably going to be deprecated in future.

Previously it wasn't possible to build python_flint with the most recent
versions of Cython but that was fixed in b3eb354. That makes it
possible to update all build dependencies and to create wheels for newer
Python versions. This commit updates the build scripts that build wheels
for OSX and Linux in GitHub Actions.

The result of these changes is that the latest versions of all build and
runtime dependencies are currently being used and x86_64 bit wheels are
being built for OSX and Linux for all supported CPython versions.

- Unpin the version of Cython used to build python_flint so that the
  newest version will always be used. At the time of writing that means
  using Cython 0.29.28 but newer versions will automatically be used
  when released.

- Build 64 bit CPython wheels for OSX and Linux for all currently
  supported Python versions (3.7, 3.8, 3.9 and 3.10).

- Fix the YASM version from 1.3.1 to 1.3.0. There does not seem to be a
  1.3.1 version of YASM any more and 1.3.0 appears to be the newest
  version. It is unclear what happened to 1.3.1 which was previously
  used successfully.

- Use the cibuildwheel GitHub Action for wheel building and bump version
  of cibuildwheel to 2.3.1 to get support for Python 3.10.

- Bump versions of flint and arb to the latest releases (flint 2.8.4 and
  arb 2.22.0).

- Check that all other dependencies are at their latest released
  versions (GMP 6.2.1 and MPIR 3.0.0 and MPFR 4.1.0).

- Wheels are still not built for Windows - this is the biggest omission.
  There are conda-forge recipes for building python-flint and it should
  be possible to adapt those.

- Newer OSX runs on Apple's M1 hardware so the OSX wheels built here
  will not work there. It is not yet possible to run M1 hardware in
  GitHub Actions so it is not immediately clear how to build those
  wheels.

- It is also possible to build 32 bit Linux wheels but these are less
  likely to be used. Building them significantly increases the build
  time because numpy does not provide 32 bit wheels and numpy is
  currently a build-time dependency. Linux wheels also consume a lot of
  disk space which is limited for artifacts in GitHub Actions.

- I have not checked whether it is possible to build wheels for pypy
  which is another likely platform that could be supported.

- The build-time dependency on numpy can probably be eliminated. A
  couple of small things are being used from numpy.distutils which is
  itself probably going to be deprecated in future.
@oscarbenjamin
Copy link
Collaborator Author

Actually it looks like it is possible to build wheels for Apple's M1 from the GitHub runner. The wheels can be either "universal2" or separate "arm64" wheels. It seems that matplotlib provides three separate wheels:
https://github.com/matplotlib/matplotlib/blob/d7828f34b0b1e8836517dad47d7518526391c382/.github/workflows/cibuildwheel.yml#L34
https://pypi.org/project/matplotlib/#files

Maybe it's just worth having a single universal2 wheel. I don't have any Apple M1 hardware to test on though...

@oscarbenjamin
Copy link
Collaborator Author

Downloading the wheels from here it seems that using them on Linux gives a core dump:

>>> import flint
>>> u = flint.fmpz(2)
>>> int(u)
2
>>> str(u)
Illegal instruction (core dumped)

The cibuildwheel script explicitly tests each wheel with the following and it passes in CI but crashes when I download the wheel and run it locally:

$ python -c 'import flint; assert str(flint.fmpq(3, 2)**2) == "9/4"'
Illegal instruction (core dumped)

In fact it fails when building locally. Maybe I've updated too many dependencies...

The OSX wheels seem to work okay.

@oscarbenjamin
Copy link
Collaborator Author

Maybe I've updated too many dependencies...

Somehow this seems to be caused by updating the flint and arb versions. I'll roll that part back for now.

@oscarbenjamin
Copy link
Collaborator Author

That doesn't seem to have fixed it. I maybe getting mixed up here about the cause of this. Under gdb I get:

(gdb) run test/test.py
Starting program: /home/oscar/current/sympy/python-flint-mine.git/38venv/bin/python test/test.py
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGILL, Illegal instruction.
0x00007ffff671a3c7 in __gmpz_set_str ()
   from /home/oscar/current/sympy/python-flint-mine.git/38venv/lib/python3.8/site-packages/flint/../python_flint.libs/libgmp-ee046dcd.so.10.4.1

@oscarbenjamin
Copy link
Collaborator Author

The core dump problem is not a new one. I tried generating wheels in CI with the old CI script and they also didn't work on this Linux machine. Locally built wheels are fine. It seems that there is some problem with the way that wheels are generated in CI for Linux. Clearly they aren't tested very well either...

@oscarbenjamin
Copy link
Collaborator Author

It seems that the wheels test fine in CI but still when I download the Linux wheels they crash locally:

$ python
Python 3.9.7 (default, Sep 16 2021, 13:09:58) 
[GCC 7.5.0] :: Anaconda, Inc. on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import flint
>>> u=flint.fmpz(2)
>>> u
Illegal instruction (core dumped)

Using gdb I get:

$ gdb python
GNU gdb (GDB) Red Hat Enterprise Linux 7.6.1-120.el7
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /space/enojb/miniconda3/bin/python3.9...done.
(gdb) run
Starting program: /space/enojb/sympy/python-flint/tmp/39venv/bin/python 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Python 3.9.7 (default, Sep 16 2021, 13:09:58) 
[GCC 7.5.0] :: Anaconda, Inc. on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import flint
>>> u = flint.fmpz(2)
>>> str(u)

Program received signal SIGILL, Illegal instruction.
0x00007fffef6bd763 in __gmpz_sizeinbase ()
   from /space/enojb/sympy/python-flint/tmp/39venv/lib/python3.9/site-packages/flint/../python_flint.libs/libgmp-ee046dcd.so.10.4.1
Missing separate debuginfos, use: debuginfo-install glibc-2.17-325.el7_9.x86_64

My guess for SIGILL would be either memory corruption or wrong architecture.

Using cibuildwheel is supposed to make the wheels portable in terms of OS and libc etc. Maybe the problem is that flint and/or gmp are being built in ways that aren't portable to other x86_64 machines.

Add jobs to the wheel build GitHub Actions workflow that will install
and test each built wheel in a fresh test runner. For each combination
of Python version and OS (currently 3.7-3.10 and OSX/Linux) this will
run a job that installs the wheel and runs the test script using the
installed wheel.

While this can give some basic confidence that the wheels do work it is
still possible that these tests can pass in CI with wheels that would
fail when transfered to another computer. At the time of this commit
that is the case with the manylinux wheels which pass all tests but fail
when installed to another Linux machine.
The manylinux wheels built with GMP crashed with SIGILL when used on
another Linux machine even though they passed tests in CI. This commit
switches to using MPIR instead of GMP for the manylinux wheels because
those wheels appear to be more "portable" according to limited testing
so far.

Maybe there is a configure option that could be used with GMP to make it
more portable...
@oscarbenjamin
Copy link
Collaborator Author

Switching to MPIR for the manylinux wheels seems to do the trick.

Maybe there is a configure option for GMP that could be used to make it build in a more portable way or something...

@oscarbenjamin
Copy link
Collaborator Author

@fredrik-johansson @isuruf does this look good to you?

I've realised I can get access to an M1 architecture Mac so I'll try adding wheels for that to test those as well. Otherwise I think this is as far as I can go without figuring out the Windows build just yet.

Is it preferable to use MPIR or GMP for the wheels? For OSX the wheels were already using MPIR and I think it's needed for Windows as well so at least using MPIR for Linux means using the same on all OSes.

Any idea how to make GMP more portable? I think GMP has a --disable-architecture configure option but I'm not sure how much optimisation that disables.

@isuruf
Copy link
Member

isuruf commented Mar 19, 2022

Is it preferable to use MPIR or GMP for the wheels? For OSX the wheels were already using MPIR and I think it's needed for Windows as well so at least using MPIR for Linux means using the same on all OSes.

Definitely GMP. MPIR hasn't received updates in a while.

Any idea how to make GMP more portable? I think GMP has a --disable-architecture configure option but I'm not sure how much optimisation that disables.

--enable-fat

@oscarbenjamin
Copy link
Collaborator Author

--enable-fat

That option is already being used. The steps to build GMP are here:
https://github.com/fredrik-johansson/python-flint/blob/b3eb3547689ea241c9dcb59bd232d9b50a13c6e4/bin/build_dependencies_unix.sh#L74-L83
The host argument is set to x86_64-unknown-linux-gnu:
https://github.com/fredrik-johansson/python-flint/blob/b3eb3547689ea241c9dcb59bd232d9b50a13c6e4/bin/cibw_before_build_linux.sh#L3-L6

Definitely GMP. MPIR hasn't received updates in a while.

So would it be better to use GMP on all OSes? I can't remember why I went with MPIR on OSX. I'm pretty sure I could build locally with either.

@oscarbenjamin
Copy link
Collaborator Author

The host argument is set to x86_64-unknown-linux-gnu:

Looking at gmpy2 it seems that the host is set to the output of GMP's configfsf.guess script.
https://github.com/aleaxit/gmpy/blob/ee03be1a2791ce5c207d599798141454711c79c8/scripts/before_ci_build.sh#L15

Running that locally on Linux I get:

$ ./configfsf.guess 
x86_64-pc-linux-gnu

@oscarbenjamin
Copy link
Collaborator Author

For OSX the wheels were already using MPIR

I misremembered this. GMP was being used for both OSX and Linux. I think maybe I was under the impression that MPIR would be needed on Windows but I haven't tried compiling on Windows yet.

Running that locally on Linux I get:

$ ./configfsf.guess 
x86_64-pc-linux-gnu

This also gives the same from the cibuildwheel docker container. I'll try setting the host to that.

@oscarbenjamin
Copy link
Collaborator Author

I'll try setting the host to that.

I'm still getting the core dump after downloading the manylinux wheels (even though they pass tests when used in CI).

@oscarbenjamin
Copy link
Collaborator Author

Actually the difference between MPIR and GMP is possibly a red herring. I've tried various linux machines and it just seems to be that the manylinux wheels here work on some machines but not on others e.g. they work on Ubuntu 20.04 but not 16.04.

I would have thought that pip would know which versions are compatible and therefore not install the wheel on the wrong platform but maybe that's not how it works.

@oscarbenjamin
Copy link
Collaborator Author

It seems that using manylinux2010 instead of the default manylinux_2_17 fixes the problem. I'm now testing manylinux2014.

@oscarbenjamin
Copy link
Collaborator Author

It seems that manylinux2014 is also fine. I'll test on more Linux machines tomorrow.

@mazar
Copy link

mazar commented Apr 6, 2022

I was going to start a separate effort to build wheels for this repo. Maybe I can help speed things up here to get this PR to finish line?

@oscarbenjamin
Copy link
Collaborator Author

Any help is definitely appreciated. Just testing the wheels that are already here is helpful.

So far it seems that things are working fine for x86_64 on Linux and OSX. I can maybe get things working on OSX M1.

The big thing that I'm not sure about is Windows. To me that's the most important platform because it's also the one where users are least likely to be able to compile this themselves. I personally don't use Windows at all though: if you're a Windows user then any help figuring this out would be great.

Part of the appeal of MPIR is support for compiling with Microsoft's compilers on Windows. Above it is mentioned that MPIR hasn't been updated in a while which would suggest that it is better to go with GMP. AFAICT the GMP library deliberately doesn't support using Microsoft's compilers.

Official Python installers for Windows give binaries compiled with MSVC that are linked with the appropriate msvc runtime DLL which is I think why it is typically good to compile Python extensions with MSVC on Windows. I'm not sure if that's problematic when compiling GMP or flint though.

I see that the MSYS2 project has packages for GMP and flint:
https://packages.msys2.org/package/mingw-w64-x86_64-gmp
https://packages.msys2.org/package/mingw-w64-x86_64-flint?repo=mingw64

Maybe those packages can be used to build the libraries and the extension module. Ideally though all of the build code would be here in this repo so that it doesn't directly depend on other packaging projects like msys at least for building the core dependencies.

@mazar
Copy link

mazar commented Apr 7, 2022

I'm not experienced building these packages on Windows either, so I tried a bunch of things and none seem to be a desirable solution. MSYS2 (and mingw) are subsystems and what is installed/built in there is only visible inside. So even if we get wheels built in mingw, those wheels cannot be used by python versions directly installed on Windows.

I tried building the dependencies (gmp/mpri, mpfi, flint, arb to name a few) but unless there is a MSVC compatible repo, it is just not feasible (at least not for me).

My advice is to release the Linux/MacOS wheels so at least those could be used.

@oscarbenjamin
Copy link
Collaborator Author

So even if we get wheels built in mingw, those wheels cannot be used by python versions directly installed on Windows.

The OSX/Linux wheels bundle the libraries that they need so I'd expect Windows wheels to do the same. The cibuildwheel action is supposed to take care of this sort of stuff. The main thing I'm not sure about is if there would be a problem with linking against different msvcrt dlls if the wheel bundles libs built with mingw but gets installed for a python.exe built with msvc.

@mazar
Copy link

mazar commented Apr 8, 2022

I see some work was done to build Windows wheels with cygwin for a different project but does not seem to have succeeded: pypa/cibuildwheel#329

@oscarbenjamin
Copy link
Collaborator Author

I think a good place to look is gmpy:
https://github.com/aleaxit/gmpy

They've managed to package gmp and make usable wheels for all platforms somehow. I'm not sure exactly how it works but I see a mingw64 directory in the repo.

@mazar
Copy link

mazar commented Apr 11, 2022

They seem to have documented the process of building the wheels in MSYS2. I will see if I can reuse any of that.

@mazar
Copy link

mazar commented Apr 13, 2022

ARB does not build correctly in MINGW64. -std=c99 needs to be added to CFLAGS in configure script. I created a PR against ARB: flintlib/arb#418

@mazar
Copy link

mazar commented Apr 15, 2022

UPDATE: I can produce shared DLLs for gmp, mpfr, flint and arb and a wheel for python-flint which does not include those DLLs and this works when those shared DLLs are in PATH. Still need to figure out how to include the DLLs in the wheel package.
In gmpy they use different components in setup.py, so same setup.py build_ext ... does not work for python-flint.

@oscarbenjamin
Copy link
Collaborator Author

Good work!

Still need to figure out how to include the DLLs in the wheel package.

For OSX/Linux once I reached that point I just used cibuildwheel which handles bundling all of these. I think you just need to make sure that the DLLs are on PATH when cibuildwheel runs:

https://github.com/fredrik-johansson/python-flint/blob/ecbaa5b0696e14a1d7f8fdd64dd10be7ee82c35e/.github/workflows/buildwheel.yml#L28-L31

@mazar
Copy link

mazar commented Apr 15, 2022

Speaking of cibuildwheel, at this time I do not have an automated way of installing MSYS2 and running its CLI so it is all manual. Looks like gmpy2 is also building Windows wheels manually.
I can use delvewheel (as suggested by others on cibuildwheel comments) to combine the DLLs into the wheel package, but still some kinks to iron out.

@oscarbenjamin
Copy link
Collaborator Author

There seems to be an action here for setting up msys2 in CI:
https://github.com/msys2/setup-msys2

Not sure if that's any good (I just googled "install msys2 github actions") but it looks both official and maintained.

@oscarbenjamin
Copy link
Collaborator Author

Since msys2 is a unix-like environment is it possible to adapt the bin/build_dependencies_unix.sh script? It would be great if a single script was used for all builds.

@mazar
Copy link

mazar commented Apr 20, 2022

I'm making progress with GitHub actions and MSYS2, but still waiting on https://github.com/fredrik-johansson/arb to release a new tag which includes fix for MINGW64 builds: flintlib/arb#418

@oscarbenjamin
Copy link
Collaborator Author

We don't need to wait for a new release of ARB. We can use git to select a commit that has the fix.

I'll merge this. Can you open a new pull request that gives a Windows build?

@oscarbenjamin oscarbenjamin merged commit 11b418f into flintlib:master Apr 21, 2022
@oscarbenjamin oscarbenjamin deleted the pr_bump_versions branch April 21, 2022 00:09
@mazar
Copy link

mazar commented Apr 21, 2022

I see the build failed at downloading mpfr. I had the same problem and used the following to download it instead:

curl -O https://ftp.gnu.org/gnu/mpfr/mpfr-$MPFRVER.tar.gz

@oscarbenjamin
Copy link
Collaborator Author

I see the build failed at downloading mpfr

Thanks. I've opened #21 to fix that.

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.

3 participants