conda-forge / openmpi-feedstock

A conda-smithy repository for openmpi.
BSD 3-Clause "New" or "Revised" License
9 stars 22 forks source link

ENH ensure external variants have same deps as regular ones for the solver #156

Closed beckermr closed 1 month ago

beckermr commented 1 month ago

Checklist

xref #153

I am hoping this will help with #153 above in that the external builds will be identical expect that they are shells, have an extra feature, and a lower build number. This should help the solver deprioritize them properly. If this works, we'd need to possibly mark some of the older 5.x ones broken.

conda-forge-webservices[bot] commented 1 month ago

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 (recipe) and found it was in an excellent condition.

beckermr commented 1 month ago

@conda-forge-admin rerender

beckermr commented 1 month ago

@conda-forge-admin rerender

github-actions[bot] commented 1 month ago

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/openmpi-feedstock/actions/runs/9293109781.

beckermr commented 1 month ago

@conda-forge/openmpi I propose we merge this one and try it out.

jaimergp commented 1 month ago

It could work but the older leaner versions will still be around so I wonder if the solver will still prefer those?

minrk commented 1 month ago

IIUC, this means that unused dependencies get added to the environment when you install openmpi-external, to ensure that any given external build has all the same dependency conflicts/compatibilities with other packages as the 'real' builds?

I'm okay with this for now if it solves a problem affecting users, but it doesn't quite seem like the right thing to do in the long run. openmpi-external shouldn't really be considered for installation without matching openmpi on the host.

Is there any chance we could come up with a way to add virtual packages to conda itself, so openmpi-external could require e.g. __openmpi=5.0.1?

It seems to me like openmpi-external should really be on a different channel (e.g. conda-forge/label/openmpi-external) to make it truly opt-in instead of deprioritizing by build number, which lets the solver consider it a valid candidate to conflict resolution, which it never is.

jaimergp commented 1 month ago

Is there any chance we could come up with a way to add virtual packages to conda itself, so openmpi-external could require e.g. __openmpi=5.0.1?

There's a virtual plugin hook we can use now, but users would need to install it next to conda in base

It seems to me like openmpi-external should really be on a different channel (e.g. conda-forge/label/openmpi-external) to make it truly opt-in instead of deprioritizing by build number, which lets the solver consider it a valid candidate to conflict resolution, which it never is.

I like this!

beckermr commented 1 month ago

I'm very much against a different channel. The solver should be able to handle this fine and the old conda solver did for a long time.

beckermr commented 1 month ago

Just to expand my comments a bit, the correct solver behavior is to globally minimize the number of track_features in the env. The mamba solver explicitly does not do this. Most of the time, this produces a correct solution. However, in this case it does not.

So, the issue here lies with the solver, not the fact that we've somehow mixed packages on a single channel that should really be on separate channels.

beckermr commented 1 month ago

I'm okay with this for now if it solves a problem affecting users, but it doesn't quite seem like the right thing to do in the long run. openmpi-external shouldn't really be considered for installation without matching openmpi on the host.

Yes. This is a correct solution and would make things easier for folks. We can probably get by with a global conda-forge-conda-plugins package that holds this virtual package, one for mpich, and eventually anything else we need in the future.

I'm happy to start a repo and help build this out. I'll need to read up on how to write plugins for conda. :)

minrk commented 1 month ago

I'm fine with whatever the simplest practical solution is. I generally don't trust relying on priority to enforce something that should never happen implicitly, since it so often results in failures like #153.

I do find the virtual package approach appealing, if it's doable.

👍 to merging this as-is for now and marking all previous 5.0 external builds as broken, since it should help in the short term.

beckermr commented 1 month ago

Sounds good @minrk. I'll take care of marking the other 5 external builds as broken and we'll see if this helps. @jaimergp is also looking into improving things on the mamba side.

jaimergp commented 1 month ago

I'm happy to start a repo and help build this out. I'll need to read up on how to write plugins for conda. :)

I can provide the boilerplate if you want! They also have a template repo somewhere in the conda org.

beckermr commented 1 month ago

Awesome. We should do some investigation on how to detect the openmpi version as well.

beckermr commented 1 month ago

This works

ompi_info --parsable | grep "ompi:version:full:" | cut -d ":" -f 4