Skip to content

Prevent reset iteration message sent twice to workers#917

Closed
ejguan wants to merge 2 commits intometa-pytorch:mainfrom
ejguan:fix_reset_twice
Closed

Prevent reset iteration message sent twice to workers#917
ejguan wants to merge 2 commits intometa-pytorch:mainfrom
ejguan:fix_reset_twice

Conversation

@ejguan
Copy link
Contributor

@ejguan ejguan commented Dec 8, 2022

Remove end_datapipe.reset() as it's going to be invoked at the beginning of iteration.
nit changes

  • move mp_ctx creation out of loop
  • Fix lint

@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 Dec 8, 2022
@facebook-github-bot
Copy link
Contributor

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

@ejguan ejguan requested a review from NivekT December 8, 2022 17:24
@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@NivekT NivekT left a comment

Choose a reason for hiding this comment

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

LGTM! Just one question to make sure it is right.

dist_info = _DistInfo(shared_seed_int, self._world_size, self._rank)
call_on_epoch_reset = partial(process_reset_fn, dist_info=dist_info, custom_reset_fn=self.worker_reset_fn)
end_datapipe.reset_epoch(call_on_epoch_reset)
end_datapipe.reset()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to call this when self.main_prefetch_cnt <= 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed because reset is always called for each iteration of datapipe.

@facebook-github-bot
Copy link
Contributor

@ejguan merged this pull request in 248ecfe.

@ejguan ejguan added the topic: improvements topic category label Dec 13, 2022
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 topic: improvements topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants