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

Core Trainer Connectors #10417

Closed
awaelchli opened this issue Nov 8, 2021 · 1 comment
Closed

Core Trainer Connectors #10417

awaelchli opened this issue Nov 8, 2021 · 1 comment

Comments

@awaelchli
Copy link
Contributor

awaelchli commented Nov 8, 2021

Proposed refactoring or deprecation

Reduce the number of connectors the Trainer relies on to only three core ones:

  • AcceleratorConnector
  • LoggerConnector
  • DataConnector

Motivation

As part of the Lightning API audit led by @ananthsub + co., we proposed already several simplifications and code quality improvements to connectors in #7493, #7654, #9778, #10161, #10119, #10108, #10110 etc.
There are still a few connectors that are problematic for several reasons.

  • They share responsibilities that are too similar to the ones the Trainer should have (e.g. data connector vs. data_loading mixin)
  • They modify the state of the trainer / impersonating the Trainer (see all connectors)
  • They have been reduced over time and are now not useful anymore (e.g. optimizer connector)

These three properties make most connectors a burden to maintain as they just obscure the fact that Trainer remains a too powerful class.

Pitch

Remove (refactor away) all connectors except the core ones:

  • AcceleratorConnector
  • LoggerConnector
  • DataConnector

We (@awaelchli @daniellepintz + co) believe that the fact they have enough complexity and encapsulate responsibility warrants their existence as standalone classes. Hence, we formulate these goals:

  1. Simplify and document the three core connectors listed above
  2. Remove Trainer references
  3. Arrange ownership of components: LoggerConnector should own the logger instance, AcceleratorConnector should own accelerator instance, etc.
  4. Refactor away all others such that their logic lives in the Trainer directly.

Additional context

There are a great many similarities between the "DataLoadingMixin" and the DataConnector. As the "DataLoadingMixin" is not a true mixin and we are aiming at removing the "mixins" from the Trainer completely, the DataConnector will be a natural choice for where this logic can go.


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 @justusschock @awaelchli @akihironitta @rohitgr7 @kaushikb11 @ninginthecloud

@tchaton
Copy link
Contributor

tchaton commented Nov 15, 2021

Hey @awaelchli,

I know there is a strong push to remove the connectors to a minimal amount and I don't like this effort.
@williamFalcon introduced the connectors in the first hand to make the Trainer approachable to new readers and contributors. The goal was to make the highest layer of Lightning the cleanest possible.

IMO, the Trainer code right now is getting more complex than it used to be: https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/trainer/trainer.py#L447 and I have seen tweets about Lightning becoming unreadable.

I would prefer for us to come with a better approach to organise the code instead of dumping everything on the Trainer class and making it un-readable.

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

3 participants