You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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?
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
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
🚀 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
The text was updated successfully, but these errors were encountered: