Make UCX/UCC required dependencies #184
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
|
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 ( |
dalcinl
commented
Oct 24, 2024
|
So far, I have addressed the build part of it. I have not changed defaults, that is, UCX/UCC are still disabled. |
|
At the very least, I think we need to update the messages for ucx/ucc support that state these packages need to be installed. I would also argue since they are packaged, we should remove the messages for ucx/ucc entirely, since there is nothing unusual there. It is the purview of documentation (i.e. GitHub issues here, I guess). I also lean toward removing all of the changed defaults that disable ucx/ucc, since the main reason was that it would break without them. That is no longer true, but I recognize that possible changes to defaults not tied to a version can be disruptive. My inclination, though, is to let the package's defaults be as default as they can be. But if folks want to tie that to a version update, I'm okay with that as well. But is it possible to keep the defaults without disabling features and then remove the overrides on a version bump? |
|
Note that it looks like ucx/ucc are already not the default, so we should be able to remove those lines from outputSo the defaults are still:
meaning (if I've understood correctly) that none of the ucc/ucx lines have any effect on the defaults once they can be loaded at all. |
|
@minrk I definitely second your comments. As I clarified in my previous comment, this PR so far deals with the build part, all the other stuff is still TODO. Regarding whether we should wait for a version bump, maybe not. Sometimes I worry too much.
Again, this is because this PR has only implemented what's necessary for building. IIRC, UCX is still disabled by default. |
To be clear, I was running the test in an env after removing the mca conf file which disables these (not with this PR), but the latest published build with: mamba install openmpi ucc ucx
rm $PREFIX/etc/openmpi-mca-params.confSo what I pasted are the defaults after removing the parameters we are setting, meaning that the disables we are adding are already having no effect on the defaults once ucc/ucx are available. So even removing those lines won't actually change the defaults. I think that means we can safely remove both the param overrides and the messages for ucc/ucx in this PR, since the defaults will not be changed by their removal. |
Please read the following excerpt from Open MPI's documentation: While users can manually select any of the above transports at run time, Open MPI will select a default transport as follows: So my guess is you performed your testing on a machine without any special network hardware... |
|
Got it, thank you. I assumed the I still support the conda packages having openmpi's default behavior rather than being unique now that there's no longer a reason to exclude ucx, but I am also okay if folks prefer waiting for a version to make that transition. |
If no one speaks on the contrary in the next couple days, let's go for it. However, it would be nice to have a |
|
Yes, and I think we will before too long (conda-forge/staged-recipes#27988). Probably best to tackle one thing at a time, though |
…nda-forge-pinning 2024.11.12.14.24.54
dalcinl
force-pushed
the
update-ucx-ucc
branch
from
f2a399b to
575dead
Compare
|
@minrk PR update as per your comments and requests. Let me now if you spot any mistakes. |
|
@minrk So, what to do now? Remove from draft state, cross fingers, and merge? |
|
Yeah, I think that's fine |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
0(if the version changed)conda-smithy(Use the phrase@conda-forge-admin, please rerenderin a comment in this PR for automated rerendering)closes #181
xref conda-forge/mpich-feedstock#104