Skip to content

Add CUDA 11 builds for Windows CI#42420

Closed
peterjc123 wants to merge 11 commits intopytorch:masterfrom
peterjc123:cuda_11_windows_ci
Closed

Add CUDA 11 builds for Windows CI#42420
peterjc123 wants to merge 11 commits intopytorch:masterfrom
peterjc123:cuda_11_windows_ci

Conversation

@peterjc123
Copy link
Copy Markdown
Collaborator

@peterjc123 peterjc123 commented Aug 2, 2020

Stacked on #42410.

@dr-ci
Copy link
Copy Markdown

dr-ci bot commented Aug 2, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit de3bef7 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 91 times.

@peterjc123 peterjc123 force-pushed the cuda_11_windows_ci branch 5 times, most recently from 268d8cb to ce87890 Compare August 2, 2020 05:21
@peterjc123 peterjc123 mentioned this pull request Aug 2, 2020
Closed
@peterjc123 peterjc123 force-pushed the cuda_11_windows_ci branch 2 times, most recently from 6422f84 to e7b2b57 Compare August 3, 2020 14:25
@peterjc123 peterjc123 changed the title Use CUDA 11 for Windows CI Add CUDA 11 builds for Windows CI Aug 3, 2020
@peterjc123 peterjc123 marked this pull request as ready for review August 3, 2020 14:39
@peterjc123 peterjc123 mentioned this pull request Aug 3, 2020
Merged
@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented Aug 4, 2020

Please don't merge without go-ahead from @malfet , cc'ing @malfet and @zpao for CI cost implications

malfet
malfet requested changes Aug 4, 2020
View reviewed changes
.circleci/cimodel/data/windows_build_definitions.py Outdated
Copy link
Copy Markdown
Contributor

@malfet malfet Aug 4, 2020

Choose a reason for hiding this comment

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

Not sure what extra coverage force_on_cpu gives us for 10.1 vs 11.0, imo we should have one or the other

Copy link
Copy Markdown
Collaborator Author

@peterjc123 peterjc123 Aug 4, 2020

Choose a reason for hiding this comment

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

Removed the one for cuda 11.

aten/src/ATen/native/sparse/cuda/SparseCUDABlas.cu Outdated
Copy link
Copy Markdown
Contributor

@malfet malfet Aug 4, 2020

Choose a reason for hiding this comment

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

Hmm, your change disables this functionality on Linux with CUDA-10.2, which is not ideal, is it?

Suggested change
#if defined(__CUDACC__) && CUSPARSE_VERSION >= 11000
#if defined(__CUDACC__) && (CUSPARSE_VERSION >= 11000 || (!defined(_MSC_VER) && CUSPARSE_VERSION >= 10301))

Copy link
Copy Markdown
Collaborator

@xwang233 xwang233 Aug 4, 2020

Choose a reason for hiding this comment

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

You can check cusparse details at #42412. On linux cuda 10.2, using new or old API should have no big differences. They are both functional. This is just to make the macro straightforward.

aten/src/ATen/native/sparse/cuda/SparseCUDABlas.cu Outdated
Copy link
Copy Markdown
Contributor

@malfet malfet Aug 4, 2020

Choose a reason for hiding this comment

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

Let's keep this enabled for CUDA 10.2 on Linux

Suggested change
#if defined(__CUDACC__) && CUSPARSE_VERSION >= 11000
#if defined(__CUDACC__) && (CUSPARSE_VERSION >= 11000 || (!defined(_MSC_VER) && CUSPARSE_VERSION >= 10301))

aten/src/ATen/native/sparse/cuda/SparseCUDABlas.cu Outdated
Copy link
Copy Markdown
Contributor

@malfet malfet Aug 4, 2020

Choose a reason for hiding this comment

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

How this is related to CUDA-11 upgrade?

cmake/ProtoBufPatch.cmake Outdated
Comment on lines 40 to 55
Copy link
Copy Markdown
Contributor

@malfet malfet Aug 4, 2020

Choose a reason for hiding this comment

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

How this related to CUDA-11 update?

Copy link
Copy Markdown
Collaborator Author

@peterjc123 peterjc123 Aug 4, 2020

Choose a reason for hiding this comment

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

The error (in comments) will be raised for CUDA 11 builds.

@xwang233
Copy link
Copy Markdown
Collaborator

xwang233 commented Aug 4, 2020

Part of the cusparse changes (cuda11+windows) were just merged at #42412.

malfet
malfet approved these changes Aug 4, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Please rebase, otherwise LGTM (all the comments I have on the diff, other than Protobuf patch coming from #42412)

@peterjc123
Copy link
Copy Markdown
Collaborator Author

peterjc123 commented Aug 4, 2020

@malfet Rebased.

facebook-github-bot
facebook-github-bot reviewed Aug 4, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@peterjc123 peterjc123 linked an issue Aug 5, 2020 that may be closed by this pull request
Caffe2 Error: more than one operator "+" matches these operands, windows and Cuda 11 #42549
Closed
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Aug 6, 2020

@malfet merged this pull request in b08347f.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malfet malfet malfet approved these changes

@kurtamohler kurtamohler kurtamohler approved these changes

@xwang233 xwang233 xwang233 approved these changes

@ezyang ezyang Awaiting requested review from ezyang

@nbcsm nbcsm Awaiting requested review from nbcsm

+2 more reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@guyang3532 guyang3532 guyang3532 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Caffe2 Error: more than one operator "+" matches these operands, windows and Cuda 11

9 participants

@peterjc123 @ezyang @xwang233 @facebook-github-bot @malfet @kurtamohler @guyang3532 @pytorchbot @mruberry