Make default implementation for train_dataloader in DataHooks raise NotImplementedError

[ananthsub]

Discussed in #8734

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

The default implementation for these is logging a warning that nothing is implemented. https://github.com/PyTorchLightning/pytorch-lightning/blob/963c26764682fa4cf64c93c5a7572ae0040e9c32/pytorch_lightning/core/hooks.py#L529

The current default implementation logs a warning, which can be lost, and which forces the framework to perform is_overridden checks. Instead, having the default implementation raise a NotImplementedError will make errors clear to users in case users forget to implement these hooks.

This applies for all the dataloader hooks in DataHooks, not just train_dataloader

[Arnab1181412]

Sir can i work on this issue as I am a new contributor to open source.

[ananthsub]

Sir can i work on this issue as I am a new contributor to open source.

Definitely! Go ahead 😄

[Arnab1181412]

Sir as I am new so can you give me a bit context about the problem what I understand from the issue is that instead of giving warning when users forget to use hooks I should raise a NotImplementedError

[Tshimanga]

@ananthsub this issue looks orphaned, and I'm keen to pick it up; but I wanted to check in first since @Arnab1181412 had initially volunteered. I want to be cognizant of any potential etiquette and not step on any toes 🙏🏽

[ananthsub]

@ananthsub this issue looks orphaned, and I'm keen to pick it up; but I wanted to check in first since @Arnab1181412 had initially volunteered. I want to be cognizant of any potential etiquette and not step on any toes 🙏🏽

Please go ahead! Thank you for checking :)

[Tshimanga]

@ananthsub so the ticket states This applies for all the dataloader hooks in DataHooks, not just train_dataloader, but based on the docstrings on all the other dataloader hooks {val, test, predict}_dataloader are intentionally optional hooks whose presence is not necessary to run the Trainer. Is the assertion here that users will be required to override all of these dataloader hooks?
https://github.com/PyTorchLightning/pytorch-lightning/blob/de57feff445744f431eaef29182b1bcbf58a1924/pytorch_lightning/core/hooks.py#L649-L651
https://github.com/PyTorchLightning/pytorch-lightning/blob/de57feff445744f431eaef29182b1bcbf58a1924/pytorch_lightning/core/hooks.py#L597-L599

Surprisingly the docstring for the predict_dataloader doesn't explicitly say that it's optional but I've never overridden it in my projects 😅

[awaelchli]

There is a problem. The IDEs now show that the LightningModule is an abstract class and complains that we must implement all abstract methods (train_dataloader etc.). But these are obviously not mandatory.

We should fix this

[ananthsub]

@awaelchli does removing ABC from the LightningModule's inheritance resolve this? https://github.com/PyTorchLightning/pytorch-lightning/blob/91ce0d0a99ec0a488e04e09997f6dd662ce217cd/pytorch_lightning/core/lightning.py#L56

[awaelchli]

no, since raising NotImplementedError will make the class abstract anyway. See also #9499 who is attempting to do that.

[awaelchli]

Given the documentation and recommended usage of NotImplementedError here, we should consider raising a different error.

[ananthsub]

no, since raising NotImplementedError will make the class abstract anyway. See also #9499 who is attempting to do that.

huh, I thought these would have to be marked with @abstractmethod decorators for the IDE to raise a warning

[justusschock]

@ananthsub you are correct afaik.

@awaelchli the difference is that for classes that inherit ABC (or have ABCMeta as metaclass) cannot be instantiated as long as there is at least one method marked as abstractmethod

[carmocca]

This is caused by a bug in PyCharm: #9529 (comment)

[carmocca]

Here's another issue about an user complaining about this: #10667

@justusschock proposed the following alternative:

An alternative could be to return None here and then raise a RuntimeError inside the trainer when data is None

[carmocca]

And one more: #12176

[awaelchli]

I wanted to fix this a while ago with #9529. We can reconsider?

[carmocca]

I'd say yes since it seems like it's important to users.