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

boost: Convert MPI compile-time to load-time constants #30105

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

eschnett
Copy link
Contributor

No description provided.

@hainest
Copy link
Contributor

hainest commented Apr 16, 2022

@eschnett I am very hesitant to alter the behavior of Boost components like this. You've taken care to make sure that the default behavior is unchanged, but this still adds behavior to spack's version of Boost.MPI that doesn't exist in the source. It also isn't detectable in spack's concretizer, but I'm not sure this package needs yet another variant. :)

I'm just not sure this is a path we should go down, but I'm definitely open to being persuaded otherwise.

@trws @asarkar-parsys @adamjstewart @alalazo @mwkrentel @tgamblin Thoughts?

@adamjstewart
Copy link
Member

I think we should wait to see what the Boost devs think. The issue/PR this patch is based off of hasn't had any feedback since last August. It doesn't feel right adding patches to libraries to add features that don't exist in those libraries.

@eschnett
Copy link
Contributor Author

I mentioned in the PR that the current implementation of Boost.MPI does not conform to the MPI standard. These changes are necessary to make MPItrampoline work with Boost. I don't consider them features but corrections.

I notice that the Boost.MPI developers haven't looked at this PR, nor at several other PRs in the recent past. It seems that the recent Boost.MPI development is only about package management, possibly as a general change to all of Boost. It seems that Boost.MPI is not very active at the moment (possibly currently unmaintained?).

I submitted similar changes to other packages (e.g. PETSc) and they were incorporated much more quickly. If you know someone who is maintaining Boost.MPI and could review this PR please let me know.

@trws
Copy link
Contributor

trws commented Apr 17, 2022

Ok, so the thought is that because the standard allows these values to be implemented as const int variables, and c and c++ don't guarantee constant propagation, and so-forth, this makes boost MPI more portable. Sure, that's true, but I am always hesitant to add patches like this to things in spack, because then they end up needing to be maintained in perpetuity. We do it sometimes to keep something like llvm building on newer compilers, but this is something else. Do you have an MPI that runs afoul of this issue?

@eschnett
Copy link
Contributor Author

@trws Yes, Boost.MPI cannot be built with MPItrampoline because of this issue.

@haampie
Copy link
Member

haampie commented Apr 19, 2022

I would say let's make life easy for Erik to promote the stable ABI for MPI and have it interface with all the MPI's available in Spack. @hainest see also https://youtu.be/g3iGSwhzgj8?t=2881. I think embracing MPITrampoline is a great idea and if Spack wants to push their binary mirror, it could also benefit from linking against MPITrampoline to reduce the number of builds.

@eschnett
Copy link
Contributor Author

The CI pipeline seems stuck. Can you re-trigger it?

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