-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Adding cuda-compat recipe #21952
Adding cuda-compat recipe #21952
Conversation
Here is the recipe for cuda-compat |
@kkraus14, did you have particular thoughts on how this would be used? Should it be a dependency of |
I imagine this would get used similarly to how people use the From the docs https://docs.nvidia.com/deploy/cuda-compatibility/#forward-compatibility-title:
I don't think we want to encourage anything explicitly depending on this package given the subset of supported GPUs. As a starting point, I'd propose we package this similarly to how the current recipe does, not in the default linker search path, which allows a user to set something like |
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 ( |
Guess the only other question is whether we add a |
I think this is an exception and does not need a
However, we might need an explicit |
recipes/cuda-compat/meta.yaml
Outdated
requirements: | ||
build: | ||
- {{ compiler("c") }} | ||
- {{ compiler("cxx") }} |
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.
#21952 (comment)
We might need CUDA 12 drivers for this package to be useful.
- {{ compiler("cxx") }} | |
- {{ compiler("cxx") }} | |
run_constrained: | |
- __cuda >=12 |
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.
Do we need a more specific pinning for driver version? Or is a lower bound like this sufficient?
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.
We might need a lower and upper bound on __cuda
?
- {{ compiler("cxx") }} | |
- {{ compiler("cxx") }} | |
run_constrained: | |
- __cuda >=12,<13 |
If using Maybe we could split into |
We could add the CUDA version to the package name as we do with |
Interesting would be tempted to use |
Would you ever want a newer That feels like a legitimate and reasonable use case that wouldn't work with the |
Yes, @kkraus14's example is exactly what I had in mind. I also don't really want to bake the version into the package name. I liked Keith's other suggestion of splitting into a |
What about |
That doesn't solve the issue if the main package uses |
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
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.
I missed fixing one instance of TESLA
earlier. Also a versioning question.
recipes/cuda-compat/meta.yaml
Outdated
doc_url: https://docs.nvidia.com/cuda/index.html | ||
|
||
- name: cuda-compat-impl | ||
version: {{ version }} |
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.
Were we planning to have a "dual version" here, for both CTK and driver versions? I'm not seeing both version
(driver) and cuda_version
(CTK version) being used in the impl/non-impl packages. Maybe I'm missing something.
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.
Both versions are being used, cuda-compat
uses cuda_version
here
And here are the artifacts:
/home/conda/staged-recipes/build_artifacts/linux-64/cuda-compat-impl-525.60.13-h59595ed_0.conda \
/home/conda/staged-recipes/build_artifacts/linux-64/cuda-compat-12.0.0-ha770c72_0.conda
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.
Should we rename version
to driver_version
for clarity?
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.
Sure, renamed it in the latest.
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Thanks all! 🙏 Let's get this in. We can follow up on anything else in the feedstock |
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).