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.

Sign up for GitHub

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

Jump to bottom

Update Github actions #298

Merged
merged 5 commits into from
Nov 15, 2024
Merged

Update Github actions #298

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d4cd5c9
Update github macos to apple silicon
hughcars Nov 14, 2024
b888d6d
Prefer OpenBLAS over other non-handled vendors
hughcars Nov 14, 2024
f5a8756
Explicitly add julia to the macos runner
hughcars Nov 14, 2024
90c8399
Attempt to fix mpich + gcc missing math library link on singularity b…
hughcars Nov 15, 2024
24ecb18
Address PR feedback
hughcars Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions .github/workflows/build-and-test-macos.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ jobs:
with-solver: strumpack
with-eigensolver: slepc

runs-on: macos-latest-xl
runs-on: macos-latest-xlarge
steps:
- uses: actions/checkout@v4
with:
Expand All @@ -53,6 +53,10 @@ jobs:
run: |
brew install pkg-config

- uses: julia-actions/setup-julia@v2
with:
version: '1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a recommendation from julia whether to also pin subversion (1.10 / 1.11)? I know these can have some breaking changes, although our usage might be light enough not to worry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I'd rather let it run along at 1 until there's a bug to prevent us otherwise. That way we'll naturally incorporate any updates. Our usage is very light anyway, so should be fine I think.


- name: Configure Open MPI
if: matrix.mpi == 'openmpi'
run: |
Expand Down Expand Up @@ -84,7 +88,7 @@ jobs:
export FC=gfortran-12
fi
if [[ "${{ matrix.math-libs }}" == 'openblas' ]]; then
export OPENBLAS_DIR=/usr/local/opt/openblas
export OPENBLAS_DIR=/opt/homebrew/opt/openblas
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, but might suggest export OPENBLAS_DIR=$(brew --prefix openblas) for consistency with llvm on line 78.

fi
export NUM_PROC_BUILD=$(nproc 2> /dev/null || sysctl -n hw.ncpu)
if [[ "$NUM_PROC_BUILD" -gt "$NUM_PROC_BUILD_MAX" ]]; then
Expand Down
10 changes: 9 additions & 1 deletion cmake/ExternalBLASLAPACK.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,15 @@ else()
else()
set(OPENBLAS_DIR)
endif()

if(NOT OPENBLAS_DIR STREQUAL "")
# If OpenBLAS was found set the vendor to avoid conflict with Accelerate on Darwin
set(BLA_VENDOR "OpenBLAS")
message(STATUS "Using BLAS/LAPACK from OpenBLAS")
else()
message(STATUS "Using BLAS/LAPACK located by CMake")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks ok — for this to trigger we need to not have defined the root paths (ARMPL|AOCL|MKL|OPENBLAS)(_DIR|ROOT|_ROOT). Then it will just call the default find lapack which will look for BLA_VENDOR.

The one question I had — do we want to treat the case specially where BLA_VENDOR is already set at the beginning of the script? Right now BLA_VENDOR is ignored or overwritten without warning/error, if even one of the root paths above is set. (e.g. if ARMPL_DIR is set in the environment and I set BLA_VENDOR=OpenBLAS I will not get OpenBLAS).

Copy link
Collaborator Author

@hughcars hughcars Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this probably worth handling, but I'll do in a follow up PR (#299), as doing it neatly is going to involve a modest amount of refactoring in this file (in particular handling the AOCL section) and then a significant amount of testing for the edges.

endif()

list(APPEND CMAKE_PREFIX_PATH ${OPENBLAS_DIR})
Copy link
Collaborator

@phdum-a phdum-a Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This line (new 175) might be better in the if clause above, although it does nothing if empty.

find_package(BLAS REQUIRED)
find_package(LAPACK REQUIRED)
Expand All @@ -181,7 +190,6 @@ else()
PATH_SUFFIXES include include/openblas
REQUIRED
)
message(STATUS "Using BLAS/LAPACK located by CMake")
endif()

# Save variables to cache
Expand Down
2 changes: 1 addition & 1 deletion cmake/ExternalHYPRE.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ if(NOT CMAKE_C_COMPILER STREQUAL MPI_C_COMPILER)
set(HYPRE_CXXFLAGS "${HYPRE_CXXFLAGS} -I${INCLUDE_DIR}")
endforeach()
string(REPLACE ";" " " HYPRE_MPI_LIBRARIES "${MPI_C_LIBRARIES}")
set(HYPRE_LDFLAGS "${HYPRE_LDFLAGS} ${HYPRE_MPI_LIBRARIES}")
set(HYPRE_LDFLAGS "${HYPRE_LDFLAGS} ${HYPRE_MPI_LIBRARIES} -lm")
endif()

# Use Autotools build instead of CMake for HIP support
Expand Down
Loading