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

Avijit/fix license files #21808

Merged

Conversation

avijit-nervana
Copy link
Contributor

@avijit-nervana avijit-nervana commented Aug 22, 2018

We discovered that the license file names are wrong when nGraph build is selected. This PR fixes these file names so that nGraph is built with TensorFlow when requested. This PR also fixes nGraph unit tests due to recent changes in the MKL bazel files.

* Fixed the dependencies with `str(Label(...))` so that third_party codes that refer to
the tensorflow as a submodule get it properly resolved.
@akshaym akshaym requested review from aaroey and tfboyd August 23, 2018 16:49
@tfboyd tfboyd requested review from thirupalanisamy and removed request for tfboyd August 23, 2018 18:58
@@ -396,7 +396,7 @@ def tf_cc_binary(
srcs = srcs + tf_binary_additional_srcs(),
deps = deps + tf_binary_dynamic_kernel_deps(kernels) + if_mkl_ml(
[
"//third_party/mkl:intel_binary_blob",
clean_dep("//third_party/mkl:intel_binary_blob"),
Copy link
Member

Choose a reason for hiding this comment

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

What's the actual difference after applying clean_dep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a C++ unit test (google test based) that depends on TensorFlow C++ as well as core.
Here's the target dependencies defined in third_party/ngraph/ngraph_tf.BUILD:

tf_cc_test(
    name = "ngraph_tf_tests",
    size = "small",
    srcs = [
        "test/tf_exec.cpp",
        "test/main.cpp",
    ],
    deps = [
        ":ngraph_tf",
        "@com_google_googletest//:gtest",
        "@org_tensorflow//tensorflow/cc:cc_ops",
        "@org_tensorflow//tensorflow/cc:client_session",
        "@org_tensorflow//tensorflow/core:tensorflow",
...

One or more of the TF dependencies are defined as tf_cc_binary(...) and the definition of tf_cc_binary(...) includes the conditional dependencies of MKL as shown in line 399 above.
It turns out that without the clean_dep(...) or str(Label(...)) which is equivalent, this dependency is resolved as @ngraph_tf//third_party/mkl which is causing the bazel error such as the following:

ERROR: /tmp/avijitch/.cache/_bazel_avijitch/7ac9ae669f8ee6504932fa4d1b9f916b/external/ngraph_tf/BUILD.bazel:67:1: no such package '@ngraph_tf//third_party/mkl': BUILD file not found on package path and referenced by '@ngraph_tf//:ngraph_tf_tests'

After making this changes, the dependencies are properly expressed and the bazel errors are gone as it's now being correctly resolved as @org_tensorflow//third_party/mkl:BUILD.

Our observation is that the ngraph unit test build started to fail since the MKL bazel files changes went in a couple of weeks ago. Comparing the before and after those changes, we noticed that several bazel files properly used the str(Label(...)) but some didn't (like the ones I added in this PR).

After making the changes proposed in the PR, the ngraph test build and runs.

Please let me know if you need more details or have any questions. Thanks :)

BTW - I used the following bazel query which was very helpful debugging this:
bazel query 'buildfiles(deps(@ngraph_tf//:ngraph_tf_tests))' --keep_going

Copy link
Member

Choose a reason for hiding this comment

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

Thanks so much for the detailed explanation! LGTM.

@aaroey aaroey added the ready to pull PR ready for merge process label Aug 26, 2018
@tensorflow-copybara tensorflow-copybara merged commit b425b0e into tensorflow:master Aug 28, 2018
tensorflow-copybara pushed a commit that referenced this pull request Aug 28, 2018
@avijit-nervana avijit-nervana deleted the avijit/fix-license-files branch September 19, 2018 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants