Change default blas_info dictionary in cmodule #444

lucianopaz · 2023-09-18T21:34:18Z

Closes #441

Motivation for these changes

numpy.__config__.get_info has been deprecated and removed, so we have to set default link libraries some other way.

Implementation details

This is a very rough first implementation. The idea is to use something like python's sys.path to look for libraries files (this would match a conda env lib folder and also the system level python lib folders, I still need to figure out what to do with venvs). Then look for some default libraries to link against, like mkl or openblas. I added LAPACK, but maybe the c code that is generated by pytensor doesn't have the proper includes to be able to link against that.

I haven't changed any other code yet, but the long term goal would be to remove most of the blas ldflag logic from the startup. In particular, I flagged a couple of platforms for deprecation (EPD and Canopy, which I think are no longer developed anyway)

Checklist

Explain motivation and implementation 👆
Make sure that the pre-commit linting/style checks pass.
Link relevant issues, preferably in nice commit messages.
The commits correspond to relevant logical changes. Note that if they don't, we will rewrite/rebase/squash the git history before merging.
Are the changes covered by tests and docstrings?
Fill out the short summary sections 👇

Major / Breaking Changes

default_blas_ldflags has changed and might give different default link flags. It no longer looks at numpy for blas information, but rather searches some common system paths to try to find BLAS libraries to link against. The order of priorities is 1) MKL, 2) OPENBLAS, 3) LAPACK, 4) BLAS

New features

...

Bugfixes

...

Documentation

...

Maintenance

...

pytensor/link/c/cmodule.py

lucianopaz · 2023-09-22T09:37:55Z

I've decided to completely change the default_blas_ldflags function. The past implementation was a mess. It tried to import mkl, had a bunch of patches here and there to try to work around old conda issues, support something called Canopy and EPD. I decided to simplify the logic a lot, so this might lead to breaks. The idea is the following:

Determine the search path of the compiler and linker
Use system tools to look at those directories and see if the required BLAS libraries are present or not
There's a list of priority libraries that are searched for. 1) MKL with Intel threading, 2) MKL with GNU threading, 3) openblas, 4) LAPACK, 5) plain and simple BLAS
If they are present, try to compile a simple program that links with BLAS libraries
1. If the compilation works handle some OMP stuff and finally return the ldflags. At this point I left some old logic about OMP thread handling in place because I didn't want to touch it, but in the future we could refactor it
2. If the compilation does not work, then move on to the next set libraries to link against
If no combination of libraries works, then return "", which means that pytensor didn't find a blas installation and will use the numpy C-API, which might be slow.

codecov-commenter · 2023-09-22T10:30:42Z

Codecov Report

Merging #444 (f0c1021) into main (36df379) will increase coverage by 0.08%.
Report is 4 commits behind head on main.
The diff coverage is 85.33%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #444      +/-   ##
==========================================
+ Coverage   80.66%   80.74%   +0.08%     
==========================================
  Files         160      160              
  Lines       46025    46007      -18     
  Branches    11266    11245      -21     
==========================================
+ Hits        37124    37148      +24     
+ Misses       6668     6635      -33     
+ Partials     2233     2224       -9

Files	Coverage Δ
pytensor/tensor/random/op.py	`96.25% <ø> (ø)`
pytensor/tensor/type.py	`94.52% <100.00%> (+0.04%)`	⬆️
pytensor/link/c/cmodule.py	`55.16% <91.80%> (+3.65%)`	⬆️
pytensor/link/c/exceptions.py	`41.66% <40.00%> (-38.34%)`	⬇️

... and 2 files with indirect coverage changes

pytensor/link/c/cmodule.py

twiecki · 2023-09-23T09:24:51Z

Seems like the function just moved to numpy.distutils.system_info.get_info("blas_opt")?

lucianopaz · 2023-09-24T06:52:25Z

Seems like the function just moved to numpy.distutils.system_info.get_info("blas_opt")?

The distutils module is deprecated and will be removed. Numpy is moving towards using meson as its build toolchain. That’s why I had to go with a different more long term solution for this problem

twiecki · 2023-10-06T16:43:00Z

This is with OSX & accelerate:

>>ipython
Python 3.11.4 | packaged by conda-forge | (main, Jun 10 2023, 18:08:41) [Clang 15.0.7 ]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.14.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import pytensor as pt
WARNING (pytensor.tensor.blas): Using NumPy C-API based implementation for BLAS functions.

In [2]: pt.__file__
Out[2]: '/Users/twiecki/projects/pytensor/pytensor/__init__.py'

In [3]: pt.config
Out[3]: <pytensor.configparser.PyTensorConfigParser at 0x109603610>

In [4]: pt.config.blas__ldflags
Out[4]: ''

lucianopaz · 2023-10-07T05:57:57Z

This is with OSX & accelerate:

>>ipython
Python 3.11.4 | packaged by conda-forge | (main, Jun 10 2023, 18:08:41) [Clang 15.0.7 ]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.14.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import pytensor as pt
WARNING (pytensor.tensor.blas): Using NumPy C-API based implementation for BLAS functions.

In [2]: pt.__file__
Out[2]: '/Users/twiecki/projects/pytensor/pytensor/__init__.py'

In [3]: pt.config
Out[3]: <pytensor.configparser.PyTensorConfigParser at 0x109603610>

In [4]: pt.config.blas__ldflags
Out[4]: ''

It’s not surprising that the flags fail to link to accelerate. I could add some platform and processor checks to this to actually only try mkl on Intel and to add the -framework flag on macOS

twiecki · 2023-10-07T12:18:19Z

I added a check for conda:

   # Check if in a Conda/Mamba environment
    conda_prefix = os.environ.get('CONDA_PREFIX')
    if conda_prefix:
        # Check for the existence of typical BLAS libraries in the Conda environment
        blas_libs_path = os.path.join(conda_prefix, 'lib')
        if os.path.exists(os.path.join(blas_libs_path, 'libblas.so')):
            return "-lblas"
        elif os.path.exists(os.path.join(blas_libs_path, 'libopenblas.so')):
            return "-lopenblas"
        elif sys.platform == 'darwin' and os.path.exists(os.path.join(blas_libs_path, 'libblas.dylib')):
            return "-lblas"
        # Add checks for other BLAS libraries as necessary

that correctly selects accelerate on my osx.

To discuss: If we don't support 3.12, maybe we should enforce this here in `pyproject.toml`? Source: <#441 (comment)> I'm not convinced that this is the right move since it might slightly hinder testing. But if we are explicit, then this seems to me like the correct place for it.

lucianopaz · 2023-10-15T20:09:48Z

@twiecki, can you try again now? The only difference was that I hadn't included the dylib extension into the default searched libraries.

twiecki · 2023-10-16T04:55:23Z

@lucianopaz That didn't help/fix the issue.

twiecki · 2023-10-16T05:37:54Z

It seems we're running into this issue: klee/klee#591

Changing extra_compile_flags=[f"-Wl,-rpath={rpath}"] if rpath is not None else [], to extra_compile_flags=[f"-Wl,-rpath,{rpath}"] if rpath is not None else [], fixes the issue.

(replacing = with a comma).

pytensor/link/c/cmodule.py

lucianopaz · 2023-10-16T08:12:14Z

(replacing = with a comma).

I didn't know that the clang linker complained about the equal sign but ld didn't. I pushed the edit and the rpath flag should work now

twiecki · 2023-10-16T08:46:24Z

@lucianopaz It now detects blas on OSX with clang. However, it prefers openblas over accelerate (3 over 5). I recommend to add a check specifically for how conda selects blas, as that covers 90% of the installs, and the subsequent check will work for other OS:

   # Check if in a Conda/Mamba environment
    conda_prefix = os.environ.get('CONDA_PREFIX')
    if conda_prefix:
        # Check for the existence of typical BLAS libraries in the Conda environment
        blas_libs_path = os.path.join(conda_prefix, 'lib')
        if os.path.exists(os.path.join(blas_libs_path, 'libblas.so')):
            return "-lblas"
        elif os.path.exists(os.path.join(blas_libs_path, 'libopenblas.so')):
            return "-lopenblas"
        elif sys.platform == 'darwin' and os.path.exists(os.path.join(blas_libs_path, 'libblas.dylib')):
            return "-lblas"
        # Add checks for other BLAS libraries as necessary

twiecki · 2023-10-16T08:47:37Z

Alternatively, I'm not sure what blas (5) implementation is referring to, can we just move that up?

twiecki · 2023-10-16T13:34:44Z

Ok, so I'll put BLAS ahead of openblas, but I'm not sure if that will ever get used as a default

It will on OSX:

>>python -c "import pytensor; print(pytensor.config.blas__ldflags)"
-L/Users/twiecki/micromamba/envs/pymc5/lib -llapack -lblas -lcblas -lm -Wl,-rpath,/Users/twiecki/micromamba/envs/pymc5/lib

twiecki · 2023-10-16T13:36:32Z

Have we tested on windows?

lucianopaz · 2023-10-16T13:37:14Z

Ok, so I'll put BLAS ahead of openblas, but I'm not sure if that will ever get used as a default

It will on OSX:
>>python -c "import pytensor; print(pytensor.config.blas__ldflags)"
-L/Users/twiecki/micromamba/envs/pymc5/lib -llapack -lblas -lcblas -lm -Wl,-rpath,/Users/twiecki/micromamba/envs/pymc5/lib

Sorry, I meant to say that I don't think that openblas will ever end up being used as a default.

twiecki · 2023-10-16T13:39:25Z

Sorry, I meant to say that I don't think that openblas will ever end up being used as a default.

Yes, that's what we want and what I was saying here:

You can see here: https://github.com/conda-forge/pytensor-suite-feedstock/pull/71/files that it's really only for non-standard OS. For Intel chips we use MKL for ARM64 we use accelerate. That covers 99%.

lucianopaz · 2023-10-16T14:05:41Z

Have we tested on windows?

I’ll test it

ricardoV94 · 2023-10-16T17:02:20Z

Do you need me to test nothing is changed for linux amd64 users (like me)?

twiecki · 2023-10-16T17:08:59Z

Do you need me to test nothing is changed for linux amd64 users (like me)?

Sure, the more testing the better. This one could cause quite some disruption if we get it wrong.

ricardoV94 · 2023-10-27T10:54:39Z

This is what I got before:

-L/home/ricardo/miniconda3/envs/pytensor/lib -lcblas -lblas -lcblas -lblas -Wl,-rpath,/home/ricardo/miniconda3/envs/pytensor/lib

And what I get after:

-L/usr/lib/x86_64-linux-gnu -L/home/ricardo/miniconda3/envs/pytensor/lib -L/usr/lib/gcc/x86_64-linux-gnu/9 -llapack -lblas -lcblas -lm -Wl,-rpath,/home/ricardo/miniconda3/envs/pytensor/lib

twiecki · 2023-10-27T11:18:50Z

Lgtm.

…

On Fri, Oct 27, 2023, 12:54 Ricardo Vieira ***@***.***> wrote: This is what I got before: -L/home/ricardo/miniconda3/envs/pytensor/lib -lcblas -lblas -lcblas -lblas -Wl,-rpath,/home/ricardo/miniconda3/envs/pytensor/lib And what I get after: -L/usr/lib/x86_64-linux-gnu -L/home/ricardo/miniconda3/envs/pytensor/lib -L/usr/lib/gcc/x86_64-linux-gnu/9 -llapack -lblas -lcblas -lm -Wl,-rpath,/home/ricardo/miniconda3/envs/pytensor/lib — Reply to this email directly, view it on GitHub <#444 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFETGDLGB7GXP2SDQHEDLLYBOHHVAVCNFSM6AAAAAA45K26F6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBSG4YTAMRSGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

twiecki · 2023-10-27T12:17:25Z

Actually, I don't think -L/usr/lib/x86_64-linux-gnu should show up there as it might override the env settings.

lucianopaz · 2023-10-27T12:45:04Z

Actually, I don't think -L/usr/lib/x86_64-linux-gnu should show up there as it might override the env settings.

That directory came from the compiler default link directories. That means that it is consistent with the env settings and the compiler configuration. Maybe the safest thing is to not include -L flags to folders that are already part of the compiler or linker’s search path, because they might affect the lookup order when linking.

twiecki · 2023-10-27T12:53:26Z

I guess conda doesn't provide all libraries and some linking still happens against OS libs?

twiecki · 2023-11-08T09:08:21Z

@lucianopaz Any thoughts on this? Should we just merge? CC @ricardoV94

lucianopaz · 2023-11-08T09:21:53Z

@lucianopaz Any thoughts on this? Should we just merge? CC @ricardoV94

I'll remove the compiler libs from the -L flags and then I think we can merge

lucianopaz · 2023-11-09T09:51:13Z

@ricardoV94, do you want to try this out on your AMD setup and see if the link flags work for you there?

ricardoV94 · 2023-11-09T10:09:30Z

@ricardoV94, do you want to try this out on your AMD setup and see if the link flags work for you there?

This is what I get now

-L/home/ricardo/miniconda3/envs/pytensor/lib -llapack -lblas -lcblas -lm -Wl,-rpath,/home/ricardo/miniconda3/envs/pytensor/lib

vs main:

-L/home/ricardo/miniconda3/envs/pytensor/lib -lcblas -lblas -lcblas -lblas -Wl,-rpath,/home/ricardo/miniconda3/envs/pytensor/lib

twiecki · 2023-11-09T10:36:17Z

@ricardoV94 that looks perfect to me

lucianopaz · 2023-11-09T10:43:23Z

@ricardoV94, LGTM

twiecki · 2023-11-09T11:25:36Z

Congrats on slaying this dragon @lucianopaz!

Time for a new release.

twiecki reviewed Sep 19, 2023

View reviewed changes

pytensor/link/c/cmodule.py Outdated Show resolved Hide resolved

twiecki reviewed Sep 19, 2023

View reviewed changes

pytensor/link/c/cmodule.py Outdated Show resolved Hide resolved

GStechschulte mentioned this pull request Sep 20, 2023

New ordered name bambinos/bambi#724

Merged

lucianopaz force-pushed the blas_info branch from e619f0b to d35f4c4 Compare September 22, 2023 09:29

matt-graham mentioned this pull request Sep 22, 2023

Tests collection failing due to error on importing pytensor matt-graham/mici#10

Closed

lucianopaz marked this pull request as ready for review September 22, 2023 09:37

lucianopaz requested a review from ricardoV94 September 22, 2023 09:38

lucianopaz force-pushed the blas_info branch from 8702bac to 97d0bd2 Compare September 22, 2023 09:58

ricardoV94 added release C-backend labels Sep 22, 2023

lucianopaz commented Sep 22, 2023

View reviewed changes

pytensor/link/c/cmodule.py Outdated Show resolved Hide resolved

ricardoV94 requested a review from ferrine September 24, 2023 11:44

ricardoV94 mentioned this pull request Oct 10, 2023

Extend support to Python 3.12 #472

Closed

lucianopaz force-pushed the blas_info branch from 97d0bd2 to 482b361 Compare October 15, 2023 20:08

twiecki reviewed Oct 16, 2023

View reviewed changes

pytensor/link/c/cmodule.py Outdated Show resolved Hide resolved

lucianopaz force-pushed the blas_info branch from 482b361 to 5d64688 Compare October 16, 2023 08:08

lucianopaz force-pushed the blas_info branch from aada5a7 to 7aa5188 Compare October 16, 2023 13:29

twiecki approved these changes Oct 16, 2023

View reviewed changes

lucianopaz added 2 commits November 8, 2023 10:32

Make default_blas_ldflags to not rely on numpy blas_info

6078345

Unpin numpy

f0c1021

lucianopaz force-pushed the blas_info branch from 7aa5188 to f0c1021 Compare November 8, 2023 09:32

twiecki approved these changes Nov 8, 2023

View reviewed changes

twiecki merged commit 893dc18 into pymc-devs:main Nov 9, 2023

lucianopaz deleted the blas_info branch November 9, 2023 18:19

twiecki mentioned this pull request Nov 16, 2023

blas IndexError: list index out of range error #504

Closed

ricardoV94 mentioned this pull request Nov 22, 2023

BUG: Regression in C blas flags detection #508

Closed

lucianopaz mentioned this pull request Dec 1, 2023

Add debug logging statements to cmodule blas #527

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default blas_info dictionary in cmodule #444

Change default blas_info dictionary in cmodule #444

lucianopaz commented Sep 18, 2023 •

edited

Loading

lucianopaz commented Sep 22, 2023

codecov-commenter commented Sep 22, 2023 •

edited

Loading

twiecki commented Sep 23, 2023

lucianopaz commented Sep 24, 2023

twiecki commented Oct 6, 2023

lucianopaz commented Oct 7, 2023

twiecki commented Oct 7, 2023 •

edited

Loading

lucianopaz commented Oct 15, 2023

twiecki commented Oct 16, 2023

twiecki commented Oct 16, 2023 •

edited

Loading

lucianopaz commented Oct 16, 2023

twiecki commented Oct 16, 2023

twiecki commented Oct 16, 2023

twiecki commented Oct 16, 2023

twiecki commented Oct 16, 2023

lucianopaz commented Oct 16, 2023

twiecki commented Oct 16, 2023

lucianopaz commented Oct 16, 2023

ricardoV94 commented Oct 16, 2023 •

edited

Loading

twiecki commented Oct 16, 2023

ricardoV94 commented Oct 27, 2023

twiecki commented Oct 27, 2023 via email

twiecki commented Oct 27, 2023

lucianopaz commented Oct 27, 2023

twiecki commented Oct 27, 2023

twiecki commented Nov 8, 2023

lucianopaz commented Nov 8, 2023

lucianopaz commented Nov 9, 2023

ricardoV94 commented Nov 9, 2023 •

edited

Loading

twiecki commented Nov 9, 2023

lucianopaz commented Nov 9, 2023

twiecki commented Nov 9, 2023

Change default blas_info dictionary in cmodule #444

Change default blas_info dictionary in cmodule #444

Conversation

lucianopaz commented Sep 18, 2023 • edited Loading

Motivation for these changes

Implementation details

Checklist

Major / Breaking Changes

New features

Bugfixes

Documentation

Maintenance

lucianopaz commented Sep 22, 2023

codecov-commenter commented Sep 22, 2023 • edited Loading

Codecov Report

twiecki commented Sep 23, 2023

lucianopaz commented Sep 24, 2023

twiecki commented Oct 6, 2023

lucianopaz commented Oct 7, 2023

twiecki commented Oct 7, 2023 • edited Loading

lucianopaz commented Oct 15, 2023

twiecki commented Oct 16, 2023

twiecki commented Oct 16, 2023 • edited Loading

lucianopaz commented Oct 16, 2023

twiecki commented Oct 16, 2023

twiecki commented Oct 16, 2023

twiecki commented Oct 16, 2023

twiecki commented Oct 16, 2023

lucianopaz commented Oct 16, 2023

twiecki commented Oct 16, 2023

lucianopaz commented Oct 16, 2023

ricardoV94 commented Oct 16, 2023 • edited Loading

twiecki commented Oct 16, 2023

ricardoV94 commented Oct 27, 2023

twiecki commented Oct 27, 2023 via email

twiecki commented Oct 27, 2023

lucianopaz commented Oct 27, 2023

twiecki commented Oct 27, 2023

twiecki commented Nov 8, 2023

lucianopaz commented Nov 8, 2023

lucianopaz commented Nov 9, 2023

ricardoV94 commented Nov 9, 2023 • edited Loading

twiecki commented Nov 9, 2023

lucianopaz commented Nov 9, 2023

twiecki commented Nov 9, 2023

lucianopaz commented Sep 18, 2023 •

edited

Loading

codecov-commenter commented Sep 22, 2023 •

edited

Loading

twiecki commented Oct 7, 2023 •

edited

Loading

twiecki commented Oct 16, 2023 •

edited

Loading

ricardoV94 commented Oct 16, 2023 •

edited

Loading

ricardoV94 commented Nov 9, 2023 •

edited

Loading