8000 MAINT: Update `openblas_support.py` to support Apple Silicon by judahrand · Pull Request #14316 · scipy/scipy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: Update openblas_support.py to support Apple Silicon #14316

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Aug 20, 2021
Merged

MAINT: Update openblas_support.py to support Apple Silicon #14316

merged 10 commits into from
Aug 20, 2021

Conversation

judahrand
Copy link
Contributor

Reference issue

What does this implement/fix?

This commit brings scipy's implementation of this script into line with numpy's. This includes an update of the OpenBlas version as well as support for macosx-arm64.

The source implementation can be found here: https://github.com/numpy/numpy/blob/7ef28b2f1c383828bebcbb87b8e487e27c6e3ff9/tools/openblas_support.py

I believe this is needed before the checks on MacPython/scipy-wheels#123 will pass.

Additional information

@tylerjereddy tylerjereddy added the maintenance Items related to regular maintenance tasks label Jun 28, 2021
tylerjereddy
tylerjereddy reviewed Jun 28, 2021
View reviewed changes
Copy link
Contributor
@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

I'm assuming you've read and dealt with the issues described here: #14188

@judahrand judahrand changed the title Update openblas_support.py to support Apple Silicon MAINT: Update openblas_support.py to support Apple Silicon Jun 28, 2021
@judahrand
Copy link
Contributor Author

I'm assuming you've read and dealt with the issues described here: #14188

I apologise, I did not! I was coming at this from a different perspective (from MacPython/scipy-wheels) and also not being familiar with the SciPy release system etc. Should have searched issues first.

@tylerjereddy
Copy link
Contributor

Ok, I don't know what to do about that set of issues, but maybe it will get more attention now that M1 support is gaining momentum.

@judahrand judahrand changed the title MAINT: Update openblas_support.py to support Apple Silicon WIP: MAINT: Update openblas_support.py to support Apple Silicon Jun 28, 2021
@judahrand judahrand force-pushed the universal2 branch 3 times, most recently from b376ffe to de82eb5 Compare June 28, 2021 22:59
@judahrand judahrand closed this Jun 29, 2021
@judahrand judahrand reopened this Jun 29, 2021
@rgommers
Copy link
Member

Not quite sure that that slowdown reported on gh-14188 is real. This PR:

image

Another recent PR (#14284):

image

Yet another recent PR (#14308):

image

@rgommers
Copy link
Member

There's also much faster runs - timings jump around a lot.

@rgommers rgommers mentioned this pull request Jun 29, 2021
Closed
@judahrand
Copy link
Contributor Author
judahrand commented Jun 30, 2021 •
edited
Loading

@rgommers Either way it looks like 0.3.15 might not be far off so perhaps worth waiting for that and including here?
MacPython/openblas-libs#58

@judahrand judahrand changed the title WIP: MAINT: Update openblas_support.py to support Apple Silicon MAINT: Update openblas_support.py to support Apple Silicon Jun 30, 2021
@judahrand judahrand requested a review from tylerjereddy July 1, 2021 10:15
@isuruf
Copy link
Contributor
isuruf commented Jul 1, 2021

To fix linux-32bit test, try merging #14333 into this branch.

@isuruf
Copy link
Contributor
isuruf commented Jul 1, 2021

Now the issue is that gfortran-5 is used in the test. Need to change that to gfortran-8.

@rgommers
Copy link
Member
rgommers commented Jul 1, 2021

Now the issue is that gfortran-5 is used in the test. Need to change that to gfortran-8.

Why is this the problem? The only failure I see is numpy.distutils.system_info.NotFoundError: No BLAS/LAPACK libraries found., and that shouldn't be related to gfortran version.

We need one test with an old gfortran, otherwise we'll run into it at wheel build time. xref MacPython/scipy-wheels#111 which got stuck, we'd like to bump the minimum gfortran version, but 8.x is definitely too high (5.5 or 6.x seems like a good bump).

@isuruf
Copy link
Contributor
isuruf commented Jul 1, 2021 •
edited
Loading

Actual error is,

/usr/bin/ld: warning: libgfortran.so.5, needed by /usr/lib/gcc/i686-linux-gnu/5/../../../../lib/libopenblas.so, not found (try using -rpath or -rpath-link)
/usr/lib/gcc/i686-linux-gnu/5/../../../../lib/libopenblas.so: undefined reference to `_gfortran_concat_string@GFORTRAN_8'
/usr/lib/gcc/i686-linux-gnu/5/../../../../lib/libopenblas.so: undefined reference to `_gfortran_etime@GFORTRAN_8'
collect2: error: ld returned 1 exit status

That's because openblas-0.3.15 was built with gfortran 8 and you need libgfortran>=8

@rgommers
Copy link
Member
rgommers commented Jul 1, 2021

Ah yes, I managed to overlook that build log output, thanks.

That's because openblas-0.3.15 was built with gfortran 8 and you need libgfortran>=8

Hmm. I looked through the PRs on https://github.com/MacPython/openblas-libs and https://github.com/MacPython/openblas-libs. There's a bunch of upgrades, but it's not entirely clear to me if this was a necessary change.

If we can make our wheels work with gfortran>=8, then we can take an ATLAS build for testing an older gfortran. I'm not sure we can get away with >=8 given manylinux standards and pip versions out in the wild though.

@isuruf
Copy link
Contributor
isuruf commented Jul 11, 2021

Linux 32bit and windows 32 bit both fail with the same error now.

@judahrand
Copy link
Contributor Author

#14188 (comment)

Would bumping the tolerances of that test be an acceptable solution as suggested?

@rgommers
Copy link
Member

Rebased on master after the test refactor and 32-bit skips for complex128 were merged. Let's see if it's happier now.

isuruf
isuruf reviewed Aug 20, 2021
View reviewed changes
isuruf
isuruf reviewed Aug 20, 2021
View reviewed changes
isuruf
isuruf reviewed Aug 20, 2021
View reviewed changes
@judahrand
Copy link
Contributor Author

@judahrand, can you use gfortran 7 instead of 8 for both windows and linux?

Okay, shall revert these changes back to fortran-8.

isuruf
isuruf reviewed Aug 20, 2021
View reviewed changes
@judahrand @isuruf
Update top level openblas_version variable to 0.3.17
0c94d42 
Co-authored-by: Isuru Fernando <isuruf@gmail.com>
@tylerjereddy
Copy link
Contributor

The pre release azure failure should go away--I just merged in Ralf's fix for that a minute ago.

@rgommers rgommers added this to the 1.8.0 milestone Aug 20, 2021
rgommers
rgommers approved these changes Aug 20, 2021 A3E2
View reviewed changes
Copy link
Member
@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

All right, all green except for one unrelated CI issue that was fixed earlier today. It's time to merge this. Thanks a lot @judahrand and @isuruf!

@rgommers rgommers merged commit 75ef7db into scipy:master Aug 20, 2021
@rgommers rgommers mentioned this pull request Aug 20, 2021
Closed
@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Aug 20, 2021
@tylerjereddy
Copy link
Contributor

Labelled for backport, I think

@isuruf isuruf mentioned this pull request Aug 23, 2021
Merged
3 tasks
@tylerjereddy tylerjereddy modified the milestones: 1.8.0, 1.7.2 Oct 1, 2021
tylerjereddy pushed a commit to tylerjereddy/scipy that referenced this pull request Oct 1, 2021
@judahrand @isuruf @tylerjereddy
MAINT: Update openblas_support.py to support Apple Silicon (scipy#1…
423ec3d 
…4316)

This commit brings `scipy`'s implementation of this script into line
with `numpy`'s. This includes an update of the OpenBlas version as well
as support for `macosx-arm64`.

The source implementation can be found here:
https://github.com/numpy/numpy/blob/7ef28b2f1c383828bebcbb87b8e487e27c6e3ff9/tools/openblas_support.py

Co-authored-by: Isuru Fernando <isuruf@gmail.com>
@tylerjereddy tylerjereddy mentioned this pull request Oct 1, 2021
Merged
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Oct 3, 2021
This was referenced Oct 17, 2021
Merged
Closed
@matthew-brett matthew-brett mentioned this pull request Oct 28, 2021
Merged
@jjerphan jjerphan mentioned this pull request Nov 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isuruf isuruf isuruf left review comments

@rgommers rgommers rgommers approved these changes

@larsoner larsoner Awaiting requested review from larsoner

@tylerjereddy tylerjereddy Awaiting requested review from tylerjereddy

Assignees
No one assigned
Labels
maintenance Items related to regular maintenance tasks
Projects
None yet
Milestone
1.7.2
Development

Successfully merging this pull request may close these issues.
9 participants
@judahrand @tylerjereddy @rgommers @isuruf @ilayn @matthew-brett @zooba @inducer @mattip
0