[RFC] DDP Plugin's find_unused_parameters should be False by default

[ananthsub]

🚀 Feature

Currently Lightning defaults to setting find_unused_parameters=True when using PyTorch's DistributedDataParallel. https://github.com/PyTorchLightning/pytorch-lightning/blob/39274273a4c0a8d0b6bf3ee862a02ec4f5bf705a/pytorch_lightning/plugins/training_type/ddp.py#L221-L225

I propose that we change it back to False

Motivation

Changing the default to False offers these benefits:

• We keep in sync with upstream PyTorch.
• For the vast majority of models, we save their time and compute resources ($). Then users don't need to worry about this setting for a performance tip: https://pytorch-lightning.readthedocs.io/en/latest/benchmarking/performance.html#when-using-ddp-set-find-unused-parameters-false
• Example: Users who are migrating from a custom PyTorch training loop to Lightning can easily miss this flag difference, causing confusion around why performance doesn't match

Cons:

• For some models this crashes

Mitigations:

• In these instances, we can have clear documentation for the error message, and that they need to instantiate a DDP plugin with find_unused_parameters=True, and pass this to the trainer like so

ddp = DDPPlugin(find_unused_parameters=True)
trainer = Trainer(..., plugins=[ddp])

Or by using the new registry

cc @Borda @justusschock @kaushikb11 @awaelchli @akihironitta @rohitgr7 @Queuecumber

Pitch

Remove this default here: https://github.com/PyTorchLightning/pytorch-lightning/blob/39274273a4c0a8d0b6bf3ee862a02ec4f5bf705a/pytorch_lightning/plugins/training_type/ddp.py#L221-L225

Alternatives

Additional context

[SeanNaren]

Motivated by the addition of the plugin registry, I agree here. We should (if possible) catch the error thrown by find_unused_parameters and set this to something that ask the user to do plugins=ddp_find_unused_parameters which would mitigate the annoyance of having to instantiate the plugin which was the reason it was defaulted to True in the first place.

I've also suggested to people to check out sharded_ddp as this has performed well enough for all cases, not requiring speciality of these cases!

[maximsch2]

I wonder if we should consider flipping it to True in fast_dev_run mode? E.g. enable as much checking as possible in fast_dev_run=True case to surface errors early, but then makig it all opt-in in the full-blown runs?

[ananthsub]

@edenlightning could we consider this for v1.5? This again would be a breaking change but as pointed out, the registry would make this easier for users, and we can prioritize special error handling to warn users about this