Skip to content

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Aug 11, 2024

Update to the newly released OpenBLAS 0.3.28. Note this uses the "shrunk" x86_64 and i686 OpenBLAS shared object with fewer kernels. The full shared object is available in the scipy-openblas 0.3.28.0.0 wheels.

For the OpenBLAS changelog see https://github.com/OpenMathLib/OpenBLAS/blob/develop/Changelog.txt. This PR also fixes up the bundled licenses:

  • fix the File: to the correct name
  • remove mention of quadmath from windows, it is no longer linked in, and the lack of linking is verified in the scipy-openblas wheel build via checking the linker map.

@mattip mattip marked this pull request as draft August 12, 2024 04:49
@mattip
Copy link
Contributor Author

mattip commented Aug 12, 2024

The wheels aren't actually smaller, see MacPython/openblas-libs#180. Marking this as draft until smaller wheels are available.

@andyfaff
Copy link
Contributor

I checked that a local wheel build (macOS M3) did not want to bundle libquadmath.

  • the macos scipy_openblas32 aarch64 wheel does not contain a libquadmath.
  • the linux scipy_openblas32 x86_64 wheel does contain a libquadmath.
  • the linux scipy_openblas32 aarch64 wheel does not contain a libquadmath.

I didn't check other wheels.

I think it might be safest to make wheels for this and check that they don't contain libquadmath

@mattip
Copy link
Contributor Author

mattip commented Aug 12, 2024

Strange. It seems none of the aarch64 scipy-openblas wheels bundle quadmath. I wonder if this is because they are compiled inside a qemu docker, or if gfortran on aarch64 does not make quad math available.

Edit: checking the manylinux2014 docker image: on aarch64 libgfortran is compiled without libquadmath.

@mattip
Copy link
Contributor Author

mattip commented Aug 12, 2024

@andyfaff good catch!

@mattip
Copy link
Contributor Author

mattip commented Aug 12, 2024

Also, AFIACT, macos arm64 does not bundle openblas. It does bundle libgcc_s, libgfortran and libquadmath.

@andyfaff
Copy link
Contributor

andyfaff commented Aug 12, 2024

Also, AFIACT, macos arm64 does not bundle openblas. It does bundle libgcc_s, libgfortran and libquadmath.

Do you mean the scipy_openblas32 wheel, or the scipy wheel? When I built a scipy wheel against scipy_openblas32 none of the dylibs depended on libquadmath. Nor could I see libquadmath in the scipy_openblas wheel.

@andyfaff
Copy link
Contributor

Perhaps we should add a post wheel test, to check that there's no libquadmath in scipy.libs.

@mattip
Copy link
Contributor Author

mattip commented Aug 12, 2024

Do you mean the scipy_openblas32 wheel, or the scipy wheel?

The scipy-1.14.0-*-macosx_14_0_arm64.whl wheel, like this one, which has shared objects in scipy/.dylibs.

Perhaps we should add a post wheel test, to check that there's no libquadmath in scipy.libs.

Only on windows do we statically link without libquadmath, otherwise it is dynamically linked and "whatever OpenBLAS and f2py needs". I think the easiest way forward is to add a conditional to the LICENCE*.txt files in tools, with may contain.

@mattip mattip marked this pull request as ready for review August 12, 2024 16:17
@mattip
Copy link
Contributor Author

mattip commented Aug 12, 2024

I updated the version of scipy-openblas32 to use the smaller wheels, and added a qualifier to the bundled license text. Another option would be to use separate license fragments for linux x86_64/aarch64 and macos x86_64/arm64, so there would be 5 fragments.


This binary distribution of SciPy also bundles the following software:
This binary distribution of SciPy can also bundle the following software
(depending on the build):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly needed, since there are no build variants on windows but keeps this file in sync with the others.

Copy link
Member

Choose a reason for hiding this comment

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

That seems okay to me, either way works.

@lucascolley lucascolley changed the title BUILD: use OpenBLAS v0.3.28 with fewer kernels, fix OpenBLAS licences BLD: use OpenBLAS v0.3.28 with fewer kernels, fix OpenBLAS licences Aug 12, 2024
@lucascolley lucascolley added Build issues Issues with building from source, including different choices of architecture, compilers and OS maintenance Items related to regular maintenance tasks labels Aug 12, 2024
@rgommers rgommers added this to the 1.15.0 milestone Aug 13, 2024
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.

I ran a full set of wheel builds to be sure - they're all happy (the one failure is a known and unrelated issue).

The changes LGTM. I'll hold off on merging in case @andyfaff still has questions/concerns.

@andyfaff
Copy link
Contributor

I think it's fine by me. I was mainly concerned that we have the licences correct.

On the whole I wish that we didn't have to bundle libquadmath though.

@andyfaff andyfaff merged commit ca02df7 into scipy:main Aug 14, 2024
@rgommers
Copy link
Member

On the whole I wish that we didn't have to bundle libquadmath though.

I'm not sure we can get rid of it on Linux and macOS as well with the same approach as Matti took for OpenBLAS. We may though, since it's the same problem of a single symbol being pulled in by gfortran. @mattip does that sound right to you?

@mattip
Copy link
Contributor Author

mattip commented Aug 14, 2024

I agree it would be nice to do. I am trying some experiments, let's see what happens.

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 maintenance Items related to regular maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants