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 4 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
1 change: 0 additions & 1 deletion include/caffe/common.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ class Caffe {

DISABLE_COPY_AND_ASSIGN(Caffe);
};

} // namespace caffe
Copy link

Choose a reason for hiding this comment

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

Unnecessary edit


#endif // CAFFE_COMMON_HPP_
30 changes: 30 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,19 @@ 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
m_thread_state = PyEval_SaveThread();
Copy link
Contributor

@ajtulloch ajtulloch Jun 29, 2016

Choose a reason for hiding this comment

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

This should be an RAII wrapper around Save/Restore thread for exception safety.

This definitely isn't the right place to put this logic though, FWIW.

Copy link
Author

Choose a reason for hiding this comment

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

RAII wrapper: Thank you for the suggestion. I will take a look to fix it.

Not right place: I agree. However, where would you suggest to place it? (consider that you have to reacquire GIL in case you forward/backward a python layer within the net). Even though this solution insert "Pythonish" code within the C++ logic, not releasing GIL for the pycaffe is a painful bug for python interface. If there is a cleaner alternative I would love to discuss about it.

Dtype result = ForwardFromTo(start, end);
PyEval_RestoreThread(m_thread_state);
m_thread_state = NULL;
return result;
}
Dtype ForwardFrom(int start);
Dtype ForwardTo(int end);
/// @brief DEPRECATED; set input blobs then use Forward() instead.
Expand All @@ -71,6 +86,18 @@ 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
m_thread_state = PyEval_SaveThread();
BackwardFromTo(start, end);
PyEval_RestoreThread(m_thread_state);
m_thread_state = NULL;
}
void BackwardFrom(int start);
void BackwardTo(int end);

Expand Down Expand Up @@ -308,6 +335,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
PyThreadState * m_thread_state;
};


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
17 changes: 17 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,17 @@ 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.

PyEval_RestoreThread(m_thread_state);
m_thread_state = NULL;
}
Dtype layer_loss = layers_[i]->Forward(bottom_vecs_[i], top_vecs_[i]);
if (is_python)
m_thread_state = PyEval_SaveThread();
loss += layer_loss;
if (debug_info_) { ForwardDebugInfo(i); }
}
Expand Down Expand Up @@ -581,10 +592,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)
PyEval_RestoreThread(m_thread_state);
layers_[i]->Backward(
top_vecs_[i], bottom_need_backward_[i], bottom_vecs_[i]);
if (is_python)
m_thread_state = PyEval_SaveThread();
if (debug_info_) { BackwardDebugInfo(i); }
}
}
Expand Down