-
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
Making python3 work with cmake and the new python wrapper #1923
Conversation
Making python3 work with cmake and the new python wrapper
Cool, but some of the Python changes appear to be superfluous... were they automatically generated? I.e., there are some extra parens in |
16482d5
to
4f7edb1
Compare
I used 2to3. The list and duplicate parens should be cleaned up now. |
Does this PR mean the future PRs regarding Python features should be compatible with Python 3? |
That would be nice. Do you have anything in mind that wouldn't work with python3? |
@@ -275,7 +275,7 @@ BOOST_PYTHON_MODULE(_caffe) { | |||
bp::class_<vector<bool> >("BoolVec") | |||
.def(bp::vector_indexing_suite<vector<bool> >()); | |||
|
|||
import_array(); | |||
import_array1(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain/reference this? Everything I've found defines import_array1
as a unary function (hence the 1), and I can't find any reason there'd be a difference between 2 and 3, so I'm confused as to why this works at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import_array1 is the same as input_array, but it takes the return value of the statement as an argument. If we leave this return statement empty it will return "void". import_array will not compile in python3 (the boost function is defined as void ..., while import_array calls return some_int_constant).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a boost issue? Surely boost should define an init function with the right type for the Python version? It does seem that import_array
does the right thing, which is to return void for 2 and NULL
for 3. What actually goes wrong with import_array
? It sounds like boost thinks it's being built for 2...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is what goes wrong:
/usr/include/python3.4m/numpy/__multiarray_api.h:1708:35: error: return-statement with a value, in function returning 'void' [-fpermissive] #define NUMPY_IMPORT_ARRAY_RETVAL NULL
The boost python function BOOST_PYTHON_MODULE(_caffe)
is always defined as void BOOST_PP_CAT(init_module_, name)()
which translates to something like void init_module__caffe()
. This is irrespective of the python version (which is good).
Numpy switches out the return value of import_array
from void to NULL, when switching from python2 to python3, for whatever reason. import_array1
fixes this.
Looks good except as noted. @Nerei, can you give a nod to the CMake additions (or comment as needed)? Regarding future PRs, yes, let's try to maintain Python 3 compatibility as long as it's easy to do so. If there's major functionality that relies on Python 2, we'll have to rethink that, but it's definitely going to be easier to maintain compatibility rather than lose it and try to restore it when we really want it. @philkr, if you want to add Python 3 testing to Travis, that would be a big step toward keeping future code compatible. |
4f7edb1
to
d8e1007
Compare
👍 This is a fine cmake change. Thank you! JFYI, old cmake also supported Python3. Just in case of both installed it selected 2.x by default, for 3.x user had to specify paths manually then. But here is an additional boost_pyhton serach feature. |
d8e1007
to
54037d3
Compare
Okay, all looks good now. Thanks for leading us into the future @philkr! |
Making python3 work with cmake and the new python wrapper
Rebase of #1894 to master.
A small change than allows the cmake based build to use python3.
Simply specify the "python_version" in the cmake configuration (either 2 or 3).
Also replaced "import_array" with "import_array1" which works for both python 2.7 and 3.? .
Protobuf and pycaffe should now work with python3. Updated the docs to reflect the fact that protobuf 3.0 alpha is required for python3 (not for python2).