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

add nvidia/cutlass #17303

Merged
merged 3 commits into from
Feb 22, 2022
Merged

add nvidia/cutlass #17303

merged 3 commits into from
Feb 22, 2022

Conversation

h-vetinari
Copy link
Member

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cutlass) and found some lint.

Here's what I've got...

For recipes/cutlass:

  • The recipe must have some tests.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cutlass) and found some lint.

Here's what I've got...

For recipes/cutlass:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@conda-forge-linter
Copy link

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 (recipes/cutlass) and found it was in an excellent condition.

@h-vetinari
Copy link
Member Author

h-vetinari commented Dec 21, 2021

Ah, the tests seem to need a GPU, which the CI doesn't have...

*** Error: Could not detect active GPU device ID [no CUDA-capable device is detected]

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but couldn't find any.
Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@h-vetinari
Copy link
Member Author

I was trying to look for recipes to lint for you, but couldn't find any.
Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@conda-forge/core, there's a bug with the linter here. As of 23186f9, there's definitely a recipe under recipes/cutlass, so something's off.

recipes/cutlass/build.sh Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member Author

So, one thing is that things seem to get installed into lib64 instead of lib, and I don't see where the switch should be in the upstream CMakeLists.txt.

The other thing is that I saw some rpath-manipulation that we might want to patch out (esp the $ORIGIN ones)? Copied here for convenience. @conda-forge/help-c-cpp, any preferences or advice?

if(NOT WIN32)
  # Add common library search paths so executables and libraries can load and run
  # without LD_LIBRARY_PATH being set.
  link_libraries(
    "-Wl,-rpath,'$ORIGIN'"
    "-Wl,-rpath,'$ORIGIN/../lib64'"
    "-Wl,-rpath,'$ORIGIN/../lib'"
    "-Wl,-rpath,'${CUDA_TOOLKIT_ROOT_DIR}/lib64'"
    "-Wl,-rpath,'${CUDA_TOOLKIT_ROOT_DIR}/lib'"
    )
endif()

@conda-forge-linter
Copy link

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 (recipes/cutlass) and found it was in an excellent condition.

recipes/cutlass/build.sh Outdated Show resolved Hide resolved
@h-vetinari h-vetinari mentioned this pull request Dec 21, 2021
9 tasks
@h-vetinari
Copy link
Member Author

OK, this now builds 🥳 (Magic variable turned out to be CMAKE_LIBRARY_OUTPUT_DIRECTORY)

I'm still a bit worried about the following though. Could someone either confirm or deny that we should patch this @conda-forge/cudatoolkit @jaimergp @xhochy?

I saw some rpath-manipulation that we might want to patch out (esp the $ORIGIN ones)? Copied here for convenience.

if(NOT WIN32)
  # Add common library search paths so executables and libraries can load and run
  # without LD_LIBRARY_PATH being set.
  link_libraries(
    "-Wl,-rpath,'$ORIGIN'"
    "-Wl,-rpath,'$ORIGIN/../lib64'"
    "-Wl,-rpath,'$ORIGIN/../lib'"
    "-Wl,-rpath,'${CUDA_TOOLKIT_ROOT_DIR}/lib64'"
    "-Wl,-rpath,'${CUDA_TOOLKIT_ROOT_DIR}/lib'"
    )
endif()

@leofang
Copy link
Member

leofang commented Feb 5, 2022

I thought conda-build would prune unnecessary rpaths using patchelf/lief, so that we don't have to worry?

@h-vetinari
Copy link
Member Author

I thought conda-build would prune unnecessary rpaths using patchelf/lief, so that we don't have to worry?

Ah, that's amazing if correct. I wasn't aware of that.

@h-vetinari
Copy link
Member Author

Well, assuming the rpaths set by the package get cleaned up correctly by patchelf/lief, then this is ready for review @conda-forge/staged-recipes

@h-vetinari h-vetinari marked this pull request as ready for review February 5, 2022 10:48
@h-vetinari h-vetinari changed the title WIP: add nvidia/cutlass add nvidia/cutlass Feb 6, 2022
@h-vetinari
Copy link
Member Author

Sorry, forgot to remove the "WIP: " from the title...

recipes/cutlass/meta.yaml Outdated Show resolved Hide resolved
recipes/cutlass/meta.yaml Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member Author

@kkraus14
I took care of all the points you raised - very happy to see that the previous work-arounds are not necessary anymore. :)

@h-vetinari
Copy link
Member Author

Thanks @kkraus14! Anyone from @conda-forge/staged-recipes wants to have a look as well? 🙃

@leofang
Copy link
Member

leofang commented Feb 12, 2022

Oh! So we hit another compiler bug?!

@h-vetinari
Copy link
Member Author

h-vetinari commented Feb 12, 2022

Oh! So we hit another compiler bug?!

Looks like it! But curiously, only on CUDA 11.1 & 11.2, not on CUDA 11.0 (ignore the current CI failure, this is running into an incompatible GCC-version, because I was too lazy to do all the cuda-version-specific cbc magic; previous runs show that CUDA 11.0 passed with GCC 9).

@h-vetinari
Copy link
Member Author

@isuruf, fb3c9b6 provides a working solution (recipe/cbc is respected). Would you like to review? Here or in a separate PR?

@h-vetinari h-vetinari mentioned this pull request Feb 13, 2022
@h-vetinari
Copy link
Member Author

fb3c9b6 provides a working solution (recipe/cbc is respected). Would you like to review? Here or in a separate PR?

Opened #18024

@h-vetinari
Copy link
Member Author

I tried copying the content of conda-forge/conda-forge-pinning-feedstock#2010 into cbc.yaml, but it didn't work. Would be happy to hear how I can set GCC=10 for CUDA 11.1/11.2, but leave GCC 9 for CUDA 11.0.

@h-vetinari
Copy link
Member Author

Any other difference between staged-recipes and a "normal" feedstock I'm overlooking here? From what I can tell, using parts of the pinning-cbc verbatim should always work (but doesn't here)?

@h-vetinari
Copy link
Member Author

I tried copying the content of conda-forge/conda-forge-pinning-feedstock#2010 into cbc.yaml, but it didn't work. Would be happy to hear how I can set GCC=10 for CUDA 11.1/11.2, but leave GCC 9 for CUDA 11.0.

Any other difference between staged-recipes and a "normal" feedstock I'm overlooking here?

Could it be that it's due to staged-recipes not using conda-smithy, and hence not knowing about CF_CUDA_ENABLED?

In that case, I won't get the recipe to pass here fully (but should be fine on the feedstock... 🤞) --- CUDA 11.0 needs GCC 9, and CUDA 11.1/11.2 (in this recipe) needs GCC 10.

The only other way I see is to try patch the logic for CF_CUDA_ENABLED into build_all.py somehow, if we're OK with the code duplication with conda-smithy... (we're talking about ~10 lines of code, under the assumption that setting the environment variable would be enough)

If I'm right with this summary, this would also mean that staged-recipes in its current state will not be able to reflect conda-forge/conda-forge-pinning-feedstock#2010 once merged.

CC @conda-forge/core (for conda-smithy)

@isuruf
Copy link
Member

isuruf commented Feb 17, 2022

If I'm right with this summary, this would also mean that staged-recipes in its current state will not be able to reflect conda-forge/conda-forge-pinning-feedstock#2010 once merged.

Nope. The version is determined by the config files. For eg: https://github.com/conda-forge/staged-recipes/blob/main/.ci_support/linux64_cuda102.yaml#L7 which we can edit after conda-forge-pinning PR is merged.

@h-vetinari
Copy link
Member Author

Nope. The version is determined by the config files. For eg: https://github.com/conda-forge/staged-recipes/blob/main/.ci_support/linux64_cuda102.yaml#L7 which we can edit after conda-forge-pinning PR is merged.

Ah, makes sense, just that it's hard-coded and can't be overridden by cbc. Well, lucky that conda-forge/conda-forge-pinning-feedstock#2010 seems to be nearing completion, otherwise this one would be stuck 😅

@h-vetinari
Copy link
Member Author

@conda-forge/staged-recipes, this is finally ready! :)

I also rebased for a clean commit history so that this doesn't have to be squash-merged. Still, if necessary, I can break out c4589d0 into a separate PR.

@h-vetinari
Copy link
Member Author

Gentle ping @conda-forge/staged-recipes 🙃

@carterbox carterbox self-assigned this Feb 21, 2022
Co-authored-by: Daniel Ching <carterbox@users.noreply.github.com>
@h-vetinari
Copy link
Member Author

h-vetinari commented Feb 21, 2022

@kerrmudgeon
Just FYI, the packaging for conda-forge is almost there. 🙃

I wanted to ask if you could provide any information on the binary compatibility policies of cutlass? I.e. would a library built against 2.8.0 be able to load 2.9.0 at runtime (assuming matching CUDA version)?

CC @kkraus14 @jakirkham @leofang

@carterbox
Copy link
Member

carterbox commented Feb 22, 2022

I'm going to merge this PR. You can relax the run_export in your new feedstock if needed.

@carterbox carterbox merged commit 8321536 into conda-forge:main Feb 22, 2022
@h-vetinari
Copy link
Member Author

Thanks for review and merge @carterbox!

@h-vetinari h-vetinari deleted the cutlass branch February 22, 2022 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants