Extend `val_check_interval` support

[amoshyc]

🐛 Bug

When I train the model by specifying number of training step instead of epoch, val_check_interval behaves strangely. Please see the following colab:

https://colab.research.google.com/drive/1I0ySRH03T9LdXHoGwCp3Q242dHEHWP_0?usp=sharing

In the code, I log the global_step on each validation.

class Model(pl.LightningModule):
    ...

    def on_validation_start(self):
        print('global_step:', self.global_step)

I set Trainer's max_steps to 100 and val_check_interval to 10.
But when I run the cells In[4] and In[5], the outputs are different.
The only different between In[4] and In[5] is the number of samples of the dataset which should not be the reason.

In[4]:

train_set = RandomDataset(1, 40)
valid_set = RandomDataset(1, 40)
train_loader = torch.utils.data.DataLoader(train_set, batch_size=2)
valid_loader = torch.utils.data.DataLoader(valid_set, batch_size=2)

trainer = pl.Trainer(
    gpus=1,
    max_steps=100,
    val_check_interval=10,
    num_sanity_val_steps=0,
    log_every_n_steps=10,
    progress_bar_refresh_rate=0,
)
model = Model()
trainer.fit(model, train_loader, valid_loader)

Out[4]:

global_step: 9
global_step: 19
global_step: 29
global_step: 39
global_step: 49
global_step: 59
global_step: 69
global_step: 79
global_step: 89
global_step: 99

In[5]:

train_set = RandomDataset(1, 32)
valid_set = RandomDataset(1, 32)
train_loader = torch.utils.data.DataLoader(train_set, batch_size=2)
valid_loader = torch.utils.data.DataLoader(valid_set, batch_size=2)

trainer = pl.Trainer(
    gpus=1,
    max_steps=100,
    val_check_interval=10,
    num_sanity_val_steps=0,
    log_every_n_steps=10,
    progress_bar_refresh_rate=0,
)
model = Model()
trainer.fit(model, train_loader, valid_loader)

Out[5]:

global_step: 9
global_step: 25
global_step: 41
global_step: 57
global_step: 73
global_step: 89

Expected behavior

Since I specify max_steps and set val_check_interval to a integer, I expect that result is the same as Out[4] no matter the number of samples in the dataset. The doc says that val_check_interval specifies the number of training step between validations, so Out[5] should be the same as Out[4].

I also expect that the number of times validation performed should be the same. BTW, the x-axis in the tensorboard are also wrong. You can see that in Out[9].

Environment

* CUDA:
	- GPU:
		- Tesla T4
	- available:         True
	- version:           10.2
* Packages:
	- numpy:             1.19.5
	- pyTorch_debug:     False
	- pyTorch_version:   1.9.0+cu102
	- pytorch-lightning: 1.3.7post0
	- tqdm:              4.41.1
* System:
	- OS:                Linux
	- architecture:
		- 64bit
		- 
	- processor:         x86_64
	- python:            3.7.10
	- version:           #1 SMP Sat Jun 5 09:50:34 PDT 2021

cc @Borda @tchaton

[tchaton]

Dear @amoshyc,

Thanks for reporting this issue. Would be interested in creating a fix PR ?
I believe this shouldn't be too hard and a great way to learn more about Lightning.

Best.
T.C

[MirMustafaAli]

Hi @tchaton ,
Can i work on this issue as it is labelled good first issue. ?

[amoshyc]

Sorry for the late reply. I'm so busy doing my research. I haven't have time to fix it.
The last time I check the code, it seems that the bug is here and someone already notice it.
But I'm not quite sure. I'm not working on it.

[tchaton]

Dear @MirMustafaAli,

Yes, feel free to work on this one and ping us if you get blocked :)

Best,
T.C

[MirMustafaAli]

Thanks

[MirMustafaAli]

@tchaton
The issue seems to be in method _should_check_fx where it batch idx of current epoch is being passed as parameter . i was able to resolve it by passing total_batch_idx of epoch training loop to the method.

https://github.com/PyTorchLightning/pytorch-lightning/blob/6b47cbe3ca8aa3fd82211bc9fa32e734753a6950/pytorch_lightning/loops/epoch/training_epoch_loop.py#L169-L170

Feedback and how to proceed required on solution

[carmocca]

train_set = RandomDataset(1, 32)
valid_set = RandomDataset(1, 32)
train_loader = torch.utils.data.DataLoader(train_set, batch_size=2)
valid_loader = torch.utils.data.DataLoader(valid_set, batch_size=2)

Given this definition, your dataset has 32 batches. With batch_size=2, that gives us 32/2=16 batches per epoch.

Since you set val_check_interval=10, and there are 16 batches, you only have one chance to run validation in the epoch (in the 10th batch, index 9).

The issue here is that the docs definition is ambiguous

            val_check_interval: How often to check the validation set. Use float to check within a training epoch,
                use int to check every n steps (batches).

"use int to check every n steps (batches)" does not explicitly state whether this is within an epoch or between epochs. The current implementation is for within an epoch.

Changing this would be a breaking change, and I can imagine users wanting to be able to choose which one to do.

cc @PyTorchLightning/core-contributors

Repro bug report model

import os

import torch
from torch.utils.data import DataLoader, Dataset

from pytorch_lightning import LightningModule, Trainer


class RandomDataset(Dataset):
    def __init__(self, size, length):
        self.len = length
        self.data = torch.randn(length, size)

    def __getitem__(self, index):
        return self.data[index]

    def __len__(self):
        return self.len


class BoringModel(LightningModule):
    def __init__(self):
        super().__init__()
        self.layer = torch.nn.Linear(1, 2)

    def forward(self, x):
        return self.layer(x)

    def training_step(self, batch, batch_idx):
        loss = self(batch).sum()
        self.log("train_loss", loss)
        return {"loss": loss}

    def validation_step(self, batch, batch_idx):
        loss = self(batch).sum()
        self.log("valid_loss", loss)

    def test_step(self, batch, batch_idx):
        loss = self(batch).sum()
        self.log("test_loss", loss)

    def configure_optimizers(self):
        return torch.optim.SGD(self.layer.parameters(), lr=0.1)

    def on_validation_start(self):
        print('global_step:', self.global_step)


def run():
    train_set = RandomDataset(1, 32)
    valid_set = RandomDataset(1, 32)
    train_loader = torch.utils.data.DataLoader(train_set, batch_size=2)
    valid_loader = torch.utils.data.DataLoader(valid_set, batch_size=2)

    model = BoringModel()
    trainer = Trainer(
        default_root_dir=os.getcwd(),
        max_steps=100,
        val_check_interval=10,
        num_sanity_val_steps=0,
        progress_bar_refresh_rate=0,
    )
    trainer.fit(model, train_dataloaders=train_loader, val_dataloaders=valid_loader)


if __name__ == "__main__":
    run()

[awaelchli]

@carmocca note there is also the check_val_every_n_epochs which is currently not mutually exclusive with val_check_interval if I understand correctly. If we did exactly what the docs said, we would have to make them mutually exclusive somehow, right?

[carmocca]

we would have to make them mutually exclusive somehow, right?

I think so.

These are all the options

• check every n batches across epochs: Implemented in Support val_check_interval values higher than number of training batches  #11993. What this issue is requesting. Potentially val_check_interval=n, check_val_every_n_epoch=None Also requested in:
  • Enable trainer val_check_interval to be greater than number of the training batches #4406
  • allow val_check_interval to be larger than training dataset size  #5413
  • val_check_interval every N global steps? #11152
  • ModelCheckpoint does NOT save anything if every_n_train_steps is greater than the number of training steps in a epoch #11979
  • Save checkpoint and validate every n steps #2534
• check every n batches per epoch: val_check_interval=n, check_val_every_n_epoch=1
• check every n epochs: val_check_interval=1.0, check_val_every_n_epoch=n
• check every n% batches across epochs: Not possible. Potentially val_check_interval=0.n, check_val_every_n_epoch=None?
• check every n% batches per epoch: val_check_interval=0.n, check_val_every_n_epoch=1
• check every n% epochs: Not possible. Potentially val_check_interval=1.0, check_val_every_n_epoch=0.n?

Also similar to #12000