Should we use an overrides package?

[ananthsub]

Lightning as a framework makes heavy use of inheritance; however, inheritance & overrides is not well supported in python.

Should we use a package like overrides https://pypi.org/project/overrides/ to address these concerns?

Python has no standard mechanism by which to guarantee that (1) a method that previously overrode an inherited method continues to do so, and (2) a method that previously did not override an inherited will not override now. This opens the door for subtle problems as class hierarchies evolve over time. For example,

A method that is added to a superclass is shadowed by an existing method with the same name in a subclass.
A method of a superclass that is overridden by a subclass is renamed in the superclass but not in the subclass.
A method of a superclass that is overridden by a subclass is removed in the superclass but not in the subclass.
A method of a superclass that is overridden by a subclass but the signature of the overridden method is incompatible with that of the inherited one.

Especially as we make tweaks to our APIs, there is no guarantee that adding new methods/properties to our classes aren't being shadowed & overridden by users in their subclasses (unless they're using typechecking and actually check their warnings)

@PyTorchLightning/core-contributors

[awaelchli]

What would be the key example usage for this in Lightning?
If the user is implementing a plugin, callback, LightningModule and they have to learn these overrides mechanisms, something that does not exist natively in Python, then I would vote no.

If it's strictly used internally, then I can imagine to adopt this but on the other hand, FB team previously have spoken out against our additions of dependencies in favor of keeping the package lightweight. cc @ananthsub

[tchaton]

Thanks for sharing this @ananthsub.

Personally, I like it for internal usage. I believe it would make the code much simpler to read as functions will be decorated, especially for accelerators which relies strongly on inheritance right now.

Best,
T.C

[ananthsub]

@tchaton the accelerator/training type was my thought as well, especially in #9045 where I missed a spot of renaming the method, but no tests failed.

I remember in Lightning 0.5(?) the project had the @pl decorators for things like dataloaders to enforce the override was applied. I do remember as a module writer that this felt like un-pythonic boilerplate to add to each overridden method. as someone who's reading module code though, it was very helpful to immediately understand what's required from the framework interface.

I do prefer using more abstract classes & methods to avoid the reliance on inheritance (and the use of packages like this). this would resolve the boilerplate or the writing side, but for readers the interface still might not jump out as clearly.

something that does not exist natively in Python, then I would vote no.

@awaelchli you're absolutely right, this is covering a language deficiency in python and it would add another dependency. i am optimistic that working on interfaces should greatly improve the situation without requiring this

Tagging @kandluis as he had mentioned this too

[Borda]

The package is nice but I do not think it addresses the case I described in #9260 (comment) as this is taking child classes and there is not insurance that user who defines own callbacks adds this wrapper...

[carmocca]

I think it fits under:

A method of a superclass that is overridden by a subclass but the signature of the overridden method is incompatible with that of the inherited one.