Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-1229] use OpenBLAS, lapack & OpenCV from conan #13400

Merged
merged 10 commits into from
Jul 18, 2019
Merged

Conversation

SSE4
Copy link

@SSE4 SSE4 commented Nov 25, 2018

Description

conan is an open-source package manager for C++ projects. it allows to manage project dependencies in transparent and declarative manner.

currently, apache incubator-mxnet project uses the following different ways to manage its dependencies:

this appears to be very heterogeneous and hard to manage/maintain, as multiple various commands are in use to achieve dependencies installation, as well as multiple places are to look for dependency versions and their updates.

with conan, it may became much more straightforward, as dependencies will be declared in single place (conanfile) and installed via single command (conan install).

as project is very complex, and has lots of dependencies, for the first prototype I've used only very few of dependencies from conan: OpenCV, OpenBLAS and lapack.
others may be easily added then one by one, but they first has to be packaged (not all of them are packaged yet, e.g. GoogleTest is available, while MKL is not).

for instance, I've used the following script to run local conan build on Windows:

mkdir build
cd build

conan remote add conan-community https://api.bintray.com/conan/conan-community/conan || echo "ignore"

conan install ^
  --update ^
  -s os=Windows ^
  -s arch=x86_64 ^
  -s compiler="Visual Studio" ^
  -s compiler.version=14 ^
  -s compiler.runtime=MT ^
  -s build_type=Release ^
  -g cmake .. 
  
REM --build opencv

cmake .. ^
  -DUSE_PROFILER=1 ^
  -DUSE_CUDA=0 ^
  -DUSE_CUDNN=0 ^
  -DUSE_NVRTC=0 ^
  -DUSE_OPENCV=1 ^
  -DUSE_OPENMP=1 ^
  -DUSE_BLAS=open ^
  -DUSE_LAPACK=1 ^
  -DUSE_DIST_KVSTORE=0 ^
  -DOpenCV_STATIC=ON ^
  -G "Visual Studio 14 2015 Win64"

cmake --build . --config Release

similarly, the same can be achieved on Linux or Mac OS X:

#!/usr/bin/env bash

mkdir -p build
pushd build

conan remote add conan-community https://api.bintray.com/conan/conan-community/conan || echo "ignore"

conan install \
  --update \
  -s arch=x86_64 \
  -s build_type=Release \
  -g cmake ..

cmake .. \
  -DUSE_PROFILER=1 \
  -DUSE_CUDA=0 \
  -DUSE_CUDNN=0 \
  -DUSE_NVRTC=0 \
  -DUSE_OPENCV=1 \
  -DUSE_OPENMP=1 \
  -DUSE_BLAS=open \
  -DUSE_LAPACK=1 \
  -DUSE_DIST_KVSTORE=0 \
  -DOpenCV_STATIC=ON \
  -DCMAKE_BUILD_TYPE=Release \
  -G "Unix Makefiles"

cmake --build . --config Release

popd

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • added conanfile.py to declare project dependencies (for now, just OpenCV, OpenBLAS and lapack)
  • added an optional conan integration to the CMakeLists.txt (project might still be built without conan)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

/cc @memsharded @lasote @danimtb @jgsogo @uilianries

@SSE4 SSE4 requested a review from szha as a code owner November 25, 2018 10:22
@stu1130
Copy link
Contributor

stu1130 commented Nov 25, 2018

@mxnet-label-bot add [pr-awaiting-review]
Thanks for your contribution @SSE4

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Nov 25, 2018
@KellenSunderland
Copy link
Contributor

Hey @SSE4 Thanks for the work here. I'm a big fan of Conan.io. I strongly support using one of the modern C++ package management systems in MXNet. However, for Apache projects we prefer to discuss changes like this in a mailing list so that it reaches all of our core contributors to collect their opinions. Would you be able to send a message to [email protected] summarizing the issue with our current dependency tracking, and how Conan could improve this? Feel free to just c + p most of the text from the description you have here.

@SSE4
Copy link
Author

SSE4 commented Nov 26, 2018

@KellenSunderland sure, I've subscribed to your mail list and moved discussion to the following mail thread

@lebeg
Copy link
Contributor

lebeg commented Nov 27, 2018

Great suggestion, thank you for your contribution @SSE4 !

@SSE4
Copy link
Author

SSE4 commented Dec 6, 2018

unix-gpu error is weird:

Sending interrupt signal to process

any clue why did it fail?

@lupesko
Copy link
Contributor

lupesko commented Dec 18, 2018

@anirudh2290 @frankfliu @samskalicky @Vikas89 @leleamol - can you guys check it out and see if you can help?

@samskalicky
Copy link
Contributor

@SSE4 it looks like a jenkins problem (according to this: https://issues.jenkins-ci.org/browse/JENKINS-37790)

Can you push an empty commit to retrigger the CI?

If that doesnt work maybe @marcoabreu has some insight from the CI side.

@SSE4
Copy link
Author

SSE4 commented Dec 18, 2018

@samskalicky done

@SSE4
Copy link
Author

SSE4 commented Dec 19, 2018

@samskalicky so build is green, feel free to review it further

@samskalicky
Copy link
Contributor

@SSE4 Just to confirm, this change enables conan builds using the example in the description. And can still be built without conan correct? This just adds another way to build for now right?

@SSE4
Copy link
Author

SSE4 commented Dec 19, 2018

@samskalicky yes, you're absolutely right, change adds conan builds which are optional, and you still can build things exactly as you used to build before.

Copy link
Contributor

@samskalicky samskalicky left a comment

Choose a reason for hiding this comment

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

LGTM

@Roshrini
Copy link
Member

Roshrini commented Jan 2, 2019

@anirudh2290 @leleamol Can you review once?

@SSE4
Copy link
Author

SSE4 commented Jan 10, 2019

ping @anirudh2290 @leleamol

Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

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

How does it affect the license of the built artifact, especially for projects with modules that carry multiple licenses? For example, opencv has a contrib module that carries additional licenses, which doesn't get used by mxnet. Do we rely on whoever publishes the particular package we use to conan, to decide which submodules to include?

@stu1130
Copy link
Contributor

stu1130 commented Jan 16, 2019

@SSE4 could you address the feedback from @szha Thanks!

@SSE4
Copy link
Author

SSE4 commented Jan 16, 2019

hi @szha
I am not very good at licensing, probably @memsharded may provide more details.
to start, yes, OpenCV has contrib module, and OpenCV conan package has contrib option as well, which is off by default, so unless you specify contrib=True in your conanfile.txt, it's probably safe to ignore.
however, OpenCV itself has tons of dependencies, and they all use different licenses. all dependencies are optional, and might be turned on or off, to list a few: ffmpeg, protobuf, libpng, libjpeg, etc. such dependencies affect OpenCV features, so depending on your project needs, you may turn some features on or off, therefore enable or disable usage of some dependencies.
moreover, dependencies of OpenCV may have their own dependencies, e.g. ffmpeg may depend on libx264, libx265, libvpx, libmp3lame, etc. depending of its encoding/decoding capabilities. so this makes things even more complicated.
you'd probably will want to inspect which licenses are in use for the complete dependency tree, and we have a command conan info ., which displays information about all packages in use, include licensing information.
the next question, could we rely on publishers that they have specified licenses correctly? I think, for official packages, included in conan center, yes (although sometimes human errors happen, but you can always report on incorrect license via bintray interface). in case you can't rely/trust publishers of packages, you may use your own packages, hosted on your own server and build on your build slaves.
so, once we have a list of licenses in use, we may check how do they affect license of build artifact, which is more likely legal question. some of licenses are viral, some aren't, some may require to include their copyrights or have another restrictions (e.g. allow only shared or only static linking). that's something to be carefully reviewed by competent person. but since you're already using OpenCV, OpenBLAS and many other libraries, but from different sources, probably you're already aware about all license restrictions of that libraries, so the only thing you need is to ensure you don't have any unwanted dependencies, because some option was enabled by default in conan package of OpenCV, OpenBLAS, ffmpeg or whatever else.

@sandeep-krishnamurthy
Copy link
Contributor

@szha - Can you please take a look back at this PR? Thanks!

@vandanavk
Copy link
Contributor

@SSE4 could you resolve the branch conflict?

@szha for comments/review

@SSE4
Copy link
Author

SSE4 commented Feb 5, 2019

@vandanavk resolved

@edisongustavo
Copy link
Contributor

edisongustavo commented Feb 6, 2019

If we add Conan support like this won't it require a connection to the conan artifacts repository while building?

My main issue with Conan is that it "spreads over" your Cmake files (like it did in this case). It looks like it is an "all or nothing" solution. What would happen if I do want to compile against the dependencies of my operating system? Could I do that?

Alternative

An alternative to provide all dependencies is to use conda (disclaimer: I'm a big conda fan). If we have a conda-environment file declaring all the dependencies, then the steps would be:

$ cd <mxnet-source-directory>
$ conda env create -f environment.yml -n mxnet-compilation-dependencies
$ conda activate mxnet-compilation-dependencies
$ mkdir build
$ cmake -DCMAKE_PREFIX_PATH=$CONDA_PREFIX ..
$ make

This is something I want to work on the next few weeks, so it is on my roadmap.

Cool, but I want Conan!

I agree that adding Conan support is important, but let's think about it more thoroughly and not let it "spread over" the cmake files.

Let's think more deeply in how to add Conan support into Mxnet.

@SSE4
Copy link
Author

SSE4 commented Feb 6, 2019

@edisongustavo
1.there is non-intrusive CMake integration for conan available as well, check out this article: conan: transparent CMake integration
2. it's still possible to use dependencies provided by operation system
3. I am not sure about your concern of connection required for conan. I think it's the same situation for conda, isn't it?

@edisongustavo
Copy link
Contributor

1.there is non-intrusive CMake integration for conan available as well, check out this article: conan: transparent CMake integration

Great! Thanks for the link. I am in favor of using this "Transparent CMake integration: “cmake_paths” generator". So we could do the dance:

$ rm -rf build && mkdir build && cd build
$ conan install ..
$ cmake .. -DCMAKE_BUILD_TYPE=Release -DCMAKE_TOOLCHAIN_FILE=./conan_paths.cmake
$ cmake --build .

This is very similar to what would be the conda dance:

$ conda env create -f environment.yml -n mxnet-dependencies
$ conda activate mxnet-dependencies
$ rm -rf build && mkdir build && cd build
$ cmake .. -DCMAKE_BUILD_TYPE=Release -DCMAKE_PREFIX_PATH=$CONDA_PREFIX
$ cmake --build .

I have some comments and questions about the conan part:

In the blog post it shows the integration using the variables created by each library (eg: ${CURL_INCLUDE_DIRS} and ${CURL_LIBRARIES}). This is the deprecated way of linking with targets. Using imported targets is the preferred way (if possible) and the CURL example is no exception. You can see that in the FindCURL.cmake file you can use the imported target CURL::libcurl, which should take care of all transitivity and order of linking problems.

Is there something I'm missing here?

  1. it's still possible to use dependencies provided by operation system

Great! I believe that it could be done if we use conan along the route specified in my comment above, correct?

  1. I am not sure about your concern of connection required for conan. I think it's the same situation for conda, isn't it?

Given that you've sent that blog post it has clarified things for me. My concern was in the case of conan working only in this "intrusive mode" (where you have to modify your CMakeLists.txt) so then requiring the internet connection every time you call cmake. Since we could use the "cmake_paths" generator then this is not a concern anymore.

A question for you: How easy is it to mirror a conan repository? Or maybe only mirroring the packages we need. Is that possible?

@SSE4
Copy link
Author

SSE4 commented Feb 7, 2019

@edisongustavo

  1. for sure I know about targets vs variables approaches in CMake. I suppose it's just matter of minimum CMake version being targeted, as there are still many projects targetting CMake 2.X, article mentions variable approach being more compatible, in general. as MXNet is targeting 3.0 and higher, it's safe to use CMake targets instead.
  2. yes, it's possible to use system libraries with route specified, and via conditional requirements. I believe, conanfile.py of MXNet will look like:
def requirements(self):
    if not self.options.use_system_openblas:
        self.requires.add("openblas/0.2.20@conan/stable")
    # similar code for other libraries

and then use something like:

conan install -o MXNet:use_system_openblas=True ...

you know, this PR is more like proof-of-concept, and conanfile.py is very flexible in terms of adding custom logic in order to satisfy requirements of specific project (MXNet)
3. yes, mirroring is definitely possible, and some conan users have already set up mirroring for their repositories. I think it's just matter of simple script combining conan download and conan upload commands.

szha
szha previously requested changes May 6, 2019
Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

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

Can we have an option to turn on and off the dependency management by conan?

@SSE4
Copy link
Author

SSE4 commented May 6, 2019

@szha yes, why not

@SSE4
Copy link
Author

SSE4 commented May 7, 2019

@anirudhacharya @KellenSunderland
so, vote thread was started, and now it's finished and results are available: https://lists.apache.org/thread.html/53305927a3730711f84f7d8ef8d2350ef50ab85492afa11e8ed4daa2@%3Cdev.mxnet.apache.org%3E
copying here as well:

Dear MXNet community,

I'm happy to announce the results of the vote.

This vote passes with 0 +1 votes (0 binding), no 0 votes, and 2 -1 vote.
+1 votes
* No votes  

0 votes
* No votes

-1 votes
* kellen sunderland
* Sheng Zha 

Vote thread can be found here [1]. The list of members can be found here
[2].


Best regards,
Konstantin Ivlev

[1]
https://lists.apache.org/thread.html/9b9a1fc32702a00b1d05b72bc69075ba63871c1a5f6ea33c47f74343@%3Cdev.mxnet.apache.org%3E  
[2] http://incubator.apache.org/projects/mxnet.html

what's the next steps?

@pinaraws
Copy link

@mxnet-label-bot add[pr-awaiting-response]

@marcoabreu marcoabreu added the pr-awaiting-response PR is reviewed and waiting for contributor to respond label May 20, 2019
@pinaraws
Copy link

@mxnet-label-bot remove[pr-awaiting-review]

@marcoabreu marcoabreu removed the pr-awaiting-review PR is waiting for code review label May 20, 2019
@pinaraws
Copy link

@SSE4 Gentle ping to address requested changes

@SSE4
Copy link
Author

SSE4 commented May 20, 2019

@szha I've added option for conan

@SSE4
Copy link
Author

SSE4 commented May 20, 2019

@pinaraws changes were addressed

@pinaraws
Copy link

@mxnet-label-bot add[pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label May 30, 2019
@pinaraws
Copy link

@mxnet-label-bot remove[pr-awaiting-response]

@marcoabreu marcoabreu removed the pr-awaiting-response PR is reviewed and waiting for contributor to respond label May 30, 2019
@piyushghai
Copy link
Contributor

@szha Gentle ping for review....

@vandanavk
Copy link
Contributor

@marcoabreu for review

@szha szha dismissed their stale review June 21, 2019 03:24

concerns addressed. thanks!

@karan6181
Copy link
Contributor

@SSE4 is this PR good to go for merge?

@SSE4
Copy link
Author

SSE4 commented Jul 18, 2019

@karan6181 yes

@karan6181
Copy link
Contributor

@szha Could you please review this PR again and merge if it's looks correct ? Thanks!

@marcoabreu marcoabreu merged commit 08a09ef into apache:conan Jul 18, 2019
@edisongustavo edisongustavo mentioned this pull request Aug 5, 2019
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.