Skip to content

Report error when ATEN_THEADING is OMP and USE_OPENMP is turned off. #40146

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.

Sign up for GitHub

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

Jump to bottom
Closed
xuhdev wants to merge 1 commit into from
Closed

Report error when ATEN_THEADING is OMP and USE_OPENMP is turned off. #40146

xuhdev wants to merge 1 commit into from

Conversation

@xuhdev
Copy link
Collaborator

@xuhdev xuhdev commented Jun 17, 2020

Currently, even if USE_OPENMP is turned off, ATEN_THEADING can still use OpenMP. This commit fixes it.

pbelevich
pbelevich reviewed Jun 23, 2020
View reviewed changes
Copy link
Contributor

@pbelevich pbelevich left a comment

Choose a reason for hiding this comment

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

can you please rebase?

@xuhdev xuhdev force-pushed the aten-threading-openmp branch from edcbb13 to f308921 Compare June 23, 2020 17:55
@dr-ci
Copy link

dr-ci bot commented Jun 23, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit f874cf4 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 3 times.

peterjc123
peterjc123 reviewed Jun 23, 2020
View reviewed changes
@xuhdev
Report error when ATEN_THEADING is OMP and USE_OPENMP is turned off.
f874cf4 
Currently, even if USE_OPENMP is turned off, ATEN_THEADING can still use OpenMP. This commit fixes it.
@xuhdev xuhdev force-pushed the aten-threading-openmp branch from f308921 to f874cf4 Compare June 23, 2020 19:29
@xuhdev xuhdev requested review from pbelevich and peterjc123 June 23, 2020 19:29
@xuhdev
Copy link
Collaborator Author

xuhdev commented Jun 24, 2020

@pytorchbot merge this please

@pytorchbot pytorchbot added the merge-this-please Was marked for merge with @pytorchbot merge this please label Jun 24, 2020
facebook-github-bot
facebook-github-bot reviewed Jun 24, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@pbelevich has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 24, 2020

@pbelevich merged this pull request in 3ed96e4.

@pbelevich
Copy link
Contributor

pbelevich commented Jun 25, 2020

@xuhdev I reverted it, because people complained that MacOS builds stopped working with the error message from this diff, I tested it myself and I confirm it:

CMake Warning at cmake/Dependencies.cmake:1022 (message):
  Not compiling with OpenMP.  Suppress this warning with -DUSE_OPENMP=OFF
...
-- Using ATen parallel backend: OMP
CMake Error at caffe2/CMakeLists.txt:26 (message):
  ATen is using OpenMP backend but USE_OPENMP is off.  Please either change
  ATEN_THREADING or turn on USE_OPENMP.

@xuhdev
Copy link
Collaborator Author

xuhdev commented Jun 25, 2020

Would the error be legitimate If ATen is using OpenMP and user intends to turn OpenMP off?

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

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@peterjc123 peterjc123 peterjc123 approved these changes

@orionr orionr Awaiting requested review from orionr

+1 more reviewer

@pbelevich pbelevich pbelevich approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

merge-this-please Was marked for merge with @pytorchbot merge this please Merged open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@xuhdev @facebook-github-bot @pbelevich @peterjc123 @pytorchbot @mruberry