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

BLD: Accelerate wheels for macOS 14+ #20510

Merged
merged 2 commits into from Apr 25, 2024
Merged

Conversation

thalassemia
Copy link
Contributor

@thalassemia thalassemia commented Apr 17, 2024

What does this implement/fix?

Adds GHA jobs to build Accelerate wheels for macOS >= 14 as discussed in #19816 (comment).

Additional information

@github-actions github-actions bot added scipy.special CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure Build issues Issues with building from source, including different choices of architecture, compilers and OS labels Apr 17, 2024
.github/workflows/wheels.yml Outdated Show resolved Hide resolved
.github/workflows/wheels.yml Outdated Show resolved Hide resolved
scipy/special/tests/test_hyp2f1.py Outdated Show resolved Hide resolved
tools/wheels/cibw_before_build_macos.sh Outdated Show resolved Hide resolved
.github/workflows/wheels.yml Show resolved Hide resolved
.github/workflows/macos_meson.yml Outdated Show resolved Hide resolved
.github/workflows/macos_meson.yml Outdated Show resolved Hide resolved
.github/workflows/wheels.yml Outdated Show resolved Hide resolved
.github/workflows/wheels.yml Outdated Show resolved Hide resolved
.github/workflows/wheels.yml Show resolved Hide resolved
@andyfaff
Copy link
Contributor

In order to get the wheels to build you'll need to have [wheel build] in the commit message.

@thalassemia thalassemia force-pushed the accelerate_whl branch 4 times, most recently from e4573a9 to 4dc9b4f Compare April 18, 2024 00:00
@andyfaff
Copy link
Contributor

It's often better to iterate on your own fork, as it can decrease CI usage. I open a PR against my own fork and iterate. When I'm happy I close the PR against my own fork and make a PR against the scipy fork.

To get the CI to run on your own fork you have to change if: github.repository == 'scipy/scipy' to if: github.repository == 'thalassemia/scipy'

@thalassemia
Copy link
Contributor Author

It's often better to iterate on your own fork, as it can decrease CI usage. I open a PR against my own fork and iterate. When I'm happy I close the PR against my own fork and make a PR against the scipy fork.

I did this before putting this PR up. You can view build logs from those early iterations via the link in the PR description. Just had a couple belated realizations afterwards haha.

@thalassemia thalassemia marked this pull request as ready for review April 18, 2024 21:06
thalassemia added a commit to thalassemia/scipy that referenced this pull request Apr 18, 2024
IP is deprecated (scipygh-20216) and these tests fail on macOS x86 Accelerate builds (scipygh-20510)
[skip circle]
@andyfaff
Copy link
Contributor

Hmm the rerun should've worked because I merged 20522. Can you rebase this PR on scipy/main after committing #20510 (comment)?

@thalassemia thalassemia marked this pull request as draft April 19, 2024 05:38
@thalassemia thalassemia force-pushed the accelerate_whl branch 2 times, most recently from 477ee53 to f06f3c4 Compare April 19, 2024 07:36
@thalassemia
Copy link
Contributor Author

I added another commit that keeps our custom gfortran compilers for all our current builds. This eliminates most of the warnings about mismatched macOS target versions. The only such remaining warnings are in the ARM OpenBLAS builds, but the discrepancy is so small that it's probably fine (12.0 vs 12.2). I like the fact that this solution is more objectively correct (avoids warnings) and gives us more control over the build toolchain. It leaves a lot of complexity in the code (took me quite a while to get things right), but hopefully we won't need to touch this often. If this seems preferable, I can squash the commits to make this PR easier to review.

@andyfaff
Copy link
Contributor

I think if you get it working then we should merge. However, I would like to transition to pre installed gfortran as it'll make life easier in the longrun

@thalassemia thalassemia marked this pull request as ready for review April 19, 2024 09:27
@thalassemia
Copy link
Contributor Author

thalassemia commented Apr 19, 2024

Many of the Linux Meson Python 3.12 failures are related to numpy/numpy#26215. Here is the relevant code in our repo.

I'll look into the sign flip in scipy/linalg/tests/test_decomp.py::TestOverwrite::test_eig_banded.

@rgommers
Copy link
Member

This accumulated one merge conflict now. Other than that, is it good to go?

@andyfaff
Copy link
Contributor

I wanted one last look after the conflict is fixed

ln -s $GFORTRAN_LOC gfortran
if [[ ${{ matrix.buildplat[3] }} == 'accelerate' ]]; then
echo CIBW_CONFIG_SETTINGS=\"setup-args=-Dblas=accelerate\" >> "$GITHUB_ENV"
# Always use preinstalled gfortran for Accelerate builds
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the pre-installed gfortran is used for the openblas builds? It'd be nice to get rid of the gfortran installation in cibw_before_build_macos.sh.

@andyfaff
Copy link
Contributor

Whilst we could probably iterate a bit further (e.g. to see if we can remove gfortran from cibw_before_build_macos), I think it's best to merge now. More mods could be done in a further PR. Thanks @thalassemia

@andyfaff andyfaff merged commit 5b793c0 into scipy:main Apr 25, 2024
52 checks passed
@rgommers
Copy link
Member

Great to get this in - thanks for the hard work! I added notes on Accelerate in https://github.com/scipy/scipy/wiki/*Release-Note-Entries-for-SciPy-1.14.0-(asterisk-makes-this-appear-at-the-top)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build issues Issues with building from source, including different choices of architecture, compilers and OS CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure scipy.special
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants