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

[metapackage] BLAS #1121

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

krystophny
Copy link
Contributor

@krystophny krystophny commented Apr 6, 2025

Merge after #1120 and fixing Intel and Mac (probably with MKL and Accelerate BLAS)

@krystophny krystophny force-pushed the metapackage_blas branch 3 times, most recently from d092129 to 11e0295 Compare April 6, 2025 11:41
@krystophny krystophny changed the title Metapackage for BLAS Draft: Metapackage for BLAS Apr 6, 2025
@krystophny krystophny changed the title Draft: Metapackage for BLAS Metapackage for BLAS Apr 7, 2025
@krystophny krystophny changed the title Metapackage for BLAS Draft: Metapackage for BLAS Apr 7, 2025
@krystophny krystophny changed the title Draft: Metapackage for BLAS Metapackage for BLAS Apr 8, 2025
@krystophny krystophny force-pushed the metapackage_blas branch 2 times, most recently from 2f98109 to a339e69 Compare April 9, 2025 08:06
@krystophny
Copy link
Contributor Author

krystophny commented Apr 10, 2025

Oh, I see now that Windows tests are merged from main, not all of them pass. Can you re-run the gcc13 one to see if this is by chance, or has something to do with this PR on BLAS? Edit: Tests are re-run right now due to one more merge from main.

@krystophny krystophny changed the title Metapackage for BLAS [metapackage] BLAS Apr 10, 2025
Copy link
Member

@perazz perazz left a comment

Choose a reason for hiding this comment

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

Nice work @krystophny. Of course there are plenty BLAS/LAPACK versions, and this will grow with time, but it's a good way to begin supporting BLAS/LAPACK as a metapackage imho.

Comment on lines 39 to 43
this%flags = string_t("-framework Accelerate")
this%has_build_flags = .true.
this%link_flags = string_t("-framework Accelerate")
this%has_link_flags = .true.
return
Copy link
Member

Choose a reason for hiding this comment

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

Most macOS versions (>=10.15) will have Accelerate, but there are users that employ very old versions as well. Should we attempt introspection #1128 first to check whether Accelerate is supported?

Suggested change
this%flags = string_t("-framework Accelerate")
this%has_build_flags = .true.
this%link_flags = string_t("-framework Accelerate")
this%has_link_flags = .true.
return
if (.not.compiler%check_flags_supported( &
compile_flags="-framework Accelerate", &
link_flags="-framework Accelerate")) then
call fatal_error(error,'no Accelerate framework detected on the local system')
return
endif
this%flags = string_t("-framework Accelerate")
this%has_build_flags = .true.
this%link_flags = string_t("-framework Accelerate")
this%has_link_flags = .true.
return

Note that we don't necessarily have to return with an error, we could just fallback to the pkg-config evaluation as a second choice. The same could be done for MKL and the Intel compilers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree we should check this and opted for the pkg-config fallback solution now. Let's see if all tests still pass.

@krystophny krystophny force-pushed the metapackage_blas branch 3 times, most recently from 8c67557 to c137d7d Compare April 10, 2025 21:38
@krystophny
Copy link
Contributor Author

Tests are passing, should be fine to merge now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants