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

Make TensorFlow pip package build on Windows #4942

Merged
merged 11 commits into from Oct 19, 2016

Conversation

Projects
None yet
9 participants
@meteorcloudy
Member

meteorcloudy commented Oct 13, 2016

@tensorflow-jenkins

This comment has been minimized.

Show comment
Hide comment
@tensorflow-jenkins

tensorflow-jenkins Oct 13, 2016

Collaborator

Can one of the admins verify this patch?

Collaborator

tensorflow-jenkins commented Oct 13, 2016

Can one of the admins verify this patch?

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Oct 13, 2016

@meteorcloudy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vrv, @keveman and @davidzchen to be potential reviewers.

mention-bot commented Oct 13, 2016

@meteorcloudy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vrv, @keveman and @davidzchen to be potential reviewers.

@googlebot googlebot added the cla: yes label Oct 13, 2016

@mrry

This is exciting progress! Just a few nits....

Show outdated Hide outdated tensorflow/contrib/BUILD
Show outdated Hide outdated tensorflow/contrib/BUILD
Show outdated Hide outdated tensorflow/contrib/BUILD
Show outdated Hide outdated tensorflow/tools/pip_package/BUILD
Show outdated Hide outdated tensorflow/tools/pip_package/setup.py
Show outdated Hide outdated tensorflow/tools/pip_package/build_pip_package.sh
@meteorcloudy

This comment has been minimized.

Show comment
Hide comment
@meteorcloudy

meteorcloudy Oct 14, 2016

Member

Change updated, please review again!

One thing I forgot to mention, the pip package build now only works on Windows with Bazel at HEAD and Visual Studio Update 2. I had some changes to make Bazel work with Python3, but for Visual Studio, we still need /WHOLEARCHIVE to link _pywrap_tensorflow.so(In fact, it's a dll). The reason is we need the global whole archive feature to force link all its dependencies no matter whether the dependency has alwaylink = 1 or not. Without /WHOLEARCHIVE, I haven't figured out a simple solution to get all the object files from all of its dependencies.

So, unfortunately if users want to build the pip package on Windows, they have to install VS 2015 update 2 or newer version. Is it going to be a serious problem or not?

Member

meteorcloudy commented Oct 14, 2016

Change updated, please review again!

One thing I forgot to mention, the pip package build now only works on Windows with Bazel at HEAD and Visual Studio Update 2. I had some changes to make Bazel work with Python3, but for Visual Studio, we still need /WHOLEARCHIVE to link _pywrap_tensorflow.so(In fact, it's a dll). The reason is we need the global whole archive feature to force link all its dependencies no matter whether the dependency has alwaylink = 1 or not. Without /WHOLEARCHIVE, I haven't figured out a simple solution to get all the object files from all of its dependencies.

So, unfortunately if users want to build the pip package on Windows, they have to install VS 2015 update 2 or newer version. Is it going to be a serious problem or not?

Show outdated Hide outdated tensorflow/BUILD
@@ -115,6 +115,10 @@ if [[ ! -e "$SWIG_PATH" ]]; then
echo "Can't find swig. Ensure swig is in \$PATH or set \$SWIG_PATH."
exit 1
fi
# Convert swig path to Windows style before writing into swig_path
if is_windows; then
SWIG_PATH="$(cygpath -m "$SWIG_PATH")"

This comment has been minimized.

@mrry

mrry Oct 16, 2016

Contributor

Is there a reason to use -m ("mixed") rather than the more natural -w ("Windows names")?

@mrry

mrry Oct 16, 2016

Contributor

Is there a reason to use -m ("mixed") rather than the more natural -w ("Windows names")?

This comment has been minimized.

@meteorcloudy

meteorcloudy Oct 17, 2016

Member

The problem is actually when writing PYTHON_BIN_PATH into tools/bazel.rc, if we use
-w, the bazel.rc file will look like:

build --define PYTHON_BIN_PATH="C:Program FilesAnaconda3python"
test --define PYTHON_BIN_PATH="C:Program FilesAnaconda3python"

So I feel using -m is a safer choice.

@meteorcloudy

meteorcloudy Oct 17, 2016

Member

The problem is actually when writing PYTHON_BIN_PATH into tools/bazel.rc, if we use
-w, the bazel.rc file will look like:

build --define PYTHON_BIN_PATH="C:Program FilesAnaconda3python"
test --define PYTHON_BIN_PATH="C:Program FilesAnaconda3python"

So I feel using -m is a safer choice.

This comment has been minimized.

@mrry

mrry Oct 17, 2016

Contributor

Ah, that makes sense. Sounds good!

@mrry

mrry Oct 17, 2016

Contributor

Ah, that makes sense. Sounds good!

Show outdated Hide outdated tensorflow/tools/pip_package/build_pip_package.sh
Show outdated Hide outdated tensorflow/tools/swig/swig.bat
$RUNFILES/external/protobuf ${TMPDIR}/google
rsync -a $RUNFILES/third_party/eigen3 ${TMPDIR}/third_party
mkdir -p ${TMPDIR}/third_party
pushd ${RUNFILES%org_tensorflow}

This comment has been minimized.

@meteorcloudy

meteorcloudy Oct 17, 2016

Member

Don't use the legacy external runfiles($RUNFILES/external/protobuf) anymore, because they are not archived into python zip file.

@meteorcloudy

meteorcloudy Oct 17, 2016

Member

Don't use the legacy external runfiles($RUNFILES/external/protobuf) anymore, because they are not archived into python zip file.

@mrry

mrry approved these changes Oct 17, 2016

LGTM.

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 17, 2016

Contributor

@tensorflow-jenkins test this please.

Contributor

mrry commented Oct 17, 2016

@tensorflow-jenkins test this please.

@meteorcloudy

This comment has been minimized.

Show comment
Hide comment
@meteorcloudy

meteorcloudy Oct 17, 2016

Member

I know why tests are failing.. Because my fix for example trainer was reverted at be3bc47 .. :(
Let's wait until we solve that problem..

Member

meteorcloudy commented Oct 17, 2016

I know why tests are failing.. Because my fix for example trainer was reverted at be3bc47 .. :(
Let's wait until we solve that problem..

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 17, 2016

Contributor

@tensorflow-jenkins test this please.

Contributor

mrry commented Oct 17, 2016

@tensorflow-jenkins test this please.

@meteorcloudy

This comment has been minimized.

Show comment
Hide comment
@meteorcloudy

meteorcloudy Oct 18, 2016

Member

I fixed the sanity checks failures, please test again :)

Member

meteorcloudy commented Oct 18, 2016

I fixed the sanity checks failures, please test again :)

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 18, 2016

Contributor

@tensorflow-jenkins test this please.

Contributor

mrry commented Oct 18, 2016

@tensorflow-jenkins test this please.

@meteorcloudy

This comment has been minimized.

Show comment
Hide comment
@meteorcloudy

meteorcloudy Oct 18, 2016

Member

Linux GPU looks like flaky, rerun it?

Member

meteorcloudy commented Oct 18, 2016

Linux GPU looks like flaky, rerun it?

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 18, 2016

Contributor

@tensorflow-jenkins test this please.

Contributor

mrry commented Oct 18, 2016

@tensorflow-jenkins test this please.

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 18, 2016

Contributor

@tensorflow-jenkins test this please.

Contributor

mrry commented Oct 18, 2016

@tensorflow-jenkins test this please.

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 18, 2016

Contributor

(Third time's a charm!)

Contributor

mrry commented Oct 18, 2016

(Third time's a charm!)

@meteorcloudy

This comment has been minimized.

Show comment
Hide comment
@meteorcloudy

meteorcloudy Oct 18, 2016

Member

Fourth time? 😂

Member

meteorcloudy commented Oct 18, 2016

Fourth time? 😂

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 18, 2016

Contributor

@tensorflow-jenkins test this please.

Contributor

mrry commented Oct 18, 2016

@tensorflow-jenkins test this please.

@drpngx

This comment has been minimized.

Show comment
Hide comment
@drpngx

drpngx Oct 19, 2016

Member

We bumped up the gradients_test size #5062, it has to work this time :-)

Member

drpngx commented Oct 19, 2016

We bumped up the gradients_test size #5062, it has to work this time :-)

@drpngx

This comment has been minimized.

Show comment
Hide comment
@drpngx

drpngx Oct 19, 2016

Member

Could you pull rebase and push again? There's a change in tensorflow/python/BUILD. Thanks!

Member

drpngx commented Oct 19, 2016

Could you pull rebase and push again? There's a change in tensorflow/python/BUILD. Thanks!

@meteorcloudy

This comment has been minimized.

Show comment
Hide comment
@meteorcloudy

meteorcloudy Oct 19, 2016

Member

@drpngx I've rebased my branch, please test again. :)

Member

meteorcloudy commented Oct 19, 2016

@drpngx I've rebased my branch, please test again. :)

@meteorcloudy

This comment has been minimized.

Show comment
Hide comment
@meteorcloudy

meteorcloudy Oct 19, 2016

Member

Wait... It's now broken because of //tensorflow/core/kernels:quantized_ops

Member

meteorcloudy commented Oct 19, 2016

Wait... It's now broken because of //tensorflow/core/kernels:quantized_ops

@meteorcloudy

This comment has been minimized.

Show comment
Hide comment
@meteorcloudy

meteorcloudy Oct 19, 2016

Member

OK, it's fixed.

Member

meteorcloudy commented Oct 19, 2016

OK, it's fixed.

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 19, 2016

Contributor

@tensorflow-jenkins test this please.

Contributor

mrry commented Oct 19, 2016

@tensorflow-jenkins test this please.

@drpngx drpngx merged commit e882241 into tensorflow:master Oct 19, 2016

10 checks passed

Android Demo App SUCCESS
Details
Linux CPU Tests SUCCESS
Details
Linux CPU Tests (Python 3) SUCCESS
Details
Linux CPU Tests CMAKE SUCCESS
Details
Linux GPU SUCCESS
Details
MacOS CPU Tests SUCCESS
Details
Sanity Checks SUCCESS
Details
Windows Cmake Tests SUCCESS
Details
ci.tensorflow.org SUCCESS
Details
cla/google All necessary CLAs are signed
@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 19, 2016

Contributor

Hooray!

Contributor

mrry commented Oct 19, 2016

Hooray!

@terrytangyuan

This comment has been minimized.

Show comment
Hide comment
@terrytangyuan

terrytangyuan Oct 19, 2016

Member

You might want to squash the commits when merging....commit history is a bit untraceable now

Member

terrytangyuan commented Oct 19, 2016

You might want to squash the commits when merging....commit history is a bit untraceable now

@erfannoury

This comment has been minimized.

Show comment
Hide comment
@erfannoury

erfannoury Oct 20, 2016

Contributor

I installed TensorFlow using pip on Python 2.7.12. When I import TensorFlow I get this error message:

In [1]: import tensorflow
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-1-a649b509054f> in <module>()
----> 1 import tensorflow

C:\Anaconda\lib\site-packages\tensorflow\__init__.py in <module>()
     21 from __future__ import print_function
     22
---> 23 from tensorflow.python import *
     24
     25

C:\Anaconda\lib\site-packages\tensorflow\python\__init__.py in <module>()
     45 # when importing numpy (gh-2034).
     46 import numpy as np
---> 47 _default_dlopen_flags = sys.getdlopenflags()
     48 sys.setdlopenflags(_default_dlopen_flags | ctypes.RTLD_GLOBAL)
     49 from tensorflow.python import pywrap_tensorflow

AttributeError: 'module' object has no attribute 'getdlopenflags'

sys module has getdlopenflags on Python 3.x.
What can be done?

Contributor

erfannoury commented Oct 20, 2016

I installed TensorFlow using pip on Python 2.7.12. When I import TensorFlow I get this error message:

In [1]: import tensorflow
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-1-a649b509054f> in <module>()
----> 1 import tensorflow

C:\Anaconda\lib\site-packages\tensorflow\__init__.py in <module>()
     21 from __future__ import print_function
     22
---> 23 from tensorflow.python import *
     24
     25

C:\Anaconda\lib\site-packages\tensorflow\python\__init__.py in <module>()
     45 # when importing numpy (gh-2034).
     46 import numpy as np
---> 47 _default_dlopen_flags = sys.getdlopenflags()
     48 sys.setdlopenflags(_default_dlopen_flags | ctypes.RTLD_GLOBAL)
     49 from tensorflow.python import pywrap_tensorflow

AttributeError: 'module' object has no attribute 'getdlopenflags'

sys module has getdlopenflags on Python 3.x.
What can be done?

@gunan

This comment has been minimized.

Show comment
Hide comment
@gunan

gunan Oct 23, 2016

Member

This PR breaks pip package building in Mac.
It added "cp --parents" which does not seem to be available on macos.

Member

gunan commented Oct 23, 2016

This PR breaks pip package building in Mac.
It added "cp --parents" which does not seem to be available on macos.

mkdir -p ${TMPDIR}/third_party
pushd ${RUNFILES%org_tensorflow}
for header in $(find protobuf -name \*.h); do
cp --parents "$header" ${TMPDIR}/google;

This comment has been minimized.

@gunan

gunan Oct 23, 2016

Member

This specific flag is not supported in BSD's cp, which macos uses.

@gunan

gunan Oct 23, 2016

Member

This specific flag is not supported in BSD's cp, which macos uses.

@meteorcloudy meteorcloudy deleted the meteorcloudy:windows_build_pip_package branch Mar 29, 2017

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