DDP sampler

[justusschock]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1506 .

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[pep8speaks]

Hello @justusschock! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-17 08:15:31 UTC

[codecov]

Codecov Report

Merging #1513 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1513   +/-   ##
======================================
  Coverage      91%     91%           
======================================
  Files          67      67           
  Lines        3784    3786    +2     
======================================
+ Hits         3439    3441    +2     
  Misses        345     345           

[Borda]

is there a way how to do it without adding extra parameters in API?
cc: @williamFalcon

[justusschock]

I think this is difficult, since for custom dataloaders it will be hard to determine when it should be set and when not.

Previously we had the check, whether a sampler was set, which was always false since the sampler is internally set by PyTorch.
Any suggestions on this? @Borda @williamFalcon

[tullie]

Rather than adding a new flag for this, is it possible to just check for the default samplers:
(isinstance(dataloader.sampler, RandomSampler) or isinstance(dataloader.sampler, SequentialSampler) or instance(dataloader.sampler, _InfiniteConstantSampler)) and need_dist_sampler?

I had a hard to find regression that happened because my custom sampler was overridden after #1425. Considering replace_sampler_ddp is set to True by default I think a lot of users will run into similar issues both in terms of regressions and new projects. E.g. users are going to expect that when they write a new sampler and pass it in the dataloader it'll be used without having to change a setting somewhere.

[justusschock]

I understand that, but I don't think, that this is a good idea. If I explicitly set a standard sampler and don't want it to be replaced, I can't do anything this way.

[tullie]

Fair enough. I'd prefer the trainer flag default to False at least, but let's leave it up to the opinion of others @PyTorchLightning/core-contributors

[williamFalcon]

@justusschock awesome addition.
@tullie i see what you mean, but I prefer to default it to True for now, but happy to change if it doesn't make sense for everyone.

Leaving it True means ddp/etc work without code changes, nor having to remember anything else.
Leaving it False means people with samplers don't spend time trying to figure out why their sampler was replaced or something.

Maybe a solution is a warning to one or the other?

[Borda]

a warning would be nice...