Convert generator in Sampler back to lazy construction #63646

ejguan · 2021-08-20T14:15:52Z

Fixes #63609

Stack from ghstack:

Revert A re-open PR: Avoid re-creating the random number generator in RandomSampler #63026
- Sampler is expected to be re-seeded if user specify seed before each epoch
- Can not attach generator to self with __iter__ because multiple iterators will ruin the use case
Add tests to prevent the same case for different Samplers

Differential Revision: D30451774

[ghstack-poisoned]

facebook-github-bot · 2021-08-20T14:15:58Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/63646
📄 Preview docs built from this PR
📄 Preview C++ docs built from this PR
↩️ [fb-only] Re-run with SSH instructions
🔧 Opt-in to CIFlow to control what jobs run on your PRs

💊 CI failures summary and remediations

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

1/1 failures introduced in this PR

1 failure not recognized by patterns:

Job	Step	Action
^{linux-bionic-py3.6-clang9 / test (default, 1, 2, linux.2xlarge)}	^Unknown	🔁 rerun

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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Fixes #63609 [ghstack-poisoned]

ejguan · 2021-08-20T14:34:56Z

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

vfdev-5

@ejguan looks good to me. Thanks. I left a nit comment.

torch/utils/data/sampler.py

Fixes #63609 Differential Revision: [D30451774](https://our.internmc.facebook.com/intern/diff/D30451774) [ghstack-poisoned]

ejguan · 2021-08-20T15:17:07Z

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

VitalyFedyunin · 2021-08-20T15:25:56Z

torch/utils/data/sampler.py

        else:
-            yield from torch.randperm(n, generator=self.generator).tolist()
+            yield from torch.randperm(n, generator=self._gen).tolist()
+        self._gen = None


This will badly impact all currently running iterators, consider creating two individual iterators in test and yielding them in random order.

Emmm, you are right. So, we have to create iter-local generator. But, then it goes back to the problem that we can not serialize the state of this iter-local generator anymore. Then, when we resume from snapshotting, we can not really fast forward the state of generator, have to iterate over it to the certain amount of iterations.

torch/utils/data/sampler.py

VitalyFedyunin

Not going to work as intended for two or more iterators created from same object.

 a = Sampler()
 i1 = iter(a)
 i2 = iter(a)

VitalyFedyunin · 2021-08-20T16:04:03Z

Return new generator from __iter__ to keep states shared

class Sampler():
    def __init__(self):
        self.iterators = {}
        pass

    def __iter__(self):
        # Everything is by pointer here
        state_dict = {}
        generator = iter(self.generator(state_dict))
        self.iterators[generator] = state_dict
        return generator

    def generator(self, state_dict):
        state_dict['gen'] = len(self.iterators)
        for i in range(10):
            yield i

s = Sampler()
i = iter(s)
i2 = iter(s)

print(i)
print(i2)

for d in i:
    print(d)

print(s.iterators)

ejguan · 2021-08-20T16:06:27Z

Return new generator from __iter__ to keep states shared

Yeah, that's also something in my mind. Will update PR.

Fixes #63609 Differential Revision: [D30451774](https://our.internmc.facebook.com/intern/diff/D30451774) [ghstack-poisoned]

ghstack-source-id: 81dde49f141f887850c1f2c87e68d35337e585a3 Pull Request resolved: #63646

Fixes #63609 Differential Revision: [D30451774](https://our.internmc.facebook.com/intern/diff/D30451774) [ghstack-poisoned]

ghstack-source-id: 17692bbbb9a7b9922e17426a543b2d0f6b59e1bf Pull Request resolved: #63646

ejguan · 2021-08-20T20:42:29Z

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

Fixes #63609 - Revert #63026 - Sampler is expected to be re-seeded if user specify seed before each epoch - Can not attach generator to self with `__iter__` because multiple iterators will ruin the use case - Add tests to prevent the same case for different Samplers Differential Revision: [D30451774](https://our.internmc.facebook.com/intern/diff/D30451774) [ghstack-poisoned]

Fixes #63609 - Revert #63026 - Sampler is expected to be re-seeded if user specify seed before each epoch - Can not attach generator to self with `__iter__` because multiple iterators will ruin the use case - Add tests to prevent the same case for different Samplers - Keep same functionality introduced in #63026 that user can serialize the Sampler. Differential Revision: [D30451774](https://our.internmc.facebook.com/intern/diff/D30451774) [ghstack-poisoned]

ejguan · 2021-09-29T21:57:43Z

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

Fixes #63609 - Revert #63026 - Sampler is expected to be re-seeded if user specify seed before each epoch - Can not attach generator to self with `__iter__` because multiple iterators will ruin the use case - Add tests to prevent the same case for different Samplers Differential Revision: [D30451774](https://our.internmc.facebook.com/intern/diff/D30451774) [ghstack-poisoned]

ejguan · 2021-09-29T22:22:16Z

I split the original PR into two PRs.
This PR would revert Sampler back to previous state that the RNG is created lazily.

The another PR provide a new feature for potential users to serialize the iter-local generator.

ejguan · 2021-09-29T22:23:00Z

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

NivekT

LGTM

Summary: Pull Request resolved: pytorch#63646 Fixes pytorch#63609 Test Plan: Imported from OSS Reviewed By: NivekT Differential Revision: D30451774 Pulled By: ejguan fbshipit-source-id: 550d77494326446d1a42b5da0559e0d384c47413

Summary: Pull Request resolved: #63646 Fixes #63609 Test Plan: Imported from OSS Reviewed By: NivekT Differential Revision: D30451774 Pulled By: ejguan fbshipit-source-id: 550d77494326446d1a42b5da0559e0d384c47413

ghstack-source-id: bf4b2badb725d51e23b570f8c43e903fc6d9bb71 Pull Request resolved: pytorch/pytorch#63646

Convert Sampler back to lazily construction

8b4ddf7

[ghstack-poisoned]

facebook-github-bot added the cla signed label Aug 20, 2021

ejguan linked an issue Aug 20, 2021 that may be closed by this pull request

Sampler should be seeded lazily #63609

Closed

Update on "Convert Sampler back to lazily construction"

468acb4

Fixes #63609 [ghstack-poisoned]

ejguan requested a review from vfdev-5 August 20, 2021 14:34

vfdev-5 reviewed Aug 20, 2021

View reviewed changes

torch/utils/data/sampler.py Outdated Show resolved Hide resolved

ejguan changed the title ~~Convert Sampler back to lazily construction~~ Convert generator in Sampler back to lazily construction Aug 20, 2021

ejguan changed the title ~~Convert generator in Sampler back to lazily construction~~ Convert generator in Sampler back to lazy construction Aug 20, 2021

Update on "Convert generator in Sampler back to lazy construction"

1b1f1e9

Fixes #63609 Differential Revision: [D30451774](https://our.internmc.facebook.com/intern/diff/D30451774) [ghstack-poisoned]

ejguan added this to In Progress in torch.utils.data via automation Aug 20, 2021

ejguan added the module: dataloader Related to torch.utils.data.DataLoader and Sampler label Aug 20, 2021

ejguan requested a review from VitalyFedyunin August 20, 2021 15:16

VitalyFedyunin reviewed Aug 20, 2021

View reviewed changes

torch/utils/data/sampler.py Outdated Show resolved Hide resolved

VitalyFedyunin reviewed Aug 20, 2021

View reviewed changes

Update on "Convert generator in Sampler back to lazy construction"

8251153

Fixes #63609 Differential Revision: [D30451774](https://our.internmc.facebook.com/intern/diff/D30451774) [ghstack-poisoned]

ejguan added a commit that referenced this pull request Aug 20, 2021

Convert generator in Sampler back to lazy construction

506ee3d

ghstack-source-id: 81dde49f141f887850c1f2c87e68d35337e585a3 Pull Request resolved: #63646

Update on "Convert generator in Sampler back to lazy construction"

4c639d5

Fixes #63609 Differential Revision: [D30451774](https://our.internmc.facebook.com/intern/diff/D30451774) [ghstack-poisoned]

ejguan added a commit that referenced this pull request Aug 20, 2021

Convert generator in Sampler back to lazy construction

89431a1

ghstack-source-id: 17692bbbb9a7b9922e17426a543b2d0f6b59e1bf Pull Request resolved: #63646

ejguan requested a review from VitalyFedyunin September 29, 2021 21:57

ejguan mentioned this pull request Sep 29, 2021

Expose iterator-local generator to RandomSampler instance #65857

Closed

ejguan requested a review from NivekT September 29, 2021 22:20

NivekT approved these changes Sep 30, 2021

View reviewed changes

facebook-github-bot closed this in b777d79 Sep 30, 2021

torch.utils.data automation moved this from In Progress to Done Sep 30, 2021

This was referenced Sep 30, 2021

Convert generator attached to Sampler back to lazily construction #65926

Merged

[v.1.10.0] Release Tracker #65438

Closed

facebook-github-bot deleted the gh/ejguan/81/head branch October 4, 2021 14:24

maglimedia pushed a commit to gtedemo/demo-codespace-pytorch that referenced this pull request Oct 9, 2021

Convert generator in Sampler back to lazy construction

e91fe23

ghstack-source-id: bf4b2badb725d51e23b570f8c43e903fc6d9bb71 Pull Request resolved: pytorch/pytorch#63646

WBobby mentioned this pull request Jul 14, 2022

Add ROCm5.2/AMDGPU support for PyTorch 1.10 WBobby/pytorch#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert generator in Sampler back to lazy construction #63646

Convert generator in Sampler back to lazy construction #63646

ejguan commented Aug 20, 2021 •

edited

facebook-github-bot commented Aug 20, 2021 •

edited

ejguan commented Aug 20, 2021

vfdev-5 left a comment

ejguan commented Aug 20, 2021

VitalyFedyunin Aug 20, 2021

ejguan Aug 20, 2021

VitalyFedyunin left a comment

VitalyFedyunin commented Aug 20, 2021

ejguan commented Aug 20, 2021

ejguan commented Aug 20, 2021

ejguan commented Sep 29, 2021

ejguan commented Sep 29, 2021

ejguan commented Sep 29, 2021

NivekT left a comment

Convert generator in Sampler back to lazy construction #63646

Convert generator in Sampler back to lazy construction #63646

Conversation

ejguan commented Aug 20, 2021 • edited

facebook-github-bot commented Aug 20, 2021 • edited

🔗 Helpful links

💊 CI failures summary and remediations

1 failure not recognized by patterns:

ejguan commented Aug 20, 2021

vfdev-5 left a comment

Choose a reason for hiding this comment

ejguan commented Aug 20, 2021

VitalyFedyunin Aug 20, 2021

Choose a reason for hiding this comment

ejguan Aug 20, 2021

Choose a reason for hiding this comment

VitalyFedyunin left a comment

Choose a reason for hiding this comment

VitalyFedyunin commented Aug 20, 2021

ejguan commented Aug 20, 2021

ejguan commented Aug 20, 2021

ejguan commented Sep 29, 2021

ejguan commented Sep 29, 2021

ejguan commented Sep 29, 2021

NivekT left a comment

Choose a reason for hiding this comment

ejguan commented Aug 20, 2021 •

edited

facebook-github-bot commented Aug 20, 2021 •

edited