Skip to content

Add Openmpi using new libfabric to stackinator options #210

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

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

Conversation

biddisco
Copy link
Contributor

  mpi:
    spec: openmpi
    gpu: cuda

should now use the latest openmpi@5 and new libfabric

a spec such as
  openmpi ^cassini-headers@main +cuda
is invalid since the+cuda variant should be applied to the openmpi spec
and not the cassini-headers one.
This patch makes sure that extra variants are added before dependents, eg
  openmpi +cuda ^cassini-headers@main
@msimberg msimberg self-requested a review February 12, 2025 18:40
@biddisco
Copy link
Contributor Author

biddisco commented Mar 4, 2025

I have improved the syntax somewhat and updated the docs. I think this is now ready for review and I have been testing it quite extensively with some variations on builds/branches/etc.
please considre eth-cscs/alps-cluster-config#23 at the same time as this

openmpi-custom-env:
mpi:
spec: [email protected]=main
xspec: +continuations +internal-pmix fabrics=cma,ofi,xpmem schedulers=slurm +cray-xpmem
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
xspec: +continuations +internal-pmix fabrics=cma,ofi,xpmem schedulers=slurm +cray-xpmem
variants: +continuations +internal-pmix fabrics=cma,ofi,xpmem schedulers=slurm +cray-xpmem

? C.f. https://github.com/eth-cscs/alps-cluster-config/pull/23/files#r1993297476.

Copy link
Member

Choose a reason for hiding this comment

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

xspec is a new keyword @biddisco introduced for this specific use case, not to be confused with the variants keyword used in the cluster configs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't make myself clear (at all; I thought I did in another comment somewhere but I've confused myself with the two PRs):

Can we please call it something other than xspec? I wasn't saying this is a bug. I think xspec is a strange name. It's not a spec, and x is too ambiguous in my opinion. If we want to keep the extra meaning of x, why not something like extra_variants? This is specifying additional variants for the chosen MPI package if I understand correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

The link to the comment on the other PR makes sort of the same point that I wanted to make here, but for another field...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry - I used xspec because in my view a bad decision was made in the original stackinator to use the keyword spec - which isn't a full spec, just a name really, ideally, we would use spec and put everything in there like a real spack spec and not have two fields where one will suffice, but this would be a significant break to api - which is ok for a major version release. stackinator is young and changing it now would be ok IMHO.
However, I'm not tied to the name xspec and wew can use anything,

" I think xspec is a strange name. It's not a spec" - well actually it is supposed to be one, when combined with the other spec field - whcih is the point I was trying to make above

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. Yeah, I agree that if spec stays in the name, it should be a spec. If it's just a "partial" thing, then I like something in the direction of what I suggested, with variants somewhere in the name.

I'm open to a breaking change. I think @bcumming is the best to comment on the original intent and motivation behind the fields, and to judge whether a change in semantics is useful at this point.

@@ -207,7 +271,7 @@ The list of software packages to install is configured in the `spec:` field of a
The `deprecated: ` field controls if Spack should consider versions marked as deprecated, and can be set to `true` or `false` (for considering or not considering deprecated versions, respectively).

The `unify:` field controls the Spack concretiser, and can be set to three values `true`, `false` or `when_possible`.
The
The
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The

?

bcumming and others added 5 commits March 18, 2025 13:21
Co-authored-by: Mikael Simberg <[email protected]>
Co-authored-by: Mikael Simberg <[email protected]>
Co-authored-by: Mikael Simberg <[email protected]>
Co-authored-by: Mikael Simberg <[email protected]>
Co-authored-by: Mikael Simberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants