nvcc wrapper: Add -ccbin only if not already supplied #54

mbargull · 2020-12-07T17:18:44Z

Checklist

Used a personal fork of the feedstock to propose changes
Bumped the build number (if the version is unchanged)
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.

Closes gh-34
Probably fixes conda-forge/faiss-split-feedstock#17 (comment)
cc @h-vetinari

…da-forge-pinning 2020.12.06.16.40.53

conda-forge-linter · 2020-12-07T17:18:50Z

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.

mbargull · 2020-12-07T17:20:17Z

(NB: If the same is needed on Windows, then I'll leave it for someone else to address. I.e., I don't have time/nerve for Windows CMD hacking.)

jaimergp · 2020-12-07T17:32:26Z

If needed, I'll handle the Windows part once this PR is merged!

h-vetinari · 2020-12-07T17:34:00Z

This is running into the same gcc-vs-nvcc version problem that I had in conda-forge/faiss-split-feedstock#17 resp. conda-forge/faiss-split-feedstock#18:

nvcc 9.2 requires gcc <= 7
nvcc 10.x requires gcc <= 8
nvcc 11.x requires gcc <= 9 [presumably; not yet relevant]

Possible solutions are manual downgrade (won't survive rerender), across-the-bank downgrade to gcc=7 on linux, or tackling conda-forge/conda-forge-pinning-feedstock#1000.

jakirkham

Thanks for working on this Marcell! 😄

Generally this makes sense. CMake has gotten a bit smarter with setting these things. So it needs less hand-holding, which is nice to see 🙂

Made one suggestion below

jakirkham · 2020-12-07T18:12:48Z

recipe/install_nvcc.sh

+for i ; do
+  case \$i in -ccbin)
+    exec "\${CUDA_HOME}/bin/nvcc" "\${@}"
+  esac
+done


Would it make sense to just loop through ${@} and see if -ccbin is there without the exec? Might simplify things a bit 🙂

Would it make sense to just loop through ${@}

This is exactly what for i does ;)

without the exec?

The exec is just an independent improvement: If we have a launcher script, it makes sense to exec the called process so that our shell process can be stopped/replaced.

Interesting would have expected that to look more like for i in "${@}". It might be clearer to write that (even if bash is doing some magic under-the-hood).

(even if bash is doing some magic under-the-hood).

It's standard POSIX shell: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_04_03

Interesting would have expected that to look more like for i in "${@}". It might be clearer to write that

I don't mind either way -- I can add a commit to change that :).

It would make it clearer I think. We already are cating into a script (my fault 😞). So it's already a bit confusing. The less confusing we can make it the better 🙂

kkraus14 · 2020-12-07T18:45:39Z

recipe/meta.yaml

+        #        due to https://github.com/conda/conda-build/issues/3974
+        #        => use hardcoded compiler package names to workaround that.
+        - {{ compiler("c") }}    # [not (linux64 and (cuda_compiler_version or "").startswith(("9.", "10.")))]
+        - gcc_linux-64 7         # [linux64 and (cuda_compiler_version or "").startswith(("9.", "10."))]


Does this have a run_export? If so can we do <= 7 so users can use an older compiler if desired still?

Similarly, it would be good to handle the CUDA 11.0, and 11.1 requirements: #51

Something like:

Suggested change

- gcc_linux-64 7 # [linux64 and (cuda_compiler_version or "").startswith(("9.", "10."))]

- gcc_linux_64 <=7 # [linux64 and (cuda_compiler_version or "").startswith(("9.2", "10.0"))]

- gcc_linux_64 <=8 # [linux64 and (cuda_compiler_version or "").startswith(("10.1", "10.2"))]

- gcc_linux_64 <=9 # [linux64 and (cuda_compiler_version or "").startswith("11.0")]

- gcc_linux_64 <=10 # [linux64 and (cuda_compiler_version or "").startswith("11.1")]

This is just the compiler version used during the build/test. We only have run_exports for the runtimes (libgcc-ng/libstdcxx-ng) but not for the compilers themselves. You should be able to build downstream packages with older compilers either way.
I've used the exact versions because that's how we build packages on conda-forge generally, with a fixed compiler version (7 or 9 recently). I've also not special cased nvcc=10.1|10.2 to have a consistent version with other packages (we hadn't pinned GCC 8 before).

My goal was to do the least necessary for the build and rather wait until someone fixes the mentioned conda-build bug instead of investing time in workarounds.
That said, I think it's best if I split this change off into another PR and just use a temporary workaround in the CI files for the intended change of this PR.

I hadn't looked at the whole recipe until now...
I don't really understand the purpose of the {{ compiler('c') }} dependency in host here. It is not used in any way.
Also the other duplicate deps for sysroot/glibc I dont understand either.

I can't deduce what the intentions of the dependency declarations overall are. They need some cleanup, but without knowing the indented use, I can't help with that.

nvcc uses the host compiler during it's compilation. Depending on the CUDA version, there's compatibilities with the version of the host compiler used that we need to respect.

Additionally, as of CUDA 11, CentOS 6 is no longer supported so we needed a way to force only CentOS 7+.

nvcc uses the host compiler during it's compilation. Depending on the CUDA version, there's compatibilities with the version of the host compiler used that we need to respect.

This recipe only creates wrapper/(de-)activation scripts => the compiler is not used. If it's meant to be declared for downstream packages, something has to be declared in requirements/run.

Additionally, as of CUDA 11, CentOS 6 is no longer supported so we needed a way to force only CentOS 7+.

The duplication of that in host/run/test/requires is what I meant.

This reverts commit 337557b.

This change will be overridden with the next rerender.

mbargull · 2020-12-07T20:55:06Z

If needed, I'll handle the Windows part once this PR is merged!

Thanks @jaimergp, much appreciated!

mbargull · 2020-12-07T20:57:48Z

@conda-forge/nvcc, this is ready for review.

I opted to postpone the unrelated GCC version pinning to a later PR (<-- not opened yet, because I had problems understanding what the desired actions would be, see #54 (comment) ).

recipe/install_nvcc.sh

kkraus14 · 2020-12-08T00:59:39Z

Will let @jakirkham take another look as well before merging

kkraus14 · 2020-12-10T15:50:46Z

Merging as no response from others.

mbargull added 5 commits December 7, 2020 16:49

Fix nvcc wrapper argument passing

51bc9b6

Use exec for nvcc wrapper

1949ad6

Avoid passing on -ccbin if already given

71b3ec9

MNT: Re-rendered with conda-build 3.20.5, conda-smithy 3.8.5, and con…

4ce54ab

…da-forge-pinning 2020.12.06.16.40.53

Bump build number

d230660

mbargull requested review from dillon-cullinan, isuruf, jakirkham, kkraus14, mike-wendt and raydouglass as code owners December 7, 2020 17:18

mbargull mentioned this pull request Dec 7, 2020

Refactor for cmake-based infra in 1.6.4; add py3.9 & cpu-only windows conda-forge/faiss-split-feedstock#17

Merged

5 tasks

jakirkham reviewed Dec 7, 2020

View reviewed changes

Restrict GCC versions for older CUDA

337557b

mbargull force-pushed the conditional-ccbin branch from 3bbc113 to 337557b Compare December 7, 2020 18:40

kkraus14 reviewed Dec 7, 2020

View reviewed changes

mbargull added 3 commits December 7, 2020 21:31

Revert "Restrict GCC versions for older CUDA"

ed06c2e

This reverts commit 337557b.

Beautify wrapper script creation

b5f40fe

Temporarily use GCC 7 for older CUDAs again

95c76f3

This change will be overridden with the next rerender.

kkraus14 reviewed Dec 7, 2020

View reviewed changes

recipe/install_nvcc.sh Show resolved Hide resolved

kkraus14 approved these changes Dec 8, 2020

View reviewed changes

kkraus14 merged commit 91b12e8 into conda-forge:master Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nvcc wrapper: Add -ccbin only if not already supplied #54

nvcc wrapper: Add -ccbin only if not already supplied #54

mbargull commented Dec 7, 2020

conda-forge-linter commented Dec 7, 2020

mbargull commented Dec 7, 2020

jaimergp commented Dec 7, 2020

h-vetinari commented Dec 7, 2020 •

edited

Loading

jakirkham left a comment

jakirkham Dec 7, 2020

mbargull Dec 7, 2020

jakirkham Dec 7, 2020

mbargull Dec 7, 2020

jakirkham Dec 7, 2020

kkraus14 Dec 7, 2020 •

edited

Loading

mbargull Dec 7, 2020

mbargull Dec 7, 2020

kkraus14 Dec 7, 2020

mbargull Dec 7, 2020

mbargull commented Dec 7, 2020

mbargull commented Dec 7, 2020

kkraus14 commented Dec 8, 2020

kkraus14 commented Dec 10, 2020

-        - gcc_linux-64 7         # [linux64 and (cuda_compiler_version or "").startswith(("9.", "10."))]
+        - gcc_linux_64 <=7       # [linux64 and (cuda_compiler_version or "").startswith(("9.2", "10.0"))]
+        - gcc_linux_64 <=8       # [linux64 and (cuda_compiler_version or "").startswith(("10.1", "10.2"))]
+        - gcc_linux_64 <=9       # [linux64 and (cuda_compiler_version or "").startswith("11.0")]
+        - gcc_linux_64 <=10      # [linux64 and (cuda_compiler_version or "").startswith("11.1")]

nvcc wrapper: Add -ccbin only if not already supplied #54

nvcc wrapper: Add -ccbin only if not already supplied #54

Conversation

mbargull commented Dec 7, 2020

conda-forge-linter commented Dec 7, 2020

mbargull commented Dec 7, 2020

jaimergp commented Dec 7, 2020

h-vetinari commented Dec 7, 2020 • edited Loading

jakirkham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkraus14 Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbargull commented Dec 7, 2020

mbargull commented Dec 7, 2020

kkraus14 commented Dec 8, 2020

kkraus14 commented Dec 10, 2020

h-vetinari commented Dec 7, 2020 •

edited

Loading

kkraus14 Dec 7, 2020 •

edited

Loading