Deprecate TrainerProperties Mixin and move property definitions directly into `trainer.py`

[daniellepintz]

🚀 Feature

Deprecate the TrainerProperties: https://github.com/PyTorchLightning/pytorch-lightning/blob/938a191406fff5f51fba03fcf824f22d8d23c2e0/pytorch_lightning/trainer/properties.py

Move the properties directly to the trainer: https://github.com/PyTorchLightning/pytorch-lightning/blob/938a191406fff5f51fba03fcf824f22d8d23c2e0/pytorch_lightning/trainer/trainer.py#L104-L161

Motivation

It's confusing to have trainer properties in multiple different classes. It would be more clear to have them all directly in trainer.py

Pitch

Alternatives

Additional context

---

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.

[ananthsub]

@daniellepintz maybe we could keep this issue specific to the properties, and file separate issues to discuss the other mixins.

I agree with the push though; these mixins are historical artifacts of the prior trainer design. The mixins are not self-contained at all.

It is very confusing having the properties listed separately, especially for what's on the public trainer API vs what's supposed to be private

I know @awaelchli had similar thoughts in the past

[daniellepintz]

Sounds good, updated the description

[awaelchli]

This is only about moving the code from one file to the trainer.py right? And there is no deprecation necessary because we won't rename any attributes, right?

[awaelchli]

It's confusing to have trainer properties in multiple different classes. It would be more clear to have them all directly in trainer.py

Just to give context of why things are the way they are:

The feedback for PL 1.0 was that the trainer is not easy to read for users. We wanted to make the trainer accessible so that users who start reading the code are guided to the right place from the very start (without needing to scroll hundreds of lines) and they can easily follow the code path.

By merging all mixins into one, the trainer will now be cluttered again with a long list of properties, dataloading methods etc. where we will sacrifice readability and lose focus of what's important and what's not.

To justify these changes, we can argue this is only a temporary state and we will invest efforts in making the trainer less a god class in the future.

[daniellepintz]

@awaelchli yes it is about moving the code. by deprecation I just meant deleting the TrainerProperties class and file.

thanks for the context! that makes sense. I am also wondering, do we really need all those properties? are they all used? Maybe we should try to reduce the number of properties.

If we can't remove any of the properties, maybe this issue isn't super hi-pri as there are pros and cons either way.

[ananthsub]

I'll also add, psychologically, we will be more incentivized to invest in efforts to make the trainer less of a god class if this is all in trainer.py.

Because so much of the trainer logic is split across files like properties.py, the connectors, and the mixins, the problem is out-of-sight, out-of-mind. Stashing the code elsewhere is not a real solution and I'm looking forward to improving the current situation. I think it'll also force us to reckon with how complex the trainer has gotten, what do we absolutely have to live with, and what can we better organize.

Given Lightning's claim to fame is that it's organized pytorch (not abstracted pytorch), it'll be critical that users and others in the community see this is true within the framework-internal codebase too!

[daniellepintz]

@ananthsub I completely agree! I'll add this task to my queue then.

[awaelchli]

I am also wondering, do we really need all those properties? are they all used? Maybe we should try to reduce the number of properties.

We can try, but most of them are exposed for convenience access for users.