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

First attemp to remove boost dependecies. #2537

Closed
wants to merge 1 commit into from

Conversation

bhack
Copy link
Contributor

@bhack bhack commented Jun 2, 2015

This is an initial effort to generally remove boost dependencies and leave it only for python wrapper.
I think that c++11 support it is quite stable now, enabled by default in gcc 5.x series, full supported by LLVM and almost supported in the android NDK.
With #2523 modularity we could really strip caffe dependencies on embedded systems and especially for android NDK builds.
@shelhamer @longjon Can you take a look at this?
@Nerei Generally the Travis build matrix of caffe doesn't seems coherent to me between Cmake and Make (Cmake really build cuda?).
I think that @shelhamer can make a run on OSX because we don't have it in Travis.
Generally this seems in a good shape, CPU and GPU tests pass fine.
There is still a problem with a python test as you can see in Travis logs.
Can you give me some feedback on this?

@thatguymike
Copy link
Contributor

While I agree with the general direction, until C++11 is supported by default compilers on distributions with long term support, this is going to be difficult for many users to contend with. For example, for Ubuntu 14.04, GCC version is 4.8.2 still by default. CentOS 7 appears to also be < 5.0.

@bhack
Copy link
Contributor Author

bhack commented Jun 2, 2015

@thatguymike This is actually building with g++4.8 on Travis. I've cited GCC 5.X series only for the "default flag".

@thatguymike
Copy link
Contributor

That makes me more comfortable, but I thought with 4.8, C++11 was "touchy", but maybe that is limited to regex and misc utilities. So perhaps this just needs a LOT of testing across platforms. I am still worried that some users will get pushed out of Caffe because they are stuck on something old like RedHat 6.x (we still see a lot of that...) or an embedded platform that doesn't quite have all the needed compiler support available.

@flx42
Copy link
Contributor

flx42 commented Jun 2, 2015

gcc 4.8 C++11 language support:
https://gcc.gnu.org/gcc-4.8/cxx0x_status.html
gcc 4.8 C++11 standard library support:
https://gcc.gnu.org/onlinedocs/gcc-4.8.4/libstdc++/manual/manual/status.html#status.iso.2011

Regex support is indeed broken with gcc 4.8, the problem is that it will fail at runtime. It's fixed in 4.9:
https://gcc.gnu.org/onlinedocs/gcc-4.9.2/libstdc++/manual/manual/status.html#status.iso.2011

Caffe is not using regexes in C++ right now, so it should be fine. But I agree with @thatguymike, this will require a lot of testing on all platforms, including Android. For RedHat 6.x or embedded platforms support, this is really a strategical/philosophical decision you have to make about the project.

@bhack
Copy link
Contributor Author

bhack commented Jun 2, 2015

@flx42 All the features we are actually using in Caffe/boost it is supported in gcc 4.8. In fact this PR it is set to build with gcc4.8 in Travis. I think that the 4.8 it is quite available in long term support distro.

@flx42
Copy link
Contributor

flx42 commented Jun 2, 2015

All the features we are actually using in Caffe/boost it is supported in gcc 4.8.

That's exactly what I said.

In fact this PR it is set to build with gcc4.8 in Travis.

It could build but fail at runtime in subtle ways. Travis doesn't run GPU tests and doesn't support all the platforms we might be interested in. So I think this requires further testing.

@bhack
Copy link
Contributor Author

bhack commented Jun 2, 2015

@flx42 Yes I've published here to collect feedbacks. People can get this PR and try to run it on his own environment. Probably you and @thatguymike could be the first two to test it 😄.
I don't think that @thatguymike has problem to find a nvidia GPU to run gpu test 😉

@vimalthilak
Copy link

@bhack: Do you need an extra pair of hands to test this PR on OS X/ubuntu bistro

PS: Please add iOS to your list of "have to support" OSes.

@bhack
Copy link
Contributor Author

bhack commented Jun 3, 2015

@vimalthilak Yes please test.
For IOS you can try caffe-preset with robovm. See bytedeco/javacpp-presets#34

@bhack
Copy link
Contributor Author

bhack commented Jun 6, 2015

@longjon Do you have any hint on why this python test is failing?

@bhack
Copy link
Contributor Author

bhack commented Jun 7, 2015

@thatguymike @flx42 Have you tested on your environment?

@flx42
Copy link
Contributor

flx42 commented Jun 8, 2015

It worked for me!
The Travis error you are getting is puzzling, did you manage to get a repro locally?

@bhack
Copy link
Contributor Author

bhack commented Jun 8, 2015

@flx42 Have you runned python test? make pycaffe then make pytest

@vimalthilak
Copy link

@bhack: On on OS X laptop, I ran into a static assert (build error) with your patch.

Please change return std::nextafter<Dtype>(... to return std::nextafter(...

std::nextafter is not a templated function.

@flx42
Copy link
Contributor

flx42 commented Jun 8, 2015

@bhack My python install is busted so it doesn't work, but for different reasons :)

@bhack
Copy link
Contributor Author

bhack commented Jun 9, 2015

@vimalthilak Nice catch. Done. But seems that a ReductionLayerTest fail. I've not touched ReductionLayerTest that was merged to master after this PR

@vimalthilak
Copy link

@bhack How do I run the ReductionlayerTest? Seems like that got added last November. Can you specify the steps needed to repro the problem on my side? Thanks.

@bhack
Copy link
Contributor Author

bhack commented Jun 9, 2015

@vimalthilak Was merged on 3 June in master. You can run that test with:
.build_release/test/test_all.testbin --gtest_filter='*Reduction*'

@bhack bhack mentioned this pull request Jun 10, 2015
@bhack
Copy link
Contributor Author

bhack commented Jun 11, 2015

@sh1r0 Are you interested to test this with your NDK tests?

@vimalthilak
Copy link

@bhack: Just to clear, did you rebase your branch in order to run the
ReductionLayer test? I will give this a try once I have some breathing
space at work.

On Thu, Jun 11, 2015 at 3:10 PM, bhack notifications@github.com wrote:

@sh1r0 https://github.com/sh1r0 Are you interested to test this with
you NDK tests?


Reply to this email directly or view it on GitHub
#2537 (comment).

@bhack
Copy link
Contributor Author

bhack commented Jun 11, 2015

@vimalthilak Yes I've rebased to test it. I can push to this branch tomorrow.

@bhack
Copy link
Contributor Author

bhack commented Jun 12, 2015

@vimalthilak It is rebased now.

sh1r0 added a commit to sh1r0/caffe that referenced this pull request Jun 12, 2015
sh1r0 added a commit to sh1r0/caffe that referenced this pull request Jun 12, 2015
@sh1r0
Copy link
Contributor

sh1r0 commented Jun 12, 2015

@bhack Thanks. I removed some boost dependencies (sh1r0/caffe-android-lib@c021b34), and it seems to be fine with some naive tests.

@bhack
Copy link
Contributor Author

bhack commented Jun 12, 2015

@sh1r0 Great! Do you have compiled also the cuda part? Some android tablet like Nvidia shield have cuda support.

@sh1r0
Copy link
Contributor

sh1r0 commented Jun 12, 2015

@bhack Nope. I hope I can but it's a shame that I don't have such a device to test.

@bhack
Copy link
Contributor Author

bhack commented Jun 12, 2015

@sh1r0 I have one. I can make some test in the next days. Can you reabase your submodule?
I think that with this and #2523 you can really simplify your commits on your caffe submodule.

Still alive only for python wrapper.
@bhack
Copy link
Contributor Author

bhack commented Jun 17, 2015

@longjon I think that you are the main developer of the boost::python wrapper. I'm not practical with boost::python because I've generally wrapped rarely and only with Cython. I think that we need to register a new converter that can support std::shared_ptr as boost ticket 6545. Do you have any hint? Can you contribute?

/test/test_python_layer.py", line 15, in reshape
    top[0].reshape(*bottom[0].data.shape)
TypeError: No registered converter was able to produce a C++ rvalue of type std::shared_ptr<caffe::Blob<float> > from this Python object of type Blob

@bhack
Copy link
Contributor Author

bhack commented Jun 18, 2015

/cc @stefanseefeld Is there anyone active in boost::python on ticket 6545?

@stefanseefeld
Copy link

Let me try to have a look...

@bhack
Copy link
Contributor Author

bhack commented Jun 21, 2015

@stefanseefeld Thanks.

@bhack
Copy link
Contributor Author

bhack commented Aug 2, 2015

@stefanseefeld Any news on boost side? @longjon?

@haosdent
Copy link

+1

@bhack
Copy link
Contributor Author

bhack commented Sep 4, 2015

@thatguymike Do you still have interest in this?

@bhack
Copy link
Contributor Author

bhack commented Sep 17, 2015

@stefanseefeld Are there any progress on https://svn.boost.org/trac/boost/ticket/6545? Is there any workaround that we could adopt here?

@stefanseefeld
Copy link

No workarounds that I'm aware of. Someone simply needs to work on a patch to add support for std::shared_ptr and submit that. (THat could be me, if no-one beats me to it.)

@bhack
Copy link
Contributor Author

bhack commented Sep 18, 2015

@stefanseefeld It seem still assigned to @jhasse but I don't know if he is still active on this. The ticket was 4 years old.

@jhasse
Copy link

jhasse commented Sep 18, 2015

Why do you think it's assigned to me? Never have been working on it (sorry), but I'm interested as well ;)

@bhack
Copy link
Contributor Author

bhack commented Sep 18, 2015

@jhasse Sorry I've taken the wrong field. You are in cc. Seems assigned to @TallJimbo

@TallJimbo
Copy link

... and I've been trying to find someone else to assign it to for years now. I just don't have any time to work on Boost.Python.

@stefanseefeld
Copy link

I'll try to look into it. Don't hold your breath, though.

@bhack
Copy link
Contributor Author

bhack commented Sep 19, 2015

Referencing boostorg/python#29

@bhack
Copy link
Contributor Author

bhack commented Dec 2, 2015

Caffe recently merged more PRs that rely on BOOST instead of using c++11 counterparts. The absence of comments by core devs here and new boost code integrated let me tough that there is no interest to switch and remove boost. If nobody is interested anymore I will close this in few days.

@bhack
Copy link
Contributor Author

bhack commented Dec 2, 2015

For python bindings we could evaluate https://github.com/wjakob/pybind11/blob/master/README.md

@bhack bhack closed this Dec 28, 2015
@willyd willyd mentioned this pull request Sep 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.