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

Use mpi attribute consistently to provide a default MPI implementation #108983

Merged
merged 4 commits into from Jan 23, 2021

Conversation

markuskowa
Copy link
Member

Motivation for this change

All programs that need MPI libraries are using openmpi at the moment. However, some use openmpi directly while recent additions tended to use mpi = openmpi as input parameter. The change in this PR now makes consistent use of mpi as an input parameter for all derivations that use MPI. This allows for an easy, system-wide switch by simply overriding the mpiattribute via overlays, e.g.:

self: super:
{
  mpi = self.mpich;
}

This follows the idea of #83888.

Things done
  • Replaced all inputs openmpi -> mpi
  • added mpi = openmpi; to top-level/all-packages.nix
  • Where mpi ? null for optional building has been used, the option useMpi ? false has been added.
  • Added a comment to the release notes, as the change from the previous point may break private overlays.
  • Added a paragraph to the nixpkgs manual.
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
    Run mpi = openmpi and partially for mpi = mpich.
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

pkgs/applications/science/biology/neuron/default.nix Outdated Show resolved Hide resolved
pkgs/applications/science/biology/raxml/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/boost/generic.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/boost/generic.nix Show resolved Hide resolved
pkgs/tools/misc/hdf5/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 108983 run on x86_64-darwin 1

6 packages marked as broken and skipped:
  • gplates
  • grass
  • pdal
  • python37Packages.pygmt
  • python38Packages.pygmt
  • saga
30 packages failed to build:
  • bicgl
  • ezminc
  • neuron
  • neuron-full
  • neuron-mpi
  • oobicpl
  • openorienteering-mapper
  • python37Packages.arviz
  • python37Packages.cartopy
  • python37Packages.geopandas
  • python37Packages.neuron
  • python37Packages.neuron-mpi
  • python37Packages.osmnx
  • python37Packages.pymc3
  • python37Packages.python-mapnik
  • python37Packages.worldengine
  • python38Packages.arviz
  • python38Packages.cartopy
  • python38Packages.geopandas
  • python38Packages.neuron
  • python38Packages.neuron-mpi
  • python38Packages.osmnx
  • python38Packages.pymc3
  • python38Packages.python-mapnik
  • worldengine-cli (python38Packages.worldengine)
  • python39Packages.cartopy
  • python39Packages.neuron
  • python39Packages.neuron-mpi
  • python39Packages.python-mapnik
  • python39Packages.worldengine
50 packages built:
  • EBTKS
  • bicpl
  • cdo
  • conglomerate
  • eccodes
  • gdal (python38Packages.gdal)
  • gdal_2
  • gmt
  • grib-api
  • inormalize
  • libLAS
  • libminc
  • mapnik
  • mapproxy
  • minc_tools
  • minc_widgets
  • mni_autoreg
  • mpi
  • n3
  • netcdf
  • netcdfcxx4
  • netcdffortran
  • perl530Packages.Tirex
  • perl532Packages.Tirex
  • postgresqlPackages.postgis (postgresql11Packages.postgis)
  • python37Packages.boltztrap2
  • python37Packages.eccodes
  • python37Packages.fiona
  • python37Packages.gdal
  • python37Packages.h5netcdf
  • python37Packages.labelbox
  • python37Packages.netcdf4
  • python37Packages.rasterio
  • python37Packages.wrf-python
  • python38Packages.boltztrap2
  • python38Packages.eccodes
  • python38Packages.fiona
  • python38Packages.h5netcdf
  • python38Packages.labelbox
  • python38Packages.netcdf4
  • python38Packages.rasterio
  • python38Packages.wrf-python
  • python39Packages.boltztrap2
  • python39Packages.eccodes
  • python39Packages.fiona
  • python39Packages.gdal
  • python39Packages.h5netcdf
  • python39Packages.labelbox
  • python39Packages.netcdf4
  • python39Packages.rasterio

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/mixed-python-c-with-python-runtime-dependencies/7679/10

@markuskowa
Copy link
Member Author

CC @matthewbauer @FRidh Any comments or suggestions?

@markuskowa
Copy link
Member Author

Rebased: resolved merge conflicts.

@markuskowa
Copy link
Member Author

Are there any objections to merging?

@markuskowa
Copy link
Member Author

Resolved another merge conflict.

@markuskowa markuskowa force-pushed the dev-mpi branch 2 times, most recently from 5ad336b to 6e3cd37 Compare January 23, 2021 10:43
Use the attribute mpi to provide a system wide default MPI
implementation. The default is openmpi (as before).
This now allows for overriding the MPI implentation by using
the overlay mechanism. Build all packages with mpich instead
of the default openmpi can now be achived like this:
self: super:
 {
   mpi = super.mpich;
 }

All derivations that have been using "mpi ? null" to provide optional
building with MPI have been change in the following way to allow for
optional builds with MPI:
{ ...
, mpi
, useMpi ? false
}
@markuskowa
Copy link
Member Author

@GrahamcOfBorg build quantum-espresso-mpi lammps-mpi

@markuskowa markuskowa merged commit 2f34b4b into NixOS:master Jan 23, 2021
@markuskowa markuskowa deleted the dev-mpi branch January 24, 2021 09:35
@markuskowa
Copy link
Member Author

markuskowa commented Jan 26, 2021

@hmenke This PR was merged into master, so it should not affect nixpkgs-20.09 (note that doing the override in 20.09 will lead to weird results since the mpi attribute is not consistently used there yet.). The rebuild on master should not be problem, since Hydra does it for you. If you choose override mpi with mpich then of course you need to rebuild these packages yourself. Most likely not all packages will build smoothly with mpich but at least now it is possible to easily replace the default (openmpi) implementation.

@hmenke
Copy link
Member

hmenke commented Jan 26, 2021

Sorry for being unclear. I meant that a build that worked on 20.09 broke on unstable, because boost is now no longer compiled with MPI support by default. A simple boost.override { withMpi = true; } fixes that but incurs a lengthy boost rebuild.

@markuskowa
Copy link
Member Author

@hmenke OK, I think I see what you mean. I checked the diffs from this PR. i can not see that MPi was enabled by default before (at least not that i can see in nixpgks). The boost input parameter was set to mpi ? null and not overridden (at least not that I could find).
if one needs a boost build with MPI, it may be a good idea to define a separate attribute: boost-mpi = boost.override { useMpi = true;}. This could be an (upstream) solution the problem.

Btw.: the potential breakage is documented in the release notes, where it is noted that useMpi needs to be used as an override parameter instead of mpi = openmpi to activate MPI for specific packages.

Can you please open an issue and also describe for which package this problem occurs.

@hmenke
Copy link
Member

hmenke commented Jan 26, 2021

This was just one of my own packages not in nixpkgs. Actually I think that I'm wrong and MPI support for boost was disable all along, but I shot myself in the foot with Nix's dynamic scoping because there is a package called mpi in my package set.

Hence I think everything is fine with this PR and there is nothing to worry about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants