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] Setting default value for KMP_BLOCKTIME #48249

Conversation

vishakha-nervana
Copy link
Contributor

To improve overall user experience and performance, this PR sets KMP_BLOCKTIME to 1, if not set already by user.

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Apr 2, 2021
@google-cla google-cla bot added the cla: yes label Apr 2, 2021
@rthadur rthadur requested a review from penpornk April 2, 2021 16:33
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 5, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 5, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Apr 6, 2021
@gbaned gbaned added the comp:mkl MKL related issues label Apr 6, 2021
@copybara-service copybara-service bot merged commit 0802e34 into tensorflow:master Apr 6, 2021
PR Queue automation moved this from Assigned Reviewer to Merged Apr 6, 2021
@penpornk
Copy link
Member

penpornk commented Apr 6, 2021

@alenik01 This PR has changes that could affect --config=mkl_aarch64 build. Could you please help check if things are okay on your end? (I plan to cherry-pick this into TF 2.5.)

@alenik01
Copy link
Contributor

Good morning, @penpornk Many thanks for the heads up. Mmain problem which I encounter with this commit is that it doesn't build even with --config=mkl, because Bazel for some reason fails to find external/llvm_openmp/include/omp.h. The similar situation is also for --config=mkl_aarch64. Did you manage to build the master after merging this commit?

Execution platform: @local_execution_config_platform//:platform
tensorflow/core/common_runtime/threadpool_device.cc:19:10: fatal error: external/llvm_openmp/include/omp.h: No such file or directory
19 | #include "external/llvm_openmp/include/omp.h"
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
Target //tensorflow/tools/pip_package:build_pip_package failed to build

@penpornk
Copy link
Member

Good morning, @alenik01! We decided not to cherry-pick this PR into 2.5 for now.

Did you manage to build the master after merging this commit?

I didn't try building it manually, but the oneDNN CI test for this PR was fine.

@alenik01
Copy link
Contributor

Good afternoon, @penpornk

Could you please help check if things are okay on your end?

I have checked --config=mkl_aarch64 with the current TF master and it works correctly on AArch64 server. The only disadvantage for us is that additional --cxxopt="-DDNNL_AARCH64_USE_ACL=1" has to be added now in the build command, i.e. defined globally for all the build instead of the necessary files only, I will try to find a more elegant solution.

@haitong
Copy link

haitong commented May 4, 2022

we should not merge this PR as tensorflow build failed with --config=mkl. it is surprising that no one complained about it except @alenik01 alenik01

@haitong
Copy link

haitong commented May 4, 2022

For example, tfserving build is failing due to this tensorflow/serving#1958 (comment)

@penpornk
Copy link
Member

penpornk commented May 4, 2022

cc: @TensorFlow-MKL @agramesh1

@agramesh1
Copy link
Contributor

HI @penpornk we can revert this PR, it does not work the way it was intended. We will submit a PR to revert. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:mkl MKL related issues ready to pull PR ready for merge process size:M CL Change Size: Medium
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

7 participants