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

Layer modules #5294

Closed
wants to merge 4 commits into from
Closed

Layer modules #5294

wants to merge 4 commits into from

Conversation

@hgaiser
Copy link
Contributor

hgaiser commented Feb 16, 2017

This PR adds support for layer modules in Caffe (as discussed in #5243) and some changes that were required to make that happen. The idea of these layer modules is that developers design their own modules which contain one or more layers which can be used via "Module" layers in their prototxts.

The usage is analogous to that of layers in Caffe with a few exceptions:

  • Instead of REGISTER_LAYER_CLASS, EXPORT_LAYER_MODULE_CLASS needs to be called to export layer modules.
  • Params are defined in the prototxt under the parameter param_str, analogous to how params are defined for Python layers. It is up to the developer to determine how this string is to be parsed (YAML parsing for instance).
  • Modules need to be found by Caffe. The module param in the prototxt defines the filename, stripped from any prefixes and suffixes. For example, if the module filename is libmodular_layer.so, the module param should be set to "modular_layer". This is to ensure OS independent behaviour where the prefix/suffix might be different (.so vs .dylib vs .dll for instance). Expected prefix/suffix can be adjusted through CMake or Makefile.config and defaults to CMakes default for cmake and "lib", ".so" for Makefile. By default Caffe searches for modules in its DEFAULT_LAYER_PATH definition, a colon separated list of directories where modules are expected to be placed. This defaults to $DESTINATION_DIR/lib/caffe/layers and can be overwritten in CMake or Makefile.config. In addition, if the environment variable CAFFE_LAYER_PATH is set, it overrides the default search path. If for the caffe tool a --layer_path flag is given it will override the previous search paths. In other words: caffe flags > cmake/Makefile.config > default.
  • The layers in the prototxt are defined as Module layers, module_param contains params that describe how to load a layer from the specified module. The most straightforward way is to define the type, which would be similar to the type used for built-in caffe layers. This definition expects certain symbols in the module to be exposed, as defined through the EXPORT_LAYER_MODULE_CLASS macro. Creators and deleters can be customized, as can be seen in ExampleModularLayer class in the comments at the end, but normally would not be required.

The PR contains an example layer module which shows a minimum example of how it can be used. In addition I made a branch of py-faster-rcnn which uses layer modules to execute the ROIPooling and SmoothL1Loss layers, allowing it to be run on upstream Caffe (including this PR ofcourse) as opposed to their outdated caffe-fast-rcnn fork.

The intention is to add unit tests still, but I figured lets ask for some feedback first.

@hgaiser hgaiser force-pushed the fizyr-forks:layer-modules branch 3 times, most recently from b05c46a to 3fb47a8 Feb 16, 2017
@hgaiser hgaiser force-pushed the fizyr-forks:layer-modules branch 2 times, most recently from 3d9e309 to 02f6ef3 Feb 20, 2017
@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented Feb 20, 2017

Rebased and skipped testing of ModuleLayer. The tests will have to be a separate unit test anyway because it requires specific parameters. I prefer to do this after some feedback on the PR so far.

@hgaiser hgaiser force-pushed the fizyr-forks:layer-modules branch 4 times, most recently from c2c2bcb to f68bfd0 Feb 20, 2017
@hgaiser hgaiser mentioned this pull request Feb 21, 2017
@hgaiser hgaiser force-pushed the fizyr-forks:layer-modules branch from f68bfd0 to 3c1c4f3 Feb 21, 2017
@hgaiser hgaiser force-pushed the fizyr-forks:layer-modules branch from 3c1c4f3 to 7a06906 Feb 28, 2017
@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented Mar 7, 2017

@shelhamer , could you give feedback on this PR?

@shelhamer

This comment has been minimized.

Copy link
Member

shelhamer commented Mar 15, 2017

Thanks for looking into this! I hope to catch up on PRs after the ICCV deadline 03/17

@williford

This comment has been minimized.

Copy link
Contributor

williford commented Mar 19, 2017

This would be a great feature. The lack of being able to extend Caffe without forking the code (and not resorting to PythonLayer) is, in my opinion, the largest pitfall of Caffe.

This comment applies to PythonLayer too and you shouldn't implement it in this PR. Would it be possible to add general variable types, like this:

param_int { name: "custom_var1" value: 10 }
param_int { name: "custom_var2" value: 5 }
param_float { name: "custom_var3" value: 0.05 }

I find param_str a bit less user friendly.

@de-vri-es

This comment has been minimized.

Copy link

de-vri-es commented Mar 19, 2017

This comment applies to PythonLayer too and you shouldn't implement it in this PR. Would it be possible to add general variable types, like this:

param_int { name: "custom_var1" value: 10 }
param_int { name: "custom_var2" value: 5 }
param_float { name: "custom_var3" value: 0.05 }

I find param_str a bit less user friendly.

That shouldn't be too difficult. In that case it would make sense to add a few utility functions in caffe to read typed parameters by name in an easy manner. Perhaps something along the lines of getParam<int>(layer_params, "custom_var1"). Could return an optional<int> or throw an exception to deal with parameters that aren't set (or set multiple times).

One downside is that such a scheme doesn't easily allow for more complicated structures in the parameters. While really complex structs might not be a common need for layers, lists may be. There are a couple of ways to go about lists. Anyway, I like the idea, and absolute worst case a param_string can still be abused to pass in YAML.

Should we discuss this further in a separate issue?

@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented Mar 27, 2017

I agree @williford , the param_str isn't that user friendly and I quite like your suggestion. I think it'd be better if I leave this PR as is though, then we can discuss this addition in a separate issue/PR.

@shelhamer

This comment has been minimized.

Copy link
Member

shelhamer commented Apr 15, 2017

Sorry for the wait @hgaiser! I will make my best effort to review module layers and Halide layers #5370 together next week. I think both are important for the extensibility of Caffe.

@williford

This comment has been minimized.

Copy link
Contributor

williford commented Apr 15, 2017

@hgaiser or @de-vri-es, would it be possible to provide a barebone example external layer?

Just some notes (to those looking at the PR / myself):
DEFAULT_LAYER_PATH is a variable for the build files. In the Makefile, it is defined using DISTRIBUTE_DIR. In Cmake, it is defined using CMAKE_INSTALL_PREFIX. CAFFE_LAYER_PATH (if set) would be an environment variable.

For people interested in evaluating the PR, but don't know how, you can do the following:

git clone git@github.com:BVLC/caffe.git delft-git
cd delft-git
git fetch origin pull/5294/head:layer-modules   # layer-modules is the local branch name 
git branch layer-modules
@de-vri-es

This comment has been minimized.

Copy link

de-vri-es commented Apr 15, 2017

@hgaiser or @de-vri-es, would it be possible to provide a barebone example external layer?

The last commit in the PR adds an example layer module: 7a06906

@de-vri-es

This comment has been minimized.

Copy link

de-vri-es commented Apr 16, 2017

It looks like this PR and PR #5370 overlap significantly. As it would be good to avoid redundant implementations could you guys ( @hgaiser and @de-vri-es ) please have a look at the other PR and note what your PR solves that mine doesn't?

I'm not familiar with Halide, but all this PR does is support loading C++ layers from separately compiled modules. I don't see how that relates to Halide, or where the overlap would come from. Could you elaborate on that?

@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented Apr 17, 2017

It looks like this PR and PR #5370 overlap significantly. As it would be good to avoid redundant implementations could you guys ( @hgaiser and @de-vri-es ) please have a look at the other PR and note what your PR solves that mine doesn't?

I'll try to do the same next week.

This PR seems to address params and search paths more thoroughly. It would be good to converge on a reasonable (minimal?) interface here.

As I understand #5370 a typical prototxt would look something like:

name: "SomeNet"
external_layers: "/full/path/to/some/library.so"
external_layers: "relative/path/to/another/library.so"

And in these libraries there have to be two functions defined, GetExtLayerName and getExtLayer ? How should additional parameters be parsed by these layers?

What I am thinking is that this PR could be used by your PR to handle the loading and creating of external layers from modules. It appears to me that this PR offers more configurability because it allows multiple layers to be defined in one library, allows parameter passing and extends the way libraries are searched.

Whether this conflicts with anything related to Halide I cannot say as I have no experience with Halide.

What I don't really understand is why #5370 has the functionality of loading libraries dynamically. Why is that necessary for your Halide implementation, could you elaborate on that?

@BlGene

This comment has been minimized.

Copy link
Contributor

BlGene commented Apr 19, 2017

Parameter passing was not addressed. Adding a generic parameter string to layer was considered.

I would consider single layer per library a feature more than a bug.

I don't know to what extent layer search should be in c++, I was considering only cases where nets are freshly baked in python and I would prefer for the problem to be solved there.

This approach is compatible with halide libraries. External layers where chosen to factor the build process. Also halide needs to be recompiled for different back ends.

@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented Apr 19, 2017

Parameter passing was not addressed. Adding a generic parameter string to layer was considered.

I would consider single layer per library a feature more than a bug.

I don't know to what extent layer search should be in c++, I was considering only cases where nets are freshly baked in python and I would prefer for the problem to be solved there.

This approach is compatible with halide libraries. External layers where chosen to factor the build process. Also halide needs to be recompiled for different back ends.

Hmm to be honest I quite like the option of having more than one layer in a module. That opens the door to modules such as libfasterrcnn.so instead of having a libproposal_layer.so, libroipooling.so, etc. Granted that some layers could be reused elsewhere, which means it'd be nicer to have it in separate modules, but with this PR that is an option left to the developer.

@BlGene

This comment has been minimized.

Copy link
Contributor

BlGene commented Apr 19, 2017

Fair point, multi-layer modules is also what TF has chosen.
https://www.tensorflow.org/extend/adding_an_op#implement_the_kernel_for_the_op.
This could be added to the halide PR by vectorizing GetExtLayerName and getExtLayer, but maybe a interface closer to this PR / TF would be better.


Is there anything preventing a TF->caffe module-layer wrapping from within a layer module i.e. writing a caffe module layer that can in turn call a TF op?

@de-vri-es

This comment has been minimized.

Copy link

de-vri-es commented Apr 19, 2017

I don't know to what extent layer search should be in c++, I was considering only cases where nets are freshly baked in python and I would prefer for the problem to be solved there.

It sounds like you are proposing for a program written in C++ to invoke python code to find the location of a shared library containing more C++ code. That seems rather more complicated than required to me. Especially considering this PR already has functionality for searching for modules in different locations using the well-known Unix convention of having colon separated paths in a environment variable.

This could be added to the halide PR by vectorizing GetExtLayerName and getExtLayer, but maybe a interface closer to this PR / TF would be better.

This PR simply allows C++ layers to be compiled in separate libraries and then loaded at runtime. If a C++ layer wants to invoke code written in another programming language, that doesn't matter for this PR. I think it makes sense to treat the problem of splitting layers into shared libraries separate from adding integration with different compiled languages.

With this approach, users of the layers don't even need to know in which language they were written. They could use the same layer type for external C++ layers or external Halide layers. I think that's a big plus point.

Is there anything preventing a TF->caffe module-layer wrapping from within a layer module i.e. writing a caffe module layer that can in turn call a TF op?

I'm not sure if that is easily possible, but this PR doesn't change the interface of layers at all, except for the parameter loading (which @williford pointed out, can be improved, but should probably be done for both python and external C++ layers together). It would definitely be interesting to see if layers written for other frameworks can be used in Caffe with a little glue, but I would keep that in a separate PR that can build on top of this one.

@hgaiser hgaiser mentioned this pull request Apr 25, 2017
@de-vri-es de-vri-es force-pushed the fizyr-forks:layer-modules branch from 7a06906 to 797e76b May 4, 2017
@de-vri-es

This comment has been minimized.

Copy link

de-vri-es commented May 4, 2017

Rebased again against latest master.

@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented May 20, 2017

Any update from the caffe maintainers?

@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented May 30, 2017

@shelhamer have you had a chance to look at this PR ?

@shelhamer

This comment has been minimized.

Copy link
Member

shelhamer commented May 31, 2017

Sorry, I was pulled away from this by the NIPS deadline and some travel but I do still intend to have a close look and shepherd this for merging as I believe in this direction for extensibility.

@williford

This comment has been minimized.

Copy link
Contributor

williford commented May 31, 2017

Just a note that this merge does require cmake to be upgraded and will make installing Ubuntu 14.04 much harder (== will require additional step that might break or be difficult for novices). Perhaps this should correspond to a new version (1.1? or 3 :-P).

Update:

It looks like it might be possible to still support Ubuntu 14.04 with something like this (https://stackoverflow.com/a/31010221/249226):

macro(use_cxx11)
  if (CMAKE_VERSION VERSION_LESS "3.1")
    if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
      set (CMAKE_CXX_FLAGS "--std=gnu++11 ${CMAKE_CXX_FLAGS}")
    endif ()
  else ()
    set (CMAKE_CXX_STANDARD 11)
  endif ()
endmacro(use_cxx11)
@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented May 31, 2017

Just a note that this merge does require cmake to be upgraded and will make installing Ubuntu 14.04 much harder (== will require additional step that might break or be difficult for novices). Perhaps this should correspond to a new version (1.1? or 3 :-P).

Update:

It looks like it might be possible to still support Ubuntu 14.04 with something like this (https://stackoverflow.com/a/31010221/249226):

macro(use_cxx11)
if (CMAKE_VERSION VERSION_LESS "3.1")
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set (CMAKE_CXX_FLAGS "--std=gnu++11 ${CMAKE_CXX_FLAGS}")
endif ()
else ()
set (CMAKE_CXX_STANDARD 11)
endif ()
endmacro(use_cxx11)

If the bump in cmake version is an issue, I'd prefer to do it the "ugly" way only, instead of both ways.

@williford

This comment has been minimized.

Copy link
Contributor

williford commented May 31, 2017

The nice thing about using the macro is that CMAKE_CXX_STANDARD will support more compilers and be more cross-platform friendly (given that CMake is not ancient). The macro probably needs to be extended for libc++ or clang++ compilers (I'm not familiar with MacOS), but I think it is nice that it will automatically work if the version of CMAKE is recent enough.

@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented Jun 1, 2017

The nice thing about using the macro is that CMAKE_CXX_STANDARD will support more compilers and be more cross-platform friendly (given that CMake is not ancient). The macro probably needs to be extended for libc++ or clang++ compilers (I'm not familiar with MacOS), but I think it is nice that it will automatically work if the version of CMAKE is recent enough.

Ah I just noticed the macro you mentioned uses gnu++11, is there a reason for that? I believe in 99% of the cases, c++11 would suffice and be more portable since it doesn't need to assume GNU compilers.

Given c++11 is as far as I know portable to all sorts of compilers, I stick with my original response :)

If the bump in cmake version is an issue, I'd prefer to do it the "ugly" way only, instead of both ways.

@de-vri-es

This comment has been minimized.

Copy link

de-vri-es commented Jun 1, 2017

Ah I just noticed the macro you mentioned uses gnu++11, is there a reason for that? I believe in 99% of the cases, c++11 would suffice and be more portable since it doesn't need to assume GNU compilers.

That can also be solved by adding set(CMAKE_CXX_EXTENSIONS off). I agree that disabling GNU extensions is a good thing since it improves portability a lot. Depending on GNU extensions has never been very nice, and with C++11 there's a lot less reason to even consider it.

Given c++11 is as far as I know portable to all sorts of compilers, I stick with my original response :)

clang, gcc and the intel C++ compiler use -std=c++11 to enable C++ 11 support. So pretty much all Unix systems should be fine with -std=c++11. I'm not sure about msvc and other niche compilers though. I suppose windows support could be a reason to already use CMAKE_CXX_STANDARD when available.

@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented Jun 26, 2017

@shelhamer could this be picked up again?

@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented Aug 2, 2017

Should this still be updated? Seems like Caffe is dead anyway, judging by the number of unresolved issues and PRs.

@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented Sep 27, 2017

Ping?

@de-vri-es de-vri-es force-pushed the fizyr-forks:layer-modules branch from 4edf0b2 to 7350fe0 Oct 16, 2017
@de-vri-es

This comment has been minimized.

Copy link

de-vri-es commented Oct 16, 2017

Rebased again.

@shelhamer

This comment has been minimized.

Copy link
Member

shelhamer commented Oct 16, 2017

Thanks @hgaiser and @de-vri-es for the effort and patience. This is still in my sights for Caffe 1.1.

@hgaiser

This comment has been minimized.

Copy link
Contributor Author

hgaiser commented Apr 29, 2019

Closing this PR due to inactivity.

@hgaiser hgaiser closed this Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.