Skip to content

Support CUDA 12 + Windows#292

Merged
jakirkham merged 1 commit into
conda-forge:mainfrom
leofang:cuda_win
Nov 9, 2023
Merged

Support CUDA 12 + Windows#292
jakirkham merged 1 commit into
conda-forge:mainfrom
leofang:cuda_win

Conversation

@leofang

@leofang leofang commented Nov 6, 2023

Copy link
Copy Markdown
Member

The logic here is to treat CUDA 12+ differently from previous CUDA versions. Specifically, for CUDA 12+ we

  • do not download/extract any CUDA installer
  • do not need to set CUDA_PATH or update PATH
  • but still need to set CONDA_OVERRIDE_CUDA

Tested in conda-forge/cupy-feedstock#228.

cc: @conda-forge/cuda

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.

@leofang leofang requested a review from a team as a code owner November 6, 2023 18:07
@conda-forge-webservices

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

set "CUDA_PATH="
set "CONDA_OVERRIDE_CUDA=%CUDA_VERSION%"
:: Export CONDA_OVERRIDE_CUDA to allow __cuda to be detected on CI systems without GPUs
echo set "CONDA_OVERRIDE_CUDA=%CONDA_OVERRIDE_CUDA%" >> "%CONDA_PREFIX%\etc\conda\activate.d\conda-forge-ci-setup-activate.bat"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need this, too, in the 12.x case? It also looks like it doesn't really do anything as it sets the variable to itself.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought the comment is self-explanatory? Also, this treatment is copied from existing code (see line 133-134 below), I didn't reinvent any wheel 🙂

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought the comment is self-explanatory?

Not fully. Reading the source again, it makes sense. "Bake-in" instead of "Export" would have made it clear to me but probably not to everyone else. Happy to keep it as-is.

@jakirkham jakirkham Nov 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Basically with how CUDA 11 is handled (both on Linux and Windows), most CUDA packages come from the system. There is a bunch of logic here to handle those CUDA 11 cases

With CUDA 12, we now have Conda packages and so don't need to use that logic. Eventually we could drop the CUDA 11 logic from here when we drop CUDA 11. In the interim, we just need to make sure we "do nothing" on CUDA 12

Edit: Though independent of CUDA version, we still have an issue when building on machines that don't have a GPU. Namely we need to convince Conda that it is ok to install CUDA dependent packages there. So we set this variable to tell Conda what CUDA version we support

set "CUDA_PATH="
set "CONDA_OVERRIDE_CUDA=%CUDA_VERSION%"
:: Export CONDA_OVERRIDE_CUDA to allow __cuda to be detected on CI systems without GPUs
echo set "CONDA_OVERRIDE_CUDA=%CONDA_OVERRIDE_CUDA%" >> "%CONDA_PREFIX%\etc\conda\activate.d\conda-forge-ci-setup-activate.bat"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought the comment is self-explanatory?

Not fully. Reading the source again, it makes sense. "Bake-in" instead of "Export" would have made it clear to me but probably not to everyone else. Happy to keep it as-is.

if errorlevel 1 (
echo Could not install CUDA
exit 1
if "%CUDA_VERSION:~0,2%" == "12" (

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

btw I tried to make this line more robust

Suggested change
if "%CUDA_VERSION:~0,2%" == "12" (
for /f "delims=." %%f in ("%CUDA_VERSION%") do (set /a "CUDA_MAJOR_VERSION=%%f")
if %CUDA_MAJOR_VERSION% geq 12 (

but for some reason the CI just hated the change (it worked fine locally), so I didn't use it here... Like John said, in the future we'll drop most of the complex logic here, so I hope this is OK.

@jakirkham

jakirkham commented Nov 8, 2023

Copy link
Copy Markdown
Member

Think we might need to update these lines as well

Edit: Specifically looking at CONDA_OVERRIDE_CUDA, which we would still set

if not "%CUDA_PATH%" == "" (
echo set "CUDA_PATH=%CUDA_PATH%" >> "%CONDA_PREFIX%\etc\conda\activate.d\conda-forge-ci-setup-activate.bat"
echo set "CUDA_HOME=%CUDA_PATH%" >> "%CONDA_PREFIX%\etc\conda\activate.d\conda-forge-ci-setup-activate.bat"
:: Export CONDA_OVERRIDE_CUDA to allow __cuda to be detected on CI systems without GPUs
echo set "CONDA_OVERRIDE_CUDA=%CONDA_OVERRIDE_CUDA%" >> "%CONDA_PREFIX%\etc\conda\activate.d\conda-forge-ci-setup-activate.bat"
)

@leofang

leofang commented Nov 8, 2023

Copy link
Copy Markdown
Member Author

Yes, this block is only reachable by CUDA 11 and below (where CUDA_PATH is set). For CUDA 12+, this block would be skipped. It's admitted an ugly hack, but my lack of cmd knowledge (and failures to make CI happy even with correct code, as noted above) is preventing me from doing a better job here.

@jakirkham

Copy link
Copy Markdown
Member

Ah gotcha. May just be my fault for looking on my phone

@jakirkham

Copy link
Copy Markdown
Member

@conda-forge/core , any other thoughts on this change?

@jakirkham jakirkham merged commit 880fd8d into conda-forge:main Nov 9, 2023
jakirkham referenced this pull request Nov 9, 2023
@jakirkham

Copy link
Copy Markdown
Member

Thanks all! 🙏

Let's give this a try and follow up on anything else after

Note: Missed we didn't have the version number bump. So did that in commit ( 902a2be )

@leofang leofang deleted the cuda_win branch November 10, 2023 01:12
@leofang

leofang commented Nov 10, 2023

Copy link
Copy Markdown
Member Author

Thanks for catching the version bump, John. Sent a backport PR (#293).

@leofang leofang mentioned this pull request Nov 10, 2023
5 tasks
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