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

[armadillo] Add dependent port superlu on osx #11063

Merged
merged 4 commits into from
Apr 30, 2020

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Apr 28, 2020

Official document:

### 4: Compilers and External Dependencies

Armadillo makes extensive use of template meta-programming and many other advanced C++ features. As such, C++ compilers which do not fully implement the C++ standard may not work correctly.

The functionality of Armadillo is partly dependent on other libraries: LAPACK, BLAS (preferably OpenBLAS), ARPACK and SuperLU. LAPACK and BLAS are used for dense matrices, while ARPACK and SuperLU are used for sparse matrices.

Armadillo can work without the above libraries, but its functionality will be reduced. Basic functionality will be available (eg. matrix addition and multiplication), but operations like eigen decomposition or matrix inversion will not be. Matrix multiplication (mainly for big matrices) may not be as fast.

As Armadillo is a template library, we recommended that optimisation is enabled during compilation of programs that use Armadillo. For example, for GCC and Clang compilers use -O2 or -O3

Related: #10767 #11037 #11018 #10476 #11016 #11036 #11020 #10295 #8871 #11058 #11017 #11057 #9550 #11010 #11055 #11057 #11058 #10943.

@JackBoosY JackBoosY added the info:internal This PR or Issue was filed by the vcpkg team. label Apr 28, 2020
@JackBoosY JackBoosY marked this pull request as ready for review April 28, 2020 05:49
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Apr 28, 2020
@JackBoosY
Copy link
Contributor Author

@strega-nil Please merge this PR first.

Thanks.

@cenit
Copy link
Contributor

cenit commented Apr 28, 2020

Why superlu is a dependency (and not a feature maybe) for armadillo only for osx?

Also, I'm not sure superlu port is ok.
On macOS it's best to avoid installing openblas, since the Accelerate framework (native in osx) includes a better version and is already available. So superlu in its control file should depend (like any other port) on openblas(!osx)...

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cbezault
Copy link
Contributor

Like @cenit pointed out superlu definitely needs to not install OpenBLAS on macOS and use the Accelerate framework instead.

Looking at the doc @JackBoosY is quoting from I'm also surprised we don't need superlu on other platforms. Perhaps OpenBLAS provides some of the same functions as superlu (OpenBLAS is known to do that kind of stuff).

@cenit
Copy link
Contributor

cenit commented Apr 28, 2020

@cbezault Exactly. Please do not merge confused pr only to apparently fix things.

@BillyONeal
Copy link
Member

@cbezault Exactly. Please do not merge confused pr only to apparently fix things.

It doesn't seem like a confused PR, it seems like a PR correctly documenting the status quo, since the only way this passed before was accidentally getting superlu on the box first?

@cenit
Copy link
Contributor

cenit commented Apr 28, 2020

  1. Superlu is not mandatory for armadillo
  2. If it were mandatory, why is it only for osx?
  3. This pr is indirectly installing OpenBLAS on OS X, which should be avoided

@BillyONeal
Copy link
Member

  1. And yet, the build for it is failing due to a missing superlu header.
  2. No idea.
  3. Maybe, but the PR seems to be documenting the status quo rather than changing something here.

@cenit
Copy link
Contributor

cenit commented Apr 28, 2020

  1. This should be investigated, not hidden in a confused pr, sorry for insisting
  2. That’s the problem. See 1
  3. Armadillo was working on OS X well before the very recent superlu port...

@BillyONeal
Copy link
Member

3. Armadillo was working on OS X well before the very recent superlu port...

Other ports vendored superlu. For example hypre.

@strega-nil
Copy link
Contributor

I think this is not correct; it works fine without updating armadillo right now on my macbook, this seems like an issue with a different port that's getting installed. Can we just skip it in CI for now and try and figure it out later?

@BillyONeal
Copy link
Member

@strega-nil You are the mac wizard :)

@cbezault
Copy link
Contributor

A vendored superlu is a plausible explanation for what's going on but I have to agree with @cenit, it's just an unknown right now. I'd rather see this port get skipped and we actually fix it right.

@Neumann-A
Copy link
Contributor

  message(STATUS "     MKL_FOUND = ${MKL_FOUND}"     )
  message(STATUS "  ACMLMP_FOUND = ${ACMLMP_FOUND}"  )
  message(STATUS "    ACML_FOUND = ${ACML_FOUND}"    )
  message(STATUS "OpenBLAS_FOUND = ${OpenBLAS_FOUND}")
  message(STATUS "   ATLAS_FOUND = ${ATLAS_FOUND}"   )
  message(STATUS "    BLAS_FOUND = ${BLAS_FOUND}"    )
  message(STATUS "  LAPACK_FOUND = ${LAPACK_FOUND}"  )

include(ARMA_FindARPACK)
message(STATUS "ARPACK_FOUND = ${ARPACK_FOUND}")

include(ARMA_FindSuperLU5)
message(STATUS "SuperLU_FOUND = ${SuperLU_FOUND}")

Armadillo's CMakeLists.txt probably needs to be patched to put all those dependencies behind an option

BLAS and LAPACK are already patched

@BillyONeal
Copy link
Member

#11091 just adds to the baseline.

@LilyWangL LilyWangL added wip and removed info:reviewed Pull Request changes follow basic guidelines labels Apr 30, 2020
@ras0219-msft
Copy link
Contributor

Since this is the underlying issue causing the mlpack:x64-osx regression, I've added a patch that disables all the dynamic dependency enablement. Through future PRs, these individual dependencies can be reenabled via features :)

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Apr 30, 2020

Note: the OSX CI check appears offline, so this should not be merged until that is resolved.

@Neumann-A
Copy link
Contributor

Since this is the underlying issue causing the mlpack:x64-osx regression

It is one reason. The other reason is that some port is vendoring SuperLU. So somebody needs to find the port doing it

@BillyONeal
Copy link
Member

Note: the OSX CI check appears offline, so this should not be merged until that is resolved.

It's part of the 'microsoft.vcpkg' check now :)

@ras0219-msft ras0219-msft merged commit 695d2a8 into microsoft:master Apr 30, 2020
@JackBoosY JackBoosY deleted the dev/jack/fix-armadillo-osx branch May 6, 2020 02:49
@JackBoosY JackBoosY removed the wip label May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants