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

Simplify CMake for Armadillo and its Dependencies #2247

Merged
merged 16 commits into from Mar 11, 2020
Merged

Conversation

@birm
Copy link
Member

birm commented Feb 28, 2020

closes #2215

I'm starting various testing. It seems to work fine on the mac os machine that I inexplicably have with armadillo configured both with and without wrapper. That said, I have to test at least with windows before I mark this ready for review.

I think that some people wanted to help with this, namely @knakul853 ... who I now realize may be working on the same thing in parallel... hopefuly I haven't wasted much of their time.

I'll update with more test results. I guess I should test bindings too, even though this shouldn't touch them.

@birm

This comment has been minimized.

Copy link
Member Author

birm commented Feb 28, 2020

@knakul853 If you'd like to extend this, or see anything wrong, let me know. We can add commits from your branch to mine, or whatever you feel is appropriate, with minor abuse of git. :) I'm very sorry for not noticing your reply to 2215 until now

@birm birm force-pushed the birm:cmake-arma-simple branch from c4a37d8 to 29ac921 Feb 28, 2020
@birm birm changed the title start simplify of arma deps Simplify CMake for Armadillo and its Dependencies Feb 28, 2020
@birm

This comment has been minimized.

Copy link
Member Author

birm commented Feb 28, 2020

I'll close this to use my own testing resources, and not clog up the org's. I'm still working on it, and should reopen soon.

@birm birm closed this Feb 28, 2020
@birm birm reopened this Feb 29, 2020
birm added 2 commits Feb 29, 2020
@birm birm marked this pull request as ready for review Feb 29, 2020
@birm

This comment has been minimized.

Copy link
Member Author

birm commented Feb 29, 2020

I still need to test the vcpkg case and minimal cmake version.

@knakul853

This comment has been minimized.

Copy link
Contributor

knakul853 commented Feb 29, 2020

@birm great work I was going the same ...let me know if I can help in any way.

@knakul853

This comment has been minimized.

Copy link
Contributor

knakul853 commented Feb 29, 2020

@birm great work I didn't notice you are working...let me know if I can help anyway :)

@knakul853 If you'd like to extend this, or see anything wrong, let me know. We can add commits from your branch to mine, or whatever you feel is appropriate, with minor abuse of git. :) I'm very sorry for not noticing your reply to 2215 until now

sure

CMakeLists.txt Outdated Show resolved Hide resolved
@@ -60,8 +60,8 @@ steps:
cmake $(CMakeGenerator) `
$(CMakeArgs) `
-DBLAS_LIBRARY:FILEPATH=$(Agent.ToolsDirectory)\OpenBLAS.0.2.14.1\lib\native\lib\x64\libopenblas.dll.a `
-DLAPACK_LIBRARY:FILEPATH=$(Agent.ToolsDirectory)\OpenBLAS.0.2.14.1\lib\native\lib\x64\libopenblas.dll.a `
-DBLAS_LIBRARIES:FILEPATH=$(Agent.ToolsDirectory)\OpenBLAS.0.2.14.1\lib\native\lib\x64\libopenblas.dll.a `

This comment has been minimized.

Copy link
@birm

birm Mar 1, 2020

Author Member

We can probably avoid having to change this flag by adding a case to map X_LIBRARY to X_LIBRARIES for the impacted libraries. Does this seem like a good idea?

This comment has been minimized.

Copy link
@conradsnicta

conradsnicta Mar 3, 2020

Contributor

Is there a specific reason for using such an ancient version (0.2.14) of OpenBLAS ?
OpenBLAS has had releases with many bug fixes since then:

This comment has been minimized.

Copy link
@birm

birm Mar 3, 2020

Author Member

That's true, but the pipeline is currently using nuget, and the latest version there seems to be 0.2.14.1
Though there seem to be other openblas nuget projects, so we could try switching to those to get all the way to 0.2.19.3.
I think it makes more sense to consider another windows package manager such as vcpkg, as that openblas seems to be up to date. Though that's probably a different PR.

This comment has been minimized.

Copy link
@conradsnicta

conradsnicta Mar 4, 2020

Contributor

Well, we can certainly trust the Windows "world" to be a royal mess in many aspects. Instead of jumping through these silly hoops, why not just simply ship a pre-compiled copy of OpenBLAS along with mlpack?

This comment has been minimized.

Copy link
@rcurtin

rcurtin Mar 4, 2020

Member

I think actually that's exactly what we're doing---we're just grabbing the version from nuget. I have no issue switching versions or anything like that... if you want to try that feel free, either in this PR or another or wherever. But yeah... the Windows distribution world is a total nightmare...

@birm

This comment has been minimized.

Copy link
Member Author

birm commented Mar 1, 2020

@knakul853 I'm very interested in your comments on some of the choices made here. Namely, the above comments, but I'd appreciate you giving your thoughts on anything that sticks out to you.

@birm birm requested a review from rcurtin Mar 2, 2020
Copy link
Member

rcurtin left a comment

Awesome work @birm, it'll be nice to remove that bit of code from the main CMakeLists.txt and I think this is heading in the right direction for that. 👍

(edit: I read the diff wrong. The comments below aren't wrong, but they're not relevant...)

A couple comments though---I see that FindArmadillo.cmake is removed; but the version that's by default included with CMake won't find any dependencies of Armadillo if ARMA_USE_WRAPPER is not defined in Armadillo's configuration:

https://github.com/Kitware/CMake/blob/master/Modules/FindArmadillo.cmake

Specifically, you can see that that CMake script doesn't actually look in armadillo_bits/config.hpp to see whether ARMA_USE_WRAPPER is defined.

If ARMA_USE_WRAPPER is defined, which it generally (but not always!) is, then we only need to link against the Armadillo library itself.

If ARMA_USE_WRAPPER isn't defined, then ARMADILLO_LIBRARIES needs to be filled with all of the libraries that Armadillo depends on, which can include any of BLAS, LAPACK, OpenBLAS, ACML, MKL+support libraries, ARPACK, HDF5, and maybe some others. I believe that ARMA_USE_WRAPPER generally won't be defined on Windows, or that it doesn't work right if it is defined, I'm not sure. But either way both cases do need to be supported.

The main difference between the FindArmadillo.cmake here and the one in CMake's repository that I linked to is that the one that was in our repository specifically looked for all those dependencies and handled those cases correctly. So I think we can avoid removing that.

(end irrelevant comments)

When I wrote this originally, I used all the ARMA_*.cmake scripts so that I could match Armadillo's searching as closely as possible: I didn't want a situation where we found Armadillo's dependencies in a different way than Armadillo itself did and thus linked incorrectly. So, maybe it's better to keep depending on those. 👍

.appveyor.yml Show resolved Hide resolved
CMake/FindBlas.cmake Outdated Show resolved Hide resolved
@birm

This comment has been minimized.

Copy link
Member Author

birm commented Mar 4, 2020

FindArmadillo.cmake is not removed here, rather it's essentially replaced with a mixture of the stock and mlpack FindArmadillo; see this section

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Mar 4, 2020

Oh! The Github display in the diff seems to show that lines were removed from it only, and so somehow I assumed that the whole file was removed. My error... let me take a look again now... :)

Copy link
Member

rcurtin left a comment

Okay! Now that I am actually reading the correct diff this makes way more sense! I'll resolve all of the comments I left before that don't make sense. :)

if(ARMA_USE_ARPACK)
find_package(ARPACK REQUIRED)
set(ARMA_SUPPORT_LIBRARIES "${ARMA_SUPPORT_LIBRARIES}" "${ARPACK_LIBRARIES}")
endif(ARMA_USE_ARPACK)

This comment has been minimized.

Copy link
@rcurtin

rcurtin Mar 4, 2020

Member

I think we'll need to handle ARMA_USE_HDF5 too. I do remember a lot of fighting with that when I initially wrote this script, so maybe some of that code should be reused instead of just a simple call to find_package(HDF5). Anyway, maybe some of it's out of date now; I'm not sure how much the HDF5 packaging situation has changed.

This comment has been minimized.

Copy link
@birm

birm Mar 4, 2020

Author Member

Ah, I had misread the HDF5 section in the original findArmadillo and thought it only applied to Armadillo < 4.300

CMake/FindArmadillo.cmake Outdated Show resolved Hide resolved
@birm birm force-pushed the birm:cmake-arma-simple branch from b1840e7 to 89b2086 Mar 4, 2020
@birm birm force-pushed the birm:cmake-arma-simple branch from 89b2086 to 65796c8 Mar 4, 2020
@birm birm force-pushed the birm:cmake-arma-simple branch from b17622c to 6b4148e Mar 4, 2020
birm added 4 commits Mar 4, 2020
@birm birm requested a review from rcurtin Mar 5, 2020
Copy link
Member

rcurtin left a comment

Ok, looking good to me. I believe we have to update some documentation though; it looks like the Windows build guide in doc/guide/build_windows.hpp will need to be updated to use BLAS_LIBRARIES and LAPACK_LIBRARIES instead. Other than that... I think that this will work fine. :) 👍

CMake/FindArmadillo.cmake Outdated Show resolved Hide resolved
@birm birm removed the s: keep open label Mar 5, 2020
@birm birm mentioned this pull request Mar 5, 2020
@birm birm requested a review from rcurtin Mar 6, 2020
@rcurtin rcurtin added this to the mlpack 3.3.0 milestone Mar 9, 2020
@birm

This comment has been minimized.

Copy link
Member Author

birm commented Mar 9, 2020

eventually maybe even this will be unnecessary, since cmake now supports armadillo as non-wrapper.

Copy link
Member

rcurtin left a comment

Awesome work @birm, especially with the upstream submission. 💯

@mlpack-bot
mlpack-bot bot approved these changes Mar 11, 2020
Copy link

mlpack-bot bot left a comment

Second approval provided automatically after 24 hours. 👍

@mlpack-bot mlpack-bot bot removed the s: needs review label Mar 11, 2020
@birm birm merged commit 97fd462 into mlpack:master Mar 11, 2020
16 of 18 checks passed
16 of 18 checks passed
mlpack.mlpack Build #20200305.12 failed
Details
mlpack.mlpack (Linux Python27) Linux Python27 failed
Details
LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
mlpack.mlpack (Linux Julia) Linux Julia succeeded
Details
mlpack.mlpack (Linux Markdown) Linux Markdown succeeded
Details
mlpack.mlpack (Linux Plain) Linux Plain succeeded
Details
mlpack.mlpack (Linux Python37) Linux Python37 succeeded
Details
mlpack.mlpack (Windows VS14 Plain) Windows VS14 Plain succeeded
Details
mlpack.mlpack (Windows VS15 Plain) Windows VS15 Plain succeeded
Details
mlpack.mlpack (Windows VS16 Plain) Windows VS16 Plain succeeded
Details
mlpack.mlpack (macOS Julia) macOS Julia succeeded
Details
mlpack.mlpack (macOS Plain) macOS Plain succeeded
Details
mlpack.mlpack (macOS Python27) macOS Python27 succeeded
Details
mlpack.mlpack (macOS Python37) macOS Python37 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.