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

Don't ask for MKL_THREADING_LAYER=GNU if NumPy is fixed #6589

Merged
merged 3 commits into from Apr 10, 2018

Conversation

abergeron
Copy link
Member

No description provided.

@abergeron
Copy link
Member Author

Should fix #6580

import numpy._mklinit # noqa
return
except ImportError:
pass
try:
import mkl
if '2018' in mkl.get_version_string():
Copy link
Member

Choose a reason for hiding this comment

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

I would update the error message below to mention updating the numpy conda package as the first option, rather than only setting MLK_THREADING, since it would fix the problem as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Could not import 'mkl'. Either install mkl-service with conda or set
MKL_THREADING_LAYER=GNU in your environment for MKL 2018.
Could not import 'mkl'. If you are using conda, update the numpy
packages to the latest build otherwise, set MKL_THREADING_LAYER=GNU in
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure updating numpy is the right solution here, especially if they have the non-mkl version.

Copy link
Member Author

Choose a reason for hiding this comment

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

We only call this function if we attempt to link with MKL. So the non-mkl case is not an issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, if they have the mkl version and update the package to a version with the _mklinit module, then we don't care if they have mkl-service installed or not (at least from this point of view).

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @abergeron answer, so I'll approve this PR. But I'll wait for @lamblin to see this before merging.

@lamblin
Copy link
Member

lamblin commented Apr 10, 2018

OK, I trust you :)

@lamblin lamblin merged commit 684ccca into Theano:master Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants