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.

Sign up for GitHub

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

Jump to bottom

document CUDA builds #1226

Merged
merged 8 commits into from
Jan 8, 2021
Merged

document CUDA builds #1226

merged 8 commits into from
Jan 8, 2021

Conversation

jaimergp
Copy link
Member

Will close #901

@jaimergp jaimergp requested a review from a team as a code owner December 20, 2020 20:44
@jaimergp jaimergp mentioned this pull request Dec 20, 2020
Closed
@beckermr
Copy link
Member

@jakirkham @kkraus14 can you double-check the CUDA info in this PR?

ocefpaf
ocefpaf approved these changes Dec 21, 2020
View reviewed changes
Copy link
Member

@ocefpaf ocefpaf left a comment

Choose a reason for hiding this comment

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

@jaimergp I'm approving mostly the text and readability b/c I'm not too familiar with the process used here.

@jakirkham or @kkraus14 can you take a look and merge?

kkraus14
kkraus14 approved these changes Dec 21, 2020
View reviewed changes
Copy link
Contributor

@kkraus14 kkraus14 left a comment

Choose a reason for hiding this comment

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

LGTM, one suggestion

src/maintainer/knowledge_base.rst Outdated Show resolved Hide resolved
jakirkham
jakirkham reviewed Dec 22, 2020
View reviewed changes
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.

Looks good. Thanks Jaime! 😄

Made a few minor suggestions above. Also generally agree with the points others have made here.

jaimergp and others added 2 commits December 22, 2020 12:43
@jaimergp @jakirkham
CLI typo
5fac9dc 
Co-authored-by: jakirkham <jakirkham@gmail.com>
@jaimergp
address feedback
5688e51
@jaimergp
Copy link
Member Author

jaimergp commented Jan 5, 2021

Just noting that this is not ready to merge yet, since some of the workarounds here described wouldn't apply after conda-forge/nvcc-feedstock#58 is approved and merged.

@jaimergp jaimergp mentioned this pull request Jan 7, 2021
Closed
5 tasks
jakirkham
jakirkham reviewed Jan 7, 2021
View reviewed changes
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 again for the work here Jaime! 😄

Included a couple minor suggestions below.

src/maintainer/knowledge_base.rst Outdated Show resolved Hide resolved
src/maintainer/knowledge_base.rst Outdated Show resolved Hide resolved
jaimergp and others added 2 commits January 8, 2021 10:12
@jaimergp @jakirkham
Apply suggestions from code review
154d885 
Co-authored-by: jakirkham <jakirkham@gmail.com>
@jaimergp
Update docs after last PRs
0e75041
jaimergp
jaimergp commented Jan 8, 2021
View reviewed changes
src/maintainer/knowledge_base.rst
Comment on lines +1221 to +1224
build:
...
missing_dso_whitelist:
- "*/nvcuda.dll" # [win]
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any way to add this the global conda-forge settings?

Copy link
Member

Choose a reason for hiding this comment

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

Not that I know of.

@xhochy xhochy mentioned this pull request Jan 8, 2021
Merged
5 tasks
@jakirkham jakirkham requested review from isuruf and beckermr January 8, 2021 18:46
@jakirkham
Copy link
Member

Would be good if people can take another look 🙂

beckermr
beckermr approved these changes Jan 8, 2021
View reviewed changes
Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Sometime in the future it would be good to document what we now think about gcc versions and cuda versions.

cc @jakirkham @kkraus14 @leofang @isuruf

leofang
leofang approved these changes Jan 8, 2021
View reviewed changes
Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

Great work here and elsewhere in the CF territory, @jaimergp! LGTM. I left a question for everyone here, but it need not be addressed in this PR.

src/maintainer/knowledge_base.rst
Comment on lines +1151 to +1154
* ``nvcc``: Nvidia's EULA does not allow the redistribution of compilers and drivers. Instead, we
provide a wrapper package that locates the CUDA installation in the system. The main role of this
package is to set some environment variables (``CUDA_HOME``, ``CUDA_PATH``, ``CFLAGS`` and others),
as well as wrapping the real ``nvcc`` executable to set some extra command line arguments.
Copy link
Member

@leofang leofang Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I understand this PR is for recipe maintainers, but I am wondering if we could mention this somewhere that the nvcc package is not for end users to install and use (as it's just a wrapper over the real compiler, which is missing)? I've seen several bug reports (including my own) in which the users do not have a local CUDA Toolkit installation and mistakenly think installing nvcc would enable them to compile.

@leofang
Copy link
Member

leofang commented Jan 8, 2021

Sometime in the future it would be good to document what we now think about gcc versions and cuda versions.

@beckermr What specifically do you have in mind?

@h-vetinari
Copy link
Member

Sometime in the future it would be good to document what we now think about gcc versions and cuda versions.

@beckermr What specifically do you have in mind?

Likely the interactions/restrictions being discussed in conda-forge/nvcc-feedstock#51, conda-forge/conda-forge-pinning-feedstock#1000, etc...?

@beckermr
Copy link
Member

beckermr commented Jan 8, 2021

Yup @h-vetinari has the right idea. There has been back and forth on this. As more people start to use CUDA builds, there will be more questions etc.

@leofang
Copy link
Member

leofang commented Jan 8, 2021

I see. Indeed it makes sense. Thanks @h-vetinari @beckermr. We should probably address it (including conda-forge/conda-forge-pinning-feedstock#1053) before the CUDA 11.1 migration starts.

@jakirkham jakirkham requested a review from isuruf January 8, 2021 19:31
@isuruf isuruf merged commit fee7b94 into conda-forge:master Jan 8, 2021
@bdice bdice mentioned this pull request Jan 11, 2021
Merged
5 tasks
@leofang leofang mentioned this pull request Jan 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h-vetinari h-vetinari h-vetinari left review comments

@jakirkham jakirkham jakirkham left review comments

@leofang leofang leofang approved these changes

@beckermr beckermr beckermr approved these changes

@kkraus14 kkraus14 kkraus14 approved these changes

@ocefpaf ocefpaf ocefpaf approved these changes

@scopatz scopatz scopatz approved these changes

@isuruf isuruf Awaiting requested review from isuruf

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Documenting GPU builds
9 participants
@jaimergp @beckermr @jakirkham @leofang @h-vetinari @scopatz @ocefpaf @kkraus14 @isuruf