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

Reform the boost::python wrapper, including layers implemented in Python #1703

Merged
merged 16 commits into from
Feb 17, 2015

Conversation

longjon
Copy link
Contributor

@longjon longjon commented Jan 10, 2015

After #1473 and #1686. Replaces #1020.

The current boost::python code uses custom wrapper classes (Py*) to handle conversion, memory management, and extra checks. This is one more layer of abstraction then necessary to use boost::python, which is really intended to directly wrap C++ interfaces.

This PR gets rid of all the custom wrapper classes, and uses boost::python as it was intended. Special memory management logic is handled with boost::python's call policies, and a few thin wrapper functions are used where necessary.

This leads to a better way to implement Python layers (formerly #1020). In Python, one simply subclasses caffe.Layer to define a layer, and uses it as in #1020.

To improve readability of the changes, the commits go like this: first one commit removes most of the existing code, then support for each class is added one-at-a-time, culminating with Python layer.

Thanks to Philipp Krähenbühl for sharing his implementation of some of the functionality of the PR, which I referred to while (re)constructing this.

Improvements

  • All solvers are now accessible via caffe.get_solver.
  • The global Caffe:: functions are exposed as top-level functions of the caffe module rather than attached to Net. This probably breaks some existing Python code, and of course the per-Net functions will come back soon enough when they make sense, but for now this is more sensible.
  • A simple Python layer test is added.

Shortcomings [see below for resolution]

  • As mentioned above, any existing calls to Net.set_{mode/phase/device} functions currently don't work.
  • The fact that the layer factory passes around raw pointers is troublesome for Python layer. The factory function GetPythonLayer needs to transfer ownership of the C++ object from itself to Net in such a way that the Python object will be cleaned up when Net is done with the C++ object. I implemented something really hacky that I think doesn't leak memory, but won't work if you try to construct a layer object outside of a net definition (but you wouldn't do that, right?). A better solution might be to make the factory deal in shared_ptrs.
  • The final commit lightly modifies the Makefile to optionally include the Python layer. This works fine if used through pycaffe, but not through the caffe tool binary, due to double linking of the layer registration (and possibly other registration). I have a fix through dynamic linking, but it causes a crash on exit for some protobuf-related reason (distinct from the previous issues in Python layer for rapidly writing nets in Python #1020). More on this in the upcoming PR, "Linking Nightmare Part II". (Note, however, that enabling WITH_PYTHON_LAYER shouldn't cause any problems with the tool binary if no Python layers are actually used.)

Should be usable as is, but not well tested, and (might?) break existing Python code.

@shelhamer
Copy link
Member

Needs rebase now that #1473, #1686, and #1728 are in.

A better solution might be to make the factory deal in shared_ptrs.

This sounds right and can follow.

More on this in the upcoming PR, "Linking Nightmare Part II"

Yeah, a dynamic linking PR should follow once this is merged.

@netheril96
Copy link
Contributor

This new style of python wrappers are really hard to read and doesn't actually simplify the existing code. The boost.python call policies are very confusing. The old code has no call policies, and instead handles the lifetime explicitly with shared_ptr. And the Zen of Python says

Explicit is better than implicit.

@longjon
Copy link
Contributor Author

longjon commented Feb 6, 2015

@netheril96, I can certainly see the argument that boost::python may be a bit too willing to embrace template metaprogramming, but consider the advantages of the new style:

  1. Significantly less code is required, meaning fewer bugs, lower maintenance cost, etc.
  2. Using the standard boost::python style makes the code more accessible to those familiar with boost::python.
  3. The memory management tricks are now explicitly stated at the wrapper declarations.
  4. With the exception of ndarrays, the call policies are built-in boost::python policies, which state quite plainly their function, e.g., copy_const_reference, manage_new_object, or with_custodian_and_ward.
  5. The NdarrayCallPolicies are a tricky business, but so was the old code, involving two levels of wrapper classes and much confusion (see boost.python wrapper, why? #1651, for example). The new version is, IMO, more straightforward except for the template business.
  6. Exposing the "real" classes instead of wrapper classes properly preserves class hierarchies, which we now use to expose all the solvers.
  7. Exposing the "real" layer class makes it subclassable in Python, which we now use to write Python layers.
  8. It's much easier to expose additional functionality and keep the wrapper in sync, because most methods don't have to be routed through wrapper classes.

So, in my view, explicitness is improved, and don't forget

Beautiful is better than ugly.

and

Flat is better than nested.

@shelhamer
Copy link
Member

@longjon I rebased on dev and archived your branch to shelhamer:longjon-pyreformation for comparison and safe keeping.

  • I adapted Python layer to Layer type is a string #1694
  • I dropped the explicit call policy for the layer type since it is a const char* and handled fine by boost::python converter for string.

TODO

  • switch layer factory to shared_ptr for safety
  • address LayerFactoryTest issue by special case for PythonLayer or do away with the test by switching tests over to LayerRegistry instead of making their own layers by new

@longjon
Copy link
Contributor Author

longjon commented Feb 17, 2015

A note for a possible future PR: With V2 protobufs, a new, nicer(?) way to implement Python layers becomes possible. Instead of explicitly declaring each Python layer to have type Python, it would be more natural to declare layers with their actual types, abstracting away from implementation language. E.g., one might add to one's prototxt import_python: my_module, thereby gaining additional layer types. There's a clear upgrade path from this PR, so let's postpone that for later.

@longjon longjon force-pushed the pyreformation branch 2 times, most recently from 45095b7 to 5ef63e0 Compare February 17, 2015 03:36
@longjon
Copy link
Contributor Author

longjon commented Feb 17, 2015

Rebased with layer factory using shared_ptr. (@shelhamer, let's work out and merge #1842 first, then I can rebase and check that the linking here works okay.)

shelhamer and others added 3 commits February 16, 2015 22:47
PythonLayer (rightfully) refuses to be instantiated when a module and
class aren't given. It's still covered by tests however through pytest.
shelhamer added a commit that referenced this pull request Feb 17, 2015
Reform the boost::python wrapper, including layers implemented in Python
@shelhamer shelhamer merged commit 5e64f5a into BVLC:dev Feb 17, 2015
@longjon longjon deleted the pyreformation branch February 17, 2015 09:34
@shelhamer
Copy link
Member

image

@longjon
Copy link
Contributor Author

longjon commented Feb 17, 2015

Postscript: the "shortcomings" noted above were addressed as follows:

  • Since Change Python interface for mode, phase, and device #1728 already moved the global functions to the caffe module, calls to Net.set_* have been eliminated, and this PR follows suit.
  • The layer factory is updated to pass around shared_ptrs.
  • With dynamic linking (Dynamic Linking #1842), and the light Makefile adjustments made here, linking issues should be resolved. Python layers can be used from pycaffe or the caffe tool with no abnormal crashes at any time.

@shelhamer shelhamer mentioned this pull request Feb 17, 2015
shelhamer added a commit to shelhamer/caffe that referenced this pull request Feb 19, 2015
The pyreformation BVLC#1703 aligned pycaffe Net with the real Caffe Net.
This broke the pre-processing helpers that had been bolted on to
`caffe.Net`.

These responsibilities are gathered in `caffe.io.Transformer` instead.
This is only an intermediate step in a real solution to pycaffe IO that
should make use of the same data pipeline as the rest of the framework.

See BVLC#1245 for further thoughts on input processing.
@jeffdonahue
Copy link
Contributor

I was just trying out PythonLayer for the first time today/yesterday -- I'm using it to input data, replacing a combination of synchronized HDF5Data and ImageData layers. I commented in #1020 that it would be sweet if the PythonLayer could run in a prefetch thread like many of the existing C++ data layers, but I found from working today that doing something like that in the Caffe core probably isn't really necessary as you can implement your own simple prefetching in the PythonLayer itself using Python's multiprocessing tools (e.g., Thread, Process, and Pool -- all three of these worked for me). I implemented it very similarly to how the prefetching is done in DataLayer etc. -- start the first Thread/Process/Pool in setup which goes and gets the first batch of data, call the join method at the beginning of forward, copy the prefetched batches to top, and finally call the start (or equivalent) method at the end of forward to begin preparing the next batch.

Using a Python Pool with 10-16 workers to load and preprocess images from disk was significantly faster (maybe 2x) than the combination of HDF5/ImageData layers. Also, it's much, much easier than dealing with the bookkeeping necessary for feeding in data through combinations of the existing data layer types.

@shelhamer
Copy link
Member

At some point there'll be a proper PythonLayer example but until then anyone interested can check out this Python loss layer gist that ports EuclideanLossLayer to Python and the PythonLayer test bundled with pycaffe. These both show example setup, reshape, forward, and backward methods.

@7hil
Copy link
Contributor

7hil commented Mar 14, 2015

Seems there is no way to pass parameters to PythonLayer. How about adding a string field into the PythonParameter as a parameter of the PythonLayer? The layer can interpret this parameter as whatever it want such as a path to a file that holds the true parameters or a json string that hold true parameters.

@tnarihi
Copy link
Contributor

tnarihi commented Mar 14, 2015

Please see my PR #2001, but this is still not merged. I would appreciate any comments or suggestions.

@7hil
Copy link
Contributor

7hil commented Mar 14, 2015

@tnarihi exactly what I want. I should search PRs first. Thanks.

@Edward12138
Copy link

Is there a way to define the blob shape or number of output of python layer?

@escorciav
Copy link

Hi @jeffdonahue, Could you share your python_layers for HDF5 and ImageData in a gist or something similar?
I'm trying to do something similar and it would be great to have a point of reference. Besides, the multithreading idea sounds fantastic.

@stas-sl
Copy link

stas-sl commented Apr 9, 2015

@escorciav
Copy link

Thank you. It looks very interesting and it's more pythonic+clean than my current attempt.
I hope that they will merge these ideas.

@elleryrussell
Copy link

This is great functionality, but is there any way to access memory on the gpu instead? It'd be nice to write gpu layers in pycuda (I've been itching for a top-k argmax/accuracy layer for multidimensional outputs so that testing is more feasible) and not have to copy the memory from the gpu to the cpu and back.

@haik
Copy link

haik commented May 6, 2017

I encounter 2 kinds of error when I use the custom python layer:

  1. Error parsing text-format caffe.NetParameter: 22:9: Expected integer or identifier, got: "Python".
  2. Error parsing text-format caffe.NetParameter: 23:3: Unknown enumeration value of "PYTHON" for field "type".
    Who can help me solve this? Thanks very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.