8000 Fix Python 3 compilation issues by jfsantos · Pull Request #764 · tensorflow/tensorflow · GitHub
[go: up one dir, main page]
Skip to content

Fix Python 3 compilation issues #764

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix Python 3 compilation issues #764

wants to merge 1 commit into from

Conversation

jfsantos
Copy link
Contributor

This fixes the build error when using Python 3 as the return types for import array operations are int in Python 3 and void in Python 2 (as discussed in #733).

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@jfsantos jfsantos mentioned this pull request Jan 13, 2016
Closed
martinwicke
martinwicke reviewed Jan 13, 2016
View reviewed changes
tensorflow/python/lib/core/py_func.h
@@ -16,6 +16,12 @@ limitations under the License.
#ifndef TENSORFLOW_PYTHON_LIB_CORE_PY_FUNC_H_
#define TENSORFLOW_PYTHON_LIB_CORE_PY_FUNC_H_

#if NUMPY_IMPORT_ARRAY_RETVAL == NULL
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment to explain why this is necessary? Something like "The return type of import_array() changed between Python 2 and 3"

@martinwicke
Copy link
Member

Jenkins, test this please.

@martinwicke
Copy link
Member

I'll merge once you add the comment, and squash the commits (or amend the existing one).

@martinwicke
Copy link
Member

I take that back, your other problem may well be related.

The #if thing must not be in the header (since we will not know about NUMPY_IMPORT_ARRAY_RETVAL until after numpy/arrayobject.h has been included). Move it to the c file, below the includes, and you should be fine.

@jfsantos
Copy link
Contributor Author

I fixed it and added the comment, but it still does not build under Linux. The build breaks at the same place as it broke before the fix, and I could not figure out what happened as the error message returned by bazel only states which rule and command failed. I will test on OS X right now.

@martinwicke
Copy link
Member

Jenkins, test this please.

@martinwicke
Copy link
Member

Fortunately, the Jenkins build produced a decent enough error message -- can you see if that's related to your failure or even the same?

@jfsantos
Copy link
Contributor Author

Yes, it seems to be the same issue.

@martinwicke
Copy link
Member

Error is enlightening:

tensorflow/python/lib/core/py_func.cc:29:31: error: operator '==' has no left operand
#if NUMPY_IMPORT_ARRAY_RETVAL == NULL

Checking for empty defines is nasty business, look at this: http://stackoverflow.com/questions/3781520/how-to-test-if-preprocessor-symbol-is-defined-but-has-no-value

@jfsantos
Copy link
Contributor Author

I was expecting the error message to be below the red "ERROR" message, not above it. Now it's clear!

Would it be acceptable to use PY_MAJOR_VERSION for the comparison instead of NUMPY_IMPORT_ARRAY_RETVAL, just to avoid adding macro black magic just for this? I think it will make the code clearer.

@jfsantos
Copy link
Contributor Author

I fixed this by using PY_MAJOR_VERSION (as mentioned above), but now there's another build error that is not related to this code:

bazel-out/local_linux-py3-opt/bin/tensorflow/python/pywrap_tensorflow.cc: In function 'PyObject* _wrap_TF_GetOpList(PyObject*, PyObject*)':
bazel-out/local_linux-py3-opt/bin/tensorflow/python/pywrap_tensorflow.cc:4636:72: error: 'PyString_FromStringAndSize' was not declared in this scope
       reinterpret_cast<const char*>((&result)->data), (&result)->length);

PyString_FromStringAndSize does not exist in Python 3, and this is dealt with in the SWIG interface platform/base.i, but not in client/tf_session.i.

@martinwicke
Copy link
Member

Can you fix it in tf_session the same way it's fixed in base.i? I think that's the right thing to do. Might as well make this PR "Fix Python 3 compilation issues" since we're almost there already.

@jfsantos jfsantos changed the title Fixed Numpy return value for Python 3 Fix Python 3 compilation issues Jan 13, 2016
@jfsantos
Copy link
Contributor Author

This is now fixed but the build is still breaking with the same error:

bazel-out/local_linux-py3-opt/bin/tensorflow/python/pywrap_tensorflow.cc:4639:72: error: 'PyString_FromStringAndSize' was not declared in this scope
       reinterpret_cast<const char*>((&result)->data), (&result)->length);

Since I have both Python 2 and 3 installed, is there a chance bazel is passing the wrong includes/libs to GCC? I tried cleaning and reconfiguring the build to no avail.

@martinwicke
8000
Copy link
Member

Jenkins, test this please.

@martinwicke
Copy link
Member

Just making another log for myself. :)

@jfsantos
Copy link
Contributor Author

I guess the problem was having both Python 2 and 3, since tests passed on Linux and Android but not Mac? I don't know how the environment used by Jenkins looks like, but I'm on Ubuntu 14.04 with python-dev and python3-dev installed.

@martinwicke
Copy link
Member

The Mac build is broken right now, unrelated to this. :/

It's strange because the compiler shouldn't even see the PyString_ version any more. Can you remove the ifdef altogether (and keep only the Py3 branch) and see if that works compiling with Python3? Those two are the only occurrences of this function, so I feel like for some reason the ifdef doesn't work as expected (maybe because it's evaluated by swig, and we need a %{ %} around it).

@jfsantos
Copy link
Contributor Author

Removing the Python 2 only branches works. According to the docs SWIG should not mess with C preprocessor stuff, but it seems that's wrong :)

I think %{ } only works for things to be included in the interface header, isn't it?

@jfsantos
Copy link
Contributor Author

To add to the weirdness of this issue: something (maybe SWIG?) adds the following macros to pywrap_tensorflow.cc:

#if PY_VERSION_HEX >= 0x03000000

#define PyClass_Check(obj) PyObject_IsInstance(obj, (PyObject *)&PyType_Type)
#define PyInt_Check(x) PyLong_Check(x)
#define PyInt_AsLong(x) PyLong_AsLong(x)
#define PyInt_FromLong(x) PyLong_FromLong(x)
#define PyInt_FromSize_t(x) PyLong_FromSize_t(x)
#define PyString_Check(name) PyBytes_Check(name)
#define PyString_FromString(x) PyUnicode_FromString(x)
#define PyString_Format(fmt, args)  PyUnicode_Format(fmt, args)
#define PyString_AsString(str) PyBytes_AsString(str)
#define PyString_Size(str) PyBytes_Size(str)    
#define PyString_InternFromString(key) PyUnicode_InternFromString(key)
#define Py_TPFLAGS_HAVE_CLASS Py_TPFLAGS_BASETYPE
#define PyString_AS_STRING(x) PyUnicode_AS_STRING(x)
#define _PyLong_FromSsize_t(x) PyLong_FromSsize_t(x)

#endif

These should be defined and avoid the issue without any other workarounds but that never happens, which probably means both PY_VERSION_HEX and PY_MAJOR_VERSION are not defined correctly. The script in util/python/python_config.sh seems to be working properly, as the links to python_include and python_lib are correct.

@martinwicke
Copy link
Member

I see that this list conspicuously does not include
PyString_FromStringAndSize.

Maybe this is a swig problem? What version of swig is this, and does
python3 maybe need a newer version of it?

On Wed, Jan 13, 2016 at 3:08 PM João Felipe Santos notifications@github.com
wrote:

To add to the weirdness of this issue: something (maybe SWIG?) adds the
following macros to pywrap_tensorflow.cc:

#if PY_VERSION_HEX >= 0x03000000

#define PyClass_Check(obj) PyObject_IsInstance(obj, (PyObject *)&PyType_Type)
#define PyInt_Check(x) PyLong_Check(x)
#define PyInt_AsLong(x) PyLong_AsLong(x)
#define PyInt_FromLong(x) PyLong_FromLong(x)
#define PyInt_FromSize_t(x) PyLong_FromSize_t(x)
#define PyString_Check(name) PyBytes_Check(name)
#define PyString_FromString(x) PyUnicode_FromString(x)
#define PyString_Format(fmt, args) PyUnicode_Format(fmt, args)
#define PyString_AsString(str) PyBytes_AsString(str)
#define PyString_Size(str) PyBytes_Size(str)
#define PyString_InternFromString(key) PyUnicode_InternFromString(key)
#define Py_TPFLAGS_HAVE_CLASS Py_TPFLAGS_BASETYPE
#define PyString_AS_STRING(x) PyUnicode_AS_STRING(x)
#define _PyLong_FromSsize_t(x) PyLong_FromSsize_t(x)

#endif

These should be defined and avoid the issue without any other workarounds
but that never happens, which probably means both PY_VERSION_HEX and
PY_MAJOR_VERSION are not defined correctly.

—
Reply to this email directly or view it on GitHub
#764 (comment)
.

@jfsantos
Copy link
Contributor Author

I guess I was too tired and did not see PyString_FromStringAndSize was missing, sorry!

My Linux box is on Ubuntu 14.04, which ships SWIG 2.0.11, but with SWIG 3.0.8 on my Mac I got the same auto-generated macros. However, SWIG's at fault: if you check line 208 on tensorflow/python/client/tf_session.i, we added a SWIG typemap which uses an #if PY_MAJOR_VERSION. This is what gets translated into the broken code, since the SWIG-generated file has a similar call with blank lines in the places the branched-out #if lines are.

Code in tf_session.i:

%typemap(out) TF_Buffer TF_GetOpList {
#if PY_MAJOR_VERSION < 3
  $result = PyString_FromStringAndSize(
#else
  $result = PyUnicode_FromStringAndSize(
#endif
      reinterpret_cast<const char*>($1.data), $1.length);
}

Code in pywrap_tensorflow.cc:

  {
    resultobj = PyString_FromStringAndSize(



      reinterpret_cast<const char*>((&result)->data), (&result)->length);
  }

Turns out SWIG was preprocessing the #if PY_MAJOR_VERSION as you suspected, and you have to use %# instead of # to make it forward preprocessing directives to the generated code. This is fixed in the last commit and the code now compiles successfully on OS X and Linux.

@martinwicke
Copy link
Member

Splendid. Can you squash, please?

@martinwicke
Copy link
Member

Jenkins, test this please.

@jfsantos
Fixed Numpy return value for Python 3
6980af7 
Using PY_MAJOR_VERSION instead of NUMPY_IMPORT_ARRAY_RETVAL

Fixed SWIG interface for Python 3 support

Make SWIG forward preprocessor directives to generated code
@jfsantos
Copy link
Contributor Author

Done!

@martinwicke
Copy link
Member

Jenkins, test this please.

@vrv vrv closed this in 60114c4 Jan 14, 2016
@vrv
Copy link
vrv commented Jan 14, 2016

Merged

@jfsantos jfsantos deleted the import_array branch January 14, 2016 19:20
darkbuck pushed a commit to darkbuck/tensorflow that referenced this pull request Jan 23, 2020
@deven-amd
Merge pull request tensorflow#764 from ROCmSoftwarePlatform/develop-u…
9567cdd 
…pstream-deven-switch-to-rocm30

Switching from ROCm 2.8 to ROCm 3.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@martinwicke martinwicke

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jfsantos @tensorflow-jenkins @martinwicke @vrv @googlebot
0