Skip to content
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

Generalize internal checks for precision plugin type, training type, accelerator type #10821

Closed
awaelchli opened this issue Nov 29, 2021 · 8 comments

Comments

@awaelchli
Copy link
Contributor

awaelchli commented Nov 29, 2021

Proposed refactor

Internally, our checks against the type of Accelerator, Precision type, strategy is not robust towards custom instances passed in by the user.

Motivation

Internally, some operations in the optimization, logging, etc. need a different code path depending on 1) Accelerator type (cpu, gpu) or 2) Precision type (apex, native) or 3) strategy type (ddp, ddp-spawn, ...). Currently we have this pattern:

if trainer._device_type == DeviceType.CPU:
    # do something only for cpu


if trainer._amp_backend == AMPType.Apex:
    # do something differently for apex

Pitch

Change these to

if isinstance(trainer.accelerator, CPUAccelerator):
    # do something only for cpu


if isinstance(trainer.precision_plugin, ApexPrecisionPlugin):
    # do something differently for apex

This has the benefits:

  • User passes in custom plugins (subclasses of our plugins)
  • Encapsulation: Protected members _device_type, _strategy_type, won't be abusively accessed publicly anymore. They remain an implementation detail of AcceleratorConnector
  • Minimally simplifies AcceleratorConnector logic

Additional context

Discusson started in #10596


If you enjoy Lightning, check out our other projects! ⚡

  • Metrics: Machine learning metrics for distributed, scalable PyTorch applications.

  • Lite: enables pure PyTorch users to scale their existing code on any kind of device while retaining full control over their own loops and optimization logic.

  • Flash: The fastest way to get a Lightning baseline! A collection of tasks for fast prototyping, baselining, fine-tuning, and solving problems with deep learning.

  • Bolts: Pretrained SOTA Deep Learning models, callbacks, and more for research and production with PyTorch Lightning and PyTorch.

  • Lightning Transformers: Flexible interface for high-performance research using SOTA Transformers leveraging Pytorch Lightning, Transformers, and Hydra.

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

@stale
Copy link

stale bot commented Jan 3, 2022

This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

@stale stale bot added the won't fix This will not be worked on label Jan 3, 2022
@awaelchli awaelchli added this to the 1.6 milestone Jan 4, 2022
@tchaton
Copy link
Contributor

tchaton commented Jan 4, 2022

This seems cleaner and easier to debug. Let's do it.

@stale stale bot removed the won't fix This will not be worked on label Jan 4, 2022
@tchaton tchaton added the let's do it! approved to implement label Jan 4, 2022
@carmocca
Copy link
Contributor

Since this is invisible to the user, this doesn't need to happen strictly before 1.6 so I'll move it out and we can update the milestone whenever this gets done.

@carmocca carmocca added the good first issue Good for newcomers label Feb 14, 2022
@carmocca carmocca modified the milestones: 1.6, future Feb 14, 2022
@carmocca carmocca assigned carmocca and unassigned carmocca Feb 14, 2022
@carmocca
Copy link
Contributor

NVM my previous comment, @justusschock will take it

@carmocca carmocca modified the milestones: future, 1.6 Feb 14, 2022
@carmocca carmocca moved this to Todo in Frameworks Planning Feb 14, 2022
@justusschock justusschock mentioned this issue Feb 23, 2022
12 tasks
@justusschock justusschock moved this from Todo to In Review in Frameworks Planning Mar 1, 2022
@carmocca carmocca modified the milestones: 1.6, 1.7 Mar 28, 2022
@carmocca carmocca removed good first issue Good for newcomers let's do it! approved to implement labels Mar 28, 2022
@carmocca carmocca modified the milestones: 1.6, 1.7 Mar 28, 2022
@carmocca carmocca added the help wanted Open to be worked on label Jul 19, 2022
@carmocca carmocca modified the milestones: pl:1.7, pl:future Jul 19, 2022
@carmocca carmocca removed the help wanted Open to be worked on label Jul 19, 2022
@carmocca
Copy link
Contributor

carmocca commented Aug 5, 2022

@justusschock Do you think you could finish this for 1.8?

@justusschock
Copy link
Member

Definitely!

@carmocca carmocca modified the milestones: pl:future, pl:1.8 Aug 5, 2022
@Borda Borda moved this from In Review to Blocked in Frameworks Planning Sep 1, 2022
@carmocca carmocca modified the milestones: v1.8, future Oct 13, 2022
@awaelchli
Copy link
Contributor Author

awaelchli commented Jan 21, 2023

You failed 😄

@awaelchli
Copy link
Contributor Author

awaelchli commented Jan 21, 2023

Sorry, couldn't resist with the stupid comment 😄

But in all seriousness, I think we completed this in the meantime already. It seems all enum types got removed. Or do you see anything left to do?

@github-project-automation github-project-automation bot moved this from Blocked to Done in Frameworks Planning Jan 21, 2023
@carmocca carmocca modified the milestones: future, 2.0 Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants