KEMBAR78
C++ Frontend: adding two distributed samples (Random and Sequential) by jaliyae · Pull Request #16910 · pytorch/pytorch · GitHub
Skip to content

Conversation

@jaliyae
Copy link
Contributor

@jaliyae jaliyae commented Feb 9, 2019

Adding two distrbuted samplers, Random and Sequential to the mix. Similar to python counterpart, DistributedSampler introduces a new method set_epoch(size_t epoch) which can be use to shuffle data determinstically between distributed processes.

Copy link
Contributor

@goldsborough goldsborough left a comment

Choose a reason for hiding this comment

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

A few comments. Particularly interested in why DistributedSampler is not more general

@jaliyae jaliyae force-pushed the jaliyae/distributed_samplers branch from 18e07e3 to 37f601e Compare February 14, 2019 00:11
@jaliyae
Copy link
Contributor Author

jaliyae commented Feb 14, 2019

Fixed all review suggestions.

Copy link
Contributor

@apaszke apaszke left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but there's one significant flaw in the random sampler. Should be good to land once this is fixed.

@jaliyae
Copy link
Contributor Author

jaliyae commented Feb 15, 2019

Fixed the review comments and hopefully the windows build issue.

@soumith soumith force-pushed the jaliyae/distributed_samplers branch from 450947e to 0b71056 Compare February 16, 2019 03:25
@soumith
Copy link
Member

soumith commented Feb 16, 2019

Windows CI is still failing.

03:33:58 bin\test_api.exe : fatal error LNK1120: 2 unresolved externals

@jaliyae
Copy link
Contributor Author

jaliyae commented Feb 17, 2019

Fix the review comments. The checkpointing mechanism is probably need to change once we have a checkpoint mechanism in the dataloader. I don't think we need to finalize it for this PR.

@soumith soumith changed the title Jaliyae/distributed samplers C++ Frontend: adding two distributed samples (Random and Sequential) Feb 19, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants