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

Add UCC support #175

Merged

Conversation

j34ni
Copy link
Contributor

@j34ni j34ni commented Sep 21, 2024

Summary

  • Added support for UCC (build_with_ucc="--with-ucc=$PREFIX")
  • Added addition ompi_info based tests

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

j34ni and others added 4 commits September 19, 2024 19:04
@conda-forge-webservices
Copy link
Contributor

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 (recipe/meta.yaml) and found some lint.

Here's what I've got...

For recipe/meta.yaml:

  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [66, 84]

 Ensured <two spaces>#<one space>[<expression>]
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint --conda-forge . from the recipe directory.

@conda-forge-webservices
Copy link
Contributor

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/meta.yaml) and found it was in an excellent condition.

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

j34ni and others added 12 commits September 21, 2024 20:27
Tried to fix build issues for OSX (where there is no need for Infiniband related libraries)
Nothing provides `binutils` for the OSX build
ucc does not seem to exist for ppc64le
Trying with `ucc  # [linux and not ppc64le]`
`- ucc  # [linux and not ppc64le and not osx]`
`- ucc  # [linux and not linux-ppc64le and not osx]`
Back to `- ucc  # [linux and not ppc64le and not osx]`
        - ucc  # [linux_64]
        - ucx  # [linux and not osx]
        - ucc  # [linux and not ppc64le and not osx]
        - ucx  # [linux and not osx]
Modified test to check that OpenMPI was configured and build with UCX (rather then that the conda list contains a package called UCX which may or may not be of any use)
Also excludes the osx_64 platform for which it does not exist
@leofang leofang mentioned this pull request Sep 26, 2024
1 task
@leofang
Copy link
Member

leofang commented Sep 26, 2024

@conda-forge-admin, please rerender

Copy link
Contributor

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

I tried to rerender for you but ran into some issues. Please check the output logs of the latest webservices GitHub actions workflow run for errors. You can also ping conda-forge/core for further assistance or you can try rerendering locally.

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

@minrk
Copy link
Member

minrk commented Sep 26, 2024

The main reason ucx was excluded as a hard dependency was to save space because it resulted in hundreds of megabytes of extra packages due to cuda stuff. That's not true anymore since #119, so would it simplify things to make ucc/ucx a plain conda dependency? I don't entirely follow the reasoning behind ucx still being optional when it seems to only save ~20MB. The functionality is still optional, right?

@leofang
Copy link
Member

leofang commented Sep 26, 2024

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 2 commits September 26, 2024 22:49
@leofang
Copy link
Member

leofang commented Sep 26, 2024

No, not your fault at all. The rerendering functionality seems to be down at least today, which affects both offline rerendering and CI services. The core team is investigating: conda-forge/conda-forge-ci-setup-feedstock#350 (comment).

To close the loop, it turns out the rerendering failed for another reason: #177 (comment) which is now fixed.

@leofang leofang changed the title To add PMIx and UCC support Add UCC support Sep 27, 2024
@leofang
Copy link
Member

leofang commented Sep 27, 2024

I don't entirely follow the reasoning behind ucx still being optional when it seems to only save ~20MB. The functionality is still optional, right?

My preference is if something is optional, we do not list them in run. In addition to extra bandwidth and storage usage, adding more dependencies could also run into dependency resolving issues (ex: #153). Right now this PR presents a minimal change to the recipe and I like it that way. Maybe we can move this discussion to a separate issue and try to get this merged once the last question on FCFLAG is resolved and the tests pass?

@minrk
Copy link
Member

minrk commented Sep 27, 2024

Makes sense!

@minrk
Copy link
Member

minrk commented Sep 27, 2024

Thanks, this LGTM now.

@minrk
Copy link
Member

minrk commented Sep 27, 2024

Doesn't need to happen here, but I think we can add actual tests that optional ucx works by including conda install ucc in run_test.sh, rather than purely inspecting the config output:

# first, test without ucx
# ...
conda install ucc ucx
# enable ucx, ucc
mpiexec -n 2 some_test
# verify ucx/ucc was used

@minrk minrk merged commit fe3412d into conda-forge:main Sep 27, 2024
10 checks passed
@minrk
Copy link
Member

minrk commented Sep 27, 2024

Thanks @j34ni and thanks @leofang for reviewing!

@j34ni
Copy link
Contributor Author

j34ni commented Sep 27, 2024 via email

@leofang
Copy link
Member

leofang commented Sep 27, 2024

Thanks @j34ni @minrk @pentschev!

@leofang
Copy link
Member

leofang commented Sep 29, 2024

FYI as a follow-up I'm adding UCC to the global pinning conda-forge/conda-forge-pinning-feedstock#6484

@leofang
Copy link
Member

leofang commented Sep 30, 2024

Maybe we can move this discussion to a separate issue

See #181.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants