Skip to content

Re-Render to support CUDA 11.8#10

Closed
DavidMChan wants to merge 7 commits into
conda-forge:mainfrom
DavidMChan:main
Closed

Re-Render to support CUDA 11.8#10
DavidMChan wants to merge 7 commits into
conda-forge:mainfrom
DavidMChan:main

Conversation

@DavidMChan

Copy link
Copy Markdown
Contributor

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.

@DavidMChan DavidMChan requested a review from h-vetinari as a code owner March 18, 2024 17:58
@conda-forge-webservices

Copy link
Copy Markdown

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.

@DavidMChan DavidMChan changed the title WIP: Re-Render to support CUDA 11.8 Re-Render to support CUDA 11.8 Mar 18, 2024
@DavidMChan

Copy link
Copy Markdown
Contributor Author

This should (probably) resolve #9, however needs review.

@DavidMChan DavidMChan mentioned this pull request Mar 18, 2024
@jakirkham

Copy link
Copy Markdown
Member

Thanks David! 🙏

Looks pretty good

Do we want to update the GPU arches here?

# the following are all the x86-relevant gpu arches; for building aarch64-packages, add: 53, 62, 72
ARCHES=(52 60 61 70)
# cuda 11.0 deprecates arches 35, 50
DEPRECATED_IN_11=(35 50)
if [ $(version2int $cuda_compiler_version) -ge $(version2int "11.1") ]; then
# Ampere support for GeForce 30 (sm_86) needs cuda >= 11.1
LATEST_ARCH=86
# ARCHES does not contain LATEST_ARCH; see usage below
ARCHES=( "${ARCHES[@]}" 75 80 )
elif [ $(version2int $cuda_compiler_version) -ge $(version2int "11.0") ]; then
# Ampere support for A100 (sm_80) needs cuda >= 11.0
LATEST_ARCH=80
ARCHES=( "${ARCHES[@]}" 75 )
elif [ $(version2int $cuda_compiler_version) -ge $(version2int "10.0") ]; then
# Turing support (sm_75) needs cuda >= 10.0
LATEST_ARCH=75
ARCHES=( "${DEPRECATED_IN_11[@]}" "${ARCHES[@]}" )
fi

Might be worth dropping the old CUDA versions (pre-11.2) and adding the new ones (11.8 and 12.0)

@DavidMChan

Copy link
Copy Markdown
Contributor Author

Ah, that's a good point - I didn't see this file. Let me take a look.

@jakirkham

Copy link
Copy Markdown
Member

If it is helpful, PyTorch has analogous logic that could be adapted here

@jakirkham

Copy link
Copy Markdown
Member

Another option might be to just move this to script_env in meta.yaml and drop the bash logic. We made similar changes in nvidia-apex, which could be a good reference point

@DavidMChan

DavidMChan commented Apr 8, 2024

Copy link
Copy Markdown
Contributor Author

So I was looking a bit closer at this today, and from what I can tell, we just ignore this code completely in our upstream CMakeLists (https://github.com/CannyLab/tsne-cuda/blob/2dad49713ba451c0953b646a85ac2567b5d1070e/CMakeLists.txt#L61), since we use the variable CUDA_ARCH, and set it based on the detected CUDAToolkit version. I removed this logic completely, and it seems to still work as expected, but I don't know for sure if these variables have other impacts beyond our upstream package.

@jakirkham - thoughts on how to handle this? I think the best thing is just to remove the dead code and handle it upstream, but I'm open to changing the way the upstream works if that's more transparent to conda-forge

@jakirkham

Copy link
Copy Markdown
Member

Sounds reasonable to me. Thanks for investigating David! 🙏

Mainly conda-forge packages set it when upstream doesn't have a constraint (or not one we can easily handle on CI here). So packages sometimes need to trim the matrix a bit to keep things manageable. Though no need to do that here if the defaults from upstream cover a good range of architectures and build ok here

@jakirkham

Copy link
Copy Markdown
Member

@conda-forge-admin , please re-render

@github-actions

github-actions Bot commented May 2, 2024

Copy link
Copy Markdown
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 rerendeing locally.

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

@conda-forge-webservices

Copy link
Copy Markdown

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.

I do have some suggestions for making it better though...

For recipe:

@jakirkham

Copy link
Copy Markdown
Member

@conda-forge-admin , please re-render

conda-forge-webservices[bot] and others added 2 commits May 2, 2024 03:04
@conda-forge-webservices

Copy link
Copy Markdown

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.

@jakirkham

Copy link
Copy Markdown
Member

@conda-forge-admin , please re-render

@h-vetinari h-vetinari mentioned this pull request Aug 9, 2024
@h-vetinari

Copy link
Copy Markdown
Member

@DavidMChan, I'm very sorry that I didn't see this PR! 😑

I rebased it into #12 (without some of the churn-y rerender commits), but I'd very much like to keep you on as a maintainer!

@DavidMChan

Copy link
Copy Markdown
Contributor Author

Thanks! No worries! Hopefully we can get #12 merged -- I've been pretty behind on issues on the source repo too, so it's all good!

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.

3 participants