Skip to content

[cudnn] auto-download only if cuda version matches#16413

Merged
strega-nil merged 8 commits intomicrosoft:masterfrom
cenit:dev/cenit/cudnn
Apr 13, 2021
Merged

[cudnn] auto-download only if cuda version matches#16413
strega-nil merged 8 commits intomicrosoft:masterfrom
cenit:dev/cenit/cudnn

Conversation

@cenit
Copy link
Contributor

@cenit cenit commented Feb 25, 2021

supersedes #16031 ?
@jacobkahn this is what I meant. Let me know if it works for you. Sorry just found some time only now

@JackBoosY JackBoosY added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Feb 26, 2021
@JackBoosY
Copy link
Contributor

JackBoosY commented Feb 26, 2021

Ready for review now?

@JackBoosY JackBoosY added the category:port-update The issue is with a library, which is requesting update new revision label Feb 26, 2021
@cenit
Copy link
Contributor Author

cenit commented Feb 26, 2021

yes

@jacobkahn
Copy link
Contributor

jacobkahn commented Feb 27, 2021

@cenit This is great - I tried to get this to work but couldn't. This looks much better. Thanks for the change! I'll close my other PR.

@jacobkahn jacobkahn mentioned this pull request Feb 27, 2021
Closed
@cenit
Copy link
Contributor Author

cenit commented Feb 27, 2021

@cenit This is great - I tried to get this to work but couldn't. This looks much better. Thanks for the change! I'll close my other PR.

I didn't propagate similar changes to nccl. After this PR is merged if you're in a hurry you will be quicker than me doing that for sure

@JackBoosY JackBoosY added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Mar 1, 2021
ras0219-msft
ras0219-msft reviewed Mar 2, 2021
View reviewed changes
ports/cuda/vcpkg_find_cuda.cmake
ENV CUDA_BIN_PATH
ENV CUDA_PATH_V11_0
ENV CUDA_PATH_V10_2
ENV CUDA_PATH_V10_1)
Copy link
Contributor

@ras0219-msft ras0219-msft Mar 2, 2021

Choose a reason for hiding this comment

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

Why remove these paths?

Copy link
Contributor Author

@cenit cenit Mar 3, 2021

Choose a reason for hiding this comment

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

a working cuda installation should have CUDA_PATH set up AFAIK.

Those variables just show up for each installed version (might even be all of them on a system) but do not express the user choice between them if multiple are available, which is done with the single CUDA_PATH.
Also, i'd lower the minimum cuda version we require, since it's not strictly installed by us (on the contrary, in this case we should support the broadest possible range of version, right?)

Copy link
Contributor

@jacobkahn jacobkahn Mar 7, 2021

Choose a reason for hiding this comment

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

@ras0219 these paths aren't standard and are version-specific. I'm unaware of any environment in which these paths are set. CUDA_PATH or CUDA_HOME is standard across many versions of CUDA.

@JackBoosY JackBoosY added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Mar 3, 2021
cenit and others added 5 commits March 3, 2021 22:31
@cenit @ras0219-msft
Update ports/cuda/vcpkg_find_cuda.cmake
19dcd4c 
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
@cenit @ras0219-msft
Update ports/cuda/vcpkg_find_cuda.cmake
b1823a8 
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
@cenit @ras0219-msft
Update ports/cudnn/portfile.cmake
2b9f218 
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
@cenit
apply review fixes
659d286
@cenit
fix baseline refs
413eff0
@jacobkahn
Copy link
Contributor

jacobkahn commented Mar 7, 2021

@cenit This is great - I tried to get this to work but couldn't. This looks much better. Thanks for the change! I'll close my other PR.

I didn't propagate similar changes to nccl. After this PR is merged if you're in a hurry you will be quicker than me doing that for sure

Sounds good. I'll send a PR for NCCL once this PR is merged.

@JackBoosY JackBoosY requested a review from ras0219-msft March 8, 2021 02:10
This was referenced Mar 11, 2021
Closed
Closed
Closed
@cenit
Copy link
Contributor Author

cenit commented Apr 11, 2021

any reason this is still unmerged?

@JackBoosY JackBoosY added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Apr 12, 2021
@JackBoosY
Copy link
Contributor

JackBoosY commented Apr 12, 2021

@strega-nil Ping for review and merge this PR.

@strega-nil strega-nil merged commit e3fee6e into microsoft:master Apr 13, 2021
@cenit cenit deleted the dev/cenit/cudnn branch April 14, 2021 10:59
@jacobkahn jacobkahn mentioned this pull request Apr 21, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ras0219-msft ras0219-msft Awaiting requested review from ras0219-msft

3 more reviewers

@jacobkahn jacobkahn jacobkahn left review comments

@strega-nil strega-nil strega-nil approved these changes

@JackBoosY JackBoosY JackBoosY approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JackBoosY JackBoosY

Labels

category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:port-update The issue is with a library, which is requesting update new revision info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cenit @JackBoosY @jacobkahn @strega-nil @ras0219-msft