Skip to content

Delete THCStreamGuard in favor of CUDAGuard, also c10d code cleanup#12849

Closed
ezyang wants to merge 8 commits intomasterfrom
export-D10457671
Closed

Delete THCStreamGuard in favor of CUDAGuard, also c10d code cleanup#12849
ezyang wants to merge 8 commits intomasterfrom
export-D10457671

Conversation

@ezyang
Copy link
Copy Markdown
Contributor

@ezyang ezyang commented Oct 19, 2018 •
edited
Loading

Stack:
    :black_circle:  #12849 Delete THCStreamGuard in favor of CUDAGuard, also c10d code cleanup  💛

I got annoyed at waiting for OSS to tell me my c10d builds were busted, so
I also added support for building the test scripts in fbcode and fixed the
warnings this uncovered.

Differential Revision: D10457671

@ezyang
V1: Initial commit
9b5d8bf 
Differential Revision: D10457671
Differential Version: 61080418
ezyang added 5 commits October 18, 2018 18:01
@ezyang
V2: Fix include for cpu build
79498d6 
Differential Revision: D10457671
Differential Version: 61081812
@ezyang
V3: Missing include
5f46634 
Differential Revision: D10457671
Differential Version: 61083745
@ezyang
V4: Fix another code review
6bac12e 
Differential Revision: D10457671
Differential Version: 61084321
@ezyang
V5: another header
cc19a47 
Differential Revision: D10457671
Differential Version: 61085552
@ezyang
V6: Fix pessimizing moves, add tests, and fix build
3620995 
Differential Revision: D10457671
Differential Version: 61086551
@ezyang ezyang mentioned this pull request Oct 19, 2018
Closed
@ezyang
V7: Fix fbcode a little more
cce9eb1 
Differential Revision: D10457671
Differential Version: 61087075
@ezyang ezyang changed the title Delete THCStreamGuard in favor of CUDAGuard. Delete THCStreamGuard in favor of CUDAGuard, also c10d code cleanup Oct 19, 2018
pietern
pietern approved these changes Oct 19, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

@ezyang Does at::cuda::createCUDAStream actually create a new stream or pull one from the pool?

cc @mruberry

@teng-li
Copy link
Copy Markdown
Contributor

teng-li commented Oct 19, 2018

@ezyang can you also run clang-format -i on all the c10d file touched?

@teng-li
Copy link
Copy Markdown
Contributor

teng-li commented Oct 19, 2018

And also, please run make test in c10d build folder

@ezyang
Copy link
Copy Markdown
Contributor Author

ezyang commented Oct 19, 2018

And also, please run make test in c10d build folder

Why isn't this covered by CI?

@ezyang
V8: rebase clang format
3dedaa6 
Differential Revision: D10457671
Differential Version: 61112012
@ezyang
Copy link
Copy Markdown
Contributor Author

ezyang commented Oct 19, 2018

@pietern It retrieves streams from the pool

@ezyang
Copy link
Copy Markdown
Contributor Author

ezyang commented Oct 19, 2018 

[0/1] Running tests...
Test project /home/ezyang/Dev/pytorch-tmp/torch/lib/build/c10d
    Start 1: FileStoreTest
1/5 Test #1: FileStoreTest ....................   Passed    0.06 sec
    Start 2: TCPStoreTest
2/5 Test #2: TCPStoreTest .....................   Passed    0.92 sec
    Start 3: ProcessGroupGlooTest
3/5 Test #3: ProcessGroupGlooTest .............   Passed    3.93 sec
    Start 4: ProcessGroupGlooAsyncTest
4/5 Test #4: ProcessGroupGlooAsyncTest ........   Passed   12.16 sec
    Start 5: ProcessGroupNCCLTest
5/5 Test #5: ProcessGroupNCCLTest .............   Passed   12.99 sec

100% tests passed, 0 tests failed out of 5

Total Test time (real) =  30.07 sec

@soumith soumith deleted the export-D10457671 branch February 21, 2019 12:11
@ezyang ezyang added the merged label Jun 25, 2019
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@ezyang @facebook-github-bot
Delete THCStreamGuard in favor of CUDAGuard, also c10d code cleanup (p…
ff08190 
…ytorch#12849)

Summary:
I got annoyed at waiting for OSS to tell me my c10d builds were busted, so
I also added support for building the test scripts in fbcode and fixed the
warnings this uncovered.

Pull Request resolved: pytorch#12849

Reviewed By: pietern

Differential Revision: D10457671

fbshipit-source-id: 5b0e36c606e397323f313f09dfce64d2df88faed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apaszke apaszke Awaiting requested review from apaszke

@teng-li teng-li Awaiting requested review from teng-li

1 more reviewer

@pietern pietern pietern approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ezyang @teng-li @pietern