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

Making python3 work with cmake and the new python wrapper #1923

Merged
merged 1 commit into from
Feb 24, 2015

Conversation

philkr
Copy link
Contributor

@philkr philkr commented Feb 21, 2015

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).

philkr added a commit to philkr/caffe that referenced this pull request Feb 21, 2015
Making python3 work with cmake and the new python wrapper
@longjon
Copy link
Contributor

longjon commented Feb 24, 2015

Cool, but some of the Python changes appear to be superfluous... were they automatically generated? I.e., there are some extra parens in print statements, and some list(...)s added where not apparently needed. What's up with that?

@philkr
Copy link
Contributor Author

philkr commented Feb 24, 2015

I used 2to3. The list and duplicate parens should be cleaned up now.

@tnarihi
Copy link
Contributor

tnarihi commented Feb 24, 2015

Does this PR mean the future PRs regarding Python features should be compatible with Python 3?

@philkr
Copy link
Contributor Author

philkr commented Feb 24, 2015

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();
Copy link
Contributor

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.

Copy link
Contributor Author

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).

Copy link
Contributor

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...

Copy link
Contributor Author

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.

@longjon
Copy link
Contributor

longjon commented Feb 24, 2015

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.

@tnarihi
Copy link
Contributor

tnarihi commented Feb 24, 2015

@philkr No. I just thought it should be documented somewhere for developers or testing should be added for Python 3 compatibility as @longjon pointed.

@Nerei
Copy link

Nerei commented Feb 24, 2015

👍 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.

@longjon
Copy link
Contributor

longjon commented Feb 24, 2015

Okay, all looks good now. Thanks for leading us into the future @philkr!

longjon added a commit that referenced this pull request Feb 24, 2015
Making python3 work with cmake and the new python wrapper
@longjon longjon merged commit b915f9d into BVLC:master Feb 24, 2015
philkr added a commit to philkr/caffe that referenced this pull request Feb 25, 2015
@philkr philkr deleted the python3_master branch February 28, 2015 18:30
longjon added a commit that referenced this pull request Mar 4, 2015
cmake and python3 bugfixes for #1939 and #1923
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.

4 participants