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

BLD: Switch order of test for lapack_mkl and openblas_lapack #7232

Merged
merged 1 commit into from
Feb 21, 2016
Merged

BLD: Switch order of test for lapack_mkl and openblas_lapack #7232

merged 1 commit into from
Feb 21, 2016

Conversation

rmalouf
Copy link
Contributor

@rmalouf rmalouf commented Feb 11, 2016

The check for openblas_lapack comes before the check for lapack_mkl in lapack_opt_info. This means that if openblas is installed on the system in one of the usual places, distutils will find it and use it even when mkl is specific in site.cfg. Reverse the order, so it checks mkl's lapack first and then openblas, lets it use mkl when requested. (See #6669)

@rmalouf rmalouf mentioned this pull request Feb 11, 2016
@pv
Copy link
Member

pv commented Feb 11, 2016

And what if you have mkl installed, but want to use openblas?

@rmalouf
Copy link
Contributor Author

rmalouf commented Feb 11, 2016

mkl wouldn't ordinarily be installed in /usr/lib, /usr/local/lib, or any of the other places the config code searches by default, so unless you specify the mkl install path in site.cfg, it's not going to find it. But yeah, I get your point, the fact that system_info tries to "do the right thing" when site.cfg is incomplete means that it'll probably do the wrong thing under some circumstances.

@rgommers
Copy link
Member

unless you specify the mkl install path in site.cfg, it's not going to find it.

Fixing this more completely by always preferring things specified in site.cfg over anything in the standard locations would require fairly significant surgery in system_info. Which is painful and very hard to test. So I'm tempted to just agree that switching the order here is an improvement.

@charris
Copy link
Member

charris commented Feb 21, 2016

system_info really needs a rewrite, but... painful.

@rgommers
Copy link
Member

system_info really needs a rewrite, but... painful.

I will send a follow-up PR with some doc improvements and cleanups, because I'm tired of re-discovering the wheel.

A full rewrite isn't actually that hard and could make it much simpler, but the big problem is that there are no tests. And there's a lot of state, so just some unit tests really aren't enough.

@charris
Copy link
Member

charris commented Feb 21, 2016

@rgommers So merge? I have no objections, but ISTM that one problem just gets exchanged for a less likely problem.

rgommers added a commit that referenced this pull request Feb 21, 2016
BLD: Switch order of test for lapack_mkl and openblas_lapack
@rgommers rgommers merged commit e5c1ac1 into numpy:master Feb 21, 2016
@rgommers
Copy link
Member

Yes, in it goes. Thanks @rmalouf

@rgommers rgommers added this to the 1.12.0 release milestone Feb 21, 2016
@rgommers
Copy link
Member

ISTM that one problem just gets exchanged for a less likely problem.

Indeed. The logic still needs a makeover at some point; it should respect site.cfg first of all and it doesn't do that now if multiple libs are installed in standard locations. But well, there's more that needs a makeover.....

@matthew-brett
Copy link
Contributor

I just ran into this one - I wanted to build scipy against ATLAS on a machine with openblas installed. I believe I would have to edit systeminfo.py to do this - is that right?

@rgommers
Copy link
Member

correct. really system_info should be reworked to always respect site.cfg first, but it doesn't do that. I have a partial cleanup of system_info somewhere, but didn't finish it (yet)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants