Trainer cleanup

[Borda]

What does this PR do?

minor cleaning Training inits in its parent classes... Partially solves #887

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[MattPainter01]

Looks good to me!

I was looking for guides on whether to use ellipsis instead of pass in abstract methods so we could use this everywhere, but as far as I can tell there is no consensus. Even pythons docs are inconsistent.

Never mind, it appears that pass is preferred.

[williamFalcon]

@Borda i think the ... is making the code unreadable to non engs... what's the advantage?
One of our core principles is to keep things simple so the code is readable.

[Borda]

The adventage of ... compare to none or pass is that it is just place holder, but let's keep it open for now, it is not a priority....
Moreover the motivation is not to depend on this package just because of single dataset lol

[luiscape]

@Borda I agree with @williamFalcon here. I think the code is less readable with ellipsis rather than with regular None. I think that's the case because:

• It is a very unusual notation for class attribute declaration
• It's not what the official Ellipsis documentation suggests for the expected use-case
• In my experience, I have only really used Ellipsis when declaring types using typing (see PEP484)

In the same spirit, maybe a potential enhancement would be to use type hinting to declare the expected type of such variables (see here)?

[Borda]

In the same spirit, maybe a potential enhancement would be to use type hinting to declare the expected type of such variables (see here)?

well we already started with type hinting, but not yet propagated everywhere lol so I would say that it fine aligned with your ref to class basic or here (maybe I missed something?)
and for class properties, I just found (feel like) it easier to understand that there is just place holder and it is not the true init value, I saw the discussion somewhere (let me see an I will update it here)

Anyway, happy to discuss it or drop my suggestion... :]

[hadim]

I don't find the code less readable with ellipsis but I guess this is a subjective opinion here.

I want to raise something kind of related: I use pyright to check my Python code while coding and the checker raise 431 errors on PL code.

Using ellipsis fixes some of them. For example, consider the following code in training_loop.py:

class TrainerTrainLoopMixin:

    def __init__(self):
        self.on_epoch_end = None

    def run_training_epoch(self):

        # (...)

        # Pyright error:
        # 'self.on_epoch_end()' has type 'None' and is not callable
        self.on_epoch_end()

Adding typing fixes the error in run_training_epoch() but raises a new one in __init__():

class TrainerTrainLoopMixin:

    def __init__(self):

        # Pyright error:
        # Cannot assign member 'on_epoch_end' for type 'TrainerTrainLoopMixin'
        #   Expression of type 'None' cannot be assigned to member 'on_epoch_end' of class 'TrainerTrainLoopMixin'
        #     'None' cannot be assigned to '(*args, **kwargs) -> Any'
        self.on_epoch_end: Callable = None

    def run_training_epoch(self):

        # (...)

        # Not Pyright error.
        self.on_epoch_end()

Adding ellipsis fixes it:

class TrainerTrainLoopMixin:

    def __init__(self):

        # Not Pyright error.
        self.on_epoch_end: Callable = ...

    def run_training_epoch(self):

        # (...)

        # Not Pyright error.
        self.on_epoch_end()

Now to be honest I don't really know whether it's a hack that appear to work or a good practice (I am not very familiar with ellipsis.

[Borda]

@hadim does pyright exist also fro Ubuntu? found it here
I have replaced the ... just with the docs, could you check it ow, pls

[MattPainter01]

I had the same understanding of ... for initialising variables as @hadim. If there are type annotations, type checkers understand that ellipsis is a placeholder, whereas None is not and so it sometimes warns of this. There is some talk of this in PEP484 for .pyi stub files but I couldn't find anything on our exact case anywhere.

An example from PyCharm with some types put in,

I think this is mostly just useful if we decide to start specifying types in the future.

For abstract methods, I can't find a particular consensus on pass vs ... vs empty but personally I prefer pass since it's used in most examples of abstract classes I could find.

[ethanwharris]

Why not just:

class TrainerTrainLoopMixin:

    on_epoch_end: Callable

Then you could remove the __init__ all together and avoid the weirdness of assigning ... or None. See further discussion here.

[luiscape]

Totally agree with @ethanwharris. Instead if just replacing None with ... let's use type annotations instead such as on_epoch_end: Callable. That makes it much more readable and helps us find bugs with Pytright & friends.

@Borda maybe we should create an epic or issue for adding type annotations to the core modules (I'm not sure there's one)?

[hadim]

@Borda Pyright is written in JS and is integrated out of the box in VSCode as an extension. You can also use it in Vim or on a command line: https://github.com/microsoft/pyright#installation

[Borda]

@Borda maybe we should create an epic or issue for adding type annotations to the core modules (I'm not sure there's one)?

we already have it :] #887

[ethanwharris]

This looks great! :)

[Borda]

@luiscape ^^ ?