Skip to content

[DataPipe] Ensures Prefetcher shuts down properly#1166

Closed
NivekT wants to merge 9 commits intogh/NivekT/124/basefrom
gh/NivekT/124/head
Closed

[DataPipe] Ensures Prefetcher shuts down properly#1166
NivekT wants to merge 9 commits intogh/NivekT/124/basefrom
gh/NivekT/124/head

Conversation

@NivekT
Copy link
Contributor

@NivekT NivekT commented May 17, 2023 •
edited
Loading

NivekT added a commit that referenced this pull request May 17, 2023
@NivekT
[DataPipe] Ensures Prefetcher shuts down properly
45882dd 
ghstack-source-id: e4ed128
Pull Request resolved: #1166
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 17, 2023
@NivekT
Copy link
Contributor Author

NivekT commented May 17, 2023

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

@NivekT NivekT requested a review from ejguan May 17, 2023 22:26
@NivekT
Copy link
Contributor Author

NivekT commented May 17, 2023

@ejguan Doesn't seem like it is strictly necessary (?) but it is a good safeguard

ejguan
ejguan approved these changes May 17, 2023
View reviewed changes
Copy link
Contributor

@ejguan ejguan left a comment

Choose a reason for hiding this comment

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

Overall LGTM with a comment below

NivekT added a commit that referenced this pull request May 17, 2023
@NivekT
[DataPipe] Ensures Prefetcher shuts down properly
543b1fe 
ghstack-source-id: be52b2f
Pull Request resolved: #1166
@NivekT
Copy link
Contributor Author

NivekT commented May 17, 2023

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

NivekT added a commit that referenced this pull request May 18, 2023
@NivekT
[DataPipe] Ensures Prefetcher shuts down properly
62127dd 
ghstack-source-id: 12ccf97
Pull Request resolved: #1166
@NivekT
Copy link
Contributor Author

NivekT commented May 18, 2023

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

NivekT added a commit that referenced this pull request May 18, 2023
@NivekT
[DataPipe] Ensures Prefetcher shuts down properly
367dd79 
ghstack-source-id: ad503d7
Pull Request resolved: #1166
@NivekT
Copy link
Contributor Author

NivekT commented May 18, 2023

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

NivekT added a commit that referenced this pull request May 19, 2023
@NivekT
[DataPipe] Ensures Prefetcher shuts down properly
f9b6e36 
ghstack-source-id: dd778b5
Pull Request resolved: #1166
NivekT added a commit that referenced this pull request May 19, 2023
@NivekT
[DataPipe] Ensures Prefetcher shuts down properly
02ea7f4 
ghstack-source-id: 916c68e
Pull Request resolved: #1166
NivekT added a commit that referenced this pull request May 19, 2023
@NivekT
[DataPipe] Ensures Prefetcher shuts down properly
9968f57 
ghstack-source-id: 0fb262b
Pull Request resolved: #1166
@NivekT
Copy link
Contributor Author

NivekT commented May 19, 2023

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

NivekT added a commit that referenced this pull request May 19, 2023
@NivekT
[DataPipe] Ensures Prefetcher shuts down properly
3919a56 
ghstack-source-id: b1b7961
Pull Request resolved: #1166
@NivekT
Copy link
Contributor Author

NivekT commented May 19, 2023

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

ejguan
ejguan approved these changes May 22, 2023
View reviewed changes
Copy link
Contributor

@ejguan ejguan left a comment

Choose a reason for hiding this comment

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

Super nit: Can we do self.thread = Thread() in __iter__?

NivekT added a commit that referenced this pull request May 23, 2023
@NivekT
[DataPipe] Ensures Prefetcher shuts down properly
56bc88a 
ghstack-source-id: cb24f3c
Pull Request resolved: #1166
@NivekT
Copy link
Contributor Author

NivekT commented May 23, 2023

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

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 24, 2023

@NivekT merged this pull request in d4d0469.

@facebook-github-bot facebook-github-bot deleted the gh/NivekT/124/head branch May 28, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ejguan ejguan ejguan approved these changes

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@NivekT @facebook-github-bot @ejguan