-
-
Notifications
You must be signed in to change notification settings - Fork 65
Support CUDA 12 + Windows #292
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,15 +91,23 @@ for %%i in ("%~dp0.") do set "SCRIPT_DIR=%%~fi" | |
| <cuda.version set /p CUDA_VERSION= | ||
| del cuda.version | ||
| if not "%CUDA_VERSION%" == "None" ( | ||
| call "%SCRIPT_DIR%\install_cuda.bat" %CUDA_VERSION% | ||
| if errorlevel 1 ( | ||
| echo Could not install CUDA | ||
| exit 1 | ||
| if "%CUDA_VERSION:~0,2%" == "12" ( | ||
| :: Don't call install_cuda, as we'll get CUDA packages from CF | ||
| 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" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 🙂
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| ) else ( | ||
| call "%SCRIPT_DIR%\install_cuda.bat" %CUDA_VERSION% | ||
| if errorlevel 1 ( | ||
| echo Could not install CUDA | ||
| exit 1 | ||
| ) | ||
| :: We succeeded! Export paths | ||
| set "CUDA_PATH=C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v%CUDA_VERSION%" | ||
| set "PATH=%PATH%;%CUDA_PATH%\bin" | ||
| set "CONDA_OVERRIDE_CUDA=%CUDA_VERSION%" | ||
| ) | ||
| :: We succeeded! Export paths | ||
| set "CUDA_PATH=C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v%CUDA_VERSION%" | ||
| set "PATH=%PATH%;%CUDA_PATH%\bin" | ||
| set "CONDA_OVERRIDE_CUDA=%CUDA_VERSION%" | ||
| ) | ||
| :: /CUDA | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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
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.