Skip to content

Conversation

@lexming
Copy link
Contributor

@lexming lexming commented Feb 11, 2020

Update for openblas easyblock to detect and raise an error on failed tests. Currently OpenBLAS is running its tests in the build step and the installation proceed regardless of their result.

  • build_step will run a custom make command that is equivalent to make all but without the tests
  • test_step will run the tests and raise an exception if any test fails.

Additional issues to be solved:

  • (already present in current easyblock) many compilation variables are redefined. Example

gcc -O2 -ftree-vectorize -march=native -fno-math-errno -O2 -DMAX_STACK_ALLOC=2048 -fopenmp -Wall -m64 -DF_INTERFACE_GFORT -fPIC - DSMP_SERVER -DUSE_OPENMP -DNO_WARMUP -DMAX_CPU_NUMBER=24 -DMAX_PARALLEL_NUMBER=1 -DVERSION="0.3.7" -march=skylake-avx512 -DASMNAME= -DASMFNAME=_ -DNAME=_ -DCNAME= -DCHAR_NAME="_" -DCHAR_CNAME="" -DNO_AFFINITY -I. *-O2 -DMAX_STACK_ALLOC=2048 -fopenmp -Wall -m64 -DF_INTERFACE_GFORT -fPIC -DSMP_SERVER -DUSE_OPENMP -DNO_WARMUP -DMAX_CPU_NUMBER=24 -DMAX_PARALLEL_NUMBER=1 -DVERSION="0.3.7" -march=skylake-avx512 -DASMNAME=cblas_isamax -DASMFNAME=cblas_isamax_ -DNAME=cblas_isamax_ -DCNAME=cblas_isamax -DCHAR_NAME="cblas_isamax_" -DCHAR_CNAME="cblas_isamax" -DNO_AFFINITY -I.. -I. -UDOUBLE -UCOMPLEX -DCBLAS -c -DUSE_ABS -UUSE_MIN imax.c -o cblas_isamax.o

  • (introduced by this PR) Header file lapack.h is not installed

@lexming lexming requested a review from boegel February 11, 2020 17:36
akesandgren
akesandgren previously approved these changes Feb 11, 2020
Copy link
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@akesandgren akesandgren added this to the 4.x milestone Feb 11, 2020
@boegel boegel modified the milestones: 4.x, next release (4.1.2?) Feb 11, 2020
@boegel boegel added the bug fix label Feb 11, 2020
@lexming lexming linked an issue Feb 11, 2020 that may be closed by this pull request
@akesandgren
Copy link
Contributor

FYI, lapack.h has never been installed, lapacke.h is the one that gets installed.

@martin-frbg
Copy link

The lapack.h thing is a recent change by Reference-LAPACK a.k.a netlib - in 3.9.0 the pure lapack definitions have been split out into their own header file for easier use with a future LAPACK++
The redefiniton of variables is (usually) an unwanted side effect of passing CFLAGS via the environment (OpenBLAS issue 818, I tried to fix it a few years ago but got nowhere with the current mess of makefiles that tend to include each other )

@lexming
Copy link
Contributor Author

lexming commented Feb 12, 2020

@martin-frbg Thank you very much for the feedback, it's been really helpful to better understand these issues.

@lexming lexming changed the title [WIP] openblas: remove tests from build_step and raise error on failed tests openblas: remove tests from build_step and raise error on failed tests Feb 12, 2020
@lexming
Copy link
Contributor Author

lexming commented Feb 12, 2020

@akesandgren @boegel all issues fixed, no longer a WIP

@akesandgren
Copy link
Contributor

Tested on broadwell and skylake with easybuilders/easybuild-easyconfigs#9852
broadwell -> no problem
skyake -> caught the fatal errors

No duplication of variable settings.
lapack.h installed.

Perfect...

Copy link
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@akesandgren akesandgren removed the request for review from boegel February 12, 2020 07:27
@akesandgren
Copy link
Contributor

Going in, thanks @lexming!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OpenBLAS installs regardless of failed tests

4 participants