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

Added PyForwardFromTo and PyBackwardFromTo to Net, for releasing GIL … #4360

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,10 @@ LIBRARY_DIRS += $(BLAS_LIB)

LIBRARY_DIRS += $(LIB_BUILD_DIR)

#for linking to the libpython2.7 for releasing scope in pycaffe
#(however should be linked even if pycaffe disabled)
LIBRARIES += python2.7

# Automatic dependency generation (nvcc is handled separately)
CXXFLAGS += -MMD -MP

Expand Down Expand Up @@ -593,19 +597,19 @@ $(TEST_ALL_BIN): $(TEST_MAIN_SRC) $(TEST_OBJS) $(GTEST_OBJ) \
| $(DYNAMIC_NAME) $(TEST_BIN_DIR)
@ echo CXX/LD -o $@ $<
$(Q)$(CXX) $(TEST_MAIN_SRC) $(TEST_OBJS) $(GTEST_OBJ) \
-o $@ $(LINKFLAGS) $(LDFLAGS) -l$(LIBRARY_NAME) -Wl,-rpath,$(ORIGIN)/../lib
-o $@ $(LINKFLAGS) -l$(LIBRARY_NAME) $(LDFLAGS) -Wl,-rpath,$(ORIGIN)/../lib

Choose a reason for hiding this comment

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

Why is this change necessary?


$(TEST_CU_BINS): $(TEST_BIN_DIR)/%.testbin: $(TEST_CU_BUILD_DIR)/%.o \
$(GTEST_OBJ) | $(DYNAMIC_NAME) $(TEST_BIN_DIR)
@ echo LD $<
$(Q)$(CXX) $(TEST_MAIN_SRC) $< $(GTEST_OBJ) \
-o $@ $(LINKFLAGS) $(LDFLAGS) -l$(LIBRARY_NAME) -Wl,-rpath,$(ORIGIN)/../lib
-o $@ $(LINKFLAGS) -l$(LIBRARY_NAME) $(LDFLAGS) -Wl,-rpath,$(ORIGIN)/../lib

$(TEST_CXX_BINS): $(TEST_BIN_DIR)/%.testbin: $(TEST_CXX_BUILD_DIR)/%.o \
$(GTEST_OBJ) | $(DYNAMIC_NAME) $(TEST_BIN_DIR)
@ echo LD $<
$(Q)$(CXX) $(TEST_MAIN_SRC) $< $(GTEST_OBJ) \
-o $@ $(LINKFLAGS) $(LDFLAGS) -l$(LIBRARY_NAME) -Wl,-rpath,$(ORIGIN)/../lib
-o $@ $(LINKFLAGS) -l$(LIBRARY_NAME) $(LDFLAGS) -Wl,-rpath,$(ORIGIN)/../lib
Copy link

Choose a reason for hiding this comment

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

Is this edit necessary?

Copy link
Author

Choose a reason for hiding this comment

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

It fixed an undefined reference to lpython2.7, that would be necessary even if not building with python this way. Thus, without it would not work.


# Target for extension-less symlinks to tool binaries with extension '*.bin'.
$(TOOL_BUILD_DIR)/%: $(TOOL_BUILD_DIR)/%.bin | $(TOOL_BUILD_DIR)
Expand Down
18 changes: 17 additions & 1 deletion include/caffe/common.hpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef CAFFE_COMMON_HPP_
#define CAFFE_COMMON_HPP_

#include <boost/make_shared.hpp>
#include <boost/shared_ptr.hpp>
#include <gflags/gflags.h>
#include <glog/logging.h>
Expand All @@ -18,6 +19,8 @@

#include "caffe/util/device_alternate.hpp"

#include "Python.h"

// Convert macro to string
#define STRINGIFY(m) #m
#define AS_STRING(m) STRINGIFY(m)
Expand Down Expand Up @@ -77,6 +80,7 @@ namespace caffe {
// We will use the boost shared_ptr instead of the new C++11 one mainly
// because cuda does not work (at least now) well with C++11 features.
using boost::shared_ptr;
using boost::make_shared;

// Common functions and classes from std that caffe often uses.
using std::fstream;
Expand Down Expand Up @@ -181,7 +185,19 @@ class Caffe {

DISABLE_COPY_AND_ASSIGN(Caffe);
};

} // namespace caffe

class ScopedGILRelease {
public:
ScopedGILRelease() {
m_thread_state = PyEval_SaveThread();
}
~ScopedGILRelease() {
PyEval_RestoreThread(m_thread_state);
m_thread_state = NULL;
}
private:
PyThreadState * m_thread_state;
};

#endif // CAFFE_COMMON_HPP_
28 changes: 28 additions & 0 deletions include/caffe/net.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
#include "caffe/layer.hpp"
#include "caffe/proto/caffe.pb.h"

#include "Python.h"

namespace caffe {

/**
Expand Down Expand Up @@ -52,6 +54,18 @@ class Net {
* included.
*/
Dtype ForwardFromTo(int start, int end);
/**
* Basically, a ForwardFromTo, that release Python GIL, so that pycaffe
* can make multithreaded predictions. This is exposed to python-boost interface
* instead of ForwardFromTo
*/
Dtype PyForwardFromTo(int start, int end) {
// Release GIL
scoped_gil_release = make_shared<ScopedGILRelease>();
Dtype result = ForwardFromTo(start, end);
scoped_gil_release.reset();
return result;
}
Dtype ForwardFrom(int start);
Dtype ForwardTo(int end);
/// @brief DEPRECATED; set input blobs then use Forward() instead.
Expand All @@ -71,6 +85,17 @@ class Net {
*/
void Backward();
void BackwardFromTo(int start, int end);
/**
* Basically, a BackwardFromTo, that release Python GIL, so that pycaffe
* can make multithreaded predictions. This is exposed to python-boost interface
* instead of BackwardFromTo.
**/
void PyBackwardFromTo(int start, int end) {
// Release GIL
scoped_gil_release = make_shared<ScopedGILRelease>();
BackwardFromTo(start, end);
scoped_gil_release.reset();
}
void BackwardFrom(int start);
void BackwardTo(int end);

Expand Down Expand Up @@ -308,6 +333,9 @@ class Net {
/// The root net that actually holds the shared layers in data parallelism
const Net* const root_net_;
DISABLE_COPY_AND_ASSIGN(Net);

// For releasing/reacquiring GIL with pycaffe
shared_ptr<ScopedGILRelease> scoped_gil_release;
Copy link

Choose a reason for hiding this comment

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

This should be scoped_gil_release_ to be consistent with other naming.

};


Expand Down
7 changes: 5 additions & 2 deletions python/caffe/_caffe.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,11 @@ BOOST_PYTHON_MODULE(_caffe) {
bp::no_init)
.def("__init__", bp::make_constructor(&Net_Init))
.def("__init__", bp::make_constructor(&Net_Init_Load))
.def("_forward", &Net<Dtype>::ForwardFromTo)
.def("_backward", &Net<Dtype>::BackwardFromTo)
// Instead of ForwardFromTo and BackwardFromTo,
// expose PyForwardFromTo and PyBackwardFromTo,
// basically the same thing but with GIL release
.def("_forward", &Net<Dtype>::PyForwardFromTo)
.def("_backward", &Net<Dtype>::PyBackwardFromTo)
.def("reshape", &Net<Dtype>::Reshape)
// The cast is to select a particular overload.
.def("copy_from", static_cast<void (Net<Dtype>::*)(const string)>(
Expand Down
5 changes: 2 additions & 3 deletions python/caffe/test/test_python_layer.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

import caffe


class SimpleLayer(caffe.Layer):
Copy link

@seanbell seanbell Aug 3, 2016

Choose a reason for hiding this comment

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

Unnecessary edit

"""A layer that just multiplies by ten"""

Expand Down Expand Up @@ -97,8 +96,8 @@ def phase_net_file():
return f.name


@unittest.skipIf('Python' not in caffe.layer_type_list(),
'Caffe built without Python layer support')
#@unittest.skipIf('Python' not in caffe.layer_type_list(),
# 'Caffe built without Python layer support')
Copy link

@seanbell seanbell Jul 30, 2016

Choose a reason for hiding this comment

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

Won't this break support for those building caffe without WITH_PYTHON_LAYER?

class TestPythonLayer(unittest.TestCase):
def setUp(self):
net_file = python_net_file()
Expand Down
16 changes: 16 additions & 0 deletions src/caffe/net.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <algorithm>
#include <cstring>
#include <map>
#include <set>
#include <string>
Expand All @@ -19,6 +20,8 @@

#include "caffe/test/test_caffe_main.hpp"

#include "Python.h"

Choose a reason for hiding this comment

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

This is not needed if WITH_PYTHON_LAYER is not set.


namespace caffe {

template <typename Dtype>
Expand Down Expand Up @@ -536,9 +539,16 @@ Dtype Net<Dtype>::ForwardFromTo(int start, int end) {
CHECK_GE(start, 0);
CHECK_LT(end, layers_.size());
Dtype loss = 0;
bool is_python;
for (int i = start; i <= end; ++i) {
// LOG(ERROR) << "Forwarding " << layer_names_[i];
is_python = strcmp(layers_[i]->type(), "Python") == 0;
if (is_python) {
Copy link

@seanbell seanbell Jul 30, 2016

Choose a reason for hiding this comment

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

Style note: your use of { is inconsistent -- you use it here but not below.

scoped_gil_release.reset();
}
Dtype layer_loss = layers_[i]->Forward(bottom_vecs_[i], top_vecs_[i]);
if (is_python)
scoped_gil_release = make_shared<ScopedGILRelease>();
Copy link

@seanbell seanbell Jul 30, 2016

Choose a reason for hiding this comment

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

These changes are only relevant if WITH_PYTHON_LAYER is set. The extra code you added here should probably check for WITH_PYTHON_LAYER.

loss += layer_loss;
if (debug_info_) { ForwardDebugInfo(i); }
}
Expand Down Expand Up @@ -581,10 +591,16 @@ template <typename Dtype>
void Net<Dtype>::BackwardFromTo(int start, int end) {
CHECK_GE(end, 0);
CHECK_LT(start, layers_.size());
bool is_python;
for (int i = start; i >= end; --i) {
if (layer_need_backward_[i]) {
is_python = strcmp(layers_[i]->type(), "Python") == 0;
if (is_python)
scoped_gil_release.reset();
layers_[i]->Backward(
top_vecs_[i], bottom_need_backward_[i], bottom_vecs_[i]);
if (is_python)
scoped_gil_release = make_shared<ScopedGILRelease>();

Choose a reason for hiding this comment

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

Same as above.

if (debug_info_) { BackwardDebugInfo(i); }
}
}
Expand Down