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

Support for pip (replaces PR#19) #24

Merged
merged 91 commits into from
Jun 17, 2020

Conversation

tilmantroester
Copy link
Contributor

This is essentially PR #19 but branched off master, so that it's easier to stay up-to-date. It also restricts the changes to setup.py and leaves the existing makefiles unchanged (up to some QOL improvements).

Other improvements: MPI and DEBUG can be set to 0 and 1 to disable/enable MPI support and debug flags. For setup.py these are the --no-mpi (MPI=0) and --debug-flags (DEBUG=1).

I also fixed the Fortran examples by putting a logzero constant in utils_module.

Copy link
Member

@williamjameshandley williamjameshandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also work out why the travis is failing for OSX ? We need to get that working before this can be merged.

.travis.yml Show resolved Hide resolved
Makefile_gnu Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
src/polychord/mpi_utils.F90 Outdated Show resolved Hide resolved
src/polychord/utils.F90 Outdated Show resolved Hide resolved
@williamjameshandley
Copy link
Member

The Travis build is failing with

$MACOSX_DEPLOYMENT_TARGET mismatch: now "10.9" but "10.13" during configure

on the osx builds. Does changing the relevant environment variable in the .travis.yml fix this issue?

@tilmantroester
Copy link
Contributor Author

The Travis build is failing with

$MACOSX_DEPLOYMENT_TARGET mismatch: now "10.9" but "10.13" during configure

on the osx builds. Does changing the relevant environment variable in the .travis.yml fix this issue?

Unfortunately not. I tried that in commit b822c82 and the travis log shows $ export MACOSX_DEPLOYMENT_TARGET=10.9 but the error message at the end still complains about MACOSX_DEPLOYMENT_TARGET=10.13.

@williamjameshandley williamjameshandley mentioned this pull request Jul 25, 2019
@tilmantroester
Copy link
Contributor Author

I made an attempt to deal with clang on macOS. As far as I can tell it's working on macOS with clang and gcc, and on linux with gcc and intel compilers. The travis tests aren't running though, for some reason.

@williamjameshandley
Copy link
Member

I agree that this is bizarre, although I'm now no longer convinced it's associated with the random number generator. I completely replaced the random number generator, but there are still memory issues that arise when MPI is turned on for OSX. It's more likely that this is some other underlying memory issue that only arises by chance around the time that the RNG is initialised.

@tilmantroester
Copy link
Contributor Author

It could be due to the fact that mpifort uses the gcc gfortran compiler, while mpicc and mpicxx use the clang compilers on the standard homebrew openmpi installation. The mismatch between gfortran and clang could maybe cause problems. On macOS there isn't an easy way around this however, since convincing homebrew to use non-Apple compilers is a pain.

@williamjameshandley
Copy link
Member

OK, I think this is almost ready to be merged. One issue that @Pablo-Lemos has encountered is that on a MAC, if mpi is not installed, then this fails. One can obviously install by passing --no-mpi to setup.py, but if we want a pip install to work, it would be much better if it could detect that no mpi compilers are present.

@tilmantroester What do you think the best way to implement this is?

@tilmantroester
Copy link
Contributor Author

One option would be to ignore the mpi flag on macOS (we already check the OS anyway) and print a warning. I'll have a look at it.

@williamjameshandley
Copy link
Member

Hi @tilmantroester -- what remains to be done to get this PR over the line?

@tilmantroester
Copy link
Contributor Author

I merged in master, so should be able to merge now. I'd check if travis passes before merging though but travis doesn't seem to run.

@williamjameshandley
Copy link
Member

Hi @tilmantroester. I think I've fixed travis now. We can check that the CI is now working by your pushing a commit which bumps the version number to 1.18.0

Whilst this makes it pip-installable with a pip install ., do you know if there are any additional steps which need to be made before it can be uploaded to pypi? (I'm happy to do these manually for now)

@tilmantroester
Copy link
Contributor Author

@williamjameshandley I haven't uploaded to pypi myself. I think it's just running setup.py with sdist, adding some meta data to setup.py, and then uploading it.
At some point a conda package would be nice. That might even fix the macOS MPI issue. But that can wait a bit.

@williamjameshandley
Copy link
Member

Many thanks for your hard work @tilmantroester. I will refrain from creating a release now as users start to try to use this in practice, but will make a 1.18.0 once I've got confirmation that it's working on a variety of systems

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.

2 participants