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

don't depend on cuda-nvcc; use cuda-nvcc-impl to avoid pulling in GCC #38

Merged
merged 6 commits into from
Jan 30, 2025

Conversation

h-vetinari
Copy link
Member

Fixes #37

@h-vetinari h-vetinari added the automerge Merge the PR when CI passes label Jan 28, 2025
@h-vetinari h-vetinari requested a review from erip as a code owner January 28, 2025 20:29
@conda-forge-admin
Copy link
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/recipe.yaml) and found it was in an excellent condition.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: failed

Thus the PR was not passing and not merged.

@h-vetinari
Copy link
Member Author

Hm, seems like we got some apparently new incompatibility:

 $PREFIX/targets/x86_64-linux/include/generated_cuda_meta.h:754:5: error: 'CUmemcpyAttributes' does not name a type; did you mean 'cudaMemcpyAttributes'?
 │ │     754 |     CUmemcpyAttributes *attrs;
 │ │         |     ^~~~~~~~~~~~~~~~~~
 │ │         |     cudaMemcpyAttributes

@h-vetinari h-vetinari removed the automerge Merge the PR when CI passes label Jan 28, 2025
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Axel! 🙏

Have a couple questions below

recipe/recipe.yaml Show resolved Hide resolved
recipe/recipe.yaml Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member Author

h-vetinari commented Jan 29, 2025

This is ready except for dealing with CUDA 12.8. I'm waiting for a response in triton-lang/triton#5737 (even though that's for the upcoming 3.2, the same principle would apply to 3.1 as well if my proposed backport is deemed OK)

Otherwise we can add some <12.8 constraint to unblock this.

@jakirkham
Copy link
Member

Let's stick to CUDA 12.6 for the moment

As great as it would be to have CUDA 12.8 here, there is some prep work needed

Will bring this up for discussion later today

@h-vetinari
Copy link
Member Author

Let's stick to CUDA 12.6 for the moment

As great as it would be to have CUDA 12.8 here, there is some prep work needed

Will bring this up for discussion later today

I'm not talking about building anything with 12.8. But currently triton does not have a runtime constraint on the CUDA version (and I'd like to keep it that way). However, that means that we end up pulling in cuda-cupti 12.8 into the test environment, and triton doesn't know which ptx format to use and fails.

It's IMO triton's bug to have an == 6 instead of >=6 in the following

if major == 12:
- return 80 + minor
+ if minor < 6:
+ return 80 + minor
+ elif minor == 6:
+ return 85
if major == 11:

This is the change I'd much rather make, but I wanted to give a bit of time for feedback to arrive in triton-lang/triton#5737.

@jakirkham
Copy link
Member

Ok for more context CUDA 12.8 adds 2 new architectures. So am thinking about how we roll that out

In any event will raise an issue to discuss and link it here

@h-vetinari
Copy link
Member Author

h-vetinari commented Jan 29, 2025

Ok for more context CUDA 12.8 adds 2 new architectures.

I don't see how that matters, as long as builds don't ask to build for those architectures. The function I referenced literally says

     def ptx_get_version(cuda_version) -> int:
         '''
         Get the highest PTX version supported by the current CUDA driver.
         '''

Note "highest". My point is that using ptx 85 should be fine regardless of whether the toolchain is 12.6 or 12.8.

@h-vetinari h-vetinari merged commit 8acb8c2 into conda-forge:main Jan 30, 2025
13 checks passed
@h-vetinari h-vetinari deleted the nvcc_impl branch January 30, 2025 21:59
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.

Do not depend on cuda-nvcc
4 participants