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.

Sign up for GitHub

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

Jump to bottom

Make libcudart an optional dependency #173

Merged
merged 29 commits into from
Apr 29, 2024
Merged

Make libcudart an optional dependency #173

merged 29 commits into from
Apr 29, 2024

Conversation

conda-forge-admin
Copy link
Contributor

@conda-forge-admin conda-forge-admin commented Apr 25, 2024 •
edited by jakirkham
Loading

Based on user feedback, this softens the cuda-cudart dependency on CUDA 12. This matches the behavior on CUDA 11 where the cudatoolkit dependency is already softened

Includes a post-link script to explain to users how the enable CUDA support in UCX with specific install commands they can copy-paste

Also adds the error overlink check to test for missing dependencies. As GNU OpenMP is getting picked up and there is not a clear way to disable it without issues ( openucx/ucx#9848 ) (despite OpenMP only being used in benchmarks and tests), this goes ahead and adds GNU OpenMP as a dependency. Hopefully we can revisit in the future

Fix #172

@conda-forge-webservices conda-forge-webservices bot mentioned this pull request Apr 25, 2024
Closed
1 task
@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) and found it was in an excellent condition.

@leofang leofang marked this pull request as draft April 25, 2024 03:39
@leofang leofang changed the title MNT: rerender Do not add cuda-cudart as a dependency Apr 25, 2024
conda-forge-webservices[bot] and others added 2 commits April 25, 2024 03:39
@leofang
Copy link
Member

leofang commented Apr 25, 2024

@conda-forge-admin, please rerender

@github-actions GitHub Actions
Copy link
Contributor

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/ucx-split-feedstock/actions/runs/8826674418.

@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) and found some lint.

Here's what I've got...

For recipe:

  • 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 . from the recipe directory.

@leofang
Copy link
Member

leofang commented Apr 26, 2024

It seems using prelink_message is unprecedented on conda-forge and is causing CI issues
https://github.com/search?q=org%3Aconda-forge+prelink_message&type=code (nothing found)
Let me switch back to use a post-link script...

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

@leofang leofang changed the title Do not add cuda-cudart as a dependency Make cudart an optional dependency Apr 26, 2024
@leofang
Copy link
Member

leofang commented Apr 26, 2024

@conda-forge-admin, please rerender

@github-actions GitHub Actions
Copy link
Contributor

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/ucx-split-feedstock/actions/runs/8841330903.

@jakirkham
Put cuda-version last if users want to tweak it
a26fb36 
Makes it easier to copy-paste and then add `.X` if needed.
@jakirkham
Add executable bit to post-link script
769a774
@jakirkham
Copy post-link script
0422c61
@jakirkham
Test post-link script is installed
568653f
@jakirkham
Quote message path
4baab0e
@jakirkham
Disable OpenMP
50d613c 
This is only used by tests and benchmarks. Disable it so that it doesn't
get linked in leading to missing DSO warnings.
@jakirkham
Enable overlinking checks
e6af696 
Make sure there are no missing library dependencies.
@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

@github-actions GitHub Actions
Copy link
Contributor

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/ucx-split-feedstock/actions/runs/8856288202.

@jakirkham
Require OpenMP
7bcfea2 
Unfortunately it appears disabling OpenMP in builds no longer works
correctly. So go ahead and enable it. At least this way the dependency
is known, tracked, and used.
jakirkham
jakirkham reviewed Apr 27, 2024
View reviewed changes
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Leo! 🙏

Made a few tweaks and noted them below. Hope that is ok. Happy to discuss as needed

recipe/meta.yaml Show resolved Hide resolved
recipe/meta.yaml Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/ucx-post-link.sh Show resolved Hide resolved
@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

@github-actions GitHub Actions
Copy link
Contributor

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/ucx-split-feedstock/actions/runs/8856699986.

@jakirkham jakirkham mentioned this pull request Apr 27, 2024
Closed
@jakirkham jakirkham requested review from leofang and removed request for leofang April 27, 2024 02:36
@jakirkham
Copy link
Member

@leofang @pentschev could you please take a look when you have a chance?

@jakirkham jakirkham changed the title Make cudart an optional dependency Make libcudart an optional dependency Apr 27, 2024
xhochy
xhochy previously requested changes Apr 27, 2024
View reviewed changes
recipe/meta.yaml Show resolved Hide resolved
pentschev
pentschev approved these changes Apr 27, 2024
View reviewed changes
Copy link
Contributor

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

Admittedly, I'm not of much help regarding all the conda options, and others will provide better feedback here. Overall this looks good to me.

leofang
leofang reviewed Apr 29, 2024
View reviewed changes
Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

@jakirkham ppc64le fails for some reason, would you mind taking a look? It seems to be failing when running the ucx_info test, but I am not 100% sure given the CI logs were interrupted around that point.

recipe/install_ucx.sh Show resolved Hide resolved
@xhochy xhochy dismissed their stale review April 29, 2024 15:27

Everything was addressed

jakirkham
jakirkham reviewed Apr 29, 2024
View reviewed changes
recipe/meta.yaml Outdated Show resolved Hide resolved
@leofang @jakirkham
Disable again ucx_info on ppc64le
272433b 
Co-authored-by: jakirkham <jakirkham@gmail.com>
@jakirkham jakirkham added the automerge Merge the PR when CI passes label Apr 29, 2024
@jakirkham
Copy link
Member

Sounds like we are onboard with the changes. Adding automerge. Thanks all! 🙏

@github-actions github-actions bot merged commit 157e34c into conda-forge:main Apr 29, 2024
5 checks passed
@github-actions GitHub Actions
Copy link
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • travis: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@jakirkham jakirkham mentioned this pull request May 2, 2024
Draft
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofang leofang leofang left review comments

@jakirkham jakirkham jakirkham left review comments

@xhochy xhochy xhochy left review comments

@pentschev pentschev pentschev approved these changes

@Akshay-Venkatesh Akshay-Venkatesh Awaiting requested review from Akshay-Venkatesh Akshay-Venkatesh is a code owner

@TomAugspurger TomAugspurger Awaiting requested review from TomAugspurger TomAugspurger is a code owner

@matthiasdiener matthiasdiener Awaiting requested review from matthiasdiener matthiasdiener is a code owner

@quasiben quasiben Awaiting requested review from quasiben quasiben is a code owner

Assignees
No one assigned
Labels
automerge Merge the PR when CI passes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ucx + cupy-core unexpectedly pulls in cuda-cudart
5 participants
@conda-forge-admin @leofang @jakirkham @xhochy @pentschev