Add support for BLAS and LAPACK dependencies (continued)

[mgorny]

This a rebase / continuation of #10921.

So far just gotten it to work again, will look through the previous review next.

[dcbaker]

Didn't take too close of a look, but just a few modernization things and type annotation things.

[mgorny]

Thanks for your comments. To be honest, I haven't gotten to cleaning up / modernizing the code yet — so far my main focus was getting it to work correctly :-). But pushed the suggested fixes now.

[mgorny]

Okay, I think this is ready for review. FWIU the "linux" test failures are due to the test images not being updated — and the Ubuntu image failure is flakiness of the zig version check.

[jpakkane]

I understand why you did this, but it's conceptually not really correct. An "interface" is not really a module. Could we have some other way of expressing this information that is more explicit about what it does?

[mgorny]

Do you have a specific suggestion? From the discussion on #10921 I got the impression that it's a good enough compromise. Eli was rather opposed to adding new keyword arguments to dependency() over this, and the only clean alternative I can think of is creating a whole new module for this — and I'm not really convinced it's worth the effort. There's also the option of duplicating dependencies and having a ton of openblas-lp64, openblas-ilp64, mkl-lp64-iomp and so on — but I'm not convinced this will actually be cleaner.

[dcbaker]

I was thinking this too. CPS has the concept of "configurations" but I'm not really sure that this is a configuration either. We have a meeting tomorrow, I will bring this up with them and see what they think.

[jpakkane]

In theory you could do something like:

blas_dep = dependency('blas',
  modules: [...],
  configuration: {'interface': 'ilp64'})

But we'd need to properly review that to make sure it works in all (or at least most) use cases.

[jpakkane]

Another big question is whether they provide different pkg-config names. That is, if there is both an openblas and openblas-ilp64 already, then using those names makes sense.

[dnicolodi]

The reason to use fake module arguments to pass additional properties to use for dependency selection is to avoid adding more keyword arguments to the dependency() function. IIUC the main reason for not adding more keyword arguments is that it sucks to have a generic function to allow arguments that apply only to a limited subset of cases.

The possible way out is to allow a Python's kwargs-style signature where the dependency() function forwards additional arguments to the concrete implementation of the dependency finding classes. IIUC kwargs-style arguments are not desired in the Meson language. However, using fake modules strings to pass arbitrary arguments effectively implements kwarg-style arguments circumventing the language limitations. It has all the drawbacks of allowing kwarg-style arguments plus all the drawbacks of having to implement parsing and error reporting in the implementation of the concrete dependency implementation.

If adding kwarg-style arguments to the Meson language is really not going to happen, maybe we should add to the dependency() function a properties arguments that takes a dictionary:

blas_dep = dependency('blas', properties: {'threading': true, 'interface': 'ilp64'})
gtest_dep = dependency('gtest', properties: {'bazel': false, 'main': true})
boost_dep = dependency('boost', modules: ['foo', 'bar'], properties: {'threading': true})

[mgorny]

Yeah, something accepting a dictionary would definitely be preferable for me over a list with custom key-value syntax.

[dcbaker]

I think the example above is mixing "configurations" and "modules"/"components". I would say the gtest "main" example is a module/component, because it's an extra piece added to the core of gtest. boost with threading is a configuration, since you can have single- or multi- threaded implementations of a given module/component. configurations likely need to be a mapping or an array, because you'll likely want to be able to give a tri-state to the configuration like a UserFeature option does, "must have", "can have", "must not have".

dependencies(
  'boost',
  modules : ['foo', 'bar'],
  configurations : {'debug': get_option('debug'), 'threading': true, 'asan': false}
)

I wish I'd gotten around to dealing with CPS stuff sooner, because I think it would help with this particular issue to have all of that work done :/

[dnicolodi]

I would say the gtest "main" example is a module/component, because it's an extra piece added to the core of gtest.

Agreed.

boost with threading is a configuration, since you can have single- or multi- threaded implementations of a given module/component.

I used the name properties instead of configuration (and I like configuration much more) but I didn't use a module for threading, thus I'm not sure I understand this objection.

configurations likely need to be a mapping or an array, because you'll likely want to be able to give a tri-state to the configuration like a UserFeature option does, "must have", "can have", "must not have".

Sure. I didn't specify which values can be associated to configuration keys. I guess for some of them a tri-state makes sense.

[h-vetinari]

I wish I'd gotten around to dealing with CPS stuff sooner, because I think it would help with this particular issue to have all of that work done :/

Would you mind opening an issue that tracks all the the CPS-related stuff? I've looked around in the repo but couldn't find anything. I could open an issue myself, but it's not likely to be of high quality (though of course you could then edit it as you see fit), because I don't know this effort well. Let me know if for whatever reason you'd still like me to do that.

[mgorny]

Open question: Fedora has OpenBLAS threading variants, and no pkg-config files, i.e. they have 9 OpenBLAS libraries — 3 interface variants (LP64, ILP64 without suffix, ILP64 with suffix) times 3 threading variants (sequential, threaded, OpenMP). Do we want to try to support this? It would imply adding threading: arg to OpenBLAS as well, though i would only be meaningful for Fedora probably.

[dcbaker]

boost already has a threading keyword argument.

edit: Which the interpreter does not allow for...

[mgorny]

Do we agree on going with the current design then? What next steps should I take here?

(the CI workflows are failing because they're running on old images)

[mgorny]

Some more thoughts based on further research I've done in the meantime:

1. I don't exactly like how Netlib uses separate blas and lapack targets, while others use lapack component for it. However, I guess it's kinda inevitable if we want to preserve backwards compatibility with dependency('lapack') — though I suppose we might support both, and possibly deprecate the explicit 'lapack' dep.
2. Sometimes blas and lapack targets evaluate to some other impl (e.g. via symlinks). Should we try hard to actually find Netlib libraries, or actual let blas and lapack evaluate to what the symlinks intended? I guess the latter. Perhaps we could add a dedicated netlib-blas if we really wanted to try to force it (though won't be trivial).
3. At least for OpenBLAS (haven't tested others yet), it is possible for ILP64 lookup to find LP64 library as a last resort. I don't think we can really determine if an arbitrary library with unsuffixed symbols is ILP64 or LP64, but I guess we might do something like requiring a library or a symbol suffix for ILP64, i.e. rejecting plain openblas if it has only unsuffixed symbols as likely LP64.
4. I'm wondering if we should provide some "more automatic" threading: openmp option that would try to determine gomp vs. iomp based on OpenMP implementation used by the compiler. But perhaps it's not worth the effort.
5. As I mentioned above, there are OpenBLAS threading variants on Fedora to consider. If we don't implement that, we will unconditionally use the serial variant (which would be rather suboptimal). However, due to inconsistency in installs, we'd probably need a fallback approach (i.e. "give me OpenMP if available, fall back to whatever openblas is").
6. It would probably make sense to cover FlexiBLAS as well, if only to provide consistent handling of ILP64 interface.

[rgommers]

Thanks for pushing on this @mgorny. Here are my thoughts on your ideas above:

I don't exactly like how Netlib uses separate blas and lapack targets, while others use lapack component for it. However, I guess it's kinda inevitable if we want to preserve backwards compatibility with dependency('lapack') — though I suppose we might support both, and possibly deprecate the explicit 'lapack' dep.

The asymmetry is annoying, but it reflects what's out there in the real world so I can't say it bothers me too much. Also, sometimes plain blas and lapack are not actually the Netlib libraries but stubs/wrappers to compile against (oh, as you also point out in your point 2), so also from that perspective I don't think it's possible to move away from plain dependency('blas')/dependency('lapack'). Deprecating that doesn't seem healthy.

2. Sometimes blas and lapack targets evaluate to some other impl (e.g. via symlinks). Should we try hard to actually find Netlib libraries, or actual let blas and lapack evaluate to what the symlinks intended? I guess the latter.

Agreed, the latter. I don't think it's necessary (nor very easy) to try and distinguish between such wrappers and Netlib libraries.

3. At least for OpenBLAS (haven't tested others yet), it is possible for ILP64 lookup to find LP64 library as a last resort. I don't think we can really determine if an arbitrary library with unsuffixed symbols is ILP64 or LP64, but I guess we might do something like requiring a library or a symbol suffix for ILP64, i.e. rejecting plain openblas if it has only unsuffixed symbols as likely LP64.

There are use cases for both. I think if there'd be a "reject a library with un-suffixed symbol names", it would have to be opt-in for backwards compatibility reasons. As well as for some "build from source and use directly" type use cases; it's probably the more common thing to do for C/Fortran end users, since dealing with symbol prefixes/suffixes can be a fair amount of work. Library authors have incentive to do that work, but scientific end users typically do not.

4. I'm wondering if we should provide some "more automatic" threading: openmp option that would try to determine gomp vs. iomp based on OpenMP implementation used by the compiler. But perhaps it's not worth the effort.

Possibly worth it. It's a fairly minor convenience though, and can always be done later if someone wants that. If we see patterns like if cc.get_id() == 'intel-llvm; threading: 'iomp'; else; threading: 'gomp'; endif in the wild, that's probably a sign that this is needed.

That said, I suspect we'd more need a higher-level "do it all automatically" rather than having other MKL/etc.-specific options.

5. As I mentioned above, there are OpenBLAS threading variants on Fedora to consider. If we don't implement that, we will unconditionally use the serial variant (which would be rather suboptimal). However, due to inconsistency in installs, we'd probably need a fallback approach (i.e. "give me OpenMP if available, fall back to whatever openblas is").

What Fedora is doing is a bit questionable, so I'm not sure how much effort to spend to work around that. What Fedora (well, one stubborn packager, not his colleagues) wants you to do is use FlexiBLAS rather than OpenBLAS. So may as well rely on that rather than more SystemDependency magic. We wouldn't have to think about this if they'd just ship the .pc file like everyone else.

6. It would probably make sense to cover FlexiBLAS as well, if only to provide consistent handling of ILP64 interface.

+1 this seems important. I think the only reason it wasn't there is because I started working on all this before we had FlexiBLAS support in SciPy and before FlexiBLAS itself had ILP64 support with symbol suffixes (IIRC that only landed sometime last year).

[mgorny]

before FlexiBLAS itself had ILP64 support with symbol suffixes (IIRC that only landed sometime last year).

I might have tested wrong but from my short experiments, it still doesn't support ILP64 with symbol suffixes.

[rgommers]

I might have tested wrong but from my short experiments, it still doesn't support ILP64 with symbol suffixes.

You are correct, I remembered wrong. The tracking issue is mpimd-csc/flexiblas#12. It was unblocked last year by Reference-LAPACK/lapack#666 being completed, but the implementation is still TODO.

[lucascolley]

great to see this work! What are the prospects for rgommers/pixi-dev-scipystack#41 in light of this? Are we any closer to figuring out why something seems "off" with the conda-forge distributions, rgommers/pixi-dev-scipystack#23 (comment) ?

[rgommers]

That's unrelated to Meson (beyond the fact that having the feature in this PR will help a little), so I would keep that discussion at rgommers/pixi-dev-scipystack#41 to not distract the Meson maintainers with it.

[dcbaker]

Sorry, I keep rewriting the dependency code out from under you on this :/

[mgorny]

No problem, so far the changes were easy to follow.

[mgorny]

Rebased.