-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Add auto-detection of Intel MKL and OpenBLAS #1316
Conversation
6c0e8b0
to
df07bd4
Compare
Thanks @ischoegl I think this is an interesting proposal. I'm concerned that turning these things on by default will cause more issues like Cantera/conda-recipes#29, especially since we also default to adding the conda directories to the path. What do you think? |
@bryanwweber … packaging is an interesting aspect here, as local installations were my primary concern. For the latter, I don’t see any potential issues, whereas for the former things are certainly more interesting. What do you mean by ‘especially since we also default to adding the conda directories to the path’? Having conda paths included should be desirable, no? |
The problem I can foresee is that you most likely don't want to Similarly, if someone has the conda directories added into their linker path, but installs into the system directories, MKL might not be found. This should be prevented by the fact that setting |
I believe most users would want to install into the same environment, see environment.yaml discussion. I believe there are a couple of edge cases, which need to be identified and where auto detection may need to be disabled.
I think that if the conda layout is used, MKL is certainly desirable, but the same is true for other layouts? Linking against dependencies that are not available in a different context is imho user error, where the same arguments apply to various ‘system’ libraries, e.g. yaml-cpp, etc.? We likewise check for existence and provide fallback options only if nothing is found? |
@bryanwweber ... I ended up adding an option that allows for control of the detection of 'optimized' BLAS/LAPACK. In addition, I ran some tests with some odd results for MKL when running speed tests for ignition
These results indicate that the 'optimized' libraries slow things down. Further, it looks like the inclusion of Python calls (which should slow things down) is advantageous for MKL. PS: There's likely overhead involved that may require fine-tuning beyond just enabling/disabling some libraries. |
@speth ... in light of the recent UG posting about speed differences, I am curious about your opinion on the slow-downs I observed when testing changes this PR is proposing. While the mechanism is small, I'd appreciate any clues? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as implemented now, with the default being to just use Eigen, this is a reasonable addition.
Regarding the performance differences you noted, I'm not really sure what's behind them. For systems as small as GRI 3.0, the linear algebra really should not be taking that much of the compute time. The place where MKL and OpenBLAS can really beat Eigen is more likely to be in larger systems where multithreading is actually helpful.
7b9ea5b
to
f06dd93
Compare
Thanks for your input on this, @speth! I updated a couple of things, most notably a new Outside of packaging, I personally don’t see major issues where auto configuration could go awry, but I’m happy to leave things ‘off’. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this just needs a small fix to the documentation. Otherwise, this looks good to me.
@speth … thanks! Before I fix the docstring, I believe that at this point the |
🤷 I'm willing to give it a try. |
7fea1f0
to
17b8137
Compare
219c01f
to
5cb2550
Compare
While updating the docstrings to use 'automatic' configuration of BLAS/LAPACK by default, I realized that
|
9bebf78
to
2a11eb4
Compare
2a11eb4
to
ce409ab
Compare
The new docstring looks good to me. What's behind the change to disable the Fortran interface on the OneAPI builders? Can you update the commit message to explain why that was needed? |
Unit tests fail for optimized LAPACK libraries (see Cantera#1393)
ce409ab
to
9da9b15
Compare
Done - I created issue #1393 to document the reason for this change: the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, looks good to me.
Changes proposed in this pull request
The following priority is used:
lapack,blas
if installedIf applicable, fill in the issue number this pull request is fixing
Closes Cantera/enhancements#144
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build
&scons test
) and unit tests address code coverage