-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 support for BLAS and LAPACK dependencies #10921
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #10921 +/- ##
==========================================
- Coverage 70.87% 64.79% -6.08%
==========================================
Files 218 416 +198
Lines 48356 90350 +41994
Branches 11485 20721 +9236
==========================================
+ Hits 34271 58541 +24270
- Misses 11568 27350 +15782
- Partials 2517 4459 +1942
☔ View full report in Codecov by Sentry. |
This pull request introduces 2 alerts when merging 5c2bb31 into 7912901 - view on LGTM.com new alerts:
|
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.
Thanks for looking at this! Miscellaneous hodgepodge of review remarks that I found when taking a hurried glance (it's the holiday season right now):
Thanks for the quick feedback @eli-schwartz. Enjoy your holiday season! |
The |
642efd8
to
e8aeae1
Compare
As far as I could tell, adding keywords or methods/attributes to a dependency object is not allowed by design, and I should be using From that follows the current draft API: openblas_dep = dependency('openblas',
version : '>=0.3.21',
language: 'c', # can be c/cpp/fortran
modules: [
'interface: ilp64', # can be lp64 or ilp64 (or auto?)
'symbol-suffix: 64_', # check/auto-detect? default to 64_ or no suffix?
'cblas',
]
)
# Query properties as needed:
has_cblas = openblas.get_variable('cblas')
is_ilp64 = openblas_dep.get_variable('interface') == 'ilp64'
blas_symbol_suffix = openblas_dep.get_variable('symbol-suffix') It'd be great to get some feedback on this. I'll probably implement Netlib and MKL as well in this PR, because those are both quite different from OpenBLAS. So if the API works for all three, then that's a good sign. |
I would say returned object methods definitely should be avoided, and really in nearly all cases Adding keywords to meson/mesonbuild/interpreter/interpreter.py Lines 252 to 272 in b8e53ed
Many of these are even documented. 🤣 The rest are carefully disclaimed in the docs with:
I think my personal opinion on the matter is I'd like it if we could avoid new kwargs, but if it would be painful or gross without them, then we might as well add them. |
Thanks for the feedback!
I think we can. If we compare, e.g., I will continue down the current path then. Just a heads up that I won't have much time to work on this for the next 10 days or so. So I'll probably continue to enhance this PR after that, and then split off pieces that are ready into more digestible new PRs. |
484833b
to
a01072d
Compare
Will NumPy need 64-bit ILP64 OpenBLAS support for |
@tylerjereddy yes, but this one will be quick compared to dealing with the SIMD stuff. I've been too swamped with other topics, but I should revisit this in the near future. We'll probably miss the 1.25.0 release window and will have to do a 1.25.1/2 to complete the switchover, just like for SciPy (IIRC 1.9.3 was the first complete release without |
2af573e
to
f6ca5f0
Compare
f6ca5f0
to
dab7a30
Compare
Hi all, an update on the status of this: this is pretty much done, and actually already shipped in the Meson that is vendored in NumPy, in the numpy The docs in this PR contain the API that Meson will expose, most of which was already pre-discussed higher up and in gh-2835. A review of that API now would be quite useful though. |
@rgommers I haven't looked at the implementation, but the the documentation needs some finishing touches: there are several |
Sorry, I meant "please look at |
Can you design this in a way that supports specifying which blas, cblas, lapack, lapacke to use via pkg-config module names? In pkgsrc, we rely on custom pkg-config module names to install differing builds of openblas into the same prefix. You have differing libray names and include directories. The CMake FindBLAS and FindLAPACK (missing FindCBLAS and FindLAPACKE yet, AFAIK) support that via I'd love the same for meson, for all components. Right now we're scratching heads over the scipy build which uses the vendored meson. When using the netlib implementation, which comes as 4 libraries with accompanying pkg-config file, it works for numpy buidl to tell it that But SciPy seems to need BLAS API directly, specifically for its vendored superlu, where a build with I just noticed that Can we have separate choice for CBLAS and LAPACKE pkg-config module names? The application would have the differing netlib library names and most often all 4 settings being identical ( |
Yes for the main part, as commented on in #2835 (comment). For separate CBLAS/LAPACKE, see below.
It's already the first thing tried in Meson, so nothing else should be needed.
I'll comment on this elsewhere (your numpy-discussion post probably) since it's SciPy-specific. Just to say here that SciPy, as of now, does not use the code in this PR or a vendored Meson (NumPy does). As a result, it's less flexible. This PR should solve SciPy's issues.
I'd much prefer not to unless it's absolutely necessary - and I've never come across this need before. There is only one known BLAS implementation AFAIK (namely Netlib) which ships CBLAS symbols as a separate library. And in all cases (including Netlib) a package needs BLAS and CBLAS pointing to the same implementation ( If you have a case that is not covered here (it sounds like |
Could you, for the sake of compleness here, mention how meson does implement CMake's Can I tell it, generically, to use myfunkyblas.pc when it encounters a desired dependency named 'blas'? I assumed the |
The description of that is: "If set, the pkg-config method will look for this module name instead of just
You can't tell it generically to do that indeed. And that's by design, because of: https://mesonbuild.com/Contributing.html#environment-variables |
Thanks for clarifying. So what I imagined is some kind of default cli by meson along
-Dmeson-rename-dep=foo=bar
acting on a dependency named foo in meson.build.
|
There is no such thing. Looking at this again, the closest things to this are a few dependency-specific env vars that do exist (Boost has 2, CUDA 1: https://mesonbuild.com/Dependencies.html#boost), or CLI flags specific to a module like the I guess the thing is that in this PR I'm implementing several new dependencies for BLAS libraries plus a uniform API for treating things like asking for CBLAS or ILP64 - but not a generic "give me any BLAS lib you know about" (yet, at least). I'm not sure if that would be appropriate later on, because there's some things that are a bit opinionated in the NumPy/SciPy way of doing things. It's significantly more flexible than https://cmake.org/cmake/help/latest/module/FindBLAS.html (e.g., setting the search order, looking for a selected set of options, handling MKL threading options, etc.). There are multiple options here I think:
Right now we're at (4). I am reasonably sure that (2) is a no-go, given Meson's preference for no env vars and the number of knobs that are possible (NumPy has 8 CLI flags now: https://github.com/numpy/numpy/blob/main/meson_options.txt). (3) or (1) could be done. For now I'd like to finish this with (4), and then see if it makes sense to go to (3) to share code between NumPy and SciPy, since those are close in behavior. |
Just free thought .. why at all meson needs such extension if both librariesa are just libraries. IMO |
It seems that the effort on upstreaming this has stalled. I have the impression that one of the issues is that the blas = import('blas').find_dependency(...) This has the advantage that the |
Another advantage is that this could easily expose command line options like the ones exposed by the |
Yeah, that's mostly because I've been quite short on time, after I was happy with the maturity of the code. There are some problems though with integration, primarily that it is impossible to make Mypy happy with the way multiple inheritance is used in the current state of this PR. And I've been dreading a large refactor a bit.
Yes, the
Hmm, that's an interesting idea. It would make the API nicer, avoid the caching problem, and probably also make it easier to add an @eli-schwartz WDYT? |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
@kloczek your input really isn't helpful or to the point. |
This comment was marked as off-topic.
This comment was marked as off-topic.
meson/mesonbuild/dependencies/detect.py Lines 41 to 67 in b8cdd06
|
I offer you fair warning: your input is not going to be considered for this PR, so anything you say is just talking into the void. |
Regarding symbol suffixes for ILP64 … Reference-LAPACK/lapack#666 got marked as resolved now. I wonder if we are approaching a world where BLAS library symbols are standardized and we could just drop in a library (or two libraries that can be trivially linked together) that offers both 32 bit and 64 bit indices. As a packager/admin, I just want to stress that whatever magic is implemented in the dependency code of the build system, I need some switch to override the magic and make it just follow a central decision to use THIS BLAS (equivalent to the undesired traditional environment variables like BLAS_LIBS). That's why I added the override to tell CMake to use a specific pkg-config module. I must admit hat my head is at some distance from the meson/numpy/etc building business right now, but I'll have to work on refreshing a HPC cluster install soon and hope that things are at least not harder than last time. Many users do use pip, conda, or whatever, where this is not my problem, but I'd like to continue to offer central install with a predictable config. |
I'd love to see us all get there, but it won't happen any time soon I'm afraid. OpenBLAS will need a lot of work that isn't planned. There's some CMake-only support in Netlib BLAS/LAPACK now I believe, but I don't know if any distros are trying to build like that already.
Definitely, that's a hard requirement. This PR doesn't even add a 'magic auto-detect' option (yet), and while NumPy and SciPy will have that, the current API for selecting an exact library won't change (e.g., |
Well, pkgsrc is, and perhaps others, as it's just the default on the CMake build. Excerpt from their CMakeLists.txt:
This results in
Both 32 bit and 64 bit build (BUILD_INDEX64) contain the added
My only gripe there is the implicit handling of cblas. It works now with our openblas builds including cblas (pkgsrc used to have separate builds of cblas on top of optimized blas) and only netlib having it as separate lib. But it's a funky special case that the 'magic' guesses that it will need to link to libcblas when I tell it that
This might be moot of if we just start using the default build of netlib BLAS with CMake (and possibly with plain Makefile build, too, if someone ports that) now, as indicated above. Major packages making use of the 32 and 64 bit symbols as standardized is a strong case for OpenBLAS and others to add them to their builds. After all, it's not Rocket Science, it's just Computer Science (or Algebra, whatever seems more trivial to you;-) The needed logic for Meson then just boils down again to just requiring a BLAS and presuming or late checking that it either provides 64 bit symbols (numpy etc. enabling big indices then) or not. Meson ocating a named pkg-config module is just fine, again. Then we can talk about me still building libcblas separately and if that should always be wrapped into one library with libblas, like it is for OpenBLAS, normally. |
|
||
def get_variable(self, **kwargs: T.Dict[str, T.Any]) -> str: | ||
# TODO: what's going on with `get_variable`? Need to pick from | ||
# cmake/pkgconfig/internal/..., but not system? |
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.
Note to self: this should be fixed in 1.6.0, see https://mesonbuild.com/Release-notes-for-1-6-0.html#support-for-variable-in-system-dependencies.
This is a start on implementing the BLAS and LAPACK support discussed in gh-2835.
This PR in its current form passes the added test case, and
dependency('openblas')
works for me now with SciPy in 3 cases beyondpkg-config
:lib/
andinclude/
dirs, but no pkg-config nor CMake installedThis is already nice to have in its current form, however there's a lot more to do. This includes dealing with 32 vs. 64 bit interfaces, supporting CBLAS and (maybe) LAPACKE, and implementing support for other implementations (MKL, Netlib, ATLAS, etc.). The docs I added try to sketch this out - I'm very much not sure about the API to expose here, some feedback would be great.
This is also the first time I'm implementing a new dependency, so I may have taken a few wrong turns. If there's better ways of doing any of this, I'd love to hear them.