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 libcufft recipe #21902

Merged
merged 49 commits into from
Apr 10, 2023
Merged

Add libcufft recipe #21902

merged 49 commits into from
Apr 10, 2023

Conversation

seberg
Copy link
Contributor

@seberg seberg commented Jan 30, 2023

Attempt at cufft, lets see how far this gets, its my first attempt and I am still a bit confused about the whole setup.

Questions:

  • What about the static version for windows? Did I just miss it, or do I need to split it into its own yaml to not create it on windows? (or can a skip be added just for one of the outputs?) Nvm, adding the skip seems pretty straight forward.
  • Was it correct to keep/use the missing_dso_whitelist?

cc @jakirkham, @adibbley
xref gh-21382

EDIT: Arrg, was meant as a draft of course, this probably needs a bit of work still.

@seberg seberg marked this pull request as draft January 30, 2023 13:00
@conda-forge-webservices
Copy link

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 (recipes/cuda-libcufft) and found it was in an excellent condition.

Copy link
Contributor

@adibbley adibbley left a comment

Choose a reason for hiding this comment

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

Here are some missing changes from the 12.0.0 branch

recipes/cuda-libcufft/bld.bat Outdated Show resolved Hide resolved
recipes/cuda-libcufft/build.sh Outdated Show resolved Hide resolved
recipes/cuda-libcufft/meta.yaml Outdated Show resolved Hide resolved
recipes/cuda-libcufft/meta.yaml Outdated Show resolved Hide resolved
recipes/cuda-libcufft/meta.yaml Outdated Show resolved Hide resolved
@seberg
Copy link
Contributor Author

seberg commented Jan 30, 2023

Here are some missing changes from the 12.0.0 branch

Aha, just noticed the windows part of it, but not the rest, thanks! That also answers my question about the whitelist :)

@conda-forge-webservices
Copy link

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 (recipes/cuda-libcufft) and found some lint.

Here's what I've got...

For recipes/cuda-libcufft:

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

Thanks to adibbley

Co-authored-by: adibbley <103537006+adibbley@users.noreply.github.com>
@conda-forge-webservices
Copy link

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 (recipes/cuda-libcufft) and found it was in an excellent condition.

@adibbley
Copy link
Contributor

Please don't rename this to cuda-libcufft, it needs to be libcufft.

@seberg
Copy link
Contributor Author

seberg commented Jan 30, 2023

Please don't rename this to cuda-libcufft, it needs to be libcufft.

Ah, sorry! Do you have a quick idea why the builds are failing with:

  ERROR (cuda-libcufft,targets/x86_64-linux/lib/libcufft.so.11.0.0.21): /lib64/libm.so.6 not found in packages, sysroot(s) nor the missing_dso_whitelist.

I am experimenting with it, but don't really have a clear idea.

Hmmm, I have been trying and chasing things but found nothing besides the use of missing_dso_whitelist for these ones like libdl? Or I am missing something obvious about use of patchelf...

@conda-forge-webservices
Copy link

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 (recipes/libcufft) and found it was in an excellent condition.

@jakirkham jakirkham mentioned this pull request Jan 30, 2023
49 tasks
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Adding cuda-version pinnings.

recipes/libcufft/meta.yaml Show resolved Hide resolved
recipes/libcufft/meta.yaml Outdated Show resolved Hide resolved
recipes/libcufft/meta.yaml Outdated Show resolved Hide resolved
recipes/libcufft/meta.yaml Outdated Show resolved Hide resolved
recipes/libcufft/meta.yaml Outdated Show resolved Hide resolved
recipes/libcufft/meta.yaml Show resolved Hide resolved
@conda-forge-webservices
Copy link

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 (recipes/libcufft) and found it was in an excellent condition.

@seberg seberg marked this pull request as ready for review March 22, 2023 17:18
@bdice
Copy link
Contributor

bdice commented Apr 8, 2023

@seberg I pushed commit f8e8938 with some minor fixes/changes to align with the libcublas recipe. Thank you so much for your help!

@jakirkham @adibbley @leofang I think this is ready to go, so I'll approve it.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Let's resolve open comment threads that are outdated.

I reviewed the recipe contents and upstream package contents carefully to ensure nothing is being missed. I have not yet validated the output package contents are complete but the tests should be covering us well. (However, I ran into an unexpected situation where tests were passing on libcublas despite having a test that I expected to fail. See conda-forge/libcublas-feedstock#10.) I can verify the package contents on Monday. This looks close enough that I feel comfortable approving it.

@jakirkham jakirkham merged commit 8ad4beb into conda-forge:main Apr 10, 2023
@jakirkham
Copy link
Member

Thanks all! 🙏

Let's follow up on anything else in the feedstock 🙂

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

Successfully merging this pull request may close these issues.

6 participants