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

[lapack] add metaport lapack to switch lapack impl with an overlay #12464

Merged
merged 6 commits into from
Aug 7, 2020

Conversation

Neumann-A
Copy link
Contributor

In preparation to add netlibs lapack-reference from #11454

@cenit
Copy link
Contributor

cenit commented Jul 17, 2020

in order to be ready even better, do you have any idea on how to handle osx inside lapack-select?
Whenever we will have lapack-reference, it would be better to use it instead of Accelerate Framework!
This will make it very easy to enable it for all lapack-depending ports

@Neumann-A
Copy link
Contributor Author

@cenit what about all those openblas (!osx)? Can they also be removed since lapack will depend on blas?

added external as a feature.
made a dummy project to find the external provided LAPACK
@Neumann-A
Copy link
Contributor Author

in order to be ready even better, do you have any idea on how to handle osx inside lapack-select

@cenit see latest commit. You basically need a select feature with a build-depends which has the platform conditions. The feature is required to not always force the dependency.

@cenit
Copy link
Contributor

cenit commented Jul 17, 2020

@cenit what about all those openblas (!osx)? Can they also be removed since lapack will depend on blas?

OpenBLAS has improved a lot and we can use it on vcpkg now maybe. For sure, it's not like clapack (which is not acceptable on osx)

@cenit
Copy link
Contributor

cenit commented Jul 17, 2020

in order to be ready even better, do you have any idea on how to handle osx inside lapack-select

@cenit see latest commit. You basically need a select feature with a build-depends which has the platform conditions. The feature is required to not always force the dependency.

Convoluted but effective. Nice!

@Neumann-A
Copy link
Contributor Author

Neumann-A commented Jul 17, 2020

OpenBLAS has improved a lot and we can use it on vcpkg now maybe. For sure, it's not like clapack (which is not acceptable on osx)

@cenit: The question was more is at a real openBLAS dependency or is it a indirect dependency due to LAPACK which should not be listed in Build-Depends.

@cenit
Copy link
Contributor

cenit commented Jul 17, 2020

We don't want blas functions from lapack-reference, so we should disable internal blas and add a real dependency on openblas. Similarly, the cblas feature should disappear from lapack-reference, since the C API for blas will be provided by openblas.
Each one for the best part they provide :)

@Neumann-A
Copy link
Contributor Author

We don't want blas functions from lapack-reference

Yeah that is clear. The question was if the ports actually use BLAS functions or if they only use LAPACK functions and thus only indirectly depend on BLAS.

@cenit
Copy link
Contributor

cenit commented Jul 17, 2020

Ah now I understand what you mean
ports depending on openblas (!osx), clapack (!osx) in fact might look suspicious, because depending on clapack (!osx) should have been enough, since clapack then internally uses blas, provided for us by openblas. But it wouldn't be so strange if they were using lapack for something, but also blas for other things, independently of their relationship, and thus correctly depending on both anyway.
I would have to check their sources. A quick way would be to scan for find_package(BLAS) and find_package(LAPACK). If only the latter, then the former would be an unnecessary direct dependency

Homepage: https://github.com/mlpack/mlpack
Description: mlpack is a fast, flexible machine learning library, written in C++, that aims to provide fast, extensible implementations of cutting-edge machine learning algorithms.
Build-Depends: openblas (!osx), clapack (!osx), boost, armadillo, ensmallen, stb
Build-Depends: openblas (!osx), lapack-select[core], boost, armadillo, ensmallen, stb
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem to depend on openblas or lapack directly.

@JackBoosY JackBoosY added category:new-port The issue is requesting a new library to be added; consider making a PR! category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Jul 20, 2020
@JackBoosY JackBoosY added the requires:testing Needs tests added before merging label Jul 24, 2020
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines requires:discussion and removed requires:testing Needs tests added before merging labels Jul 27, 2020
@strega-nil
Copy link
Contributor

@Neumann-A is there a reason that you're depending on lapack-select[core] as opposed to just lapack-select in the dependencies?

@Neumann-A
Copy link
Contributor Author

@strega-nil: depending on lapack-select always means lapack-select[core,default] which forces the default feature as a build dependency. So lapack-select[core,otherfeature] does not fulfill lapack-select

@JackBoosY
Copy link
Contributor

@strega-nil See #11602, use core in Build-Depends doesn't work currentlly.

@strega-nil
Copy link
Contributor

@Neumann-A I see... so we want the lapack-select dependency to be fulfilled when the user installs lapack-select with her own chosen feature... hrmm...

@Neumann-A
Copy link
Contributor Author

@strega-nil vcpkg still chooses a default for the user if it is not explicutly specified. If you ask why something like this is necessary just take a peek in cmakes FindLAPACK.cmake

@strega-nil
Copy link
Contributor

Right, yeah, I'm just thinking about how best to do this, since at some point I'm hoping that we stop installing default features for dependencies listed with core.

@ras0219
Copy link
Contributor

ras0219 commented Jul 30, 2020

Features are designed to be additive, not exclusive; they shouldn't be used to model "exactly-one-of" -- they model "Any subset of". Since this is an "exactly-one-of", it isn't a good fit for features.

Instead, for this situation, our current best approach is to use overlay ports. In this approach, there is an intermediate port exactly like in this PR, ideally named like the interface (perhaps just lapack and just blas). That port is an empty "alias" for the default implementation:

# CONTROL
Source: lapack
Version: 0
Description: Metapackage for packages which provide LAPACK
Build-Depends: clapack (!osx)
# portfile.cmake
set(VCPKG_POLICY_EMPTY_PACKAGE enabled)

Users can then redirect this metapackage by copying the port, replacing the Build-Depends: however they like, and using --overlay-ports=.

@JackBoosY JackBoosY added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines requires:discussion labels Jul 31, 2020
@Neumann-A Neumann-A changed the title [lapack-select] add port lapack-select for selecting lapack implementation [lapack] add metaport lapack to switch lapack impl with an overlay Jul 31, 2020
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Aug 3, 2020
@strega-nil
Copy link
Contributor

cc @ras0219 this look good to you?

@strega-nil strega-nil merged commit da839ba into microsoft:master Aug 7, 2020
@Neumann-A Neumann-A deleted the add_lapack_select branch August 13, 2020 21:02
@BillyONeal BillyONeal mentioned this pull request Nov 2, 2020
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! category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants