DataConnector.attach_datamodule doesn't respect LightningModule.datamodule anymore

[yifuwang]

🐛 Bug

#8002 changed attach_datamodule() so that it doesn't respect LIghtningModule.datamodule anymore. Unless this is intentional, we should bring back the old behavior.

https://github.com/PyTorchLightning/pytorch-lightning/pull/8002/files#diff-906ad7bb3bf022c44bada799c11c2b45ef0c12c3de2cc743d3078bd95cd0d778L117-L118

Please reproduce using the BoringModel

To Reproduce

Will provide along with a PR.

Expected behavior

DataConnector considers datamodule present when LightningModule.datamodule is set.

Environment

This should be environment agnostic.

Additional context

This broke some FB tests.

[carmocca]

Hi!

Accessing LightningModule.datamodule was deprecated in #7168.

#8002 changed that piece of code to avoid using the deprecated property.

Can you elaborate on what exactly broke? Ideally with a test/repro snipept

[yifuwang]

Hi @carmocca, thanks for the quick follow up. I see, I wasn't aware of the deprecation. Many of our use cases are batch jobs so it's hard to notice the deprecation warning. @ananthsub we should figure out something to better handle deprecations in general.

Can you elaborate on what exactly broke? Ideally with a test/repro snipept

We still have use cases that set LightningModule.datamodule and expect the datamodule to have access to the trainer. After #8002, the datamodule stopped having access to the trainer.

[carmocca]

We could add back the deprecated access with the disadvantage that the deprecation message will appear for everybody calling trainer functions without the datamodule argument.

the datamodule stopped having access to the trainer.

You can still access self.trainer inside your LightningDataModule. Is that what you are referring to?

It gets set here assuming a datamodule is passed:

https://github.com/PyTorchLightning/pytorch-lightning/blob/6dd7797c97984c0e05e98b3cb7fed633cc409dce/pytorch_lightning/trainer/connectors/data_connector.py#L133

[yifuwang]

You can still access self.trainer inside your LightningDataModule. Is that what you are referring to?
It gets set here assuming a datamodule is passed:

Right. But not anymore with the deprecated usage pattern since attach_datamodule would be shortcircuited.

We could add back the deprecated access with the disadvantage that the deprecation message will appear for everybody calling trainer functions without the datamodule argument.

It would be great if we can do this to give us a bit more leeway, so we can help our users migrate to the recommended/supported semantic (I fear there are other such use cases that are not caught by tests).

@ananthsub - In the future, we should probably monitor deprecation warnings in a centralized fashion so we can help our users modernize their code in a more timely fashion.

[carmocca]

It would be great if we can do this to give us a bit more leeway, so we can help our users migrate to the recommended/supported semantic (I fear there are other such use cases that are not caught by tests).

Yeah, we can do this. Would like to know Ananth's thoughts first

What's not clear to me is if the broken usage is wrong and should be replaced or instead it relies on a valid use of lightningmodule.datamodule and it shouldnt be deprecated.

As mentioned before, a snippet of what worked before but is breaking now would be helpful 😄