#14699 closed defect (fixed)
Install ATLAS header files and cleanup IML spkg
Reported by: | jhpalmieri | Owned by: | jdemeyer |
---|---|---|---|
Priority: | blocker | Milestone: | sage-5.10 |
Component: | packages: standard | Keywords: | atlas IML testsuite check cblas.h |
Cc: | leif, jpflori, jdemeyer, vbraun | Merged in: | sage-5.10.rc2 |
Authors: | Jean-Pierre Flori, Volker Braun, Jeroen Demeyer | Reviewers: | Jean-Pierre Flori |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #10508 | Stopgaps: |
Description (last modified by )
With the new ATLAS spkg from #10508, iml fails its self tests. See http://trac.sagemath.org/sage_trac/ticket/10508#comment:518 and later comments.
Use spkgs at:
- http://boxen.math.washington.edu/home/vbraun/spkg/atlas-3.10.1.p1.spkg (spkg diff)
- http://boxen.math.washington.edu/home/jdemeyer/spkg/iml-1.0.1.p15.spkg (spkg diff without deletes)
Apply 14699_env_version_bump.patch to SAGE_ROOT
Attachments (3)
Change History (82)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
Or just install which should call both of them.
comment:3 follow-up: ↓ 4 Changed 7 years ago by
John, U sure about sage.math?
128.208.160.191 sage.math.washington.edu sage sage.math
comment:4 in reply to: ↑ 3 Changed 7 years ago by
Replying to leif:
John, U sure about sage.math?
128.208.160.191 sage.math.washington.edu sage sage.math
(As opposed to
;; ANSWER SECTION: sage.math.washington.edu. 600 IN A 128.208.160.197
which is boxen.)
comment:5 Changed 7 years ago by
It would be nice to fix that on top of #14605 which I've tested quite extensively and is finished since some time.
comment:6 Changed 7 years ago by
+1 to also again install static libs.
comment:7 Changed 7 years ago by
- Cc jeroen vbraun added
- Keywords atlas added
- Priority changed from major to critical
- Summary changed from Get iml to install correctly and pass its self-tests on sage.math to Install ATLAS headers file and static libraries
In fact disregard my comment about #14605. We should fix the issue here quickly, hopefully before 5.10 official release if it's not too late. Although as it does not seem to prevent anything from working except for IML testsuite, 5.11 should be ok as well.
comment:8 Changed 7 years ago by
- Cc jdemeyer added; jeroen removed
- Keywords IML testsuite check cblas.h added
- Summary changed from Install ATLAS headers file and static libraries to Install ATLAS header files and static libraries
comment:9 Changed 7 years ago by
A side-effect of this ticket is probably that numpy gets built without ATLAS acceleration.
comment:10 Changed 7 years ago by
For the record, I did get
ATLAS_CFLAGS='' ATLAS_LIBS='-lcblas -latlas'
-L${SAGE_LOCAL}/lib
"incidentally" came from / for GMP, although after -lcblas -latlas
, but we meanwhile add ${SAGE_LOCAL}/lib
to LIBRARY_PATH
(in sage-env
), so that it worked.
comment:11 follow-up: ↓ 13 Changed 7 years ago by
P.S.: Although IIRC according to POSIX, -Ldir1 -lfoo -Ldir2 -lbar
is the same as -Ldir1 -Ldir2 -lfoo -lbar
.
comment:12 Changed 7 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:13 in reply to: ↑ 11 Changed 7 years ago by
Replying to leif:
P.S.: Although IIRC according to POSIX,
-Ldir1 -lfoo -Ldir2 -lbar
is the same as-Ldir1 -Ldir2 -lfoo -lbar
.
At least GNU ld
docs agree with this:
All -L options apply to all -l options, regardless of the order in which the options appear.
comment:14 follow-ups: ↓ 15 ↓ 16 Changed 7 years ago by
You'd still need a line
cmd += ' --incdir=' + os.path.join(conf['SAGE_LOCAL'],'include')
in configure_atlas_library()
for the header files to go the correct place?
And currently numpy is built without Atlas (it uses another blas): local/lib/python2.7/site-packages/numpy/core/_dotblas.so normally contains ATL_* symbols (for an old Atlas 3.8.3 version, at least).
comment:15 in reply to: ↑ 14 Changed 7 years ago by
Replying to Koen:
You'd still need a line
cmd += ' --incdir=' + os.path.join(conf['SAGE_LOCAL'],'include')in
configure_atlas_library()
for the header files to go the correct place?
I don't think so. We use prefix and that should be enough from what I've seen in ATLAS configure script.
comment:16 in reply to: ↑ 14 Changed 7 years ago by
Replying to Koen:
And currently numpy is built without Atlas (it uses another blas): local/lib/python2.7/site-packages/numpy/core/_dotblas.so normally contains ATL_* symbols (for an old Atlas 3.8.3 version, at least).
On my gcc110 install (where IML testsuite fails), numpy uses ATLAS correctly. There is no ATLAS symbols in the file you pointed because we now use a shared version of ATLAS. But if I run ldd on the file it points to the ATLAS libs.
And I seem to remember we had lots of troubles correctly biulding numpy and scipy with the new ATLAS so hopefully it was actually used.
comment:17 follow-up: ↓ 18 Changed 7 years ago by
s/inlcude/include/
and IMHO (=WTF?) also s/serial/single-threaded/g s/parallel/multi-threaded/g ...
(but the latter is not JP's invention I guess)
comment:18 in reply to: ↑ 17 Changed 7 years ago by
Replying to leif:
s/inlcude/include/
Done.
and IMHO (=WTF?) also s/serial/single-threaded/g s/parallel/multi-threaded/g ...
(but the latter is not JP's invention I guess)
Not mine indeed.
comment:19 Changed 7 years ago by
Replying to jpflori:
Replying to Koen:
And currently numpy is built without Atlas (it uses another blas): local/lib/python2.7/site-packages/numpy/core/_dotblas.so normally contains ATL_* symbols (for an old Atlas 3.8.3 version, at least).
On my gcc110 install (where IML testsuite fails), numpy uses ATLAS correctly. There is no ATLAS symbols in the file you pointed because we now use a shared version of ATLAS. But if I run ldd on the file it points to the ATLAS libs.
Yes.
$ readelf -d local/lib/python2.7/site-packages/numpy/core/_dotblas.so Dynamic section at offset 0x4dc0 contains 26 entries: Tag Type Name/Value 0x0000000000000001 (NEEDED) Shared library: [libptf77blas.so.2] 0x0000000000000001 (NEEDED) Shared library: [libptcblas.so.2] 0x0000000000000001 (NEEDED) Shared library: [libatlas.so.2] 0x0000000000000001 (NEEDED) Shared library: [libpython2.7.so.1.0] 0x0000000000000001 (NEEDED) Shared library: [libpthread.so.0] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6] ...
comment:20 Changed 7 years ago by
- Status changed from needs_review to needs_work
make_atlas() takes no argument.
comment:21 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:22 Changed 7 years ago by
I saw the error on boxen. I thought that sage.math was dead? In any case, it's the machine I get to by typing ssh sage.math.washington.edu
, and when I log off it says Connection to sage.math.washington.edu closed.
The $HOSTNAME
is boxen, though.
comment:23 Changed 7 years ago by
On boxen, after installing the new ATLAS spkg, the current iml spkg installs and passes its tests. Same for an iml spkg which doesn't copy over the patched version of configure
.
comment:24 Changed 7 years ago by
P.S.: I guess the odd "patch" to IML's configure
dates back to the times when ATLAS was somehow optional, or at least did not get properly detected on all systems. Running / building IML's test suite on such systems was always broken then.
(In other parts of Sage using IML, BLAS libraries get explicitly added by us when linking.)
comment:25 Changed 7 years ago by
- Priority changed from critical to blocker
comment:26 follow-up: ↓ 31 Changed 7 years ago by
Do not install static libraries, its a feature that we don't.
Also, nobody in his right mind expects blas headers. This is why the iml testsuite, which the authors probably didn't expect to be distributed, is the only place that trips over it. For starters, there is no universal naming standard (blas.h vs. cblas.h vs. foo_cblas.h vs. foo/cblas.h). There are multiple blas implementations and if you all install them at the same place then they would overwrite each other.
comment:27 Changed 7 years ago by
And if you need a reason for not installing static libraries you just have to look at this ticket where you got confused about whether something links against atlas or not. Shared libraries are the only way that make it clear which particular atlas version you are linking against and that you didn't accidentally pick up some system atlas.
comment:28 Changed 7 years ago by
I disagree that nobody expects the blas headers. If I try to install a package on top of Sage, e.g. the Shogun machine learning toolbox, then it will only compile in certain features if it can find a blas - and for that it needs blas headers. With older sage versions this works just fine.
comment:29 Changed 7 years ago by
Shogun just makes it unnecessarily complicated for themselves, you need to define preprocessor constants to tell it which blas header to look for...
I agree that we should have blas/cblas/f77blas headers since its the right thing to do. But when we fix the IML testsuite we shouldn't introduce that as a dependency, because very often the headers are either not installed or installed with a funky name.
comment:30 Changed 7 years ago by
- Milestone changed from sage-5.11 to sage-5.10
comment:31 in reply to: ↑ 26 ; follow-up: ↓ 33 Changed 7 years ago by
Replying to vbraun:
Do not install static libraries, its a feature that we don't.
At least they shouldn't get discarded, which is simply stupid.
comment:32 Changed 7 years ago by
Btw., the new spkg and afterwards IML installed without problems, and both passed their test suites on Linux x86_64 (Ubuntu 10.04.4, Sage 5.10.rc0).
comment:33 in reply to: ↑ 31 ; follow-up: ↓ 35 Changed 7 years ago by
Replying to leif:
At least they shouldn't get discarded, which is simply stupid.
Do you also object to discarding the *.o
files? We delete what we don't want around in the install, thats the way it works. Ideally upstream would have an --disable-static
switch, but they don't.
comment:34 Changed 7 years ago by
- Description modified (diff)
Removed the static library install bit from the spkg
comment:35 in reply to: ↑ 33 ; follow-up: ↓ 36 Changed 7 years ago by
comment:36 in reply to: ↑ 35 Changed 7 years ago by
Replying to leif:
man ar
?
I take it that this means that you agree with me, and we should neither install object files nor static libraries ;-)
comment:37 Changed 7 years ago by
- Summary changed from Install ATLAS header files and static libraries to Install ATLAS header files
comment:38 follow-up: ↓ 40 Changed 7 years ago by
- Status changed from needs_review to needs_info
Probably vote on sage-devel on whether to discard the static libraries or not.
comment:39 Changed 7 years ago by
Replying to leif:
Replying to vbraun:
Do not install static libraries, its a feature that we don't.
At least they shouldn't get discarded, which is simply stupid.
Having static libraries rather than nothing on systems where building shared ones is problematic might be nice... Also note it's the only option upstream really supports.
comment:40 in reply to: ↑ 38 Changed 7 years ago by
Replying to leif:
Probably vote on sage-devel on whether to discard the static libraries or not.
... or make it optional.
comment:41 Changed 7 years ago by
comment:42 follow-up: ↓ 45 Changed 7 years ago by
Can we please concentrate on the actual ticket issue here, which is about header files and iml failing? I don't think this has anything to do with static libraries (but please correct me if I'm wrong).
Changed 7 years ago by
comment:43 Changed 7 years ago by
- Reviewers set to Jean-Pierre Flori
- Status changed from needs_info to needs_review
Ok. Only concentrating on that, I'm ok with Volker changes to my changes.
comment:44 Changed 7 years ago by
- Status changed from needs_review to positive_review
I'm ok with Jean-Pierre changes to the extent that I haven't changed them, of course.
comment:45 in reply to: ↑ 42 Changed 7 years ago by
Replying to jdemeyer:
Can we please concentrate on the actual ticket issue here, which is about header files and iml failing? I don't think this has anything to do with static libraries (but please correct me if I'm wrong).
Well, if we used ATLAS' install rather than further hacking, we'd get both the headers and the static libraries.
I don't see any reason why discarding static libraries should be done by default; if someone feels they're dangerous to him/her, we could add some option to automatically remove them, although that's IMHO a pretty unimportant feature.
comment:46 follow-up: ↓ 47 Changed 7 years ago by
P.S.: Removing the "patch" to IML's configure
from the IML spkg IMHO belongs to this ticket as well, as that was hiding (or rather obscuring) the real problem.
comment:47 in reply to: ↑ 46 ; follow-up: ↓ 48 Changed 7 years ago by
Replying to leif:
P.S.: Removing the "patch" to IML's
configure
from the IML spkg IMHO belongs to this ticket as well, as that was hiding (or rather obscuring) the real problem.
Then either attach a fixed iml spkg to this packet real soon (TM) or open a ticket for it. But don't leave footnotes on finished tickets.
comment:48 in reply to: ↑ 47 Changed 7 years ago by
Replying to vbraun:
Replying to leif:
P.S.: Removing the "patch" to IML's
configure
from the IML spkg IMHO belongs to this ticket as well, as that was hiding (or rather obscuring) the real problem.Then either attach a fixed iml spkg to this packet real soon (TM) or open a ticket for it. But don't leave footnotes on finished tickets.
I was saying it's IMHO not finished.
comment:49 follow-up: ↓ 52 Changed 7 years ago by
- Work issues set to enable cblas.h test in iml spkg
Indeed, the ticket original title was about IML testsuite, so we should also provide an IML spkg (quite trivial just remove the horrible patch disabling test for cblas.h).
leif: could you make the spkg? I'll quickly review it.
comment:50 Changed 7 years ago by
- Status changed from positive_review to needs_work
comment:51 Changed 7 years ago by
Or at least we should provide this fixed IML spkg in a follow up ticket mentioned here and put as blocker for 5.10.
comment:52 in reply to: ↑ 49 Changed 7 years ago by
This doesn't at all work with SAGE_ATLAS_LIB
[unless the headers happen to be installed system-wide, which is the same situation as before], since headers don't get copied / symlinked.
What happens on MacOS X (where Sage's ATLAS by default doesn't get built)?
(Similar for Cygwin perhaps.)
Replying to jpflori:
Indeed, the ticket original title was about IML testsuite, so we should also provide an IML spkg (quite trivial just remove the horrible patch disabling test for cblas.h).
So I'm not sure if it's really that easy.
comment:53 Changed 7 years ago by
Yeah, I was just looking into that... Not really anything useful to do when a system-wide ATLAS is used.
Does really no software linking to ATLAS expect to find headers in some standard or provided place? Do they all provide their own BLAS headers and expect everything to be fine?
comment:54 Changed 7 years ago by
Its pretty much common practice to use your own blas/lapack headers. For example, linbox/fflas: http://linalg.org/projects/fflas-ffpack/browser/fflas-ffpack/config-blas.h Definitely weird, but then its probably the most stable API ever. And the lack of uniform naming of the header across vendors doesn't help.
comment:55 Changed 7 years ago by
A test spkg is at:
Not really wonderful as the src directory still includes a modified upstream source, but maybe a little bit better looking. Not tested yet, I'm doing it right now.
comment:56 Changed 7 years ago by
- Status changed from needs_work to needs_review
- Work issues enable cblas.h test in iml spkg deleted
comment:57 Changed 7 years ago by
- Description modified (diff)
comment:58 Changed 7 years ago by
- Summary changed from Install ATLAS header files to Install ATLAS header files and cleanup IML spkg
comment:59 Changed 7 years ago by
- Status changed from needs_review to needs_work
The use of "$SAGE_LOCAL"/lib/libatlas.so.*.*
seems to assume that there is exactly one file matching "$SAGE_LOCAL"/lib/libatlas.so.*.*
I doubt that this assumption is true in general.
comment:60 Changed 7 years ago by
Quite true (just wondering if we really want to support Sage's installs with multiple ATLAS version installed?).
comment:61 Changed 7 years ago by
Working on IML spkg, hang on...
comment:62 Changed 7 years ago by
Of course I guess the IML testsuite will still fail on OS X or Cygwin.
comment:63 Changed 7 years ago by
- Description modified (diff)
comment:64 Changed 7 years ago by
- Status changed from needs_work to needs_review
IML spkg needs review, I restored upstream sources and added patches in the patches/
directory. I know the patches can be catalogued and documented better, but let's leave that for next time...
Tested on Linux and OS X x86_64, IML (with testsuite) works.
comment:65 Changed 7 years ago by
- Description modified (diff)
Changed 7 years ago by
comment:66 follow-up: ↓ 67 Changed 7 years ago by
- Description modified (diff)
I'm adding 14699_env_version_bump.patch because ATLAS now requires the FC
environment variable to be set, so it needs an up-to-date sage-env
.
comment:67 in reply to: ↑ 66 Changed 7 years ago by
Replying to jdemeyer:
I'm adding 14699_env_version_bump.patch because ATLAS now requires the
FC
environment variable to be set, so it needs an up-to-datesage-env
.
What does that achieve exactly? Just looking at the patch, I cannot really guess. Does it force upgrading the sage-env script?
A question for Volker: How does the SO_VERSION variable in the ATLAS autotools project be updated?
comment:68 Changed 7 years ago by
I'll copy the comments from sage-env
:
# Don't execute the commands more than once for the same version of # sage-env. Check this after checking the validity of SAGE_ROOT, but # before modifying its value. # # The idea of versioning is that SAGE_ENV_SOURCED will be set to a # "version number" of sage-env. If a different version of sage-env was # sourced earlier (when upgrading), we still source the new version. # The sage-env version should be increased whenever a newer sage-env is # required for upgrading. Note that spkg/standard/deps might also need # to be changed: the packages which need the new sage-env must depend on # SAGE_ROOT_REPO (the root repo contains sage-env). # # sage-env version history: # - sage-4.7.1: version 1 (#10469) # - sage-5.4: version 2 (#13395) # SAGE_ENV_VERSION=2 if [ "$SAGE_ENV_SOURCED" = "$SAGE_ENV_VERSION" ]; then # Already sourced, nothing to do. return 0 fi export SAGE_ENV_SOURCED=$SAGE_ENV_VERSION
comment:69 follow-up: ↓ 70 Changed 7 years ago by
- Status changed from needs_review to needs_work
- Work issues set to SAGE64,
In the IMl spkg, you don't reexport CFLAGS after checking for SAGE64 (both in spkg-check and spkg-install).
And when looking through trac there is a strange .patch part about renaming sage3_memleak, but that may just be a trac issue.
comment:70 in reply to: ↑ 69 ; follow-up: ↓ 72 Changed 7 years ago by
- Status changed from needs_work to needs_review
- Work issues SAGE64, deleted
Replying to jpflori:
In the IMl spkg, you don't reexport CFLAGS after checking for SAGE64 (both in spkg-check and spkg-install).
It's already exported before. No need to "reexport" it.
And when looking through trac there is a strange .patch part about renaming sage3_memleak, but that may just be a trac issue.
Indeed, that's a Trac bug.
comment:71 Changed 7 years ago by
I should also note that the only new patch in IML is patches/remove_repl.patch
, all the others were already (pre-patched) in the spkg.
comment:72 in reply to: ↑ 70 Changed 7 years ago by
Replying to jdemeyer:
Replying to jpflori:
In the IMl spkg, you don't reexport CFLAGS after checking for SAGE64 (both in spkg-check and spkg-install).
It's already exported before. No need to "reexport" it.
Damn, I was not aware of that!
And when looking through trac there is a strange .patch part about renaming sage3_memleak, but that may just be a trac issue.
Indeed, that's a Trac bug.
comment:73 Changed 7 years ago by
- Status changed from needs_review to positive_review
The spkg looks good, seems to work on my systems.
comment:74 Changed 7 years ago by
- Merged in set to sage-5.10.rc2
- Resolution set to fixed
- Status changed from positive_review to closed
comment:75 Changed 7 years ago by
Thanks for the review, do you by chance feel like reviewing the IML upgrade at #748?
comment:76 Changed 7 years ago by
- Merged in sage-5.10.rc2 deleted
- Milestone changed from sage-5.10 to sage-5.11
- Resolution fixed deleted
- Status changed from closed to new
Reverting the ATLAS package: #14753.
comment:77 Changed 7 years ago by
- Dependencies set to #10508
comment:78 Changed 7 years ago by
- Merged in set to sage-5.10.rc2
- Milestone changed from sage-5.11 to sage-5.10
- Resolution set to fixed
- Status changed from new to closed
comment:79 Changed 7 years ago by
Keeping "merged in" information for future reference, we should not change this ticket further.
Surely an ATLAS problem, see http://trac.sagemath.org/sage_trac/ticket/10508#comment:526 for a solution.
I suggest we call at least install_inc and potentially install_lib from ATLAS build system to install headers and static libs.