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

[INTEL MKL] Fixes for regressions in unit tests. #31955

Merged

Conversation

agramesh1
Copy link
Contributor

@agramesh1 agramesh1 commented Aug 25, 2019

This PR fixes a number of regressions caused by this commit da3f7b1 when MKL DNN is enabled.
pywrap_tensorflow depends on a number of prebuilt shared libraries when MKL DNN is enabled, so importing of _pywrap_utils fails if pywrap_tensorflow has not already been imported. Fixed this by switching the order of imports or adding imports of pywrap_tensorflow when needed.

@tensorflow-bot tensorflow-bot bot added the size:XS CL Change Size: Extra Small label Aug 25, 2019
@gbaned gbaned self-assigned this Aug 26, 2019
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Aug 26, 2019
from tensorflow.python import pywrap_tensorflow
from tensorflow.python import _pywrap_utils
Copy link
Contributor

Choose a reason for hiding this comment

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

The linter will likely complain here; disable it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alextp Thanks for approving the PR. Pylint did not report any warnings in our internal testing for this line and it also passed in the Ubuntu sanity tests. Let me know if I need to update this PR.

from tensorflow.python import pywrap_tensorflow
from tensorflow.python import _pywrap_utils
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 26, 2019
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Aug 26, 2019
@kokoro-team kokoro-team removed kokoro:force-run Tests on submitted change labels Aug 26, 2019
@agramesh1
Copy link
Contributor Author

Hi @gbaned, it looks like the PR is stuck at importing. Can you check the status?

@alextp alextp added the kokoro:force-run Tests on submitted change label Aug 27, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 27, 2019
tensorflow-copybara pushed a commit that referenced this pull request Aug 28, 2019
@tensorflow-copybara tensorflow-copybara merged commit a18d5ee into tensorflow:master Aug 28, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Aug 28, 2019
tensorflow-copybara pushed a commit that referenced this pull request Oct 22, 2019
… swig. This is part of a larger effort to deprecate swig and eventually with modularization break pywrap_tensorflow into smaller components. It will also make exporting C++ ops to Python significantly easier. XLA is using the pybind11 macros already. Please refer to https://github.com/tensorflow/community/blob/master/rfcs/20190208-pybind11.md for more information.

We are adding `toco::` to the exported namespaces for pywrap_tensorflow's shared object. A few downstream modules also require a previous import of pywrap tensorflow, because the wrapper depends on the shared library. See #31955 for additional information.

PiperOrigin-RevId: 276096778
Change-Id: I042f488c36b00818b2344fb39c36cad97cee6eb8
@nammbash nammbash deleted the agramesh/pywrap_order branch June 16, 2020 19:40
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 size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants