Separate dependencies for configurable installation #2523

eelstork · May 28, 2015

Addresses #1738.
The PR benefited earlier attempts by @cypof (#2402) and @kloudkl (#1074).
Options provided include USE_OPENCV, USE_LMDB, USE_LEVELDB, USE_HDF5, USE_SNAPPY, with both make and CMake flags available.
This allows selecting which IO libraries to build with. Wherever possible functions that would behave incorrectly as a result of disabling a library are excluded before runtime.

Added two items to the test matrix:
- WITH_CUDA=false WITH_CMAKE=false WITH_IO=false
- WITH_CUDA=false WITH_CMAKE=true WITH_IO=false

Without bloating the test matrix, it is essential to have at least one of these, otherwise correct separation of these dependencies would be unmaintainable.

flx42 · May 28, 2015

Instead of adding patches fixing your own mistakes, squash multiple patches in one patch and force push to your branch to update the PR.

bhack · May 29, 2015

/cc @Nerei

typo

Don't use definitions. Use caffe_config.h instead

Hm.. in this case the config have to be installed. And I don't know how to do the same for Makefile

Suppose this is not required (just a forward declaration. Also for many methods in headers forward declaration of Mat is also enough

shelhamer · May 29, 2015

@eelstork do keep hacking at this! It will be great to have separate dependencies in line with #1738.

Nerei · May 29, 2015

This PR doesn't generate proper CaffeConfig.cmake. All such definitions should be propagated to client code too.

eelstork · May 30, 2015

@Nerei thanks for the many helpful suggestions. Working on this.

eelstork · Jun 3, 2015

@Nerei the forward declaration of cv::Mat would not be helpful since cv:Mat will not be available at runtime?

Nerei · Jun 3, 2015

@eelstork Agree. If user try to call opencv dependent method, it will get link errors. Well this is all up to code organization. In general, It's preferable to hide as much as possible USE_* defines into cpp, but this might require more significant caffe code change. That's IMHO.

Regarding commit c5ebe85, I think it should be reverted. Such config should be included into each header (otherwise that defines are always undefined) but it is not generated by Make. I put my comment above and then disproved it.

eelstork · Jun 5, 2015

@Nerei referring c5ebe85, If add_definitions() is not portable this may need fixing later but for now caffe_config.h appears to be somewhat ineffective. I would suggest that we leave things as-is (consistent with handling of 'CPU_ONLY' flag).

eelstork · Jun 5, 2015

@Nerei please have a look at the generated CaffeConfig.cmake; if this is not appropriate, I welcome suggestions to improve it.

eelstork · Jun 5, 2015

Pending further feedback, standing by.

flx42 · Jun 5, 2015

Please squash the patches, it's difficult to review right now.

Nerei · Jun 7, 2015

Fine with CaffeConfig.cmake

bhack · Jun 10, 2015

@eelstork Can you rebase and squash?

eelstork · Jun 10, 2015

@bhack @flx42 squashed #2523

keenbrowne · Jun 17, 2015

@shelhamer @bhack @flx42 @eelstork
What work is left to get this merged? We would like to depend on these changes in our caffe based application.

eelstork · Jun 18, 2015

@shelhamer @keenbrowne ready for review/merge.

The trailing comments (# {IO-flag}) might cause problems for value evaluation.

bhack · Jun 18, 2015

@eelstork Have you tested if with this new modularity can build with androd-cmake: https://github.com/taka-no-me/android-cmake/blob/master/README.md?

bhack · Jun 18, 2015

@sh1r0 This add also apk generation capabilties: https://github.com/Discordia/android-cmake

eelstork · Jun 18, 2015

Removed trailing comments in makefile.config. Thanks to @sh1r0 for the suggestion.

eelstork · Jun 18, 2015

@bhack No, I haven't; nor do I know whether it was possible before? Although we hope that removing the dependencies will help porting Caffe to other platforms, I believe that compatibility with a specific platform/config should be addressed separately.

bhack · Jun 18, 2015

@eelstork Yes of course. I've asked only if somebody tested it with this modularization to know now if could be any other modularization that could improve the process with android NDK.

bhack · Jun 21, 2015

I really hope that we are not starting to income in a preprocessor abuse syndrome caused by bad code modularization design.

bhack · Jun 21, 2015

Android effort started at #2619

eelstork · Jun 22, 2015

@bhack We need more information (and more time/resources) to improve modularisation design. Imho, directives can help transition towards a better design later.

eelstork · Jun 27, 2015

Rebased, squashed. Still awaiting review/merge.

bhack · Jun 27, 2015

@eelstork You need to wait. The status of core devs It is still unkown. The last know one was CVPR 2015 and there is no community management other than core devs and occasional BVLC traiger students.

This isn't strictly needed since lmdb / leveldb data that is unencoded does not require OpenCV IO. I think DataLayer should only require lmdb / leveldb. If one tries to use encoded data without OpenCV the DataTransformer guards will report the dependency failure.

Although ugly, it would be safer to always run this for the layers that are instantiated and have guards within the loop to skip the disabled layer types.

"This tool requires OpenCV." would match the text for the examples although it might be best if the message everywhere were "This [whatever] requires [dependency]: compile with [dependency] to use."

Match this to other messages.

shelhamer · Jul 29, 2015

@eelstork thanks for splitting the dependencies! @longjon @jeffdonahue and I agree that it is better to separate IO. I defer to @Nerei regarding the CMake changes; I only know the Makefile (and that looks fine).

hdf5 can be left bundled since we plan to switch to it for serializing weights and solverstates to fix #2006.

Since the Makefile is more simple, the test matrix could include the CMake == true, IO == false case to catch more issues if the two make testing too slow.

Nerei · Jul 31, 2015

CMake changes look fine for me.

eelstork · Jul 31, 2015

@shelhamer about hdf5: currently, default config is with all dependencies enabled to maintain backwards compatibility.
So, if I understand correctly, you suggest that the option to build without HDF5 should be removed.

@shelhamer @Nerei thanks for your feedback, I will update, resolve conflicts and resubmit for review/merge.

bhack · Aug 14, 2015

Actually hdf5 it is the new default model format. So it is an hard dependency for Caffe and cannot be modularized anymore if we not want to fallback to proto. So actually if we follow the defaults the minimal dependencies of caffe are increased.

bhack · Sep 2, 2015

Is this mergiable?

eelstork · Sep 2, 2015

@bhack I have completed several updates reflecting Shelhamer's suggestions; I will merge these back into the PR branch momently; in the meantime please refer https://github.com/eelstork/caffe/tree/separate-io-update
Still need to remove USE_HDF5 but current commit works.

eelstork · Sep 3, 2015

@shelhamer actioned all suggested changes; I would suggest keeping the test matrix as is since it doesn't appear to be slowing things down much and will help maintaining this feature.
Ready for review/merge.

hdf5 should be listed with protobuf, glog, and gflags as required. Could you place these required dependencies under boost in this list so that all the hard requirements come before the optional?

To be clear the Travis CI tests are CPU-only and this should stay in the comments. The GPU code is compiled but not tested for lack of hardware.

shelhamer · Sep 4, 2015

Thanks @eelstork! I made last comments on minor docs + comment changes so this looks good to me for merge once those are taken care of.

Second opinions @longjon @jeffdonahue ?

fix indentation

jeffdonahue · Sep 4, 2015

See above nit, otherwise mostly LGTM. I'd prefer if the #endif guard comments matched the #if ones like the current header guards, like so:

#ifdef USE_LEVELDB
...
#endif  // USE_LEVELDB

But that's minor and a lot of work to change; not really necessary IMO.

bhack · Sep 4, 2015

@jeffdonahue @shelhamer What do you think of the related #2619?

eelstork · Sep 8, 2015

@shelhamer and @jeffdonahue thanks for these suggestions, I have made changes accordingly (see relevant commits); squash?
Pending review/merge.

shelhamer · Sep 8, 2015

Thanks @eelstork! This looks good to me, so please squash for merge. However 526372a can stay its own commit to explain Travis CI is CPU-only although the message should change to reflect that.

eelstork · Sep 9, 2015

@shelhamer Thanks! Before letting go, I suggest we drop the option to explicitly enable/disable snappy. This is because caffe does not directly depend on Snappy, instead, it is LevelDB that requires it. Where build scripts relied on associate flag, USE_LEVELDB would be used instead. Intending to push a commit to illustrate proposed solution.

eelstork · Sep 9, 2015

@shelhamer @jeffdonahue @bhack @Nerei, opinions about 58f017e ?

shelhamer · Sep 9, 2015

@eelstork 584f017e is good, but please squash with a5bdc06 and add an explanatory commit message that lists the new flags and explain that leveldb + snappy are coupled since snappy is just a leveldb dependency. Thanks.

eelstork · Sep 10, 2015

@shelhamer - updated; squashed.

eelstork · Sep 16, 2015

@shelhamer Anything missing to get this merged?

jeffdonahue · Sep 16, 2015

@eelstork I took another look -- sorry for the last minute comments. Otherwise looks good for merge to me.

eelstork · Sep 17, 2015

@jeffdonahue updated. I would suggest leaving 2349c6d as a separate commit.

jeffdonahue · Sep 17, 2015

Thanks @eelstork!

bhack · Sep 17, 2015

@jeffdonahue Can you take a look at related #2619?

mtngld · Sep 17, 2015

Hi,

After pulling the latest from master, rebuilding and caffe train on my LMDB I was getting this error:

I0917 12:13:07.674737 26390 net.cpp:433] data -> data
I0917 12:13:07.674770 26390 net.cpp:433] data -> label
F0917 12:13:07.675629 26393 db.cpp:20] Unknown database backend

Turns out my old Makefile.config is not backward compatible with this PR (+USE_LMDB := 1 was missing).

Solved it easily by copying again the new Makefile.config.example to Makefile.config, and making my regular modifications.

putting this comment here for anyone who might encounter the same issue.

shelhamer · Sep 17, 2015

@mtngld thanks for pointing this out.

@jeffdonahue thoughts on making all IO on by default for backwards compatibility? The Makefile.config / CMake can then explicitly disable dependencies as desired. That seems reasonable to me.

jeffdonahue · Sep 17, 2015

Oops, I didn't realize this changed the default to no IO dependencies. Yes, I agree the default should be to have them all enabled as before.

d4nst · Oct 30, 2015

Currently it is not possible to disable leveldb and lmdb (at least on Windows) which would be useful to use Caffe for deployment only.

eelstork · Oct 31, 2015

@danst18 I believe that disabling LevelDB and LMDB is possible on Windows (via CMake), granted that building caffe on Windows does require tweaks. What motivates your comment?

d4nst · Oct 31, 2015

@eelstork I mean when both of them are disabled at the same time. In db.cpp I get an error saying that GetDB should return a value. If I remove db.h and db.cpp then I have to add guards to other parts of the code where they are referenced, for example in data_reader.cpp

eelstork · Nov 8, 2015

@danst18 sorry for the late reply. I submitted a fix for this. Thank you for reporting the issue.

shelhamer added compatibility build labels May 28, 2015

bhack referenced this pull request Jun 2, 2015
Closed
First attemp to remove boost dependecies. #2537

This was referenced Jun 12, 2015

Closed

Can you rebase the caffe submodule? sh1r0/caffe-android-lib#11

Closed

Provide a Caffe package in Debian #2601

sh1r0 referenced this pull request Jun 18, 2015
Closed
Make HDF5 components optional for configurable build #2619

bhack referenced this pull request in bytedeco/javacpp-presets Aug 14, 2015
Merged
Build Caffe on CentOS 6 with dependencies included #77

shelhamer referenced this pull request Aug 26, 2015
Closed
Enable the users to disable optional dependencies #1074

shelhamer added the focus label Sep 2, 2015

jeffdonahue commented on an outdated diff Sep 4, 2015

View full outdated diff
src/caffe/util/db.cpp

}

-}

+#endif

+ LOG(FATAL) << "Unknown database backend";

+ }

jeffdonahue Sep 4, 2015
Contributor

fix indentation

jeffdonahue added a commit that referenced this pull request Sep 17, 2015

jeffdonahue Merge pull request #2523 from BonsaiAI/separate-io-dependencies
Separate dependencies for configurable installation
5ff3734

jeffdonahue merged commit 5ff3734 into BVLC:master Sep 17, 2015
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

This was referenced Sep 17, 2015

Merged

include IO dependencies by default #3081

Closed

Add Optional AudioDataLayer and LibSndFile Dependancy #3065

Merged

Open LMDB files with MDB_NOLOCK if no write access #3088

Closed

Make IO dependencies optional (leveldb lmdb snappy hdf5 opencv) #2402

lukeyeager referenced this pull request in NVIDIA/caffe Nov 10, 2015
Merged
Adding out of memory handler, making arena bins more detailed #73

eelstork deleted the BonsaiAI:separate-io-dependencies branch Nov 18, 2015

shelhamer referenced this pull request Feb 26, 2016
Closed
Separate dependencies for configurable installation #1738

        caffe_status("  protobuf          : " PROTOBUF_FOUND THEN "Yes (ver. ${PROTOBUF_VERSION})" ELSE "No" )
-    -  caffe_status("  lmdb              : " LMDB_FOUND THEN "Yes (ver. ${LMDB_VERSION})" ELSE "No")
-    -  caffe_status("  Snappy            : " SNAPPY_FOUND THEN "Yes (ver. ${Snappy_VERSION})" ELSE "No" )
-    -  caffe_status("  LevelDB           : " LEVELDB_FOUND THEN  "Yes (ver. ${LEVELDB_VERSION})" ELSE "No")
-    -  caffe_status("  OpenCV            :   Yes (ver. ${OpenCV_VERSION})")
+    +  if(USE_LMDB)
+    +    caffe_status("  lmdb              : " LMDB_FOUND THEN "Yes (ver. ${LMDB_VERSION})" ELSE "No")
+    +  endif()
+    +  if(USE-SNAPPY)

@@ -24,24 +24,36 @@ list(APPEND Caffe_LINKER_LIBS ${GFLAGS_LIBRARIES})
      include(cmake/ProtoBuf.cmake)
      # ---[ HDF5
-    -find_package(HDF5 COMPONENTS HL REQUIRED)
-    -include_directories(SYSTEM ${HDF5_INCLUDE_DIRS} ${HDF5_HL_INCLUDE_DIR})
-    -list(APPEND Caffe_LINKER_LIBS ${HDF5_LIBRARIES})
+    +if(USE_HDF5)
+    +  find_package(HDF5 COMPONENTS HL REQUIRED)
+    +  include_directories(SYSTEM ${HDF5_INCLUDE_DIRS} ${HDF5_HL_INCLUDE_DIR})
+    +  list(APPEND Caffe_LINKER_LIBS ${HDF5_LIBRARIES})
+    +  add_definitions(-DUSE_HDF5)

@@ -65,8 +65,10 @@ private:\
      // is executed we will see a fatal log.
      #define NOT_IMPLEMENTED LOG(FATAL) << "Not Implemented Yet"
+    +#ifdef USE_OPENCV

@@ -7,6 +7,13 @@
      # CPU-only switch (uncomment to build without GPU support).
      # CPU_ONLY := 1
+    +# Comment out to disable IO dependencies
+    +USE_LEVELDB := 1 # {IO-flag}
+    +USE_LMDB := 1    # {IO-flag}
+    +USE_HDF5 := 1    # {IO-flag}
+    +USE_OPENCV := 1  # {IO-flag}
+    +USE_SNAPPY := 1  # {IO-flag}
+    +

@@ -2889,6 +2889,7 @@ TEST_F(NetUpgradeTest, TestImageNet) {
        this->RunV1UpgradeTest(expected_v1_proto, expected_v2_proto);
      }  // NOLINT(readability/fn_size)
+    +#if defined (USE_OPENCV) && defined(USE_HDF5)

		@@ -1,3 +1,4 @@
		+#ifdef USE_OPENCV

BVLC/caffe

Join GitHub today

Separate dependencies for configurable installation #2523

Conversation

eelstork commented May 28, 2015

shelhamer added compatibility build labels May 28, 2015

flx42 commented May 28, 2015

bhack commented May 29, 2015

Show 1 comment Hide comment Nerei commented on an outdated diff May 29, 2015

Show 2 comments Hide comments Nerei commented on an outdated diff May 29, 2015

Show 1 comment Hide comment Nerei commented on an outdated diff May 29, 2015

shelhamer commented May 29, 2015

Nerei commented May 29, 2015

eelstork commented May 30, 2015

bhack referenced this pull request Jun 2, 2015

eelstork commented Jun 3, 2015

Nerei commented Jun 3, 2015

eelstork commented Jun 5, 2015

eelstork commented Jun 5, 2015

eelstork commented Jun 5, 2015

flx42 commented Jun 5, 2015

Nerei commented Jun 7, 2015

bhack commented Jun 10, 2015

eelstork commented Jun 10, 2015

This was referenced Jun 12, 2015

keenbrowne commented Jun 17, 2015

eelstork commented Jun 18, 2015

Show 1 comment Hide comment sh1r0 commented on an outdated diff Jun 18, 2015

bhack commented Jun 18, 2015

bhack commented Jun 18, 2015

eelstork commented Jun 18, 2015

eelstork commented Jun 18, 2015

bhack commented Jun 18, 2015

sh1r0 referenced this pull request Jun 18, 2015

bhack commented Jun 21, 2015

bhack commented Jun 21, 2015

eelstork commented Jun 22, 2015

eelstork commented Jun 27, 2015

bhack commented Jun 27, 2015

shelhamer commented on the diff Jul 29, 2015

Show 1 comment Hide comment shelhamer commented on an outdated diff Jul 29, 2015

Show 1 comment Hide comment shelhamer commented on an outdated diff Jul 29, 2015

Show 1 comment Hide comment shelhamer commented on an outdated diff Jul 29, 2015

shelhamer commented Jul 29, 2015

Nerei commented Jul 31, 2015

eelstork commented Jul 31, 2015

bhack referenced this pull request in bytedeco/javacpp-presets Aug 14, 2015

bhack commented Aug 14, 2015

shelhamer referenced this pull request Aug 26, 2015

bhack commented Sep 2, 2015

eelstork commented Sep 2, 2015

shelhamer added the focus label Sep 2, 2015

eelstork commented Sep 3, 2015

Show 1 comment Hide comment shelhamer commented on an outdated diff Sep 4, 2015

shelhamer commented on the diff Sep 4, 2015

shelhamer commented Sep 4, 2015

Show 1 comment Hide comment jeffdonahue commented on an outdated diff Sep 4, 2015

jeffdonahue commented Sep 4, 2015

bhack commented Sep 4, 2015

eelstork commented Sep 8, 2015

shelhamer commented Sep 8, 2015

eelstork commented Sep 9, 2015

eelstork commented Sep 9, 2015

shelhamer commented Sep 9, 2015

eelstork commented Sep 10, 2015

eelstork commented Sep 16, 2015

jeffdonahue commented Sep 16, 2015

eelstork added some commits Jun 27, 2015

eelstork commented Sep 17, 2015

jeffdonahue commented Sep 17, 2015

jeffdonahue added a commit that referenced this pull request Sep 17, 2015

Hide details View details jeffdonahue merged commit 5ff3734 into BVLC:master Sep 17, 2015 1 check passed

1 check passed

bhack commented Sep 17, 2015

mtngld commented on f3a933a Sep 17, 2015

shelhamer commented Sep 17, 2015

jeffdonahue commented Sep 17, 2015

This was referenced Sep 17, 2015

d4nst commented Oct 30, 2015

eelstork commented Oct 31, 2015

Nerei commented on an outdated diff May 29, 2015

Nerei commented on an outdated diff May 29, 2015

Nerei commented on an outdated diff May 29, 2015

sh1r0 commented on an outdated diff Jun 18, 2015

shelhamer commented on an outdated diff Jul 29, 2015

shelhamer commented on an outdated diff Jul 29, 2015

shelhamer commented on an outdated diff Jul 29, 2015

shelhamer commented on an outdated diff Sep 4, 2015

jeffdonahue commented on an outdated diff Sep 4, 2015

jeffdonahue merged commit `5ff3734` into BVLC:master Sep 17, 2015
1 check passed

mtngld commented on `f3a933a` Sep 17, 2015