OpenBLAS - alterations for TARGET #23272

thecaligarmo · 2017-06-18T18:17:08Z

On purchasing a new laptop, I attempted to MAKE sage as I normally have and ran into the following error:

Error building Sage. 

The following package(s) may have failed to build (not necessarily during this run of 'make all'):

* package: openblas-0.2.19.p0 
  log file: /sage/sage/logs/pkgs/openblas-0.2.19.p0.log
  build directory: /sage/sage/local/var/tmp/sage/build/openblas-0.2.19.p0

The error seemed to be really coming from:

make[3]: *** [getarch_2nd] Error 1
Makefile:123: *** OpenBLAS: Detecting CPU failed. Please set TARGET explicitly, e.g. make TARGET=your_cpu_target. Please read README for the detail..  Stop.
make[3]: Leaving directory '/sage/sage/local/var/tmp/sage/build/openblas-0.2.19.p0/src'
Error building OpenBLAS

It turned out that this error has been seen multiple times in Sage:

etc.

I noticed that updating the config (as mentioned in the threads above) accurately solved the problem, but I wanted to try and fix the problem head on rather than just temporarily solve it. With that in mind, I conversed with the OpenBLAS people:
OpenMathLib/OpenBLAS#1204
and they suggested I update the cpu file for the build, which I have included.

Additionally, I wanted to fix this problem for future users and thus I wanted to add a fail-safe for the code so that if this error happens again, then we automatically try the TARGET=ATOM method. But, I want to create this with the caveat that a user can change the method.

Before finalizing these changes, we should note a few things, but as I've not worked that long with sage I don't know how to address them. In essence:

Are the changes I made appropriate for the make?
I'm adding documentation on this as there is no documentation.
There was questions on why we have "p0" at the end of our version? I assume this is from the patch, but I don't know.
The programmer suggested we change the " top level option to use -lblas (debian/ubuntu) -lopenblas (suse/fedora/epel)". I don't know what that means in regards to us, but figured someone might be able to answer this question and maybe we can ensure this is happening.

If there are any questions feel free to ask, and if you believe this is not necessarily or if something else is better practice, let me know. I've never done a bash alteration so not sure what the details for that.

The tarball is here https://github.com/xianyi/OpenBLAS/archive/v0.2.20.tar.gz
(using the browser to download it will give it correct name)

CC: @jpflori @vbraun @jdemeyer @mo271

Component: packages: standard

Keywords: openblas

Author: Aram Dermenjian

Branch: 895b48f

Reviewer: Volker Braun, Dima Pasechnik

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

The text was updated successfully, but these errors were encountered:

thecaligarmo · 2017-06-18T18:34:48Z

Branch: u/aram.dermenjian/openblas___alterations_for_target

thecaligarmo · 2017-06-18T18:37:26Z

New commits:

`9ce1ab8`	`OpenBLAS changes for CPUs`
`d5d2e42`	`Update trac tags`
`56a64c1`	`Merge branch 'openblas' into t/23272/openblas___alterations_for_target`

thecaligarmo · 2017-06-18T18:37:26Z

Author: Aram Dermenjian

thecaligarmo · 2017-06-18T18:37:26Z

Commit: 56a64c1

thecaligarmo · 2017-06-18T18:38:37Z

comment:3

I'm setting the status to needs review mainly to get people's attention. I assume there will be things that need to be changed.

nthiery · 2017-06-22T21:14:40Z

comment:5

We discussed a bit. In the long run, we would like OpenBlas to provide a short mantra
to configure itself with "use Atom by default if target unavailable".

For now, I wondered about separating somewhat more the configuration part from the compilation, by adding a new dependency in OpenBlas makefile like:

    is_architecture_available:
         <here the logic from the beginning of the `libs` dependency

and then, in spkg-install / spkg-check do first a call to

    make is_architecture_available

do the additional logic if needed, and then just call make as usual.

Just throwing ideas in the air; not sure this is the right thing to do.

Insights anyone?

jdemeyer · 2017-06-30T14:07:57Z

comment:6

I don't really like that we are just guessing ATOM as a fall-back when the build failed. I would prefer to just let the failed build be failed (possibly with an error message suggesting OPENBLAS_CONFIGURE="TARGET=ATOM" or something).

Also, if you add a patch, you should bump the version number of openblas (from 0.2.19 to 0.2.19.p0).

sagetrac-git · 2017-07-14T11:24:36Z

Changed commit from 56a64c1 to 4be16f3

sagetrac-git · 2017-07-14T11:24:36Z

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

`4be16f3`	`Check directly that Target is needed`

thecaligarmo · 2017-07-14T11:29:01Z

New commits:

`4be16f3`	`Check directly that Target is needed`

thecaligarmo · 2017-07-14T11:32:55Z

comment:9

So the reason we are just guessing "ATOM" is because that was the recommended fall-back by the openblas team. (You can see the conversation I had in the link I posted)

THe problem with just letting the build fail is that as new computers come out with new CPUs these CPUs will not be included in the openblas config which tests against CPU. In other words all future computers will start having problems when new CPUs get introduced. Since OpenBLAS doesn't update regularly, we can't rely on us to "remember" every time a new CPU gets released.

The point of this ticket is to upgrade the openblas script through a patch so that new CPUs that have been added in the last year will now properly be able to get a target. Additionally, it will protect us for future CPUs by defaulting to ATOM until we get an update to OpenBLAS.

Note that the version is already 0.2.19.p0 so I've let it as is.

vbraun · 2017-07-22T15:40:19Z

comment:10

Typo: OpenBAS -> OpenBLAS

I agree that it would be best if OpenBLAS would just default to something sensible if it can't detect the CPU. But, unless thats implemented, we should just try a second time with TARGET=ATOM. I'd prefer if we just unconditionally try that a second time and not rely on the spelling of the logged error message, or the location of the log file.

OpenBLAS 0.2.20 should be released really soon now...

vbraun · 2017-07-22T15:41:33Z

comment:11

PS: the patchlevel should be increased 0.2.19.p0 -> 0.2.19.p1 to trigger recompilation.

kiwifb · 2017-07-24T04:31:22Z

comment:12

I just received a notice that openblas 0.2.20 has been released

Version 0.2.20

24-Jul-2017



common:

        * Improved CMake support

        * Fixed several thread race and locking bugs

        * Fixed default LAPACK optimization level

        * Updated LAPACK to 3.7.0

        * Added ReLAPACK (https://github.com/HPAC/ReLAPACK, make BUILD_RELAPACK=1)



POWER:

        * Optimizations for Power9

        * Fixed several Power8 assembly bugs

....
x86_64:

        * Detect Intel Bay Trail and Apollo Lake

        * Detect Intel Sky Lake and Kaby Lake

        * Detect Intel Knights Landing

        * Detect AMD A8, A10, A12 and Ryzen

        * Support 64bit builds with Visual Studio

        * Fix building with Intel and PGI compilers

        * Fix building with MINGW and TDM-GCC

        * Fix cmake builds for Haswell and related cpus

        * Fix building for Sandybridge with CLANG 3.9

* Add support for the FLANG compiler

....

While some of the improvements in this ticket should be kept, I think it would be best if it became an upgrade ticket.

kiwifb · 2017-07-24T05:27:24Z

comment:13

Upgrading should also get rid of us of all the current patches except for openblas-0.2.19-SMP_conditional.patch (because the PR for it is still sitting on my HD - real life happened and it got forgotten).

dimpase · 2017-07-24T11:21:45Z

comment:14

alive and kicking: https://github.com/xianyi/OpenBLAS/commits/develop

embray · 2017-07-31T11:44:40Z

comment:15

if grep -q "Detecting CPU failed. Please set TARGET explicitly" "${SAGE_ROOT}/logs/pkgs/openblas-$(<${SAGE_ROOT}/build/pkgs/openblas/package-version.txt).log"; then

This isn't going to work for a number reasons; for one the log files are usually appended to so this will be true for builds that failed but then later succeeded if the log files were not deleted first.

Better to explicitly tee the configure command to a separate log file and grep that.

sagetrac-git · 2017-08-10T14:18:23Z

Changed commit from 4be16f3 to 8426078

sagetrac-git · 2017-08-10T14:18:23Z

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

`90e56c8`	`Update OpenBLAS package and update install error handling`
`39094e3`	`Merge branch 'develop' into t/23272/openblas___alterations_for_target`
`5d758c5`	`Remove unused envar`
`8426078`	`Typo fix`

thecaligarmo · 2017-08-10T14:21:56Z

comment:17

So in this update I've gone ahead and updated the version and removed the unnecessary patches (keeping the one mentioned by fbissey). I've not worked with packages before so I don't know how I'm supposed to actually get the package itself onto the servers? I personally added it directly into my personal upstream, but not sure if that is a viable solution?

I've also removed the documentation test and instead went with a TARGET test like I had done before. I've gone to my old method of just retrying with "TARGET=ATOM" as was also suggested by vbraun.

dimpase · 2017-08-17T09:21:04Z

comment:18

Where is the updated tarball (the one with

+sha1=a186074145a24823e82c65672dad1cd1ca6fe89c
+md5=48637eb29f5b492b91459175dcc574b1
+cksum=607023019

How does one get it?
Please provide a link.

vbraun · 2017-09-28T12:20:43Z

comment:27

I think this needs updating for openblas-0.2.19-cygwin.patch

thecaligarmo · 2017-09-28T13:10:06Z

comment:28

I had removed all of the patches except for one. If the cygwin patch is required I can re-add it in.

vbraun · 2017-09-28T13:16:45Z

comment:29

It was recently added; please merge in the latest beta

thecaligarmo · 2017-09-28T19:10:48Z

comment:30

@vbraun I don't know what you mean by the latest beta. Can you be a little more specific? (Do you have a link? I can try mergeing it in and making sure it works. Alternatively, if you've already access to the beta on your machine you can try integrating it and then I can pull and test it on mine as well?)

vbraun · 2017-09-29T06:54:10Z

comment:31

I just mean the latest develop branch, e.g. on trac: git remote update trac && git merge trac/develop

sagetrac-git · 2017-10-04T17:27:23Z

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

`4c76040`	`Merge branch 'develop' into t/23272/openblas___alterations_for_target`
`5d405e4`	`update cygwin patch`
`2298700`	`Merge branch 'develop' into t/23272/openblas___alterations_for_target`

sagetrac-git · 2017-10-04T17:27:23Z

Changed commit from 3831e40 to 2298700

thecaligarmo · 2017-10-04T17:31:28Z

comment:33

I kept having problems merging in the cygwin patch (kept failing on my build for some reason) I fixed it and have merged again with the latest develop. It should be ready for testing now.

mo271 · 2017-10-12T20:04:30Z

comment:34

I don't have much time to look into the ticket, but just wanted to confirm, that it worked for me.

sage 8.1.beta7 didn't compile on a debian box with a kaby lake i7-7500U CPU, because of the 'missing target' openblas error.

After loading this patch it worked like a charm (All tests passed). I guess it is useful to have a fix even after the problem for my specific cpu will be fixed upstream; there always will be newer cpus...

mo271 · 2017-10-17T17:44:24Z

comment:35

In sage 8.1.beta8, I simply compiled with
export OPENBLAS_CONFIGURE="TARGET=HASWELL"
(with a kabylake CPU..) seems to work.

vbraun · 2017-11-06T21:12:39Z

Reviewer: Volker Braun

vbraun · 2017-11-08T19:23:45Z

comment:37

On two 32-bit buildbots:

**********************************************************************
File "src/sage/numerical/sdp.pyx", line 100, in sage.numerical.sdp
Failed example:
    p.dual_variable(1)  # rel tol 1e-03
Expected:
    [ 85555.0 -85555.0]
    [-85555.0  85555.0]
Got:
    [ 85463.87196917446 -85463.87196914306]
    [-85463.87196914306  85463.87196917432]
Tolerance exceeded in 4 of 4:
    85555.0 vs 85463.87196917446, tolerance 1e-03 > 1e-03
    -85555.0 vs -85463.87196914306, tolerance 1e-03 > 1e-03
    -85555.0 vs -85463.87196914306, tolerance 1e-03 > 1e-03
    85555.0 vs 85463.87196917432, tolerance 1e-03 > 1e-03
**********************************************************************
1 item had failures:
   1 of  41 in sage.numerical.sdp
    [283 tests, 1 failure, 0.75 s]
----------------------------------------------------------------------
sage -t --long src/sage/numerical/sdp.pyx  # 1 doctest failed
----------------------------------------------------------------------

dimpase · 2017-11-08T22:10:18Z

comment:39

OK, I'll increase the tolerance to 2e-03 there.

dimpase · 2017-11-08T22:12:57Z

Changed branch from u/aram.dermenjian/openblas___alterations_for_target to public/openblas___alterations_for_target

dimpase · 2017-11-08T22:12:57Z

Changed commit from 2298700 to 895b48f

dimpase · 2017-11-08T22:12:57Z

Changed reviewer from Volker Braun to Volker Braun, Dima Pasechnik

dimpase · 2017-11-08T22:12:57Z

New commits:

`0af8f5b`	`Merge branch 'u/aram.dermenjian/openblas___alterations_for_target' of trac.sagemath.org:sage into oblas`
`895b48f`	`increase tol`

sagetrac-andy · 2017-11-13T01:07:13Z

comment:41

This fixed the build under error I was having on Ubuntu 17.10. The /proc/cpuinfo identifies the CPU as:

Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz

vbraun · 2017-11-15T01:24:30Z

Changed branch from public/openblas___alterations_for_target to 895b48f

slel · 2017-11-18T22:38:33Z

Changed commit from 895b48f to none

slel · 2017-11-18T22:38:33Z

comment:43

(Minor fix to ticket description formatting.)

thecaligarmo added this to the sage-8.0 milestone Jun 18, 2017

thecaligarmo added c: packages: standard labels Jun 18, 2017

thecaligarmo added t: enhancement and removed t: bug labels Jun 18, 2017

thecaligarmo added the s: needs review label Jun 18, 2017

This comment has been minimized.

Sign in to view

vbraun added s: positive review and removed s: needs review labels Nov 6, 2017

vbraun added s: needs work and removed s: positive review labels Nov 8, 2017

dimpase added s: positive review and removed s: needs work labels Nov 8, 2017

vbraun removed the s: positive review label Nov 15, 2017

vbraun closed this as completed in 84eb9a5 Nov 15, 2017

This comment has been minimized.

Sign in to view

Uh oh!

OpenBLAS - alterations for TARGET #23272

OpenBLAS - alterations for TARGET #23272

Comments

thecaligarmo commented Jun 18, 2017

thecaligarmo commented Jun 18, 2017

Uh oh!

thecaligarmo commented Jun 18, 2017

Uh oh!

thecaligarmo commented Jun 18, 2017

Uh oh!

thecaligarmo commented Jun 18, 2017

Uh oh!

thecaligarmo commented Jun 18, 2017

Uh oh!

nthiery commented Jun 22, 2017

Uh oh!

jdemeyer commented Jun 30, 2017

Uh oh!

sagetrac-git mannequin commented Jul 14, 2017

Uh oh!

sagetrac-git mannequin commented Jul 14, 2017

Uh oh!

thecaligarmo commented Jul 14, 2017

Uh oh!

This comment has been minimized.

thecaligarmo commented Jul 14, 2017

Uh oh!

vbraun commented Jul 22, 2017

Uh oh!

vbraun commented Jul 22, 2017

Uh oh!

kiwifb commented Jul 24, 2017

Uh oh!

kiwifb commented Jul 24, 2017

Uh oh!

This comment has been minimized.

dimpase commented Jul 24, 2017

Uh oh!

embray commented Jul 31, 2017

Uh oh!

sagetrac-git mannequin commented Aug 10, 2017

Uh oh!

sagetrac-git mannequin commented Aug 10, 2017

Uh oh!

thecaligarmo commented Aug 10, 2017

Uh oh!

dimpase commented Aug 17, 2017

Uh oh!

vbraun commented Sep 28, 2017

Uh oh!

thecaligarmo commented Sep 28, 2017

Uh oh!

vbraun commented Sep 28, 2017

Uh oh!

thecaligarmo commented Sep 28, 2017

Uh oh!

vbraun commented Sep 29, 2017

Uh oh!

sagetrac-git mannequin commented Oct 4, 2017

Uh oh!

sagetrac-git mannequin commented Oct 4, 2017

Uh oh!

thecaligarmo commented Oct 4, 2017

Uh oh!

mo271 commented Oct 12, 2017

Uh oh!

mo271 commented Oct 17, 2017

Uh oh!

vbraun commented Nov 6, 2017

Uh oh!

vbraun commented Nov 8, 2017

Uh oh!

dimpase commented Nov 8, 2017

Uh oh!

dimpase commented Nov 8, 2017

Uh oh!

dimpase commented Nov 8, 2017

Uh oh!

dimpase commented Nov 8, 2017