Mark the trainer's connectors as protected

[ananthsub]

Proposed refactoring or deprecation

Mark these attributes of the Trainer as protected:
https://github.com/PyTorchLightning/pytorch-lightning/blob/ab207921b9149a2c20ff9f797b6381f61cb09b1e/pytorch_lightning/trainer/trainer.py#L389-L419

Motivation

We are auditing the Lightning components and APIs to assess opportunities for improvements:

• https://docs.google.com/document/d/1xHU7-iQSpp9KJTjI3As2EM0mfNHHr37WZYpDpwLkivA/edit#
• Review Lightning architecture & API #7740

By python coding conventions, the connectors are technically part of the trainer's public API, despite not being documented and not intending to be used by callers.

However, we do not honor backward compatibility for these connectors because we (framework developers) consider them as implementation details of the trainer. Examples:
#9680
#9566

Pitch

Let's make this explicit that these connectors are not intended to be called by end users
Proposal: prefix all the attributes with an underscore to mark them as protected.
This gives us users more clarity around what is part of the public trainer API, and gives framework developers more flexibility to evolve the implementation here.

To make the PRs easy to review, we can migrate one connector at a time (1 PR per connector)

Alternatives

Keep as is

Additional context

Part of #7740

---

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

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

• Flash: The fastest way to get a Lightning baseline! A collection of tasks for fast prototyping, baselining, finetuning 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.

[awaelchli]

I agree overall with this choice. For most connectors this will be fine because they are only accessed by the trainer itself.

However I want to comment on the perspective of "protectedness" mentioned here. I see the value of protected members in the encapsulation of functionality rather than the reduction of exposure to users. Hence I would only do it for the connectors that are actually not accessed anywhere outside the trainer (e.g. the config validator you started doing). Some connectors are accessed in plugins or elsewhere and if we mark them protected without refactoring these access points we are at the same time violating that protected rule. So what I'm trying to say is, I believe the biggest motivation of this issue should be the encapsulation aspect and changes should be carried out primarily with that in mind, and then the rest of what you mention comes for free.

[ananthsub]

Some connectors are accessed in plugins or elsewhere and if we mark them protected without refactoring these access points we are at the same time violating that protected rule. So what I'm trying to say is, I believe the biggest motivation of this issue should be the encapsulation aspect and changes should be carried out primarily with that in mind, and then the rest of what you mention comes for free.

I agree. The underlying motivation here is to enable the refactoring of these connectors. Moving away from Trainer Mixins (where everything is publicly available) and public connectors is targeted first to limit their exposure in the API. This enables the next step of refactoring to happen more expediently.

This partially came up based on #9777 (cc @daniellepintz)
where we want to enable stronger typing on the trainer. but in order to do so, we need to move away from the pattern of Connector.on_trainer_init which sets properties dynamically on the trainer. However, it's unclear who might be relying on on_trainer_init outside of the trainer.

This is just one such example

[awaelchli]

Do we want to make also a separate issue for ideas how we want to refactor connectors (off of the architecture doc)? I would like to comment on that as well. I think we can also cleanly separate the discussion and refactor efforts for connectors and mixins as they are not really related I think.

[daniellepintz]

Do we want to make also a separate issue for ideas how we want to refactor connectors (off of the architecture doc)?

Yes, @ananthsub and I are working on auditing the connectors and mixins in the architecture doc, once we are done we will create issues! if you have any ideas you can feel free to share them with us now too :)

[ananthsub]

@daniellepintz the last one here is the logger connector. We must do this before v1.6 - otherwise it's a bad experience for uers to encounter these breaking changes in multiple Lightning minor release versions. We must do this before v1.6 to ensure that all changes are rolled out simultaneously for consistency.

fyi @carmocca , the changes should be minimal to be able to close out this issue.