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

Unify Mac and Linux tests #4925

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

titodalcanton
Copy link
Contributor

This is a new attempt at #4723, essentially just a rebase of that off current master.

@titodalcanton
Copy link
Contributor Author

Currently seeing errors from:

  • mysqliteclient not being installable
  • clang failing to compile code in python-ligo-lw
  • clang failing to compile code in ligo.skymap.

@titodalcanton
Copy link
Contributor Author

titodalcanton commented Nov 14, 2024

Ok, now I see the following issues.

First, on Linux, BBHx fails to build again with this old error:

      g++ -pthread -B /home/runner/work/pycbc/pycbc/.tox/py-docs/compiler_compat -Wno-unused-result -Wsign-compare -DNDEBUG -fwrapv -O2 -Wall -fPIC -O2 -isystem /home/runner/work/pycbc/pycbc/.tox/py-docs/include -fPIC -O2 -isystem /home/runner/work/pycbc/pycbc/.tox/py-docs/include -pthread -B /home/runner/work/pycbc/pycbc/.tox/py-docs/compiler_compat -shared build/temp.linux-x86_64-cpython-310/src/PhenomHM.o build/temp.linux-x86_64-cpython-310/src/phenomhm_cpu.o -lgsl -lgslcblas -lgomp -llapack -llapacke -o build/lib.linux-x86_64-cpython-310/pyPhenomHM_cpu.cpython-310-x86_64-linux-gnu.so
      /home/runner/work/pycbc/pycbc/.tox/py-docs/compiler_compat/ld: cannot find -llapacke: No such file or directory
      collect2: error: ld returned 1 exit status
      error: command '/usr/bin/g++' failed with exit code 1

Apparently the workaround I had put in tox.ini does not work anymore.

Second, lalsuite does not have wheels for macOS 13 and 14 on PyPI, so we cannot use the target macos-latest. @duncanmmacleod, this seems a question for you.

@spxiwh
Copy link
Contributor

spxiwh commented Nov 14, 2024

@titodalcanton The linux failure is only happening in this PR, right? It looks like lapack is removed from tox.ini in the current patch.

@titodalcanton
Copy link
Contributor Author

titodalcanton commented Nov 14, 2024

@spxiwh correct, but the PR is moving the lapack installation from tox.ini to the GitHub actions definition. For some reason, the {envdir}/lib trick does not work anymore, probably because liblapacke.so ends up in a different directory. I should add some prints and figure that out.

@spxiwh
Copy link
Contributor

spxiwh commented Nov 14, 2024

I think it would be fine if you installed it systemwide (using yum/apt-get/oh-wait-mac-doesn't-have-this). I don't think you can use a conda install and then expect tox to find it.

@spxiwh
Copy link
Contributor

spxiwh commented Nov 14, 2024

Also conda install -c conda-forge lapack==3.9.0 I don't think this version of lalapack ships the necessary lapacke (or the headers)

@titodalcanton
Copy link
Contributor Author

Right, the point of the PR (originally #4723) is to unify Linux and Mac, thus removing OS-specific installs. I would expect tox to find LAPACK if I told it explicitly where it is… Which apparently is not working yet.

@duncanmmacleod
Copy link
Member

Second, lalsuite does not have wheels for macOS 13 and 14 on PyPI, so we cannot use the target macos-latest. @duncanmmacleod, this seems a question for you.

LALSuite uploads wheels that should work for macOS 11+ (x86_64) and 12+ (ARM), please see

@titodalcanton are you seeing build failures as a direct result of LALSuite compatibility issues?

@titodalcanton
Copy link
Contributor Author

@duncanmmacleod no, it was my mistake, sorry. Disregard.

@titodalcanton
Copy link
Contributor Author

titodalcanton commented Nov 15, 2024

I made some progress squashing the various build errors. One difficulty I had to realize and work around is the fact that we create a Conda env, but then tox wants to create a new Conda env, and the original env carries a bunch of libraries and header files which things in the new env must be able to find. The only way I found to do this is explicitly set env variables in the YAML.

The limiting factor now is a failure to build mpi4py due to unresolved symbols. I do not understand why that happens, I guess there may be more env vars to set.

@spxiwh
Copy link
Contributor

spxiwh commented Nov 15, 2024

@titodalcanton Given that tox will make it's own conda env, why the desire to move things from tox's conda env to the higher-level one? If tox is finding it hard to find those locations, why not keep them in tox's env (also note there's an additional layer of abstraction when building anything from source, as wheel will also construct a build env).

@titodalcanton
Copy link
Contributor Author

Looks like #4946 might make this easier. Will try again after that is merged.

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.

4 participants