bodono / scs-python

Python interface for SCS
MIT License
41 stars 33 forks source link

Hopefully fixes builds on 3.12. #70

Closed enzbus closed 11 months ago

enzbus commented 1 year ago

Turns out extra numpy.distutils.get_info args were superflous, at least for plain builds.

bodono commented 1 year ago

Thanks for doing this Enzo! Does this still pick up the right blas / lapack libraries to link against?

enzbus commented 1 year ago

No :( I had a bug in my local compilation; I also tried to copy-paste the output of sys.info under python 3.11 and use it in python 3.12 but it didn't work. I suspect it's better to just rewrite the whole thing. (On another note, I noticed that the MacOS pypi package of scs was slower than the one I compiled on the test suite, so maybe the linkage was broken already.)On 7 Oct 2023, at 17:36, bodono @.***> wrote: Reopened #70.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you modified the open/close state.Message ID: @.***>

enzbus commented 1 year ago

Confirmed, linking was broken already. On python 3.11, clean environment.

pip install scs
otool -L env/lib/python3.11/site-packages/_scs_indirect.cpython-311-darwin.so 

gives:

env/lib/python3.11/site-packages/_scs_indirect.cpython-311-darwin.so:
    @loader_path/scs/.dylibs/libopenblasp-r0.3.21.dylib (compatibility version 0.0.0, current version 0.0.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.100.1)

While if I compile from source (inside scs-python)

pip install .
otool -L env/lib/python3.11/site-packages/_scs_indirect.cpython-311-darwin.so 

gives

env/lib/python3.11/site-packages/_scs_indirect.cpython-311-darwin.so:
    /System/Library/Frameworks/Accelerate.framework/Versions/A/Accelerate (compatibility version 1.0.0, current version 4.0.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.100.3)

on Mac amd64.

bodono commented 1 year ago

Confirmed, linking was broken already. On python 3.11, clean environment.

pip install scs
otool -L env/lib/python3.11/site-packages/_scs_indirect.cpython-311-darwin.so 

gives:

env/lib/python3.11/site-packages/_scs_indirect.cpython-311-darwin.so:
  @loader_path/scs/.dylibs/libopenblasp-r0.3.21.dylib (compatibility version 0.0.0, current version 0.0.0)
  /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.100.1)

While if I compile from source (inside scs-python)

pip install .
otool -L env/lib/python3.11/site-packages/_scs_indirect.cpython-311-darwin.so 

gives

env/lib/python3.11/site-packages/_scs_indirect.cpython-311-darwin.so:
  /System/Library/Frameworks/Accelerate.framework/Versions/A/Accelerate (compatibility version 1.0.0, current version 4.0.0)
  /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.100.3)

on Mac amd64.

Yes I also noticed that SCS was much slower from pip than when compiling locally, but I haven't had the bandwidth to investigate it unfortunately!

bodono commented 1 year ago

Thanks again for looking at this @enzbus and @rgommers ! Fingers crossed this works and can replace the very hacky code that was there before.

enzbus commented 1 year ago

@bodono @rgommers, current status: Basic build works, but there's no linking to blas in the compiled, so I'm probably not giving the right argument to py.extension_module. Also, right now pip installs _scs_direct.so but not the scs/__init__.py that is required to use it. Any hint?

rgommers commented 1 year ago

I'm just working on BLAS/LAPACK improvements in NumPy, so I can propose something. Rather than make it complicated now with 64-bit integer support (which seems incomplete?), the new Accelerate version for macOS >=13.3 (see note here), etc., maybe do only the basic setup first. Once NumPy has shipped the fancy version (auto-detect every option, full ILP64, switches for order of preference of known libraries, etc.) it can then be updated to mirror NumPy.

Sounds like you want regular 32-bit Accelerate on macOS preferably? And OpenBLAS or MKL elsewhere?

rgommers commented 1 year ago

but not the scs/__init__.py that is required to use it

You are missing a py.install_sources() to specify what to install - see at the bottom of numpy/scipy's meson.build files for examples. Alternatively, if you have a subdir with no compiled code and you want to install everything in that subdir, you can use install_subdir().

enzbus commented 1 year ago

I'm just working on BLAS/LAPACK improvements in NumPy, so I can propose something. Rather than make it complicated now with 64-bit integer support (which seems incomplete?), the new Accelerate version for macOS >=13.3 (see note here), etc., maybe do only the basic setup first. Once NumPy has shipped the fancy version (auto-detect every option, full ILP64, switches for order of preference of known libraries, etc.) it can then be updated to mirror NumPy.

Sounds like you want regular 32-bit Accelerate on macOS preferably? And OpenBLAS or MKL elsewhere?

That would be great, and I guess it can be done by if/else on dependency.found(). I'm just trying to get a basic build now, and it's not linking BLAS after giving this output to meson compile

Program python3 found: YES (/usr/local/opt/python@3.11/bin/python3.11)
Library openblas found: NO
Library blas found: YES
Library cblas found: YES

Thoughts? And thanks!!

rgommers commented 1 year ago

I just tried meson setup build in my default SciPy dev env and got:

...
Program python3 found: YES (/home/rgommers/mambaforge/envs/scipy-dev/bin/python3.10)
Library openblas found: YES
Found pkg-config: /home/rgommers/mambaforge/envs/scipy-dev/bin/pkg-config (0.29.2)
Build targets in project: 2

You probably don't have pkg-config installed? What's happening is that for dependency('pkgname'), when Meson isn't aware of and has special handling for pkgname, it queries first pkg-config and then CMake to detect the dependency.

rgommers commented 1 year ago

Here are a couple of minor fixes, for a missing git submodule and removing -O3 (the commit message explains why): https://github.com/rgommers/scs-python/tree/meson-fixes.

The other compile args look fine and specific to SCS; when quickly checking what -DPYTHON did, I did notice a bug in SCS that you may want to fix: Python.h should always be included before any C standard library headers (see these CPython docs, and that's not the case in include/glbopts.h.

Having a closer look at BLAS now.

enzbus commented 1 year ago

I just tried meson setup build in my default SciPy dev env and got:

...
Program python3 found: YES (/home/rgommers/mambaforge/envs/scipy-dev/bin/python3.10)
Library openblas found: YES
Found pkg-config: /home/rgommers/mambaforge/envs/scipy-dev/bin/pkg-config (0.29.2)
Build targets in project: 2

You probably don't have pkg-config installed? What's happening is that for dependency('pkgname'), when Meson isn't aware of and has special handling for pkgname, it queries first pkg-config and then CMake to detect the dependency.

Can you check if the compiled artifacts (_scs_direct....so and _scs_indirect....so) are actually linked to blas? On my build they aren't (and it says it finds blas/cblas).

rgommers commented 1 year ago

Ah they aren't indeed - but looking in build/build.ninja I do see the expected -lopenblas linker flag. So probably the SCS code is missing some macro that it expects to be defined to actually use BLAS.

rgommers commented 1 year ago

Indeed, adding '-DUSE_LAPACK=1' to c_args fixes that.

enzbus commented 1 year ago

Indeed, adding '-DUSE_LAPACK=1' to c_args fixes that.

It works! You're a savior.

rgommers commented 1 year ago

One more fix, to make the build output warning-free: https://github.com/rgommers/scs-python/commit/321755c09f748ebb18b82b07d3914badae2dcce1

rgommers commented 1 year ago

There's a lot of places to look when something is off, none of which distutils provides, so maybe this helps for diagnostics:

enzbus commented 1 year ago

Ok, great!!!! We do have working pip packages for 3.12 now, apparently. @bodono, please review and think of how to proceed: least deviation would be to merge this in a branch of this repo that only builds PyPI packages for 3.12. However the current build is broken or at least (very)hard-to-maintain. Your call.

enzbus commented 1 year ago

There's a lot of places to look when something is off, none of which distutils provides, so maybe this helps for diagnostics:

  • builddir/build.ninja is the best place to look for the full compiler/linker invocations
  • builddir/meson-logs/meson-log.txt is quite useful when there's an issue with some check (for dependencies, supported compiler args, etc.)
  • for a nicely formatted "build plan", meson introspect build --targets -i > build-details.json is helpful

Thanks so much!!! I see that meson supports all sort of conditional builds, so if @bodono wants to continue support all the variations he has developed it looks like a great way forward.

rgommers commented 1 year ago

Yes, the support for CLI options is good - you can easily add that in a meson.options file and then read them inside meson.build with get_option().

Here is one more, to default to Accelerate on macOS: https://github.com/rgommers/scs-python/tree/use-accelerate. I checked that it builds and links correctly - I didn't run the tests.

rgommers commented 1 year ago

Both using numpy and using BLAS/LAPACK should get nicer over the coming months, but it's already pretty nice compared to numpy.distutils I'd say:)

enzbus commented 1 year ago

Yes, the support for CLI options is good - you can easily add that in a meson.options file and then read them inside meson.build with get_option().

Here is one more, to default to Accelerate on macOS: https://github.com/rgommers/scs-python/tree/use-accelerate. I checked that it builds and links correctly - I didn't run the tests.

Tests pass on local build on 3.12 (and it's linked to /System/Library/Frameworks/Accelerate.framework/Versions/A/Accelerate!)

enzbus commented 1 year ago

Hello @bodono , I also edited the github yaml; strategy is to use OLD_setup.py to test alternative builds (mkl, openmp) with existing code, and use PyPA build for the builds. I strongly suspect that the vast majority of the yaml code is superflous, including all the hacks for cross compilation, the conda installs, etc. It's very hard to debug, I think you should trim it to keep this repo maintainable.

bodono commented 1 year ago

Thanks for doing this Enzo. I think I managed to update the settings so that you can run the checks now, rather than waiting for me to do it manually.

bodono commented 1 year ago

Hello @bodono , I also edited the github yaml; strategy is to use OLD_setup.py to test alternative builds (mkl, openmp) with existing code, and use PyPA build for the builds. I strongly suspect that the vast majority of the yaml code is superflous, including all the hacks for cross compilation, the conda installs, etc. It's very hard to debug, I think you should trim it to keep this repo maintainable.

Yes I would love to simplify it if possible! I can try that in follow-up work if you want.

enzbus commented 1 year ago

Ok I'll give a try at debugging the yaml as well. Isn't anybody in cvxpy (@SteveDiamond , ...) concerned about making 3.12 packages or are they only worrying about ruff and callback parameters?

bodono commented 1 year ago

Ok I'll give a try at debugging the yaml as well. Isn't anybody in cvxpy (@SteveDiamond , ...) concerned about making 3.12 packages or are they only worrying about ruff and callback parameters?

Indeed I started looking into this because cvxpy is trying to release 3.12 packages now.

enzbus commented 1 year ago

Ok I'll give a try at debugging the yaml as well. Isn't anybody in cvxpy (@SteveDiamond , ...) concerned about making 3.12 packages or are they only worrying about ruff and callback parameters?

Indeed I started looking into this because cvxpy is trying to release 3.12 packages now.

Then you should take it from here, I don't want to make too many changes in your project.

bodono commented 1 year ago

Ok I'll give a try at debugging the yaml as well. Isn't anybody in cvxpy (@SteveDiamond , ...) concerned about making 3.12 packages or are they only worrying about ruff and callback parameters?

Indeed I started looking into this because cvxpy is trying to release 3.12 packages now.

Then you should take it from here, I don't want to make too many changes in your project.

Ok, thanks for getting this started @enzbus! I will try and take over from here, I'm pretty swamped at the moment and going on paternity leave next week, but hopefully I can get this in soon.

enzbus commented 11 months ago

Hello @bodono, I got linux and mac builds working on github, but I'm afraid windows builds require more work. If I understand correctly, you used setup.py to copy the conda-compiled openblas.so into SCS' wheels. I'm not sure if meson supports that approach (which I wouldn't like either). In any case, meson is not picking up the location of the conda-installed openblas, and I don't have access to a windows development machine to figure out what's wrong. You can however start shipping mac and linux packages, I would suggest compiling them locally rather than relying on gh for now.

enzbus commented 11 months ago

Here's how you do it. Install brew (or whatever, to get a python3.12 -> https://docs.brew.sh/Installation)

brew install python@3.12
git clone https://github.com/enzbus/scs-python
cd scs-python
git submodule update --init
python3.12 -m venv env
source env/bin/activate
pip install .
pip install pytest
pytest
pip install build
python -m build
pip install twine
twine check dist/*
twine upload --skip-existing dist/*

the last step will ask you for your PyPI username and password. Repeat on mac (ideally amd64 and aarch64) and linux. This will upload compiled wheels for mac and linux and the source distribution for python 3.12 only. Then, at least, if a py3.12 windows user tries to install cvxpy (people are starting to notice https://github.com/cvxpy/cvxpy/issues/2269 ) it will work IF they have a working blas findable, or someone will pitch in. I've also given you write access to my branch so you can make changes directly to this PR.

rgommers commented 11 months ago

Re vendoring the openblas shared library, the standard tool to do that is delvewheel. It's pretty painful to deal with this vendoring though, especially on Windows (because of the lack of RPATH support).

For NumPy and SciPy we're vendoring an OpenBLAS version that we build ourselves (here), and that includes the necessary libgfortran.so and libquadmath.so. Right now it's messy to get at the relevant tarballs, but we're in the process of turning those into wheels so you then should be able to do:

pip install scipy-openblas
python -c "import scipy_openblas as s; s.write_pkgconfig_file()"
# build your package, where `dependency('openblas')` will now work with the .pc file from the line above
delvewheel xxx # vendors relevant DLLs

That may take some weeks/months to materialize there - could be sooner, but hard to predict.

enzbus commented 11 months ago

Thanks again for sharing your knowledge @rgommers !

SCS only uses one or two Lapack functions (eigendecompositions). Do you think it would make sense to put openblas as a submodule and tweak the meson.build to find them and compile them into SCS directly (we only need the relevant codepaths), as a fallback if no linkable Blas is found? I guess that would simplify distributing a lot.

(I guess this is relevant here -> https://mesonbuild.com/CMake-module.html)

rgommers commented 11 months ago

Do you think it would make sense to put openblas as a submodule ...

Answered at https://github.com/bodono/scs-python/issues/69#issuecomment-1778816259

bodono commented 11 months ago

Sorry for the delay in responding here, we just had a baby last week!

Is this ready to merge (without the Windows version)? Or should I just patch in and manually release wheels for mac / linux?

enzbus commented 11 months ago

Congrats @bodono !!! Yes, it's very close. For mac it's all good. For Linux and Windows we need to link openblas statically to mimic the way it worked before. In the case of windows, we need to find the exact location of openblas.dll installed by conda, and pass it to meson.

enzbus commented 11 months ago

@bodono : status. Blas is linked statically when building on github CI (via a meson option passed to the command line). Still no luck for windows, but I think we're super close. If someone has a windows development machine they can probably fix it easily, see lines 20-31 of meson.build. If I were you I would start shipping Linux/Mac packages (or at least the source distribution), for example by:

Here's how you do it. Install brew (or whatever, to get a python3.12 -> https://docs.brew.sh/Installation)

brew install python@3.12
git clone https://github.com/enzbus/scs-python
cd scs-python
git submodule update --init
python3.12 -m venv env
source env/bin/activate
pip install .
pip install pytest
pytest
pip install build
python -m build
pip install twine
twine check dist/*
twine upload --skip-existing dist/*
enzbus commented 11 months ago

It builds and tests on windows!!! @rgommers have a look, we're using the same technique @bodono devised , with a few meson tweaks :)

bodono commented 11 months ago

Amazing work @enzbus ! Thanks again for doing this, drinks on me next time you're in London!

bodono commented 11 months ago

It looks like the github runners are no longer able to schedule on macos-10.15, I think we can change that to macos-latest, which should schedule easier and hopefully run successfully now. I can do it in a PR that you can merge, or you can do it here if you prefer.

enzbus commented 11 months ago

It looks like the github runners are no longer able to schedule on macos-10.15, I think we can change that to macos-latest, which should schedule easier and hopefully run successfully now. I can do it in a PR that you can merge, or you can do it here if you prefer.

OK, that explains why the MacOS jobs weren't running. As you prefer, I have limited time but would like to clean it a bit (it doesn't seem urgent, nobody replied on https://github.com/cvxpy/cvxpy/issues/2269 ). Ideally we can also migrate the optional builds (openmp, cuda) to meson and reduce the size of the github yaml file. (We only need conda for installing openblas on windows, etc.) Just a note, meson doesn't support python <= 3.7, so if we eliminate the old setup.py we'll have to drop python 3.7 (but the wheels and source dists will remain on PyPI).

rgommers commented 11 months ago

Just a note, meson doesn't support python <= 3.7, so if we eliminate the old setup.py we'll have to drop python 3.7 (but the wheels and source dists will remain on PyPI).

In practice that makes it harder to do Python 3.5-3.6, but not impossible. Meson by design does not depend on it being implemented in Python, it has its own DSL and you invoke it via a meson executable that's on PATH. So it is possible to install Meson in, say, a Python 3.11 env, prepend its bin directory to PATH, and then use it in a non-isolated build from an env with an older Python version.

That said, meson-python is tied to Python version too (and is at >=3.7), and avoiding that constraint is much harder. So in practice, yes, >=3.7 is the limit now.

bodono commented 11 months ago

All checks have passed! I will merge as soon as you let me know it's ready @enzbus .

enzbus commented 11 months ago

Ok, @bodono, I approve too. Please go on and merge, hopefully there won't be issues when the final build and upload is triggered.