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

Deprecate the connector + trainer dependency inside the Trainer class #7493

Closed
ananthsub opened this issue May 12, 2021 · 4 comments
Closed
Assignees
Labels
feature Is an improvement or enhancement refactor
Milestone

Comments

@ananthsub
Copy link
Contributor

🚀 Feature

Title says it all

Motivation

This is a legacy pattern internal to the trainer codebase today:
https://github.com/PyTorchLightning/pytorch-lightning/blob/b9a52fa2ef31f12f6992ece18a033318ec551907/pytorch_lightning/trainer/trainer.py#L315-L331

Inside the Trainer constructor, these connector classes are passed the trainer in their own constructors.
The connector class can then expose a set of hooks that the Trainer calls. Internally, the connector class updates the state on the trainer.

This adds an extra level of indirection, obfuscates what properties are set on the trainer, and makes unit testing more challenging since these components all end up requiring a trainer object.

For simple cases like the profiler connector, we should directly instantiate the object inside of the trainer constructor itself. https://github.com/PyTorchLightning/pytorch-lightning/blob/b9a52fa2ef31f12f6992ece18a033318ec551907/pytorch_lightning/trainer/connectors/profiler_connector.py#L38-L55

This is the approach the most recent AcceleratorConnector has taken, so we should ensure consistency across the codebase.

Pitch

Deprecate connector classes in favor of utility functions and/or direct instantiation/property setting from within the trainer constructor

Alternatives

Keep as is

@ananthsub ananthsub added feature Is an improvement or enhancement help wanted Open to be worked on refactor labels May 12, 2021
@ananthsub ananthsub self-assigned this May 12, 2021
@edenlightning edenlightning added this to the v1.4 milestone May 12, 2021
@edenlightning edenlightning removed the help wanted Open to be worked on label May 12, 2021
@awaelchli
Copy link
Contributor

Can we also consider the AcceleratorConnector here as an example? It doesn't take the trainer as a reference and it owns the following objects: accelerator, training_type_plugin, precision_plugin, cluster_environment. Is this in contrast with this feature request or are you asking to remove that as well?

@ananthsub
Copy link
Contributor Author

Can we also consider the AcceleratorConnector here as an example? It doesn't take the trainer as a reference and it owns the following objects: accelerator, training_type_plugin, precision_plugin, cluster_environment. Is this in contrast with this feature request or are you asking to remove that as well?

Not in contrast, the accelerator connector is an example of the new style we can follow for more complex object initialization. That being said, we can greatly simplify the accelerator connector once we have more clarity on how we want to reshape training tpe plugins / accelerators / precision / cluster environments. a lot of the current connector is bridging code. @shuyingsunshine21

@shuyingsunshine21
Copy link
Contributor

agree, I feel the interface/responsibility of connector should be just setting some trainer property, so we could explicitly make utilities for setting those properties.

For accelerator simplification, needs to think bit about this and we could follow up in #7324

@edenlightning edenlightning modified the milestones: v1.4, v1.5 Jul 6, 2021
@awaelchli awaelchli modified the milestones: v1.5, v1.6 Nov 4, 2021
@carmocca
Copy link
Contributor

carmocca commented Feb 1, 2022

Closing in favor of #10417 which is more up to date and specifically defines the steps left to do

@carmocca carmocca closed this as completed Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement refactor
Projects
None yet
Development

No branches or pull requests

5 participants