Opened 4 years ago

Closed 4 years ago

#20129 closed enhancement (fixed)

OpenBLAS package

Reported by: vbraun Owned by:
Priority: major Milestone: sage-7.1
Component: packages: optional Keywords:
Cc: fbissey Merged in:
Authors: Volker Braun Reviewers: Jeroen Demeyer
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: 4a2b4dc (Commits) Commit: 4a2b4dc898db741c394622af3a3ddcf3a003d272
Dependencies: Stopgaps:

Description (last modified by vbraun)

Provide an OpenBLAS optional package

Tarball is on the mirrors

Change History (33)

comment:1 Changed 4 years ago by jdemeyer

Duplicate of #20096

comment:2 Changed 4 years ago by vbraun

  • Branch set to u/vbraun/openblas_package

comment:3 Changed 4 years ago by vbraun

  • Authors set to Volker Braun
  • Commit set to fd198fa6b664d6e7420b1437b4f68a7ec4e67bcd
  • Component changed from PLEASE CHANGE to packages: standard
  • Description modified (diff)
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to enhancement

No its not


New commits:

fd198faOpenBLAS optional package

comment:4 Changed 4 years ago by vbraun

  • Cc fbissey added
  • Component changed from packages: standard to packages: optional

comment:5 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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,...

comment:6 Changed 4 years ago by jdemeyer

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

comment:7 Changed 4 years ago by jdemeyer

You should add a dependencies file.

comment:8 Changed 4 years ago by git

  • Commit changed from fd198fa6b664d6e7420b1437b4f68a7ec4e67bcd to 8e224281365087f4a3f4109b6173b106663e693c

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

8e22428Rename variable

comment:9 Changed 4 years ago by vbraun

  • Status changed from needs_work to needs_review

comment:10 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. Add a spkg-check which runs $MAKE tests.
  1. Apply this patch for powerpc or use version 0.2.16.rc1

comment:11 Changed 4 years ago by fbissey

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.

comment:12 Changed 4 years ago by fbissey

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.

comment:13 follow-ups: Changed 4 years ago by vbraun

  • 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

comment:14 Changed 4 years ago by git

  • Commit changed from 8e224281365087f4a3f4109b6173b106663e693c to 6b9fab69633e77a59e38b71b42c289da76bcdd6e

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

6b9fab6Add spkg-check

comment:15 Changed 4 years ago by git

  • Commit changed from 6b9fab69633e77a59e38b71b42c289da76bcdd6e to f48e69b966f19b14582c7acf9d81deeaf47096ec

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

f48e69bAdd spkg-check

comment:16 Changed 4 years ago by vbraun

  • Status changed from needs_work to needs_review

comment:17 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by fbissey

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.

comment:18 in reply to: ↑ 13 Changed 4 years ago by jdemeyer

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.

comment:19 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by vbraun

Replying to fbissey:

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.

comment:20 in reply to: ↑ 19 Changed 4 years ago by jdemeyer

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?

comment:21 Changed 4 years ago by jdemeyer

  • Branch changed from u/vbraun/openblas_package to u/jdemeyer/openblas_package

comment:22 Changed 4 years ago by jdemeyer

  • Commit changed from f48e69b966f19b14582c7acf9d81deeaf47096ec to 4a2b4dc898db741c394622af3a3ddcf3a003d272

New commits:

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

comment:23 Changed 4 years ago by jdemeyer

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

comment:24 Changed 4 years ago by jdemeyer

  • Report Upstream changed from N/A to Fixed upstream, but not in a stable release.

comment:25 Changed 4 years ago by vbraun

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

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

comment:26 follow-up: Changed 4 years ago by vbraun

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

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

comment:27 in reply to: ↑ 26 Changed 4 years ago by jdemeyer

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".

comment:28 follow-up: Changed 4 years ago by vbraun

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.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 4 years ago by jdemeyer

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?

comment:30 Changed 4 years ago by jdemeyer

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.

comment:31 in reply to: ↑ 29 Changed 4 years ago by vbraun

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.

comment:32 Changed 4 years ago by fbissey

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

comment:33 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/openblas_package to 4a2b4dc898db741c394622af3a3ddcf3a003d272
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.