Skip to content

[DataPipe] Fix FullSync shutdown hanging issue while paused#1153

Closed
NivekT wants to merge 4 commits intogh/NivekT/123/basefrom
gh/NivekT/123/head
Closed

[DataPipe] Fix FullSync shutdown hanging issue while paused#1153
NivekT wants to merge 4 commits intogh/NivekT/123/basefrom
gh/NivekT/123/head

Conversation

@NivekT
Copy link
Contributor

@NivekT NivekT commented May 5, 2023

Stack from ghstack:

Before this PR, the executor within FullSync fails to shutdown if it were currently paused. This PR allows shutdown without submitting additional jobs.

Differential Revision: D45610885

NivekT added a commit that referenced this pull request May 5, 2023
@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 5, 2023
@NivekT NivekT requested a review from ejguan May 5, 2023 16:49
@NivekT
Copy link
Contributor Author

NivekT commented May 5, 2023

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

dp3.pause()
it2 = iter(dp3) # Reset
next(it2)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test failing without the patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for DataLoader2 with fullsync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, it fails without the patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't really add a DataLoader2 test because DistributedRS currently doesn't support pause....

I will have to add that separately. Let me know if I should land this as it is or add that on top of this.

NivekT added a commit that referenced this pull request May 5, 2023
@NivekT
Copy link
Contributor Author

NivekT commented May 5, 2023

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

Before this PR, the executor within FullSync fails to shutdown if it were currently paused. This PR allows shutdown without submitting additional jobs.

Differential Revision: [D45610885](https://our.internmc.facebook.com/intern/diff/D45610885)

[ghstack-poisoned]
NivekT added a commit that referenced this pull request May 5, 2023
@NivekT
Copy link
Contributor Author

NivekT commented May 5, 2023

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

Before this PR, the executor within FullSync fails to shutdown if it were currently paused. This PR allows shutdown without submitting additional jobs.

Differential Revision: [D45610885](https://our.internmc.facebook.com/intern/diff/D45610885)

[ghstack-poisoned]
NivekT added a commit that referenced this pull request May 12, 2023
@NivekT
Copy link
Contributor Author

NivekT commented May 12, 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 0b39117.

@facebook-github-bot facebook-github-bot deleted the gh/NivekT/123/head branch May 19, 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