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

[BLAS] add metaport #13448

Merged
merged 7 commits into from
Nov 18, 2020
Merged

[BLAS] add metaport #13448

merged 7 commits into from
Nov 18, 2020

Conversation

cenit
Copy link
Contributor

@cenit cenit commented Sep 10, 2020

Almost identical to LAPACK approach, I am suggesting this metaport to ease BLAS+LAPACK port override (macOS Accelerate framework is almost the only one working properly on the upcoming Apple Silicon)

@cenit
Copy link
Contributor Author

cenit commented Sep 10, 2020

Note: I added f90ffde commit to keep current BLAS setup also on macOS. It's an open question if we should use Apple Accelerate framework there by default (and in this case just reverting the commit and make blas depend on openblas(!osx)

ports/clapack/CONTROL Outdated Show resolved Hide resolved
@NancyLi1013 NancyLi1013 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Sep 11, 2020
Copy link
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

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

Is it necessary to update Port-Version for other ports?

@cenit
Copy link
Contributor Author

cenit commented Sep 24, 2020

Any feedback here to consider for merging?

@NancyLi1013
Copy link
Contributor

I noticed that Port-Version is not updated for some ports. I'm not sure if it is necessary to update it.

Note: Seems need to resolve the conflicts caused by shogun.

@cenit
Copy link
Contributor Author

cenit commented Sep 25, 2020

I noticed that Port-Version is not updated for some ports. I'm not sure if it is necessary to update it.

Note: Seems need to resolve the conflicts caused by shogun.

Ok but @ras0219 requested port version bump only for clapack, I assume the others are ok in this way

@cenit
Copy link
Contributor Author

cenit commented Oct 5, 2020

@NancyLi1013 may I ask why "requires:author-response"?

@NancyLi1013
Copy link
Contributor

@cenit
Sorry for the late delay. We're on holiday in these days.
There were some conflicts caused by shogun before this. So I added the label. Seems you have solved them now. I will try to rerun and check the new results. I will remove this label if all CI checks have passed.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@NancyLi1013
Copy link
Contributor

@cenit

Could you please resolve the conflicts and also merge master?

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Oct 30, 2020
@NancyLi1013
Copy link
Contributor

LGTM now, thanks for your PR @cenit.

ports/blas/CONTROL Show resolved Hide resolved
ports/blas/portfile.cmake Show resolved Hide resolved
ports/blas/CMakeLists.txt Outdated Show resolved Hide resolved
@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Oct 30, 2020
@cenit
Copy link
Contributor Author

cenit commented Oct 31, 2020

@BillyONeal i copy-pasted same exact messages and behavior from the lapack port! But I will modify pr according to your review.

About the openblas(!osx), it’s a residual from the past. In the beginning I did a pr to exclude OpenBLAS and clapack from Ports using them on osx, because the accelerate framework was better. Then lapack-reference came and it superseded the behavior, to uniform platform better. This made OpenBLAS the default everywhere, too.
Now with the advent of arm64 arch for macOS the accelerate framework is very relevant again, so also an easy way to override vcpkg default to its own blas/lapack libs

@BillyONeal
Copy link
Member

@Neumann-A Can you explain what those comments are saying that were added in #12464 ? It seems like the lapack port is trying to be both an indicator of use of a system outside-of-vcpkg provided lapack but it simultaneously wants clapack or lapack-reference to provide an implementation.

/cc @strega-nil

@Neumann-A
Copy link
Contributor

@BillyONeal: there was a external/extern feature once but somebody did not like it. The idea was if you install lapack[core, extern] the lapack port simply checked if find_package(LAPACK) finds something. anyway the ports needs updating for the intel-mkl port which should also be able to supply blas/lapack functionality

@BillyONeal
Copy link
Member

@Neumann-A I see, so the comment is just wrong and should be removed from both places?

@Neumann-A
Copy link
Contributor

@BillyONeal: I still think the port should be used to test if find_package(BLAS/LAPACK) works. Maybe rename the project and remove the comment.

@BillyONeal
Copy link
Member

@Neumann-A That's fine, I'm objecting to the comment, not the behavior

@strega-nil strega-nil merged commit 3e2120f into microsoft:master Nov 18, 2020
@strega-nil
Copy link
Contributor

Thanks @cenit, this is great :)

@cenit cenit deleted the dev/cenit/blas branch February 8, 2021 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants