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

make Amber easyblock aware of FlexiBLAS #2720

Merged
merged 2 commits into from
Aug 25, 2022

Conversation

boegel
Copy link
Member

@boegel boegel commented May 5, 2022

(created using eb --new-pr)

Comment on lines +144 to +146
# note: for Amber 20, a patch is required to fix the CMake scripts so they're aware of FlexiBLAS:
# - cmake/patched-cmake-modules/FindBLASFixed.cmake
# - cmake/patched-cmake-modules/FindLAPACKFixed.cmake
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this code is traversed for Amber 19 too, don't we need a version check here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be harmless as is: if you're using a toolchain that includes FlexiBLAS, you'll use -DBLA_VENDOR=FlexiBLAS, which will cause a clear configuration error if the patches are not included (they most likely also work with Amber 19, I didn't check).
The patch is available in fwautele/easybuild-easyconfigs#1, so it should become part of easybuilders/easybuild-easyconfigs#14386 soon (the patch for Amber 20 is exactly the same as for AmberTools 21).

If you're using a toolchain that doesn't include FlexiBLAS but does include OpenBLAS, you'll use -DBLA_VENDOR=OpenBLAS, which is what the CMake script does automatically already (we're just making that explicit now and taking control).

For other toolchains, we don't specify -DBLA_VENDOR and we leave it up to the CMake scripts to figure out which BLAS/LAPACK to use, which is also what happens without these changes.

Long story short: none of the existing easyconfigs should be "hurt" by the changes being made here...

if mklroot:
env.setvar('MKL_HOME', os.getenv('MKLROOT'))
elif openblasroot:
elif flexiblas_root or openblas_root:
lapack = os.getenv('LIBLAPACK')
if lapack is None:
raise EasyBuildError("LIBLAPACK (from OpenBLAS) not found in environment.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change this error to mention the correct BLAS? i.e. FlexiBLAS or OpenBLAS?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @boegel

Copy link
Member Author

Choose a reason for hiding this comment

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

… not defined when using OpenBLAS or FlexiBLAS
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
Copy link
Contributor

Going in, thanks @boegel!

@akesandgren akesandgren merged commit 6cfcfa9 into easybuilders:develop Aug 25, 2022
@boegel boegel deleted the 20220505222438_new_pr_amber branch August 25, 2022 13:30
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.

2 participants