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

py312-scipy: fix BLAS variants #21953

Closed
wants to merge 1 commit into from

Conversation

erikbs
Copy link
Contributor

@erikbs erikbs commented Dec 27, 2023

Description

Note: Originally a PR for adding py312 subport and switching build system to Meson for Python >= 3.12, I have reduced it to one that fixes the BLAS variants when building with Meson. I have therefore edited this comment.

I tested my original code with all combinations of subports py311 and py312 and variants +atlas, +openblas and +mkl, as well as -openblas (Accelerate). Tested by importing scipy and using functions from scipy.linalg.blas and scipy.linalg.lapack (exception for Accelerate, see below).

The rebased solution is tested with py311 (+atlas) and py312 (+atlas and default). The rebase only involved moving a few instructions that were common to all variants. The variant-specific code was untouched.

Note that I tested building the Accelerate variant on a system that lacks a couple of symbols (macOS 11 is the max version that my HW supports), but I verified that this variant is linked against the same libraries as the py311 subport and that it fails “the right way” (i.e., complains about missing BLAS symbols in Accelerate when I try to use BLAS functionality). Then I added a guard to the Portfile to make the installation fail on pre-fetch if attempting to link against Accelerate on macOS < 13.3.

Main changes:

  • I followed the recommendation in the SciPy docs on finding BLAS libraries, i.e., “craft your own pkg-config file. It […] may be located anywhere”. This was only necessary for ATLAS and MKL, and the only purpose of these .pc files is to let Meson know that the libraries exist and to provide the right linker flags (these flags could perhaps be set in the Portfile instead).
    • Meson uses CMake as fallback solution when pkg-config does not find the library. It was supposed to support an option cmake_module_path to dependency, which I thought I could use to point it to MKL’s CMake config file, but that did not work (log indicates that the option is ignored). Intel will axe macOS support in MKL with the first release in 2024, so I did not think it was a priority to look more into this, especially considering that I had to use the pkg-config file solution for ATLAS.
  • Set BLAS library using Meson build options instead of environment variables

Notes:

  • I explicitly link against ATLAS with -ltatlas and against MKL with -lmkl_rt.2 to match what was done before
  • For Cython etc. to work, I copied the solution from py-pywavelets, which is to add ${python.prefix}/bin to PATH during the build phase. I have replaced the symlink-based solution on master.

Potential problems:

  • From what I read, it is possible that Meson does not work on older releases of Mac OS X (especially PowerPC?). If this is the case, it must be restricted to versions older than OS X 10.9. I do not have a PowerPC system to test on. Per discussion below, the problem will not be relevant until the same issue is solved for py312-numpy.
Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.9.5 13F1911 x86_64
Xcode 6.2 6C131e

MKL variant and Accelerate:
macOS 11.6.2 20G314 x86_64
Xcode 13.2 13C90

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

Tests: There are no tests turned on in the Portfile.

Binary files: no binaries to test. Tested by importing scipy into the REPL and using a couple of functions instead

Variants: I have tested the various BLAS variants, but so far I have only tested one compiler variant properly (gfortran). The current SciPy version requires GCC >= 8, so variants gcc5, gcc6 and gcc7 do not work anyway.

On my system +gcc13 fails both for the new py312 subport and the old py311 subport (on libc++ stuff, it seems, not SciPy code). It seems to be because GCC tries to use libc++, but no matter what I do in the Portfile I cannot stop it from overwriting my CXXFLAGS with -stdlib=libc++. I had to manually cd to the build directory and import the environment variables, then set -stdlib=libstdc++ in CXXFLAGS instead and call the build command. Then it would finally pass the correct compiler flags and the build succeeded.

@macportsbot
Copy link

Notifying maintainers:
@michaelld for port py-scipy.

@macportsbot macportsbot added type: enhancement maintainer: open Affects an openmaintainer port labels Dec 27, 2023
@erikbs
Copy link
Contributor Author

erikbs commented Dec 27, 2023

The Cython problem can be solved by adding PATH=${python.prefix}/bin:$::env(PATH) to build.env (done in py-pywavelets). I have also submitted the CYTHON environment variable solution from py-numpy to Meson (PR).

python/py-scipy/Portfile Outdated Show resolved Hide resolved
@erikbs
Copy link
Contributor Author

erikbs commented Jan 6, 2024

Updated for Scipy 1.11.4. Ready for review once the new Pythran version lands, so that #21909 can be finished.

@erikbs erikbs force-pushed the feature/py312-scipy-meson branch 2 times, most recently from afe5f86 to fdd3381 Compare January 7, 2024 22:06
@erikbs erikbs mentioned this pull request Jan 7, 2024
10 tasks
@erikbs
Copy link
Contributor Author

erikbs commented Jan 7, 2024

Pushed an update that removes the Pythran < 0.15 requirement (it builds successfully with Pythran 0.15).

@erikbs erikbs marked this pull request as ready for review January 9, 2024 07:25
@erikbs
Copy link
Contributor Author

erikbs commented Jan 9, 2024

@barracuda156 If I understood you correctly, there is a build problem with Meson on PowerPC systems that makes Meson think it is cross-compiling, which does not work. Did you find a solution or workaround? If not, should I restrict the py312 subport to non-PowerPC?

@barracuda156
Copy link
Contributor

@erikbs For numpy there could be three solutions:

  1. Do not pass arch flags from the portfile (unneeded for native build), which bypasses a bug in meson.
  2. Use cross-file. (Should work with Macports meson, but bundled one lacks the needed cross-file.)
  3. Fix the issue in meson so that it understands that Power Macintosh is an alias for ppc, as it is done in cython.

I did not try to verify the build yet. Perhaps it does not help to restrict scipy, since until the issue is fixed, py312-numpy gonna fail to begin with. And fixing should be easy, at least locally for Macports.

@erikbs
Copy link
Contributor Author

erikbs commented Jan 9, 2024

Thanks! I agree that if numpy fails, there is no need to restrict scipy now.

@barracuda156
Copy link
Contributor

Thanks! I agree that if numpy fails, there is no need to restrict scipy now.

Perhaps the easiest fix will be not to pass archflags into environment if building for ppc/ppc64. Universal builds are broken anyway, so it has no practical relevance.
This is the view of upstream, AFAIU, that archflags are needed only for cross-builds. (I still think that passing archflags should not break anything, even if redundant, but upstream seems not to be concerned.)

@erikbs
Copy link
Contributor Author

erikbs commented Jan 12, 2024

The py312 subport has been added to master in the meantime, so I rebased this PR on @reneeotten’s changes on master. I have removed a few lines that turned out to be unnecessary in my initial code (setting envvars for F77 etc. and a reinplace) after comparing with master.

I used a different solution for finding pybind and cython than on master, but after reading this discussion I kept mine.

I will update the PR title and description since it now only adds the BLAS variants for py312.

@erikbs erikbs changed the title py-scipy: add py312 subport, build py>=312 with Meson py312-scipy: fix BLAS variants Jan 12, 2024
@reneeotten
Copy link
Contributor

reneeotten commented Jan 12, 2024

Last time I checked the corresponding variants in NumPy actually don't do what they should. So this cannot be merged as-is as that should be fixed first.

Additionally, we do not want to make new .pc files as the port(s)for these other variants already install them. I will have to test things locally to make sure everything works ; I do not feel comfortable merging this.

@reneeotten
Copy link
Contributor

reneeotten commented Jan 13, 2024

the immediate issue of not having a py312-scipy port has been resolved now. From the ports.macports.org status page it looks like it builds on (almost) all systems for which there are buildbots. So likely we should just switch everything to using the "meson" backend and then fix the variants in both NumPy and SciPy. There is no rush for that and it should be tested properly before we merge anything.

@erikbs
Copy link
Contributor Author

erikbs commented Jan 13, 2024

we do not want to make new .pc files as the port(s)for these other variants already install them

MKL does, but it installs several and I was unsure how to point pkgconfig to it. If I remember correctly, none of the .pc files actually matched what we wanted, but I will have to boot up macOS 11 to verify that.

ATLAS does not install a .pc file.

Edit:
Should mention that before pkgconfig I tried using envvars for CMake (Meson’s last resort). It worked, but not well – it would pick OpenBLAS if available and only pick ATLAS if I port deactivated OpenBLAS first. I went with the .pc file solution because that is what SciPy recommended. Except the linker flags, which can be moved to the Portfile if wanted, they are essentially dummy files that only serve the purpose of letting Meson/pkgconfig know that these BLAS libraries exist on the system (they do not specify paths). I am aware that no other Python port does it like this, so I agree that I would have preferred a different solution. I thought about setting the linker flags in the Portfile and generating atlas.pc and mkl.pc in worksrcdir with name: atlas and name: mkl as their only contents to make the solution behave more like a patch, but I was unsure if there really was any point in doing that compared to just supplying the files.

Considering that ATLAS only supports obsolete Mac OS X versions and that Intel just (understandably) killed MKL for Mac, I think it is unlikely that there will be put much effort into getting SciPy detect these libraries in a nicer way, so I guess some solution involving pkgconfig is the only option here, but I am absolutely open to adjusting this proposal. Perhaps there is a more clever way to add support for BLIS, I have not looked into that.

I agree that there is no rush. The most important thing for me personally is to have any variant of scipy working on my system, so that I could get scikit-image on 3.12 and ditch 3.11.

@barracuda156
Copy link
Contributor

@reneeotten If you allow, I can verify what fixes meson build for both on PowerPC (there should be a trivial fix to go around meson bug).

@barracuda156
Copy link
Contributor

@erikbs For BLAS variants: you may be interested to add support for FlexiBLAS and blis (they are available via linearalgebra PG now). If lapack is used here, then blis variant needs to use a separate lapack (which there is a port for).

@reneeotten reneeotten marked this pull request as draft January 13, 2024 14:27
@pmetzger
Copy link
Member

This has been sitting for quite a while now.

@barracuda156
Copy link
Contributor

It is needed to take care at least to avoid opportunistic linking to blis and flexiblas if those are not yet supported, otherwise bad things happen.

@pmetzger
Copy link
Member

If it's needed, it seems odd that everything has been okay for five months without it.

@barracuda156
Copy link
Contributor

More Python ports are likely to be affected, so it is either a python bug or MacPorts does not handle it correctly.
Another example: https://trac.macports.org/ticket/70105

@pmetzger
Copy link
Member

Regardless, this has now been sitting around waiting for quite a while. We could close it and it could be reopened when someone wanted to work on it.

@erikbs
Copy link
Contributor Author

erikbs commented May 29, 2024

To be honest I am unsure what needs to be done. This was my proposal to fix the existing BLAS variants on py312 that I tested on two OS versions, but from what I understood there was something that had to be fixed before this could be considered.

Regarding opportunistic linking, my changes do not introduce that possibility in more cases than the existing py311 solutions. Au contraire, if I remember correctly, the pkgconfig solution made the build fail if the requested BLAS library was absent. I also restricted linking to Apple’s BLAS library to macOS versions with a usable library (13.3+).

@barracuda156
Copy link
Contributor

barracuda156 commented May 31, 2024

@erikbs My concern re BLAS variants is that so far the build system ignored our settings and picked some BLAS version opportunistically. So you have a port built as +openblas but linked to blis etc.

If you consider it resolved (with active blis and flexiblas they are not linked to, but requested variant is actually used), I can test that.

There is no necessity to provide variants for those additional BLAS implementations (it is desirable but optional), but it is necessary to ensure that opportunistic linking does not happen. We will not see that on CI, that has to be tested locally.

P. S. Identical issue happens with py-numpy btw.

@pmetzger
Copy link
Member

pmetzger commented Jun 1, 2024

Failing some plan to finish this, I'd recommend that we close it and then it can be re-opened when there's something someone actually wants to commit.

@barracuda156
Copy link
Contributor

@pmetzger This is not up to me to decide, of course, but the problem is rather serious, IMO, and has to be addressed without postponing it indefinitely. It affects every platform and arch, AFAIK, nothing specific to legacy or exotic ones.

@pmetzger
Copy link
Member

pmetzger commented Jun 1, 2024

I don't doubt that the problem is real and important, but the purpose of the pull queue isn't to provide a ticketing system for tracking a problem but to track patches about to be imminently merged into the source tree. As there is nothing here to merge, the use of the pull queue to deal with this is inappropriate.

@pmetzger pmetzger closed this Jun 1, 2024
@pmetzger
Copy link
Member

pmetzger commented Jun 1, 2024

(BTW, arguably, MacPorts should migrate to using github for ticketing instead of trac as it's easier for most people to watch, but that's a distinct issue.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer: open Affects an openmaintainer port type: enhancement
7 participants