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.

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

[DataPipe] Ensures Prefetcher shuts down properly #1166

Closed
wants to merge 9 commits into from

Conversation

NivekT
Copy link
Contributor

@NivekT NivekT commented May 17, 2023

NivekT added a commit that referenced this pull request May 17, 2023
ghstack-source-id: e4ed1282793ee9f9491761275ff6c67c22bc3039
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

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

torchdata/datapipes/iter/util/prefetcher.py Outdated Show resolved Hide resolved
NivekT added a commit that referenced this pull request May 17, 2023
ghstack-source-id: be52b2f2999a2942c596cf9242c2eae8950e9c24
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
ghstack-source-id: 12ccf97a9b1c4a922020653b9be74d54a763f0d1
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
ghstack-source-id: ad503d7719137f964c705978574cbc78cd7e5598
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
ghstack-source-id: dd778b5c86cc13a8b084c86385cc10bd163659d2
Pull Request resolved: #1166
NivekT added a commit that referenced this pull request May 19, 2023
ghstack-source-id: 916c68e42219e852212e4b54b0b7b1f1365a5f08
Pull Request resolved: #1166
NivekT added a commit that referenced this pull request May 19, 2023
ghstack-source-id: 0fb262b3ac3ed596db1f08161d7adb5bdc20e04f
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
ghstack-source-id: b1b79614345ba2e0a432269c026661249d9c2ba5
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.

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
ghstack-source-id: cb24f3c6ad0c01c7fbe7bf31ad532db9a0bae27e
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

@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
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
Development

Successfully merging this pull request may close these issues.

3 participants