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

OpenBLAS package #20129

Closed
vbraun opened this issue Feb 27, 2016 · 39 comments
Closed

OpenBLAS package #20129

vbraun opened this issue Feb 27, 2016 · 39 comments

Comments

@vbraun
Copy link
Member

vbraun commented Feb 27, 2016

Provide an OpenBLAS optional package

Tarball is on the mirrors

Upstream: Fixed upstream, but not in a stable release.

CC: @kiwifb

Component: packages: optional

Author: Volker Braun

Branch/Commit: 4a2b4dc

Reviewer: Jeroen Demeyer

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

@vbraun vbraun added this to the sage-7.1 milestone Feb 27, 2016
@jdemeyer
Copy link

comment:1

Duplicate of #20096

@vbraun
Copy link
Member Author

vbraun commented Feb 27, 2016

Branch: u/vbraun/openblas_package

@vbraun
Copy link
Member Author

vbraun commented Feb 27, 2016

comment:3

No its not


New commits:

fd198faOpenBLAS optional package

@vbraun
Copy link
Member Author

vbraun commented Feb 27, 2016

Commit: fd198fa

@vbraun
Copy link
Member Author

vbraun commented Feb 27, 2016

Author: Volker Braun

@vbraun

This comment has been minimized.

@jdemeyer
Copy link

comment:5

Can you rename COMPILE_ARGS to OPENBLAS_CONFIGURE? I know it's not using configure, but that would make the environment variable compatible with similar variables in Sage like ATLAS_CONFIGURE, PARI_CONFIGURE,...

@jdemeyer
Copy link

comment:6

And also remove this: COMPILE_ARGS=*. It's a *feature'' that users can set this.

@jdemeyer
Copy link

comment:7

You should add a dependencies file.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 27, 2016

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

8e22428Rename variable

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 27, 2016

Changed commit from fd198fa to 8e22428

@jdemeyer
Copy link

comment:10
  1. Add a spkg-check which runs $MAKE tests.

  2. Apply this patch for powerpc or use version 0.2.16.rc1

@kiwifb
Copy link
Member

kiwifb commented Feb 27, 2016

comment:11

I'll add something to Jeroen's last comment, if we are only going to install dynamic libraries (NO_STATIC=1) may be we shouldn't build them either - I don't think that will shave much time of the build process though.

@kiwifb
Copy link
Member

kiwifb commented Feb 27, 2016

comment:12

A note on tests, the lapack testsuite is currently run as part of the build process. The blas part is tested with make tests, I am not sure how to disable lapack test suite and run it later.

@vbraun
Copy link
Member Author

vbraun commented Feb 28, 2016

comment:13
  • I don't have access to a power machine, so I'm not going to add random patches.

  • I don't think you can avoid building the static library, NO_STATIC is really only referenced in Makefile.install

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2016

Changed commit from 8e22428 to 6b9fab6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2016

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

6b9fab6Add spkg-check

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2016

Changed commit from 6b9fab6 to f48e69b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2016

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f48e69bAdd spkg-check

@kiwifb
Copy link
Member

kiwifb commented Feb 28, 2016

comment:17

Replying to @vbraun:

  • I don't have access to a power machine, so I'm not going to add random patches.

Well Jeroen (power8) and I (power6, power7) do. If you don't include this, we'll need a follow up ticket. And we are both ready to review such patch.

  • I don't think you can avoid building the static library, NO_STATIC is really only referenced in Makefile.install

I checked little and I agree it is in the too hard basket.

@jdemeyer
Copy link

comment:18

Replying to @vbraun:

  • I don't have access to a power machine, so I'm not going to add random patches.

It's not at all a random patch. I found an issue on my POWER8, looked on github and saw that this issue was fixed by the patch I mentioned.

@vbraun
Copy link
Member Author

vbraun commented Feb 28, 2016

comment:19

Replying to @kiwifb:

Well Jeroen (power8) and I (power6, power7) do.

But I can't test it, nor do I think its a priority for this ticket. There are a couple of pieces missing before you can build Sage with OpenBLAS at all. But if you don't have the 10 seconds it takes to create a followup ticket when you actually need it feel free to commit it here. Just test it because I can't.

@jdemeyer
Copy link

comment:20

Replying to @vbraun:

when you actually need it feel free to commit it here.

Sounds good. If I do that, will you review that patch-that-you-cannot-test then?

@jdemeyer
Copy link

Changed branch from u/vbraun/openblas_package to u/jdemeyer/openblas_package

@jdemeyer
Copy link

Changed commit from f48e69b to 4a2b4dc

@jdemeyer
Copy link

New commits:

759612bMinor fixes to spkg-install
4a2b4dcAdd patch for powerpc

@jdemeyer
Copy link

comment:23

With this branch:

openblas-0.2.15
====================================================
Setting up build directory for openblas-0.2.15
mv: cannot stat 'OpenBLAS-0.2.15*': No such file or directory
Finished set up
****************************************************
Host system:
Linux sardonis 3.19.0-15-generic #15-Ubuntu SMP Thu Apr 16 23:32:13 UTC 2015 ppc64le ppc64le ppc64le GNU/Linux
****************************************************
C compiler: gcc
C compiler version:
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/home2/jdemeyer/local/libexec/gcc/powerpc64le-unknown-linux-gnu/5.3.0/lto-wrapper
Target: powerpc64le-unknown-linux-gnu
Configured with: ../gcc-5.3.0/configure --prefix=/home2/jdemeyer/local --enable-languages=c,c++,fortran MAKE=make
Thread model: posix
gcc version 5.3.0 (GCC) 
****************************************************
patching file c_check
Building OpenBLAS: make 
[.......]
 cblas_zhpr2  PASSED THE COMPUTATIONAL TESTS (   577 CALLS)
 cblas_zhpr2  PASSED THE COMPUTATIONAL TESTS (   577 CALLS)

 END OF TESTS
make[3]: Leaving directory '/home2/jdemeyer/sage-check/local/var/tmp/sage/build/openblas-0.2.15/xianyi-OpenBLAS-afedc8e/ctest'
make[2]: Leaving directory '/home2/jdemeyer/sage-check/local/var/tmp/sage/build/openblas-0.2.15/xianyi-OpenBLAS-afedc8e'

real    0m4.521s
user    0m8.894s
sys     0m3.871s
Deleting temporary build directory
/home2/jdemeyer/sage-check/local/var/tmp/sage/build/openblas-0.2.15
Finished installing openblas-0.2.15.spkg

@jdemeyer
Copy link

Upstream: Fixed upstream, but not in a stable release.

@vbraun
Copy link
Member Author

vbraun commented Feb 29, 2016

Reviewer: Jeroen Demeyer

@vbraun
Copy link
Member Author

vbraun commented Feb 29, 2016

comment:25

I guess you didn't have the 10 seconds to open a followup ticket...

@vbraun
Copy link
Member Author

vbraun commented Feb 29, 2016

Reviewer: Jeroen Demeyer

@vbraun
Copy link
Member Author

vbraun commented Feb 29, 2016

comment:26

I guess you didn't have the 10 seconds to open a followup ticket...

@jdemeyer
Copy link

comment:27

Replying to @vbraun:

I guess you didn't have the 10 seconds to open a followup ticket...

I never understood why you wanted a follow-up ticket for this...

I think it's completely normal to review a ticket by saying "X doesn't work, but after applying patch Y it does work, so please apply patch Y".

@vbraun
Copy link
Member Author

vbraun commented Feb 29, 2016

comment:28

You fail to mention that it doesn't work on an exotic architecture on which Sage currently doesn't work. Porting to said architecture shouldn't be mashed into every other ticket just because you are interested in it.

@jdemeyer
Copy link

comment:29

Replying to @vbraun:

Porting to said architecture shouldn't be mashed into every other ticket just because you are interested in it.

I obviously disagree with this... if a trivial accepted-by-upstream patch can be added, I don't see the problem.

And I don't see how it's relevant if Sage currently works or not on said architecture. Shouldn't we try to support as much architectures as possible?

@jdemeyer
Copy link

comment:30

For the record, Sage mostly works on powerpc64le after applying #19719. There are some pexpect issues, but I guess those might be OS bugs unrelated to Sage.

@vbraun
Copy link
Member Author

vbraun commented Feb 29, 2016

comment:31

Replying to @jdemeyer:

Shouldn't we try to support as much architectures as possible?

Yes, but ideally by having one ticket per issue. And not delaying unrelated tickets until we can also stuff some porting effort in there.

@kiwifb
Copy link
Member

kiwifb commented Feb 29, 2016

comment:32

I'll be a pain in the ass and point out that we support power7 and that this patch concerns power7 as well.

@vbraun
Copy link
Member Author

vbraun commented Mar 1, 2016

Changed branch from u/jdemeyer/openblas_package to 4a2b4dc

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

3 participants