-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
mpi metapackage #1501
mpi metapackage #1501
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
copy of blas metapackage. Only mpich for now, while we wait for openmpi.
Also cc @pelson, @jakirkham if this seems like the right way to support mpi via features. blas looks like a pretty similar situation, though I haven't seen any packages that use blas other than the default. My understanding, based on looking at openblas, blas, and numpy/scikit-learn:
|
Great thought here. Here's my understanding. mpi-providing packages should define features (in the build section), but should not track features. You're essentially saying "this package is part of this system of packages" more than you're saying "this package provides this feature" mpi4py and others should install an mpi metapackage that activates whatever feature you want. What is missing from this story is a way to choose which mpi metapackage you want. I am not sure that there is presently a solid way to do that, but there is discussion at https://gist.github.com/mcg1969/20667002c79f702e3c7003c81b5f24c8 |
@msarahan that makes sense to me in principle, but the blas packages don't currently do that. Does that mean that the openblas feedstock be defining |
IMO, yes. This implies one blas implementation per environment (or by extrapolation, one mpi implementation per environment), and also implies that for full support, you'd need builds of something like mpi4py for all available mpi providers. @jakirkham, I think you did the OpenBLAS implementation here. Do you remember your thoughts on including or excluding blas_openblas from your recipe? |
@msarahan I tried adding the mpi_mpich feature to the mpich package for testing, but discovered that you cannot install a package with a feature without also installing a package that tracks that feature. The result being that mpich can't be installed on its own without also installing the mpi metapackage unless mpich itself also tracks the mpi_mpich feature. And if mpich both provides and tracks the mpi_mpich feature, it seems like the mpi metapackage doesn't accomplish much. Though I admit I've never had the best grasp of features. |
That is totally accurate, and features are squirrelly. Until the notion of variants is solidified, I agree that the mpi metapackage does not accomplish much. I think you want the notion of "provides" - and we do too, but it's not here yet. |
Also take a look at the |
recipes/mpi/LICENSE.txt
Outdated
+ | ||
+3. Neither the name of the copyright holder nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission. | ||
+ | ||
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we got a second set of +
by accident.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed. fixed.
copying blas recipe again
What do you think about the following proposal:
Does that make sense? If so, this PR should be ready as-is, and I can open PRs to openmpi and mpich to add the features. The alternative is to follow the blas example exactly (differences in bold):
Neither case changes this metapackage, so if the idea of an mpi metapackage makes sense, I think this package ought to be ready as-is, unless I'm missing something. The next step that I'm not super familiar with is the best way to add MPI flavors to the build matrix, given the conda-smithy tooling. How do we most easily add iteration over feature flavors to feedstocks? |
I answered my last question immediately after asking it: conda-forge.yml has a matrix field. Are the editable fields of conda-forge.yml documented anywhere? |
No. 😞 When it comes to these, @pelson is the expert. |
@minrk I still don't fully understand why we need an mpi metapackage. As the MPI API is not ABI-compatible, every library out there using MPI has to link against a specific MPI implementation, and you cannot switch it at runtime. In your previous comment, you mention: "downstream packages that require mpi but don't care about implementation depend on mpi (e.g. nothing linked at build time), but don't track features." It is very hard to have a package using MPI with nothing linked at build time. It is teoretically possible, you could lookpup the MPI shared library, dlopen and probe some symbols, then distinguish between MPICH and Open MPI, and handle each differently. But to be honest, this would be quite involved, and I doubt anyone will ever write such a messy approach that can easily break because of ABI changes in major releases. If you look at my original MPI recipes in https://bitbucket.org/petsc/conda-recipes, the MPI recipes both define and trach their own MPI features. Everything was working just fine. If you run Maybe the whole point of an mpi metapackage is to have a clean way of |
I'm fine with using features, as well. I think the only benefit of the mpi metapackage is it allows packages to depend on any provider of an mpi runtime by not tracking features, such as a package that's not linked to mpi, but will use I'd just like a policy decision for how packages should express which mpi they use, then we can get started on the build matrix for these things. |
Could it use the variant name for the version?
In this case, the recipe wouldn't need to manually set the One great advantage of using version is that it would not be possible to install two different MPI flavors at the same time. Also, making packages differ only by build string seems weird. |
Also, about the run requirement, sometimes MPI is installed in the system, so could this metapackage not depend on an MPI implementation package? |
@tadeu I'm not sure about abusing the version to select the backend MPI implementation. Your example should just be |
But in this case, what would happen with the following command? Also, I'm thinking about the case when the user already has MPI installed in the system. For example, on Windows, the "correct" way seems to be the user to install the Microsoft MPI packages in the system, totally outside of conda, so the user would only need to choose the correct metapackage version. |
@tadeu On windows, users should just |
@dalcinl AFAIK, The problem on Windows is that the user could have |
Picking this up again: I'd like to propose the following, based on @dalcinl's points:
So the mpich package looks like this: features:
- mpi_{{name}}
track_features:
- mpi_{{name}} and mpi4py would look like this: {% set mpi = os.environ.get('MPI_VARIANT', 'mpich') %}
track_features:
- mpi_{{mpi}}
requires:
build:
- {{mpi}}
run:
- {{mpi}} Another option is to follow @dalcinl's example exactly, and:
for example: {% set mpi = os.environ.get('MPI_VARIANT', 'mpich') %}
features:
- mpi_{{mpi}}
requires:
build:
- {{mpi}}
run:
- {{mpi}} (which is the same as the above, but declaring instead of tracking the The same thing should work for msmpi, however it turns out msmpi packages should be defined. |
@isuruf can you take a look at this one? |
by recommendation of conda-forge meeting
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/mpi:
|
it’s a metapackage, there isn’t anything to test
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
From conda-forge meeting discussion, the proposal was changed to: basically the same as before, but use build strings directly instead of setting any features:
BTW, this proposal means that downstream packages can begin adopting mpi variants right away, since there aren't features to wait for in the upstream packages. Only the mutual-exclusivity provided by the metapackage is something to wait for, but that's independent of changes to the packages. |
I've updated the description of this PR with the todo items after merge. I think this is good to go now. |
@minrk Is there anything left? This PR has been sitting here for more than a month, what should we do to get it merged? |
I believe there is nothing left to do. It is awaiting approval (and potentially requests for further changes). The description at the top has several TODO items, all of which are to be done after merge of this PR. |
Let's go for this. I guess we dropped the ball here. We can improve things as we go. |
Yay! |
Dear @minrk, I begin looking into maintaining MPI-related packages for our own Conda ecosystem. I see that the current practice is summarized in your #1501 (comment) (thanks for documenting it btw!), but I get confused: what's the advantage of having the metapackage Taking mpi4py → mpich ↘
mpi
mpi4py → openmpi ↗ But it seems |
the mpi metapackage is really only there for mutual exclusivity. Without it, installing mpi4py with mpich and then later installing mpi4py with openmpi would result in all three of mpi4py, openmpi, and mpich installed at the same time, which isn't good because e.g. openmpi and mpich both provide |
Thank you @minrk for the explanation! |
Since more recent conda has the (undocumented, I think) But it should be considered an implementation detail (no package or user should interact directly with the mpi metapackage other than mpi providers themselves). |
Hi, bringing this up again -- could you explain about this commit[1]? I couldn't quite understand why it doesn't break mpi4py: the build string is omitted, but the discussion here suggests a build string with the mpi impl is required. [1] conda-forge/mpi4py-feedstock@25a6265#diff-e178b687b10a71a3348107ae3154e44c |
Putting the mpi provider in the build string accomplished two things:
|
Thanks for the explanation!
…On Mon, Jul 22, 2019, 1:29 AM Min RK ***@***.***> wrote:
Putting the mpi provider in the build string accomplished two things:
1. allow uploading multiple variants. This was made obsolete by
conda-build 3 including a hash of dependencies in the build string, so
variants no longer need explicit custom build strings. The default build
strings work.
2. allow users specifying a variant at install time. This is not
generally needed, since conda install mpich mpi4py accomplishes the
same thing and is clearer and simpler than conda install
mpi4py=*=*mpich*. Putting the provider in the build string is optional
and mainly useful in cases where a nompi variant is desired.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1501?email_source=notifications&email_token=AABBWTHTRFTUYIBIVHWYPLTQAVVU3A5CNFSM4CO2YIIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2PFUCI#issuecomment-513694217>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABBWTCTO3W4UNX37PLO333QAVVU3ANCNFSM4CO2YIIA>
.
|
Current proposal (updated 2017-12-12):
mpi
metapackage, which will have a variant for each mpi provider (only mpich variant in this PR, conda-forge.yml matrix will add openmpi)mpich_0
)mpi 1.0 mpich
(and marking previous builds as broken)TODO after merging this PR:
Sample for mpi provider (not much to do without features):
Sample for mpi consumer:
Original description below:
NOTE: the current state of this discussion suggests that there would be no metapackage, in which case this PR should not be merged.
copy of blas metapackage. Only mpich for now, while we wait for openmpi.
Following the example of #643, I'll add the build matrix on the feedstock (after openmpi is packaged).
Still to decide: MPI default preference (openmpi or mpich)? I really don't have a preference, but currently mpich has fortran working, while openmpi seems to need to disable it on OS X currently,
so that would push my vote toward mpich.
cc @dalcinl for input on defaults. Not sure who else to ping.