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

Update wheel builds to include Python 3.10 #6021

Merged

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Nov 9, 2021

Description

This PR attempts to add Python 3.10 wheels to our wheel building workflow.

@leej3 and @viniciusdc, it would be great if you can confirm that the changes proposed here look sane. I based them on ones I made recently for PyWavelets.

For PyWavelets, the combination of Musl linux wheels and aarch64 was leading to timeouts while running the tests, so we disabled these wheels. I think they are pretty niche, so I just disabled the musllinux cases altogether here. If we want to provide them we can try it, but I am not sure it is currently worth the effort.

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

cibuildwheel no longer supports building Python 3.5 wheels
This is related to PEP 656 (https://www.python.org/dev/peps/pep-0656/). In late Oct 2021, cibuildwheel started adding
Musl builds by default. This commit is to disable these for now.
we will not have any new v0.17.x releases
@grlee77 grlee77 added the 🤖 type: Infrastructure CI, packaging, tools and automation label Nov 9, 2021
@grlee77
Copy link
Contributor Author

grlee77 commented Nov 9, 2021

Also, we do not specify CIBW_ARCHS_MACOS, so my understanding is that this will by default produce x86_64, arm64 and universal2 wheels (related cibuildwheel docs).

@grlee77 grlee77 added this to the 0.19 milestone Nov 9, 2021
@leej3
Copy link
Contributor

leej3 commented Nov 9, 2021

it would be great if you can confirm that the changes proposed here look sane

Looks reasonable to me. One detail that I am not sure on is the two sections for MacOS. Perhaps these can now be merged. I cannot spot any differences between them other than the python versions specified. It may have been that a particular python version caused the package to be compiled slightly differently at one point but now I can't spot the reason for the separation (and indeed duplication): As far as I can tell we build for 3.9 and 3.10, and then build for 3.7, 3.8, 3.10.

I think they are pretty niche, so I just disabled the musllinux cases altogether here. If we want to provide them we can try it, but I am not sure it is currently worth the effort.

I am thinking alpine users would be the main ones to be impacted by this. We would need to know they exist... That sounds reasonable, building from source is an adequate alternative when the wheels themselves are not passing tests.

we do not specify CIBW_ARCHS_MACOS, so my understanding is that this will by default produce x86_64, arm64 and universal2 wheels

I read this as, the default used is "auto" which for MacOS is: arm64, universal2

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 9, 2021

One detail that I am not sure on is the two sections for MacOS.

Yes, that did seem a bit odd. The difference is that the run section installs libomp via homebrew for the 3.9 (now also 3.10) case:

          # Make sure to use a libomp version binary compatible with the oldest
          # supported version of the macos SDK as libomp will be vendored into the
          # scikit-learn wheels for macos. The list of bottles can be found at:
          # https://formulae.brew.sh/api/formula/libomp.json. Currently, the oldest
          # supported macos version is: High Sierra / 10.13. When upgrading this, be
          # sure to update the MACOSX_DEPLOYMENT_TARGET environment variable
          wget https://homebrew.bintray.com/bottles/libomp-11.0.0.high_sierra.bottle.tar.gz
          brew install libomp-11.0.0.high_sierra.bottle.tar.gz

I am not sure why this was being done only for Python 3.9, though? I see the comment mentions "scikit-learn" so it was apparently copied from there at some point.

I read this as, the default used is "auto" which for MacOS is: arm64, universal2

yeah, I think that is right. We built all three for PyWavelets, but in the script there I did specify "x86_64 arm64 universal2" instead of "auto".

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 10, 2021

I pushed a tag in my fork, and there are a lot of failures, mostly related to pip trying to build SciPy and Pillow from source. Looking on PyPI it seems that the most recent Pillow 8.4.0 and SciPy 1.7.2 only provide manylinux2014 wheels while we are only trying to use manylinux2014 for aarch64 and Python10 while staying on manylinux2010 for Python 3.9 and manylinux1 for 3.7 and 3.8.

Pillow 8.3.2 and SciPy 1.7.1 do have manylinux1 wheels, but instead of using those, pip is trying to build the latest release from source and fails due to missing dependencies like libjpeg and BLAS/LAPACK.

On OS X there is a separate issue: trying to download libomp from that homebrew link returns a 403. The same 403 occurs if I try to download that URL locally.

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 11, 2021

aarch64 builds are still running, but everything else has completed successfully for the latest attempt: https://github.com/grlee77/scikit-image/runs/4180032144?check_suite_focus=true

The primary issues are:
1.) arm64 and universal2 wheels have been disabled for now. I think we may want to do a release in the immediate term without these and add them in a point release. This is mainly because it will be hard to test the wheels until we have a release of SciPy that supports this architecture
2.) 32-bit wheels were dropped on Windows for Python 3.10 since NumPy doesn't provide these. I think this is fine
3.) i686 (32-bit) linux builds had a couple of test failures to look into. If we can solve those, we can re-enable it.

These are the test failures that occurred in a previous run for i686:

=================================== FAILURES ===================================
  _______________________ test_denoise_bilateral_negative2 _______________________
  
      def test_denoise_bilateral_negative2():
          img = np.ones((50, 50))
          img[2, 2] = 2
      
          out1 = restoration.denoise_bilateral(img)
          out2 = restoration.denoise_bilateral(img - 10)  # contains negative values
      
          # 2 images with a given offset should give the same result (with the same
          # offset)
  >       assert_array_equal(out1, out2 + 10)
  E       AssertionError: 
  E       Arrays are not equal
  E       
  E       Mismatched elements: 245 / 2500 (9.8%)
  E       Max absolute difference: 2.2204460493e-16
  E       Max relative difference: 2.2204460493e-16
  E        x: array([[1., 1., 1., ..., 1., 1., 1.],
  E              [1., 1., 1., ..., 1., 1., 1.],
  E              [1., 1., 2., ..., 1., 1., 1.],...
  E        y: array([[1., 1., 1., ..., 1., 1., 1.],
  E              [1., 1., 1., ..., 1., 1., 1.],
  E              [1., 1., 2., ..., 1., 1., 1.],...
  
  /tmp/tmp.uDFYzUj770/venv/lib/python3.7/site-packages/skimage/restoration/tests/test_denoise.py:274: AssertionError
  _________________ test_slic_consistency_across_image_magnitude _________________
  
      def test_slic_consistency_across_image_magnitude():
          # verify that that images of various scales across integer and float dtypes
          # give the same segmentation result
          img_uint8 = data.cat()[:256, :128]
          img_uint16 = 256 * img_uint8.astype(np.uint16)
          img_float32 = img_as_float(img_uint8)
          img_float32_norm = img_float32 / img_float32.max()
      
          seg1 = slic(img_uint8)
          seg2 = slic(img_uint16)
          seg3 = slic(img_float32)
          seg4 = slic(img_float32_norm)
      
          np.testing.assert_array_equal(seg1, seg2)
          np.testing.assert_array_equal(seg1, seg3)
  >       np.testing.assert_array_equal(seg1, seg4)
  E       AssertionError: 
  E       Arrays are not equal
  E       
  E       Mismatched elements: 2539 / 32768 (7.75%)
  E       Max absolute difference: 12
  E       Max relative difference: 1.
  E        x: array([[ 1,  1,  1, ...,  7,  7,  7],
  E              [ 1,  1,  1, ...,  7,  7,  7],
  E              [ 1,  1,  1, ...,  7,  7,  7],...
  E        y: array([[ 1,  1,  1, ...,  7,  7,  7],
  E              [ 1,  1,  1, ...,  7,  7,  7],
  E              [ 1,  1,  1, ...,  7,  7,  7],...
  
  /tmp/tmp.uDFYzUj770/venv/lib/python3.7/site-packages/skimage/segmentation/tests/test_slic.py:120: AssertionError

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 11, 2021

SciPy 1.8.0 is currently targeted for early Jan 2022 but it is likely a 1.7.3 release adding Mac OS arm64 support will occur earlier. See discussion here: https://mail.python.org/archives/list/scipy-dev@python.org/thread/LPKBGLCMDAWVMX5FLAADGGLM4JRQ3WFZ/

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 11, 2021

The first of the two i686 failures looks like a minor difference that could be fixed by testing with assert_allclose instead of assert_array_equals. The other one looks a bit tougher to address.

I opened a PR that should avoid both failures: #6031

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 11, 2021

Some aarch64 cases timed out after 6 hours. They seem to have gotten stuck indefinitely at 79% tests complete. See, for example the log that shows it stuck for ~1 hour 45 minutes in skimage/measure/tests/test_profile.py:

2021-11-11T20:04:16.6953242Z measure/tests/test_polygon.py ..                                         [ 79%]
2021-11-11T20:04:16.6953948Z measure/tests/test_profile.py .........................                  [ 79%]
2021-11-11T21:50:32.1074133Z ##[error]The operation was canceled.
2021-11-11T21:50:32.1181608Z Post job cleanup.
2021-11-11T21:50:32.2950604Z [command]/usr/bin/git version

no idea why the Python 3.8 case passed (total job time ~3.5 hours and test_profile.py completed in 2-3 minutes)

@hmaarrfk
Copy link
Member

are you emulating for arm? if so, it might be pretty hard to get test suites to run in time.

we see a lot of flutlctuation in time to complete when emulation or others techniques like that are used.

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 12, 2021

Yes, cibuildwheel uses QEMU for aarch64

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 12, 2021

This has now been updated to restore i686 wheel builds on linux for Python 3.7-3.9. I split the aarch cases out to a separate job. Let's see if they get stuck again. In progress here: https://github.com/grlee77/scikit-image/actions/runs/1455089432

@hmaarrfk
Copy link
Member

very nice

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 13, 2021

Okay, things are now working pretty well. All non-aarch64 builds are passing. This time around 2/4 aarch64 completed and the other two made it to around 90% and no particular test seemed to get stuck.

We may want to disable the wheel testing on aarch64 when making a release in order to be sure nothing times out.

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 13, 2021

@scikit-image/core: please see the following summary of wheel building (also posted to Zulip)

The cases that are now working are:

linux x86_64: Python 3.7-3.10
linux i686: Python 3.7-3.9 (numpy seems to have dropped i686 for 3.10)
linux aarch64: Python 3.7-3.10 (only issue is we may need to skip tests to avoid timeouts)
mac OS: x86_64: Python 3.7-3.10
windows AMD64: Python 3.7-3.10
windows x86: Python 3.7-3.9 (numpy seems to have dropped x86 for 3.10)

manylinux variants used

For x86_64 and i686 linux wheels, I currently have Python 3.7 and 3.8 wheels using manylinux1, 3.9 using manylinux2010 and 3.10 using manylinux2014 while all aarch64 wheels must use manylinux2014. This was what I did for the recent PyWavelets release, but I saw that the most recent SciPy point release just used manylinux2014 for most things (i686 wheels and the Python 3.7 x86_64 wheels were manylinux2010), so now I am not sure if we should do the same. I will ping @rgommers and see if he has a recommendation.

Not working, but propose to add in a future point release (0.19.1):

mac OS: arm64, universal2 : will be easiest to wait for SciPy to provide these, otherwise we have problems with SciPy trying to build from source and not finding BLAS/LAPACK, etc. during wheel testing.
musllinux wheels for linux:. a bit niche, but if SciPy adds them it should make it easy for us to do the same

@rgommers
Copy link

This was what I did for the recent PyWavelets release, but I saw that the most recent SciPy point release just used manylinux2014 for most things (i686 wheels and the Python 3.7 x86_64 wheels were manylinux2010)

The upcoming numpy 1.22.0 will do this too I believe (and that's confirmed by https://anaconda.org/scipy-wheels-nightly/numpy/files only having manylinux2014 wheels). manylinux1 is no longer needed, it's obsolete.

mac OS: arm64, universal2 : will be easiest to wait for SciPy to provide these

Agreed. Any day now ....

musllinux wheels for linux:. a bit niche, but if SciPy adds them it should make it easy for us to do the same

No plans for adding musllinux for NumPy or SciPy, that's way too much work for too little gain until we get rid of multibuild and redo our whole build setup with cibuildwheel (that is in progress for NumPy, the goal is to have that ready for 1.23.0).

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 13, 2021

The upcoming numpy 1.22.0 will do this too I believe (and that's confirmed by https://anaconda.org/scipy-wheels-nightly/numpy/files only having manylinux2014 wheels). manylinux1 is no longer needed, it's obsolete.

Okay, thanks. I will try setting Python 3.7 to manylinux2010 and use manylinux2014 for everything else.

This is needed for x86 builds with Pythran
this was added during debugging, but makes the log MUCH longer
@grlee77
Copy link
Contributor Author

grlee77 commented Nov 17, 2021

The changes in a0e0f7a here were to get clang-cl to be used as the compiler for x86 windows wheels. This was needed to build wheels using Pythran (See discussion in #3226 (comment))

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome @grlee77!!!

@alexdesiqueira alexdesiqueira merged commit 461a8a0 into scikit-image:main Nov 17, 2021
@alexdesiqueira
Copy link
Member

Thank you @grlee77!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 type: Infrastructure CI, packaging, tools and automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants