-
Notifications
You must be signed in to change notification settings - Fork 74k
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
Tidy up MKL configuration. #11212
Tidy up MKL configuration. #11212
Changes from all commits
1cf372e
92ad2b6
cdb7581
a8ca182
5a87ad1
9f0d757
8aeefe8
ecd1201
e2add5c
01cc103
8f66df0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,10 @@ | |
|
||
set -e | ||
|
||
function real_path() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I try that on macOS, I see this:
readlink -f seems to be OK with GNU coreutils, but it does not seem to work with BSD coreutils. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need a realpath function? |
||
[[ $1 = /* ]] && echo "$1" || echo "$PWD/${1#./}" | ||
} | ||
|
||
function cp_external() { | ||
local src_dir=$1 | ||
local dest_dir=$2 | ||
|
@@ -41,7 +45,7 @@ function main() { | |
exit 1 | ||
fi | ||
|
||
DEST=$1 | ||
DEST=$(real_path $1) | ||
TMPDIR=$(mktemp -d -t tmp.XXXXXXXXXX) | ||
|
||
GPU_FLAG="" | ||
|
@@ -79,23 +83,6 @@ function main() { | |
bazel-bin/tensorflow/tools/pip_package/simple_console_for_window_unzip/runfiles \ | ||
"${TMPDIR}/external" | ||
RUNFILES=bazel-bin/tensorflow/tools/pip_package/simple_console_for_window_unzip/runfiles/org_tensorflow | ||
elif [ ! -d bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow ]; then | ||
# Really old (0.2.1-) runfiles, without workspace name. | ||
cp -R \ | ||
bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/tensorflow \ | ||
"${TMPDIR}" | ||
mkdir "${TMPDIR}/external" | ||
cp_external \ | ||
bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/external \ | ||
"${TMPDIR}/external" | ||
RUNFILES=bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles | ||
# Copy MKL libs over so they can be loaded at runtime | ||
if [ -d bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow/_solib_k8/_U_S_Sthird_Uparty_Smkl_Cintel_Ubinary_Ublob___Uthird_Uparty_Smkl ]; then | ||
mkdir "${TMPDIR}/_solib_k8" | ||
cp -R \ | ||
bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow/_solib_k8/_U_S_Sthird_Uparty_Smkl_Cintel_Ubinary_Ublob___Uthird_Uparty_Smkl \ | ||
"${TMPDIR}/_solib_k8" | ||
fi | ||
else | ||
if [ -d bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow/external ]; then | ||
# Old-style runfiles structure (--legacy_external_runfiles). | ||
|
@@ -107,11 +94,13 @@ function main() { | |
bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow/external \ | ||
"${TMPDIR}/external" | ||
# Copy MKL libs over so they can be loaded at runtime | ||
if [ -d bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow/_solib_k8/_U_S_Sthird_Uparty_Smkl_Cintel_Ubinary_Ublob___Uthird_Uparty_Smkl ]; then | ||
mkdir "${TMPDIR}/_solib_k8" | ||
cp -R \ | ||
bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow/_solib_k8/_U_S_Sthird_Uparty_Smkl_Cintel_Ubinary_Ublob___Uthird_Uparty_Smkl \ | ||
"${TMPDIR}/_solib_k8" | ||
so_lib_dir="bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow/_solib_k8" | ||
if [ -d ${so_lib_dir} ]; then | ||
mkl_so_dir=$(ls ${so_lib_dir} | grep mkl) | ||
if [ $? -eq 0 ]; then | ||
mkdir "${TMPDIR}/_solib_k8" | ||
cp -R ${so_lib_dir}/${mkl_so_dir} "${TMPDIR}/_solib_k8" | ||
fi | ||
fi | ||
else | ||
# New-style runfiles structure (--nolegacy_external_runfiles). | ||
|
@@ -124,11 +113,13 @@ function main() { | |
bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles \ | ||
"${TMPDIR}/external" | ||
# Copy MKL libs over so they can be loaded at runtime | ||
if [ -d bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow/_solib_k8/_U_S_Sthird_Uparty_Smkl_Cintel_Ubinary_Ublob___Uthird_Uparty_Smkl ]; then | ||
mkdir "${TMPDIR}/_solib_k8" | ||
cp -R \ | ||
bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow/_solib_k8/_U_S_Sthird_Uparty_Smkl_Cintel_Ubinary_Ublob___Uthird_Uparty_Smkl \ | ||
"${TMPDIR}/_solib_k8" | ||
so_lib_dir="bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow/_solib_k8" | ||
if [ -d ${so_lib_dir} ]; then | ||
mkl_so_dir=$(ls ${so_lib_dir} | grep mkl) | ||
if [ $? -eq 0 ]; then | ||
mkdir "${TMPDIR}/_solib_k8" | ||
cp -R ${so_lib_dir}/${mkl_so_dir} "${TMPDIR}/_solib_k8" | ||
fi | ||
fi | ||
fi | ||
RUNFILES=bazel-bin/tensorflow/tools/pip_package/build_pip_package.runfiles/org_tensorflow | ||
|
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.
Why this change?
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.
previously all the headers were manually copied to third_paarty/mkl/include.
However, now we use "includes" field of the cc_library rule "intel_binary_blob", which adds the include folder for mkl to the gcc commands as a "-iquote" flag. Then all these libraries become available to include without the need for a relative path.
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.
Don't do that. This will cause various issues. Always use full include path in tensorflow code please.
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.
Could you elaborate on 'various issues'?
It worked as is before, because of all the non-standard things that were done in configure. This is what we do for any other external dependency headers, libjpeg, libpng, etc. The path here was just a hand fabricated path due to the way things were copied in
confiure
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.
In general tensorflow always use full include path for C++/ObjC programs if allowed. That's the convention.
If you have no other options, there's a couple of things you can do
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.
That convention is for internal includes. Anything that is a part of tensorflow. If something is an external dependency, we point our build rules to their include folder:
https://github.com/tensorflow/tensorflow/blob/master/third_party/png.BUILD#L31
Then include them directly in our files. This is what I am doing here. Getting rid of brittle downloading through configure script mechanism end up moving us to here.
Unless we check in the headers into our third_party/mkl folder, we cannot make them accessible through the
third_party/mkl/include
path.Moving everything into a single header is something I had in mind, but that will come in a later PR.
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.
Gotcha.