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

spkg-configure.m4 for $(BLAS) (i.e. atlas and openblas) #27870

Closed
dimpase opened this issue May 25, 2019 · 85 comments
Closed

spkg-configure.m4 for $(BLAS) (i.e. atlas and openblas) #27870

dimpase opened this issue May 25, 2019 · 85 comments

Comments

@dimpase
Copy link
Member

dimpase commented May 25, 2019

one needs to understand the existing design first.

  • (as far as I can see) currently Sage generates *.pc files for pkg-config, for blas, cblas, and lapack. This is done for Atlas (and for external blas/lapack fed in by SAGE_ATLAS_LIB ?). For openblas these *.pc files are basically all point to the same library.

  • how are they used? Are they all used?

  • is the description of SAGE_ATLAS_LIB outdated? I cannot imagine any need for f77 libraries in 2019...

Depends on #28883

CC: @embray @vbraun @sagetrac-tmonteil @mezzarobba @orlitzky @EmmanuelCharpentier @kiwifb @isuruf @mkoeppe

Component: build: configure

Author: Dima Pasechnik

Branch: 81bf522

Reviewer: Isuru Fernando

Issue created by migration from https://trac.sagemath.org/ticket/27870

@dimpase dimpase added this to the sage-8.8 milestone May 25, 2019
@embray
Copy link
Contributor

embray commented May 31, 2019

comment:1

Yes, we should probably get rid of some of the ATLAS-specific stuff (or re-appropriate it). The thing about a lot of the ATLAS stuff is that it doesn't actually care that much about having ATLAS BLAS; the way much of it is written it will work if it can find some BLAS anywhere. That's why on Cygwin it works when you install the lapack package, because it comes with a built-in netlib BLAS. It has nothing to do with ATLAS.

It would make sense to have an option to look for openblas if it exists, or else fall back on a generic BLAS if one can be found via various sources (pkg-config, some autoconf tests, etc.).

@embray
Copy link
Contributor

embray commented Jun 14, 2019

comment:2

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

@embray embray removed this from the sage-8.8 milestone Jun 14, 2019
@dimpase
Copy link
Member Author

dimpase commented Dec 10, 2019

Commit: b446427

@dimpase
Copy link
Member Author

dimpase commented Dec 10, 2019

comment:5

Debian has all the needed .pc files, so there it appears to work.


New commits:

b446427a bare minumim openblas config

@dimpase
Copy link
Member Author

dimpase commented Dec 10, 2019

@dimpase dimpase added this to the sage-9.1 milestone Dec 10, 2019
@embray
Copy link
Contributor

embray commented Dec 12, 2019

comment:7

That's certainly better than nothing. There should probably also be a check in REQUIRED-CHECK (the third argument to SAGE_SPKG_CONFIGURE) to set sage_require_openblas=no if $with_blas != openblas). This way it won't check for openblas if someone configured Sage with --with-blas=atlas.

@dimpase
Copy link
Member Author

dimpase commented Dec 12, 2019

comment:8

One can only check for openblas with pkg-config and make blas.pc and lapack.pc just as copies of openblas.pc.

But I don't know how to look for openblas.pc to copy, other than doing Unix find /usr/ - something that is neither robust nor fast.

@dimpase
Copy link
Member Author

dimpase commented Dec 12, 2019

comment:9

One can fill in templates for blas.pc by calling pkg-config on openblas, but this looks like an ugly hack...

@dimpase
Copy link
Member Author

dimpase commented Dec 12, 2019

comment:10

OK, in fact one get the location of openblas.pc file:

pkg-config --variable=pcfiledir openblas

(this works), and the corresponding autoconf macro (not tested yet):

 PKG_CHECK_VAR([OPENBLASPCDIR], [openblas], [pcfiledir],
               [ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2019

Changed commit from b446427 to cb725d3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

22300f6use openblas.pc for blas and lapack, don't require atlas is to be used
cb725d3move blas handling to build/pkgs/

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2019

comment:12

this is a minimal fix for the use of system's openblas, identifiable from pkg-config.

I have not touched Atlas installation procedure.

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2019

Author: Dima Pasechnik

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Dec 13, 2019

comment:13

Well... I did git trac checkout 27870, then make, and make went directly to checking the documentation.

I guess that testing your patch involves either make clean or make distclean. Which one ?

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2019

comment:14

Replying to @EmmanuelCharpentier:

Well... I did git trac checkout 27870, then make, and make went directly to checking the documentation.

I guess that testing your patch involves either make clean or make distclean. Which one ?

Not only this :-)

make openblas-clean # uninstall package
./bootstrap # update ./configure
./configure # here the output should show that openblas is not being installed
make build

on OSX for some reason I also needed to ./sage -f numpy before make build

make distclean before bootstrap is more robust. I'm not sure in which cases one should re-install all the packages that depend upon blas, too, this might be
platform-dependent.


One naturally is interested in testing against system's openblas: e.g. on debian and derived systems one needs apt install pkg-config libopenblas-dev

On OSX's Homebrew one would need brew install openblas and then export certain
environment variable (as Homebrew will tell you), as for some reason it's not installed into a default location.

@orlitzky
Copy link
Contributor

comment:15

There is a larger mess to deal with here, but TBH I'm fine with solving it one piece at a time. It looks like a few packages (R, numpy,... ?) do use pkg-config to discover information about the blas/lapack being used. And our pkg-config is hacked (build/pkgs/pkgconf/patches/pkg-config.in) to look in a custom directory for .pc files if they aren't found on the system. So I think the idea was that if pkg-config couldn't find e.g. lapack.pc, that it would then look in SAGE_LOCAL to find it.

This only raises more questions. If openblas is installed on the system, can we count on the system pkg-config to know that the system blas/lapack are just the system openblas? If so, then we shouldn't need to create "fallback" symlinks. If not, would linking to some other blas/lapack cause problems? Why don't those packages check for openblas or atlas directly instead of "blas" and "lapack", in that case? And so on. Your approach looks better the deeper I go.

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2019

comment:16

I more or less copy the logic of openblas/dpkg-install - which does install openblas.pc under 3 different names - openblas, blas, and lapack.

@jhpalmieri
Copy link
Member

comment:17

I tried this (meaning make distclean; ./bootstrap; make) with an Ubuntu virtual machine. config.log says

configure:20206: result:     atlas-3.10.2.p3 will not be installed (configure check)
...
configure:20206: result:     openblas-0.3.6.p0 will not be installed (configure check)

but scipy installation fails with

numpy.distutils.system_info.NotFoundError: No lapack/blas resources found.

There are also various warnings in the numpy installation log about not finding atlas or blas.

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2019

comment:18

scipy depends on numpy to supply blas interface, so something goes wrong with numpy installation (it is weird that it foes not fail).

it works for me on Debian 10. numpy has its own extermely convoluted configure routine, which emits a lot of noise about not finding this or that.

Could you try building numpy with SAGE_CHECK ?

@orlitzky
Copy link
Contributor

comment:19

Replying to @dimpase:

I more or less copy the logic of openblas/dpkg-install - which does install openblas.pc under 3 different names - openblas, blas, and lapack.

Yeah I agree with that. Maybe two years from now, no one is using atlas any more and this is easy to clean up by dropping support for it and by having those other packages call pkg-config openblas... instead of pkg-config lapack and pkg-config blas. No need to worry about it now.

@orlitzky
Copy link
Contributor

comment:20

Replying to @jhpalmieri:

There are also various warnings in the numpy installation log about not finding atlas or blas.

FWIW numpy is one of those packages that uses pkg-config to find these libraries; see build/pkgs/numpy/lapack_conf.py.

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2019

comment:21

Apart from Atlas and openblas there are other perfectly ok implementations of blas and lapack, which may be used.

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2019

comment:22

e.g. there are highly optimised by hardware vendors blas and lapacks (e.g. MKL)

@vbraun
Copy link
Member

vbraun commented Dec 22, 2019

Changed branch from 6341576 to u/dimpase/packages/openblas/openblasconf

@vbraun
Copy link
Member

vbraun commented Dec 22, 2019

Commit: 6341576

@vbraun
Copy link
Member

vbraun commented Dec 22, 2019

New commits:

e5c91dbupdate pkgconfig to v1.5.1. See trac #28883
1720b38a bare minumim openblas config
925266buse openblas.pc for blas and lapack, don't require atlas is to be used
b89df80move blas handling to build/pkgs/
6341576create cblas.pc link, too, not only blas and lapack

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2019

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

f74f554update pkgconfig to v1.5.1. See trac #28883
fb8f4e5allow absent zlib.pc
b3a8891a bare minumim openblas config
c031c9euse openblas.pc for blas and lapack, don't require atlas is to be used
b49f79emove blas handling to build/pkgs/
81bf522create cblas.pc link, too, not only blas and lapack

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2019

Changed commit from 6341576 to 81bf522

@dimpase
Copy link
Member Author

dimpase commented Dec 28, 2019

comment:70

trivial rebase over updated #28883

@embray
Copy link
Contributor

embray commented Jan 8, 2020

comment:72

Currently doesn't detect system openblas on Cygwin (which just gets installed as the generic libblas when you install it). Not sure why yet. I won't hold up this ticket over it but if I can find a quick fix I might like to do that.

@dimpase
Copy link
Member Author

dimpase commented Jan 8, 2020

comment:73

what does pkg-config --modversion openblas say?

@embray
Copy link
Contributor

embray commented Jan 8, 2020

comment:74

Replying to @embray:

Currently doesn't detect system openblas on Cygwin (which just gets installed as the generic libblas when you install it). Not sure why yet. I won't hold up this ticket over it but if I can find a quick fix I might like to do that.

I see; it's simply that Cygwin does not install an openblas.pc. That's annoying. I'll see if I can get that fixed upstream. In the meantime we can just leave this as-is.

I think as a broader improvement to the current situation it's worth recognizing that the spkg-install for "atlas" actually does two things:

  • If you have the old SAGE_ATLAS_LIB environment variable set it will just detect and use any old generic BLAS it can find in th given path; it doesn't care if it's actually ATLAS or not.
  • Otherwise it contains steps for specifically building and installing ATLAS from source.

These two cases seem to be quite at odds with each-other, and the naming of SAGE_ATLAS_LIB is misleading.

I think what we should do is this (in a separate ticket):

  • Deprecate use of SAGE_ATLAS_LIB; it can still work for now but display a deprecation message if it is set sage is configured with --with-blas=atlas.

  • Clean up the atlas SPKG a bit so it's only responsible for installing ATLAS if requested.

  • Add a generic package called just "blas" (we could keep openblas as the default though). The spkg-configure.m4 for blas would implement the --with-blas flag, and also extend it to take an optional argument of a path (e.g. instead of --with-blas=openblas or --with-blas=atlas one can pass something like --with-blas=/usr/lib. This latter case would have essentially functionality as the current SAGE_ATLAS_LIB (maybe a little better): use that path to search for a generic blas library; maybe also check that it contains some known symbols.

    • This is nice, because if you use the --with-blas=<path> option you don't get any SPKGs installed for blas, emphasizing that some generic blas on the system is being used.

@dimpase
Copy link
Member Author

dimpase commented Jan 8, 2020

comment:75

iirc, SAGE_ATLAS_LIB is used for two rather different purposes. So there is some cleaning up due.

@embray
Copy link
Contributor

embray commented Jan 8, 2020

comment:76

Replying to @dimpase:

iirc, SAGE_ATLAS_LIB is used for two rather different purposes. So there is some cleaning up due.

The only use of it I could find was in build/pkgs/atlas/spkg-install.py. If set, the directory it's set to is searched (in a fairly flimsy manner) for files that look blas-ish, although in one case it does explicitly look for an existing ATLAS install (if any functionality like this is to be kept it should be moved to atlas/spkg-configure.m4). If anything is found it creates some symlinks to the found libraries into SAGE_LOCAL (this should go away), and also generates some .pc files (also probably can go away and handled more like you did with zlib...)

@vbraun
Copy link
Member

vbraun commented Jan 9, 2020

Changed branch from u/dimpase/packages/openblas/openblasconf to 81bf522

@embray
Copy link
Contributor

embray commented Jan 13, 2020

comment:78

Apparently my rather old Ubuntu lacks a PKG_CONFIG_VAR macro, so since this ticket I get:

$ ./bootstrap
rm -rf config configure build/make/Makefile-auto.in
bootstrap:69: installing 'config/config.rpath'
configure.ac:328: installing 'config/compile'
configure.ac:113: installing 'config/config.guess'
configure.ac:113: installing 'config/config.sub'
configure.ac:68: installing 'config/install-sh'
configure.ac:68: installing 'config/missing'
configure.ac:453: error: possibly undefined macro: AC_CONFIG_COMMANDS
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.
configure:17006: error: possibly undefined macro: PKG_CHECK_VAR
configure:17007: error: possibly undefined macro: AC_CONFIG_LINKS
configure:17012: error: possibly undefined macro: AC_MSG_WARN

(I don't know why it complains first about AC_CONFIG_COMMANDS since that's standard autoconf, but it seems likely the problem is stemming from PKG_CHECK_VAR being missing.)

I'm going to look into it.

@embray
Copy link
Contributor

embray commented Jan 13, 2020

Changed commit from 81bf522 to none

@dimpase
Copy link
Member Author

dimpase commented Jan 13, 2020

comment:79

you must have a past-EOL Ubuntu, I gather.

@embray
Copy link
Contributor

embray commented Jan 13, 2020

comment:80

Replying to @embray:

Replying to @embray:

  • Add a generic package called just "blas" (we could keep openblas as the default though). The spkg-configure.m4 for blas would implement the --with-blas flag, and also extend it to take an optional argument of a path (e.g. instead of --with-blas=openblas or --with-blas=atlas one can pass something like --with-blas=/usr/lib. This latter case would have essentially functionality as the current SAGE_ATLAS_LIB (maybe a little better): use that path to search for a generic blas library; maybe also check that it contains some known symbols.

Perhaps the idea of a "generic package" is overcomplicated for now. Although it's something I've wanted to try to do in the past (#23465) but as a first pass detection of a "generic blas" can live in the spkg-configure.m4 for openblas (sort of as we do with gmp/mpir).

@embray
Copy link
Contributor

embray commented Jan 13, 2020

comment:81

On the third-hand, because of the way the Sage build uses .pc files for cblas and lapack, and the fact that both the openblas and atlas SPKGs install these .pc files into SAGE_LOCAL, I understand now that even when we detect a system openblas, this ticket uses AC_CONFIG_LINKS to create those .pc files as symlinks directly into $SAGE_LOCAL.

I don't really like this, because part of my long-term goals has been to make it so that running configure doesn't create and write anything to $SAGE_LOCAL (this preference is in service to issues like #21532). I'm okay with it for now as a temporary solution, but it does seem like a step backwards.

I'm going to see if I can come up with an alternative solution to this...

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

No branches or pull requests

7 participants