Fix persistent worker exits before pin_memory thread #71579
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 0802991 (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).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
Fixes #1551 As the comment in the code, adding a function to manually delete the iterator with persistent worker. It would invoke `__del__` inside Iterator object and make sure pin_memory_thread exits before worker process. I choose using `atexit` rather than adding `__del__` to DataLoader because it's not safe as the destructor function may not be invoked when Python interpreter exits. [ghstack-poisoned]
Fixes #1551 As the comment in the code, adding a function to manually delete the iterator with persistent worker. It would invoke `__del__` inside Iterator object and make sure pin_memory_thread exits before worker process. I choose using `atexit` rather than adding `__del__` to DataLoader because it's not safe as the destructor function may not be invoked when Python interpreter exits. [ghstack-poisoned]
ejguan
commented
Jan 20, 2022
torch/utils/data/dataloader.py
Outdated
Comment on lines
949
to
955
| # In some rare cases, persistent workers (daemonic processes) | ||
| # would be terminated before `__del__` of iterator is invoked | ||
| # when main process exits | ||
| # It would cause failure when pin_memory_thread tries to read | ||
| # corrupted data from worker_result_queue | ||
| # atexit is used to prevent persistent workers exiting before | ||
| # pin_memory_thread |
There was a problem hiding this comment.
Here is the comment.
test/test_dataloader.py
Outdated
Comment on lines
2305
to
2339
| @unittest.skipIf(not TEST_CUDA, "CUDA unavailable") | ||
| def test_early_exit(self): | ||
| import subprocess | ||
| proc = subprocess.run([sys.executable, '-c', """\ | ||
| import torch | ||
| from torch.utils.data import DataLoader, IterableDataset | ||
|
|
||
| class RandomDataset(IterableDataset): | ||
| def __init__(self, len, size): | ||
| super(RandomDataset).__init__() | ||
| self.len = len | ||
| self.size = size | ||
|
|
||
| def __iter__(self): | ||
| return self | ||
|
|
||
| def __next__(self): | ||
| if self.len <= 0: | ||
| raise StopIteration | ||
| self.len -= 1 | ||
| return torch.randn(self.size) | ||
|
|
||
| if __name__ == '__main__': | ||
| dl = DataLoader( | ||
| RandomDataset(64, (28, 28)), | ||
| batch_size=16, | ||
| num_workers=2, | ||
| pin_memory=True, | ||
| persistent_workers=True, | ||
| ) | ||
|
|
||
| for _ in dl: | ||
| break | ||
| """], stderr=subprocess.PIPE) | ||
| self.assertTrue(proc.stderr == b'') |
There was a problem hiding this comment.
This test won't pass before this PR.
|
@ejguan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
ejguan
commented
Jan 21, 2022
| # corrupted data from worker_result_queue | ||
| # atexit is used to prevent persistent workers exiting before | ||
| # pin_memory_thread | ||
| if self._persistent_workers and self._pin_memory: |
There was a problem hiding this comment.
In order to potentially fix part of #71187 for the case of persistent workers, we may want to remove this condition on self._pin_memory
VitalyFedyunin
approved these changes
Jan 21, 2022
facebook-github-bot
pushed a commit
that referenced
this pull request
Jan 21, 2022
Summary: Pull Request resolved: #71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. Using `atexit` to make sure termination of persistent workers always happens at the end (after pin_memory_thread exits). We need such mechanism because Python interpreter would clean up worker process before DataLoader iterator in some rare cases. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33694867 Pulled By: ejguan fbshipit-source-id: 0847f4d424a0cd6b3c0be8235d505415970254e8
pytorchmergebot
pushed a commit
that referenced
this pull request
Jan 21, 2022
Summary: Pull Request resolved: #71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. Using `atexit` to make sure termination of persistent workers always happens at the end (after pin_memory_thread exits). We need such mechanism because Python interpreter would clean up worker process before DataLoader iterator in some rare cases. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33694867 Pulled By: ejguan fbshipit-source-id: 0847f4d424a0cd6b3c0be8235d505415970254e8 (cherry picked from commit 18ad462)
|
Reverting as it broke Windows test sanity(from https://github.com/pytorch/pytorch/runs/4903195822?check_suite_focus=true): |
|
This pull request has been reverted by 401ac7b52a96377814c37dfeaa3f0bdea231d058. To re-land this change, follow these steps. |
|
This pull request has been reverted by 86aefdc. To re-land this change, follow these steps. |
Fixes #1551 As the comment in the code, register a function to terminate persistent workers. By adding a reference of these workers in `atexit`, it would prevent Python interpreter kills these persistent worker processes before `pin_memorh_thread` exits. And, if users explicitly kills DataLoader iterator, such function in `atexit` would be a no-op. [ghstack-poisoned]
Fixes #1551 As the comment in the code, register a function to terminate persistent workers. By adding a reference of these workers in `atexit`, it would prevent Python interpreter kills these persistent worker processes before `pin_memorh_thread` exits. And, if users explicitly kills DataLoader iterator, such function in `atexit` would be a no-op. [ghstack-poisoned]
|
@ejguan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Fixes #1551 As the comment in the code, register a function to terminate persistent workers. By adding a reference of these workers in `atexit`, it would prevent Python interpreter kills these persistent worker processes before `pin_memorh_thread` exits. And, if users explicitly kills DataLoader iterator, such function in `atexit` would be a no-op. Differential Revision: [D33896537](https://our.internmc.facebook.com/intern/diff/D33896537) [ghstack-poisoned]
|
@ejguan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Feb 1, 2022
Summary: Pull Request resolved: #71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. By adding a reference of these workers in `atexit`, it would prevent Python interpreter kills these persistent worker processes before `pin_memorh_thread` exits. And, if users explicitly kills DataLoader iterator, such function in `atexit` would be a no-op. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33896537 Pulled By: ejguan fbshipit-source-id: 36b57eac7523d8aa180180c2b61fc693ea4638ae
pytorchmergebot
pushed a commit
that referenced
this pull request
Feb 1, 2022
Summary: Pull Request resolved: #71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. By adding a reference of these workers in `atexit`, it would prevent Python interpreter kills these persistent worker processes before `pin_memorh_thread` exits. And, if users explicitly kills DataLoader iterator, such function in `atexit` would be a no-op. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33896537 Pulled By: ejguan fbshipit-source-id: 36b57eac7523d8aa180180c2b61fc693ea4638ae (cherry picked from commit 05add2a)
|
Hey ejguan. |
|
@ejguan can you also put a topic label to it? 😊 |
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Feb 3, 2022
Summary: Pull Request resolved: pytorch/pytorch#71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. Using `atexit` to make sure termination of persistent workers always happens at the end (after pin_memory_thread exits). We need such mechanism because Python interpreter would clean up worker process before DataLoader iterator in some rare cases. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33694867 Pulled By: ejguan fbshipit-source-id: 0847f4d424a0cd6b3c0be8235d505415970254e8 (cherry picked from commit 18ad462)
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Feb 3, 2022
Summary: Pull Request resolved: pytorch/pytorch#71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. By adding a reference of these workers in `atexit`, it would prevent Python interpreter kills these persistent worker processes before `pin_memorh_thread` exits. And, if users explicitly kills DataLoader iterator, such function in `atexit` would be a no-op. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33896537 Pulled By: ejguan fbshipit-source-id: 36b57eac7523d8aa180180c2b61fc693ea4638ae (cherry picked from commit 05add2a)
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Feb 3, 2022
F438
Summary: Pull Request resolved: pytorch/pytorch#71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. Using `atexit` to make sure termination of persistent workers always happens at the end (after pin_memory_thread exits). We need such mechanism because Python interpreter would clean up worker process before DataLoader iterator in some rare cases. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33694867 Pulled By: ejguan fbshipit-source-id: 0847f4d424a0cd6b3c0be8235d505415970254e8 (cherry picked from commit 18ad462)
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Feb 3, 2022
Summary: Pull Request resolved: pytorch/pytorch#71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. By adding a reference of these workers in `atexit`, it would prevent Python interpreter kills these persistent worker processes before `pin_memorh_thread` exits. And, if users explicitly kills DataLoader iterator, such function in `atexit` would be a no-op. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33896537 Pulled By: ejguan fbshipit-source-id: 36b57eac7523d8aa180180c2b61fc693ea4638ae (cherry picked from commit 05add2a)
ejguan
added a commit
to ejguan/pytorch
that referenced
this pull request
Feb 3, 2022
ghstack-source-id: 2d15b14 Pull Request resolved: pytorch#71579
This was referenced Feb 3, 2022
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Feb 9, 2022
Summary: Pull Request resolved: pytorch/pytorch#71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. Using `atexit` to make sure termination of persistent workers always happens at the end (after pin_memory_thread exits). We need such mechanism because Python interpreter would clean up worker process before DataLoader iterator in some rare cases. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33694867 Pulled By: ejguan fbshipit-source-id: 0847f4d424a0cd6b3c0be8235d505415970254e8 (cherry picked from commit 18ad462)
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Feb 9, 2022
Summary: Pull Request resolved: pytorch/pytorch#71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. By adding a reference of these workers in `atexit`, it would prevent Python interpreter kills these persistent worker processes before `pin_memorh_thread` exits. And, if users explicitly kills DataLoader iterator, such function in `atexit` would be a no-op. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33896537 Pulled By: ejguan fbshipit-source-id: 36b57eac7523d8aa180180c2b61fc693ea4638ae (cherry picked from commit 05add2a)
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Feb 9, 2022
Summary: Pull Request resolved: pytorch/pytorch#71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. Using `atexit` to make sure termination of persistent workers always happens at the end (after pin_memory_thread exits). We need such mechanism because Python interpreter would clean up worker process before DataLoader iterator in some rare cases. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33694867 Pulled By: ejguan fbshipit-source-id: 0847f4d424a0cd6b3c0be8235d505415970254e8 (cherry picked from commit 18ad462)
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Feb 9, 2022
Summary: Pull Request resolved: pytorch/pytorch#71579 Fixes #1551 As the comment in the code, register a function to terminate persistent workers. By adding a reference of these workers in `atexit`, it would prevent Python interpreter kills these persistent worker processes before `pin_memorh_thread` exits. And, if users explicitly kills DataLoader iterator, such function in `atexit` would be a no-op. Test Plan: Imported from OSS Reviewed By: VitalyFedyunin Differential Revision: D33896537 Pulled By: ejguan fbshipit-source-id: 36b57eac7523d8aa180180c2b61fc693ea4638ae (cherry picked from commit 05add2a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1551
Stack from ghstack:
As the comment in the code, register a function to terminate persistent workers.
By adding a reference of these workers in
atexit, it would prevent Python interpreter kills these persistent worker processes beforepin_memorh_threadexits.And, if users explicitly kills DataLoader iterator, such function in
atexitwould be a no-op.Differential Revision: D33896537