Fix mac tests with conda

[duncanmmacleod]

Standard information about the request

This is a: bug fix

This change affects: continuous integration

This change changes: continuous integration

This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines

This change will: n/a

Motivation

Fixes #4939

Contents

This PR fixes #4939 by updating the CI/tox configuration for macOS.

Links to any issues or associated PRs

Fixes #4939
Closes #4944

Testing performed

Additional notes

• The author of this pull request confirms they will adhere to the code of conduct

[duncanmmacleod]

@titodalcanton @ahnitz, I think this fixes the test configuration, but exposes a real-world test failure:

E       AttributeError: module 'numpy' has no attribute 'float128'. Did you mean: 'float16'?

I don't think this object is available on arm64. Can/should the calling code be patched to use numpy.longdouble?

[spxiwh]

The lapack pinned version was important ...

[duncanmmacleod]

The lapack pinned version was important ...

Why? It is (I think) precisely the reason why the build is failing, that old version of lapack has not been, and will not be, built for macOS on ARM64.

[spxiwh]

@duncanmmacleod I think lapack stopped shipping the headers after this release, making it very hard to build codes from source that compile against it. There's also this weirdness about liblapacke, which I think also isn't in the new releases.

https://github.com/mikekatz04/BBHx?tab=readme-ov-file#prerequisites (because PyCBC does not use lapack directly, we use it via numpy like most).

I have built a BBHx env on my ARM mac though using this version of lapack ... Though it does cause an issue on the IGWN envs (where lapack, or the lapack headers, are not present), which is a discussion I'd been meaning to have with you at some point.

[spxiwh]

Although I now see that my mac env has lapack=3.9.0 installed, so I'm not sure how that worked/is working.

[ahnitz]

@duncanmmacleod Thanks for looking into this!

A couple of these are python packages that we should be picking up through the requirements. At least

ligo-segments and python-ligo-lw should probably be removed from conda_deps, no?

[duncanmmacleod]

@ahnitz, these two packages are specifically called out in the conda requirements because PyPI only has a source distribution and these projects don't build cleanly with newer Python versions and newer versions of Clang. This means that pip install python-ligo-lw fails.

The conda packages include the necessary patches to build, but that hasn't (yet) been backported into the source distribution.

[ahnitz]

@duncanmmacleod Ok, that's fair enough. In which case, I'd just request a comment explaining this. In principle these lines should be removed once those changes have been included in the upstream code.

[duncanmmacleod]

@ahnitz, I have separated things out so that there is a unique commit that adds these packages to the tox.ini file (with a comment) that can be simply reverted when appropriate.

[duncanmmacleod]

Although I now see that my mac env has lapack=3.9.0 installed, so I'm not sure how that worked/is working.

This is the difference between the packages that provided the headers, and those that provide the libraries. The headers do seem to be a little muddled, it took me a while to find that we need the packages liblapacke to get lapacke.h and openblas to get lapack.h.

[spxiwh]

@titodalcanton @ahnitz, I think this fixes the test configuration, but exposes a real-world test failure:

E       AttributeError: module 'numpy' has no attribute 'float128'. Did you mean: 'float16'?

I don't think this object is available on arm64. Can/should the calling code be patched to use numpy.longdouble?

@duncanmmacleod The failure related to this that I see above is in a test case that is really designed to test "have we broken anything in PyCBC Live". It compares to an old version of PyCBC Live which wouldn't run on a mac (though the test may now run on a mac with this change), so this comparison may not make sense on a mac ... But all other places have changed float128 to numpy.longdouble ... I'm confused how this didn't break before??

So in short, I would be happy with just changing this. If the test still fails, disable it on mac.

[duncanmmacleod]

So in short, I would be happy with just changing this. If the test still fails, disable it on mac.

@spxiwh, I took the liberty of adding that change, and it fixes the tests, so I'm happy.

There are other uses of numpy.float128, e.g.

pycbc/pycbc/waveform/pycbc_phenomC_tmplt.py

Line 21 in 161bcea

from numpy import sqrt, log, float128

, but I am making no attempt to address those.

[spxiwh]

Interesting! I hadn't been aware that module even existed. Something to be removed, I think!

[titodalcanton]

I think this also cleans up a bit the problem mentioned in #4857. Thanks @duncanmmacleod.

[duncanmmacleod]

Is this restriction understood?

[spxiwh]

The reason was discussed here #4208 ... It may no longer apply.

[titodalcanton]

We list the same conda-deps twice now, is it possible to deduplicate that with something like {[base-conda-deps]}?

[duncanmmacleod]

We list the same conda-deps twice now, is it possible to deduplicate that with something like {[base-conda-deps]}?

@titodalcanton I have attempted to clean this up by moving the conda_deps into [base] and creating a new [bbhx] section with the options specific to that extra project.

I am not very familiar with tox and tox.ini syntax, so I could easily have done something bad.

[titodalcanton]

As far as I can tell the tests now work for Mac as well, they pass, and the CI configuration has been simplified.

[spxiwh]

Thanks @duncanmmacleod ... I see no further comments on this, and the tests are passing, so merging now.