Skip to content

[RFC] Introduce Strategy in favor of TrainingTypePlugin #10549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kaushikb11 opened this issue Nov 15, 2021 · 7 comments
Closed

[RFC] Introduce Strategy in favor of TrainingTypePlugin #10549

kaushikb11 opened this issue Nov 15, 2021 · 7 comments

Comments

@kaushikb11
Copy link
Contributor

kaushikb11 commented Nov 15, 2021

Proposed refactor

Part of #9932
Related PR: #10548

Motivation

As we are introducing StrategyPlugin in favor of TrainingTypePlugin for this release.

@ananthsub raised some really good points for discussion regarding StrategyPlugin. Reference

Proposed solution

Three main points to be discussed here

  • Drop Plugin from the name of the class
  • It should be under pytorch_lightning/strategy directory
  • Over time, these classes should be renamed as DDPStrategy and so on.

Would like your thoughts? @PyTorchLightning/core-contributors

cc @Borda @justusschock @awaelchli @akihironitta @kaushikb11 @ananthsub

@kaushikb11 kaushikb11 added discussion In a discussion stage refactor labels Nov 15, 2021
@awaelchli
Copy link
Contributor

Yes, definitely. We agreed on this I think. This was already set in motion by the introduction of the strategy argument in the Trainer.

Over time, these classes should be renamed as DDPStrategy and so on.

Has to be done for 1.6. We are targeting stable API for these components at 1.6.

@awaelchli
Copy link
Contributor

awaelchli commented Dec 17, 2021

Should we name all classes without the "Plugin"/"Strategy" suffix? Here is how this would look like:

DataParallel, (or just DP)
DDP2,
DDP,
DDPSpawn,
DDPFullySharded,
DeepSpeed,
Horovod,
IPU,
SingleDevice,
SingleTPU,
TPUSpawn,
TrainingType,
Parallel,
DDPSharded,
DDPSpawnSharded,

@ananthsub @kaushikb11 @carmocca @justusschock

@ananthsub
Copy link
Contributor

On naming, Loggers, Profilers, Accelerators, and ClusterEnvironments are suffixed with the component name (e.g. TensorboardLogger, SimpleProfiler, GPUAccelerator, SlurmEnvironment).

If we drop the suffix for strategy:

  • Would we also make this change for other components of Lightning so the naming scheme is unified across components (leading to one less thing to think about for users) ?
  • Do you think there's a risk with naming collisions? For instance, the IPU accelerator and IPU strategy could both be IPU classes. As a result, users would likely need to change the import for them to be:
from pytorch_lightning.accelerators import IPU as IPUAccelerator
from pytorch_lightning.strategy import IPU as IPUStrategy

@awaelchli
Copy link
Contributor

Proposed plan:

  1. Rename classes XPlugin to XStrategy
  2. Move files from pl/plugins/training_type to pl/strategy or pl/strategies
  3. Reroute pl/plugins/training_type to pl/strategy with a deprecation message

Step 3) is necessary because of certain usages of these plugins, even when they are not being subclassed. One example is:
trainer = Trainer(plugins=DDPPlugin(find_unused_parameters=False)) in which case we don't want to break user's code.

More follow-ups will include:

  • Rename TTP registry
  • Rename tests, comments, docstrings that use the TTP terms

wat3rBro added a commit to wat3rBro/d2go that referenced this issue Dec 29, 2021
Summary: DDPPlugin has been renamed to DDPStrategy (as part of Lightning-AI/pytorch-lightning#10549), causing oss CI to fail. Simply skipping the import to unblock CI since DDP feature is not used in test.

Differential Revision: D33351636

fbshipit-source-id: 76c751725b05bc818fb1c4b18e8d064fca71bc18
facebook-github-bot pushed a commit to facebookresearch/d2go that referenced this issue Dec 29, 2021
Summary: DDPPlugin has been renamed to DDPStrategy (as part of Lightning-AI/pytorch-lightning#10549), causing oss CI to fail. Simply skipping the import to unblock CI since DDP feature is not used in test.

Reviewed By: kazhang

Differential Revision: D33351636

fbshipit-source-id: 7a1881c8cd48d9ff17edd41137d27a976103fdde
@awaelchli
Copy link
Contributor

We also have to remember to rename the test files and test names that include the word plugin.

@kaushikb11
Copy link
Contributor Author

We also have to remember to rename the test files and test names that include the word plugin.

I could take care of it.

@kaushikb11
Copy link
Contributor Author

Closing this issue, as the Strategy transition has been completed! Thank you to everyone involved ❤️

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

No branches or pull requests

4 participants