-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Conversation
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. |
@thatguymike This is actually building with g++4.8 on Travis. I've cited GCC 5.X series only for the "default flag". |
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. |
gcc 4.8 C++11 language support: Regex support is indeed broken with gcc 4.8, the problem is that it will fail at runtime. It's fixed in 4.9: 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. |
@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. |
That's exactly what I said.
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. |
@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 😄. |
@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. |
@vimalthilak Yes please test. |
@longjon Do you have any hint on why this python test is failing? |
@thatguymike @flx42 Have you tested on your environment? |
It worked for me! |
@flx42 Have you runned python test? make pycaffe then make pytest |
@bhack: On on OS X laptop, I ran into a static assert (build error) with your patch. Please change std::nextafter is not a templated function. |
@bhack My python install is busted so it doesn't work, but for different reasons :) |
@vimalthilak Nice catch. Done. But seems that a ReductionLayerTest fail. I've not touched ReductionLayerTest that was merged to master after this PR |
@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. |
@vimalthilak Was merged on 3 June in master. You can run that test with: |
@sh1r0 Are you interested to test this with your NDK tests? |
@bhack: Just to clear, did you rebase your branch in order to run the On Thu, Jun 11, 2015 at 3:10 PM, bhack notifications@github.com wrote:
|
@vimalthilak Yes I've rebased to test it. I can push to this branch tomorrow. |
@vimalthilak It is rebased now. |
(refer to BVLC#2537)
(refer to BVLC#2537)
@bhack Thanks. I removed some boost dependencies (sh1r0/caffe-android-lib@c021b34), and it seems to be fine with some naive tests. |
@sh1r0 Great! Do you have compiled also the cuda part? Some android tablet like Nvidia shield have cuda support. |
@bhack Nope. I hope I can but it's a shame that I don't have such a device to test. |
Still alive only for python wrapper.
@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?
|
/cc @stefanseefeld Is there anyone active in boost::python on ticket 6545? |
Let me try to have a look... |
@stefanseefeld Thanks. |
@stefanseefeld Any news on boost side? @longjon? |
+1 |
@thatguymike Do you still have interest in this? |
@stefanseefeld Are there any progress on https://svn.boost.org/trac/boost/ticket/6545? Is there any workaround that we could adopt here? |
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.) |
@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. |
Why do you think it's assigned to me? Never have been working on it (sorry), but I'm interested as well ;) |
@jhasse Sorry I've taken the wrong field. You are in cc. Seems assigned to @TallJimbo |
... 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. |
I'll try to look into it. Don't hold your breath, though. |
Referencing boostorg/python#29 |
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. |
For python bindings we could evaluate https://github.com/wjakob/pybind11/blob/master/README.md |
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?